Re: [PATCH v3 2/5] staging: rtl8723bs: rtw_mlme: fix lines exceeding 100 columns

[Dan Carpenter <error27@gmail.com>]

On Mon, Apr 27, 2026 at 01:54:59AM +0300, Salman Alghamdi wrote:
> Wrap long lines and extract local variables to bring all lines
> within the 100 column limit.
> 
> Signed-off-by: Salman Alghamdi <me@cipherat.com>
> ---

This patch is too big and too complicated.  If you send a patch that
only adds newlines, then I have automated ways to review that but
when you're adding variables and change code to use min_t() then
it's hard to review.


>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 284 +++++++++++++++-------
>  1 file changed, 201 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 268f294528e6..cdc631464565 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -52,7 +52,10 @@ int	rtw_init_mlme_priv(struct adapter *padapter)
>  	pmlmepriv->pscanned = NULL;
>  	pmlmepriv->fw_state = WIFI_STATION_STATE; /*  Must sync with rtw_wdev_alloc() */
>  	pmlmepriv->cur_network.network.infrastructure_mode = Ndis802_11AutoUnknown;
> -	pmlmepriv->scan_mode = SCAN_ACTIVE;/*  1: active, 0: passive. Maybe someday we should rename this varable to "active_mode" (Jeff) */
> +	/*  1: active, 0: passive. Maybe someday we should rename
> +	 *  this varable to "active_mode" (Jeff)
> +	 */
> +	pmlmepriv->scan_mode = SCAN_ACTIVE;
>  
>  	spin_lock_init(&pmlmepriv->lock);
>  	INIT_LIST_HEAD(&pmlmepriv->free_bss_pool.queue);
> @@ -125,7 +128,8 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv)
>  	rtw_free_mlme_ie_data(&pmlmepriv->p2p_beacon_ie, &pmlmepriv->p2p_beacon_ie_len);
>  	rtw_free_mlme_ie_data(&pmlmepriv->p2p_probe_req_ie, &pmlmepriv->p2p_probe_req_ie_len);
>  	rtw_free_mlme_ie_data(&pmlmepriv->p2p_probe_resp_ie, &pmlmepriv->p2p_probe_resp_ie_len);
> -	rtw_free_mlme_ie_data(&pmlmepriv->p2p_go_probe_resp_ie, &pmlmepriv->p2p_go_probe_resp_ie_len);
> +	rtw_free_mlme_ie_data(&pmlmepriv->p2p_go_probe_resp_ie,
> +			      &pmlmepriv->p2p_go_probe_resp_ie_len);
>  	rtw_free_mlme_ie_data(&pmlmepriv->p2p_assoc_req_ie, &pmlmepriv->p2p_assoc_req_ie_len);
>  }
>  
> @@ -369,7 +373,8 @@ int is_same_network(struct wlan_bssid_ex *src, struct wlan_bssid_ex *dst, u8 fea
>  			(d_cap & WLAN_CAPABILITY_ESS));
>  }
>  
> -struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue, struct wlan_network *network)
> +struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue,
> +					    struct wlan_network *network)
>  {
>  	struct list_head *phead, *plist;
>  	struct wlan_network *found = NULL;
> @@ -420,7 +425,8 @@ void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
>  	long rssi_final;
>  
>  	/* The rule below is 1/5 for sample value, 4/5 for history value */
> -	if (check_fwstate(&padapter->mlmepriv, _FW_LINKED) && is_same_network(&padapter->mlmepriv.cur_network.network, src, 0)) {
> +	if (check_fwstate(&padapter->mlmepriv, _FW_LINKED) &&
> +	    is_same_network(&padapter->mlmepriv.cur_network.network, src, 0)) {
>  		/* Take the recvpriv's value for the connected AP*/
>  		ss_final = padapter->recvpriv.signal_strength;
>  		sq_final = padapter->recvpriv.signal_qual;
> @@ -431,11 +437,15 @@ void update_network(struct wlan_bssid_ex *dst, struct wlan_bssid_ex *src,
>  			rssi_final = rssi_ori;
>  	} else {
>  		if (sq_smp != 101) { /* from the right channel */
> -			ss_final = ((u32)(src->phy_info.signal_strength) + (u32)(dst->phy_info.signal_strength) * 4) / 5;
> -			sq_final = ((u32)(src->phy_info.signal_quality) + (u32)(dst->phy_info.signal_quality) * 4) / 5;
> +			ss_final = ((u32)(src->phy_info.signal_strength) +
> +				    (u32)(dst->phy_info.signal_strength) * 4) / 5;
> +			sq_final = ((u32)(src->phy_info.signal_quality) +
> +				    (u32)(dst->phy_info.signal_quality) * 4) / 5;
>  			rssi_final = (src->rssi + dst->rssi * 4) / 5;
>  		} else {
> -			/* bss info not receiving from the right channel, use the original RX signal infos */
> +			/* bss info not receiving from the right channel, use
> +			 * the original RX signal infos
> +			 */
>  			ss_final = dst->phy_info.signal_strength;
>  			sq_final = dst->phy_info.signal_quality;
>  			rssi_final = dst->rssi;
> @@ -462,13 +472,18 @@ static void update_current_network(struct adapter *adapter, struct wlan_bssid_ex
>  		&pmlmepriv->cur_network.network,
>  		&pmlmepriv->cur_network.network);
>  
> -	if (check_fwstate(pmlmepriv, _FW_LINKED) && (is_same_network(&pmlmepriv->cur_network.network, pnetwork, 0))) {
> +	if (check_fwstate(pmlmepriv, _FW_LINKED) &&
> +	    (is_same_network(&pmlmepriv->cur_network.network, pnetwork, 0))) {
> +		u32 ie_len;
> +		u8 *ie;
> +
>  		update_network(&pmlmepriv->cur_network.network, pnetwork, adapter, true);
> -		if (pmlmepriv->cur_network.network.ie_length < sizeof(struct ndis_802_11_fix_ie))
> +		ie_len = pmlmepriv->cur_network.network.ie_length;
> +		if (ie_len < sizeof(struct ndis_802_11_fix_ie))
>  			return;
>  
> -		rtw_update_protection(adapter, (pmlmepriv->cur_network.network.ies) + sizeof(struct ndis_802_11_fix_ie),
> -								pmlmepriv->cur_network.network.ie_length - sizeof(struct ndis_802_11_fix_ie));
> +		ie = pmlmepriv->cur_network.network.ies + sizeof(struct ndis_802_11_fix_ie);
> +		rtw_update_protection(adapter, ie, ie_len - sizeof(struct ndis_802_11_fix_ie));
>  	}
>  }
>  
> @@ -604,7 +619,16 @@ static bool rtw_is_desired_network(struct adapter *adapter, struct wlan_network
>  	privacy = pnetwork->network.privacy;
>  
>  	if (check_fwstate(pmlmepriv, WIFI_UNDER_WPS)) {
> -		if (rtw_get_wps_ie(pnetwork->network.ies + _FIXED_IE_LENGTH_, pnetwork->network.ie_length - _FIXED_IE_LENGTH_, NULL, &wps_ielen))
> +		u8 *ie_ptr;
> +		u32 ie_len_val;

Why is this not call ie and ie_len like the earlier code?
Also the declarations should be in reverse Christmas tree
order.

	long long variable_name;
	medium name;
	short n;

When you start doing things which are more complicated than
just adding newlines, then it gets controversial in ways you
don't expect.  For example, we don't really use min_t() these
days.

> +
> +		if (pnetwork->network.ie_length < _FIXED_IE_LENGTH_)
> +			return false;

This is a behavior change so we can't merge it.

regards,
dan carpenter

> +
> +		ie_ptr = pnetwork->network.ies + _FIXED_IE_LENGTH_;
> +		ie_len_val = pnetwork->network.ie_length - _FIXED_IE_LENGTH_;
> +
> +		if (rtw_get_wps_ie(ie_ptr, ie_len_val, NULL, &wps_ielen))
>  			return true;
>  		else
>  			return false;