linux-kernel - Re: [PATCH] staging: rtl8723bs: simplify control flow

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YlYPuIMr8mq66Lea@iweiny-desk3>
Date:   Tue, 12 Apr 2022 16:48:08 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Sevinj Aghayeva <sevinj.aghayeva@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev
Subject: Re: [PATCH] staging: rtl8723bs: simplify control flow

On Sun, Apr 03, 2022 at 06:42:07PM -0400, Sevinj Aghayeva wrote:
> Checkpatch issues "WARNING: else is not generally useful after a break
> or return" for the following code:
> 
> while (1) {
> 	do_join_r = rtw_do_join(padapter);
> 	if (do_join_r == _SUCCESS) {
> 		break;
> 	} else {
> 		rtw_dec_to_roam(padapter);
> 
> 		if (rtw_to_roam(padapter) > 0) {
> 			continue;
> 		} else {
> 			rtw_indicate_disconnect(padapter);
> 			break;
> 		}
> 	}
> }
> 
> We simplify this code in multiple steps. First, we remove do_join_r

I can't say how Greg would like to see a change like this but my gut says that
each of these steps should be a patch in a series...

> variable because it is only used right after it is assigned. Second,
> we remove the unnecessary else statement right after break:
> 
> while (1) {
> 	if (rtw_do_join(padapter) == _SUCCESS)
> 		break;
> 	rtw_dec_to_roam(padapter);
> 
> 	if (rtw_to_roam(padapter) > 0) {
> 		continue;
> 	} else {
> 		rtw_indicate_disconnect(padapter);
> 		break;
> 	}
> }
> 
> Next, we move the call to rtw_do_join into the while test because the
> while will loop only until the call is successful:
> 
> while (rtw_do_join(padapter) != _SUCCESS) {
> 	rtw_dec_to_roam(padapter);
> 	if (rtw_to_roam(padapter) > 0) {
> 		continue;
> 	} else {
> 		rtw_indicate_disconnect(padapter);
> 		break;
> 	}
> }
> 
> Finally, looking at the code above, it is clear that the code will
> break out of the loop if rtw_to_roam call is <= 0. Hence:
> 
> while (rtw_do_join(padapter) != _SUCCESS) {
> 	rtw_dec_to_roam(padapter);
> 	if (rtw_to_roam(padapter) <= 0) {
> 		rtw_indicate_disconnect(padapter);
> 		break;
> 	}
> }

...  that said, this commit message made reviewing the change much easier,
thanks.

Did you submit a patch for the r8188eu driver too?  I just noticed it has a
similar loop in _rtw_roaming().

Reviewed-by: Ira Weiny <ira.weiny@...el.com>

> 
> Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva@...il.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 3eacf8f9d236..a45df775d535 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -2594,30 +2594,20 @@ void _rtw_roaming(struct adapter *padapter, struct wlan_network *tgt_network)
>  {
>  	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>  	struct wlan_network *cur_network = &pmlmepriv->cur_network;
> -	int do_join_r;
>  
>  	if (rtw_to_roam(padapter) > 0) {
>  		memcpy(&pmlmepriv->assoc_ssid, &cur_network->network.ssid, sizeof(struct ndis_802_11_ssid));
>  
>  		pmlmepriv->assoc_by_bssid = false;
>  
> -		while (1) {
> -			do_join_r = rtw_do_join(padapter);
> -			if (do_join_r == _SUCCESS) {
> +		while (rtw_do_join(padapter) != _SUCCESS) {
> +			rtw_dec_to_roam(padapter);
> +			if (rtw_to_roam(padapter) <= 0) {
> +				rtw_indicate_disconnect(padapter);
>  				break;
> -			} else {
> -				rtw_dec_to_roam(padapter);
> -
> -				if (rtw_to_roam(padapter) > 0) {
> -					continue;
> -				} else {
> -					rtw_indicate_disconnect(padapter);
> -					break;
> -				}
>  			}
>  		}
>  	}
> -
>  }
>  
>  signed int rtw_linked_check(struct adapter *padapter)
> -- 
> 2.25.1
>