Re: [PATCH v2 2/2] staging: rtl8723bs: refactor rtw_joinbss_event_prehandle to reduce indentation

[Dan Carpenter]

On Fri, Mar 20, 2026 at 12:13:35AM +0100, Jose A. Perez de Azpillaga wrote:
> The rtw_joinbss_event_prehandle function has excessive indentation due
> to deeply nested if-statements.
> 
> Refactor the function using early returns and guard clauses for the
> failure paths. This flattens the code and significantly improves
> readability.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@xxxxxxxxx>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 134 +++++++++++-----------
>  1 file changed, 69 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 805427353aa6..2a192c1dd038 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -1175,84 +1175,88 @@ void rtw_joinbss_event_prehandle(struct adapter *adapter, u8 *pbuf)
>  	pmlmepriv->link_detect_info.traffic_transition_count = 0;
>  	pmlmepriv->link_detect_info.low_power_transition_count = 0;
>  
> -	if (pnetwork->join_res > 0) {
> -		spin_lock_bh(&pmlmepriv->scanned_queue.lock);
> -		retry = 0;
> -		if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING)) {
> -			/* s1. find ptarget_wlan */
> -			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> -				if (the_same_macaddr) {
> -					ptarget_wlan = rtw_find_network(&pmlmepriv->scanned_queue, cur_network->network.mac_address);
> -				} else {
> -					pcur_wlan = rtw_find_network(&pmlmepriv->scanned_queue, cur_network->network.mac_address);
> -					if (pcur_wlan)
> -						pcur_wlan->fixed = false;
> -
> -					pcur_sta = rtw_get_stainfo(pstapriv, cur_network->network.mac_address);
> -					if (pcur_sta)
> -						rtw_free_stainfo(adapter,  pcur_sta);
> -
> -					ptarget_wlan = rtw_find_network(&pmlmepriv->scanned_queue, pnetwork->network.mac_address);
> -					if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> -						if (ptarget_wlan)
> -							ptarget_wlan->fixed = true;
> -					}
> -				}
> +	if (pnetwork->join_res == -4) {
> +		rtw_reset_securitypriv(adapter);
> +		_set_timer(&pmlmepriv->assoc_timer, 1);
>  
> -			} else {
> -				ptarget_wlan = _rtw_find_same_network(&pmlmepriv->scanned_queue, pnetwork);
> -				if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> -					if (ptarget_wlan)
> -						ptarget_wlan->fixed = true;
> -				}
> -			}
> +		if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING))
> +			_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
>  
> -			/* s2. update cur_network */
> -			if (ptarget_wlan) {
> -				rtw_joinbss_update_network(adapter, ptarget_wlan, pnetwork);
> -			} else {
> -				netdev_dbg(adapter->pnetdev,
> -					   "Can't find ptarget_wlan when joinbss_event callback\n");
> -				spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> -				goto ignore_joinbss_callback;
> -			}
> +		goto ignore_joinbss_callback;
> +	}
>  
> -			/* s3. find ptarget_sta & update ptarget_sta after update cur_network only for station mode */
> -			if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> -				ptarget_sta = rtw_joinbss_update_stainfo(adapter, pnetwork);
> -				if (!ptarget_sta) {
> -					spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> -					goto ignore_joinbss_callback;
> -				}
> -			}
> +	if (pnetwork->join_res <= 0) {

This preserves how the existing code works, but originally there was
a comment here which said that join_res could only be < 0.  Emotionally,
I would prefer if zero were treated as success.  Can join_res actually
be zero?

Update: I looked and join_res can be zero.  It comes from:

	res = pmlmeinfo->aid = (int)(le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 4))&0x3fff);

in the OnAssocRsp() function.  I guess preserve the <= 0, but also
preserve the comment.

> +		_set_timer(&pmlmepriv->assoc_timer, 1);
> +		_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
> +		goto ignore_joinbss_callback;
> +	}
> +
> +	spin_lock_bh(&pmlmepriv->scanned_queue.lock);
> +	retry = 0;
> +
> +	if (!check_fwstate(pmlmepriv, _FW_UNDER_LINKING)) {
> +		spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> +		goto ignore_joinbss_callback;
> +	}
> +
> +	/* s1. find ptarget_wlan */
> +	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> +		if (the_same_macaddr) {
> +			ptarget_wlan = rtw_find_network(&pmlmepriv->scanned_queue, cur_network->network.mac_address);
> +		} else {
> +			pcur_wlan = rtw_find_network(&pmlmepriv->scanned_queue, cur_network->network.mac_address);
> +			if (pcur_wlan)
> +				pcur_wlan->fixed = false;
> +
> +			pcur_sta = rtw_get_stainfo(pstapriv, cur_network->network.mac_address);
> +			if (pcur_sta)
> +				rtw_free_stainfo(adapter, pcur_sta);
>  
> -			/* s4. indicate connect */
> +			ptarget_wlan = rtw_find_network(&pmlmepriv->scanned_queue, pnetwork->network.mac_address);
>  			if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> -				pmlmepriv->cur_network_scanned = ptarget_wlan;
> -				rtw_indicate_connect(adapter);
> +				if (ptarget_wlan)
> +					ptarget_wlan->fixed = true;
>  			}
> +		}
> +	} else {
> +		ptarget_wlan = _rtw_find_same_network(&pmlmepriv->scanned_queue, pnetwork);
> +		if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> +			if (ptarget_wlan)
> +				ptarget_wlan->fixed = true;
> +		}
> +	}
>  
> -			spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> +	/* s2. update cur_network */
> +	if (!ptarget_wlan) {
> +		netdev_dbg(adapter->pnetdev, "Can't find ptarget_wlan when joinbss_event callback\n");

This line is too long.  In the original code there was a newline after
the comma so it would have been 84 characters.  Try to make as few
unrelated changes as possible.


> +		spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> +		goto ignore_joinbss_callback;
> +	}
>  
> -			spin_unlock_bh(&pmlmepriv->lock);
> -			/* s5. Cancel assoc_timer */
> -			timer_delete_sync(&pmlmepriv->assoc_timer);
> -			spin_lock_bh(&pmlmepriv->lock);
> -		} else {
> +	rtw_joinbss_update_network(adapter, ptarget_wlan, pnetwork);
> +
> +	/* s3. find ptarget_sta & update ptarget_sta after update cur_network only for station mode */
> +	if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> +		ptarget_sta = rtw_joinbss_update_stainfo(adapter, pnetwork);
> +		if (!ptarget_sta) {
>  			spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> +			goto ignore_joinbss_callback;
>  		}
> -	} else if (pnetwork->join_res == -4) {
> -		rtw_reset_securitypriv(adapter);
> -		_set_timer(&pmlmepriv->assoc_timer, 1);
> -
> -		if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING))
> -			_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
> +	}
>  
> -	} else {/* if join_res < 0 (join fails), then try again */
> -		_set_timer(&pmlmepriv->assoc_timer, 1);
> -		_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
> +	/* s4. indicate connect */
> +	if (check_fwstate(pmlmepriv, WIFI_STATION_STATE)) {
> +		pmlmepriv->cur_network_scanned = ptarget_wlan;
> +		rtw_indicate_connect(adapter);
>  	}
>  
> +	spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
> +
> +	/* s5. Cancel assoc_timer */

Put this comment after the spinlock (the way it was in the original
code).

> +	spin_unlock_bh(&pmlmepriv->lock);
> +	timer_delete_sync(&pmlmepriv->assoc_timer);
> +	return;

Originally, we took a lock and then fell through to the unlock after
the ignore_joinbss_callback label.  Which is obviously bogus so your
return is better.  I also think it might be even better yet to get
rid of the ignore_joinbss_callback label entirely.  A simple

	spin_unlock_bh(&pmlmepriv->lock);
	return;

Is only one more line of code, but it's much more readable.

regards,
dan carpenter

> +
>  ignore_joinbss_callback:
>  
>  	spin_unlock_bh(&pmlmepriv->lock);