Re: [PATCH v5] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Julia Lawall <julia.lawall@inria.fr>]

On Sat, 5 Apr 2025, Abraham Samuel Adekunle wrote:

> When a value has been tested for NULL in an expression, a
> second NULL test on the same value in another expression
> is unnecessary when the value has not been assigned to NULL.
>
> Remove unnecessary duplicate NULL tests on the same value
> that has previously been tested.
>
> Found by Coccinelle.

The changes are found in the same way, but the code patterns are overall
quite different.  It could make sense to make separate patches for them.
Then you could make a log message that is really specialized to the code
in each patch and it would be easier for the reviewer to be convinced that
you have done the right thing.

julia

>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
> Changed in v4:
> 	- Separated initially integrated suggested change
> 	 "use modulo % 4096 over & 0xfff" to a different patch.
> Changes in v3:
> 	- Changed other cases to use modulo (% 4096) over (& 0xofff).
> 	- Modified commit message to reflect these changes.
> Changes in v2:
> 	- Dropped patch files for media drivers from patchset as it is
> 	  not meant for outreachy applicants.
> 	- Added full-stop aign to text in commit message.
> 	- Made code more readable by adding a line break.
> 	- Changed cases to use modulo (% 4096) over (& 0xfff).
> Changes in v1
> 	- Patch for drivers/staging/media/av7110/sp8870.c and
> 	- drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> 	  is split into two different patches in the patchset for use by the different
> 	  driver maintainers.
> 	- Added subject title for each of the separated patches.
> 	- Patch 1: Removed unnecessary curly braces {} initially inserted.
> 	- Patch 2: Unnecessary {} was also removed for v1.
>
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_xmit.c     | 56 +++++++++----------
>  2 files changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 90966b7034ab..675226535cd1 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1323,7 +1323,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  	spin_unlock_bh(&pstapriv->asoc_list_lock);
>
>  	/*  now the station is qualified to join our BSS... */
> -	if (pstat && (pstat->state & WIFI_FW_ASSOC_SUCCESS) && (status == WLAN_STATUS_SUCCESS)) {
> +	if ((pstat->state & WIFI_FW_ASSOC_SUCCESS) && (status == WLAN_STATUS_SUCCESS)) {
>  		/* 1 bss_cap_update & sta_info_update */
>  		bss_cap_update_on_sta_join(padapter, pstat);
>  		sta_info_update(padapter, pstat);
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 026061b464f7..f817cab2f831 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -941,35 +941,33 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
>  			if (!(psta->state & _FW_LINKED))
>  				return _FAIL;
>
> -			if (psta) {
> -				psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
> -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> -				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> -
> -				SetSeqNum(hdr, pattrib->seqnum);
> -
> -				/* check if enable ampdu */
> -				if (pattrib->ht_en && psta->htpriv.ampdu_enable)
> -					if (psta->htpriv.agg_enable_bitmap & BIT(pattrib->priority))
> -						pattrib->ampdu_en = true;
> -
> -				/* re-check if enable ampdu by BA_starting_seqctrl */
> -				if (pattrib->ampdu_en == true) {
> -					u16 tx_seq;
> -
> -					tx_seq = psta->BA_starting_seqctrl[pattrib->priority & 0x0f];
> -
> -					/* check BA_starting_seqctrl */
> -					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> -						pattrib->ampdu_en = false;/* AGG BK */
> -					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> -
> -						pattrib->ampdu_en = true;/* AGG EN */
> -					} else {
> -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum+1)&0xfff;
> -						pattrib->ampdu_en = true;/* AGG EN */
> -					}
> +			psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
> +			psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> +			pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> +
> +			SetSeqNum(hdr, pattrib->seqnum);
> +
> +			/* check if enable ampdu */
> +			if (pattrib->ht_en && psta->htpriv.ampdu_enable)
> +				if (psta->htpriv.agg_enable_bitmap & BIT(pattrib->priority))
> +					pattrib->ampdu_en = true;
> +
> +			/* re-check if enable ampdu by BA_starting_seqctrl */
> +			if (pattrib->ampdu_en == true) {
> +				u16 tx_seq;
> +
> +				tx_seq = psta->BA_starting_seqctrl[pattrib->priority & 0x0f];
> +
> +				/* check BA_starting_seqctrl */
> +				if (SN_LESS(pattrib->seqnum, tx_seq)) {
> +					pattrib->ampdu_en = false;/* AGG BK */
> +				} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> +					psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> +
> +					pattrib->ampdu_en = true;/* AGG EN */
> +				} else {
> +					psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum+1)&0xfff;
> +					pattrib->ampdu_en = true;/* AGG EN */
>  				}
>  			}
>  		}
> --
> 2.34.1
>
>