Re: [PATCH] staging: rtl8723bs: fix network selection and A-MPDU reordering in rtw_mlme.c

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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