lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7e94b75e-6959-75c-ab4f-58147fc37dc@inria.fr>
Date: Sat, 5 Apr 2025 09:30:29 -0400 (EDT)
From: Julia Lawall <julia.lawall@...ia.fr>
To: Abraham Samuel Adekunle <abrahamadekunle50@...il.com>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, outreachy@...ts.linux.dev, 
    linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] staging: rtl8723bs: Prevent duplicate NULL tests on
 a value



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@...il.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
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ