linux-kernel - Re: [PATCH] staging: rtl8723bs: fix network selection and A-MPDU reordering in rtw

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: <2024122411--0ad4@gregkh>
Date: Tue, 24 Dec 2024 09:02:03 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Atharva Tiwari <evepolonium@...il.com>
Cc: Meir Elisha <meir6264@...il.com>,
	Philipp Hortmann <philipp.g.hortmann@...il.com>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: fix network selection and A-MPDU
 reordering in rtw_mlme.c

On Tue, Dec 24, 2024 at 12:57:52PM +0530, Atharva Tiwari wrote:
> this patch fixes the network selection logic to avoid selecting a network with the same ESSID as the oldest scanned network
> if it was scanned within the last second. it also improves A-MPDU reordering bu enabling it only if the AP supports it,and disabling it otherwise

What commit id does this fix?

And please wrap your changelog at 72 columns like your editor asked you
to do when making the commit :)

> and also i have a question what does "new enough" mean on line 481?

This doesn't belong in a changelog, but rather below the --- line,
right?

> 
> Signed-off-by: Atharva Tiwari <evepolonium@...il.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 28 +++++++++++++++--------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 5ded183aa08c..b33846f88680 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -481,7 +481,9 @@ void rtw_update_scanned_network(struct adapter *adapter, struct wlan_bssid_ex *t
>  		}
>  
>  		if (rtw_roam_flags(adapter)) {
> -			/* TODO: don't select network in the same ess as oldest if it's new enough*/
> +			if (is_same_ess(&pnetwork->network, &oldest->network) &&
> +				time_after(pnetwork->last_scanned, (unsigned long)msecs_to_jiffies(1000)))

Where did this magic number come from?

> +				continue;
>  		}
>  
>  		if (!oldest || time_after(oldest->last_scanned, pnetwork->last_scanned))
> @@ -1000,15 +1002,23 @@ static struct sta_info *rtw_joinbss_update_stainfo(struct adapter *padapter, str
>  
>  		/* for A-MPDU Rx reordering buffer control for bmc_sta & sta_info */
>  		/* if A-MPDU Rx is enabled, resetting  rx_ordering_ctrl wstart_b(indicate_seq) to default value = 0xffff */
> -		/* todo: check if AP can send A-MPDU packets */
> -		for (i = 0; i < 16 ; i++) {
> -			preorder_ctrl = &psta->recvreorder_ctrl[i];
> -			preorder_ctrl->enable = false;
> -			preorder_ctrl->indicate_seq = 0xffff;
> -			preorder_ctrl->wend_b = 0xffff;
> -			preorder_ctrl->wsize_b = 64;/* max_ampdu_sz;ex. 32(kbytes) -> wsize_b =32 */
> +		if (rtw_check_ap_supports_ampdu(pnetwork)) {
> +			for (i = 0; i < 16 ; i++) {
> +				preorder_ctrl = &psta->recvreorder_ctrl[i];
> +				preorder_ctrl->enable = true; /* Enable A-MPDU reordering */
> +				preorder_ctrl->indicate_seq = 0; /* Starting sequence number */
> +				preorder_ctrl->wend_b = 0xffff;
> +				preorder_ctrl->wsize_b = 64;/* max_ampdu_sz;ex. 32(kbytes) -> wsize_b =32 */
> +			}
> +		} else {
> +			for (i = 0; i < 16; i++) {
> +				preorder_ctrl = &psta->recvreorder_ctrl[i];
> +				preorder_ctrl->enable = false;
> +				preorder_ctrl->indicate_seq = 0xffff;

So only two fields are different, right?  Why not just set those in a
separate loop instead?

> +				preorder_ctrl->wend_b = 0xffff;
> +				preorder_ctrl->wsize_b = 64;  /* max_ampdu_sz; adjust as needed */

When is this "needed"?

And you have tested all of this, right?

thanks,

greg k-h