Re: [PATCH v2] staging: rtl8723bs: rtw_mlme: fix line length warnings

[Luka Gejak <luka.gejak@linux.dev>]

On April 26, 2026 4:33:23 AM GMT+02:00, Salman Alghamdi <me@cipherat.com> wrote:
>> glad to see v2, just one thing before inline reviews of the code. You 
>> don't have to state what did you do to make lines shorter than 100 
>> characters, because we can read the diff and this commit message is too 
>> long for cleanup patch.
>>
>
>Understood. Will keep it shorter in v3.
>
>>>  }
>>>  
>>> -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)
>> 					    ^^^^^
>> This is still not aligned correctly. Please do it like:
>> struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue,
>> 					    struct wlan_network *network)
>>
>
>Will investigate and fix the alignment issues in v3.
>
>>> @@ -462,10 +470,12 @@ 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))) {
>>> +	u8 *ie = pmlmepriv->cur_network.network.ies + sizeof(struct ndis_802_11_fix_ie);
>>
>> You should declare u8 *ie inside the if statement to reduce its scope as
>> much as possible and if left this way it will declare it even if 
>> condition is false.
>>
>>> +
>>> +	if (check_fwstate(pmlmepriv, _FW_LINKED) &&
>>> +	    is_same_network(&pmlmepriv->cur_network.network, pnetwork, 0)) {
>>>  		update_network(&pmlmepriv->cur_network.network, pnetwork, adapter, true);
>>
>> Put u8 *ie ... here, under update_network.
>>
>
>Should variables always be declared at the nearest enclosing scope
>where they're used, or at the top of the function?

Generally nearest scope, but in this case you should declare u8 *ie 
inside the block and after update_network(), because by doing that you
ensure the pointer is initialized with the freshest data. It also 
prevents the variable from being used elsewhere in the function where 
it might be invalid.

>
>>> -		rtw_update_protection(adapter, (pmlmepriv->cur_network.network.ies) + sizeof(struct ndis_802_11_fix_ie),
>>> -								pmlmepriv->cur_network.network.ie_length);
>>> +		rtw_update_protection(adapter, ie, pmlmepriv->cur_network.network.ie_length);
>>>  	}
>>>  }
>>>  
>>> @@ -600,12 +610,16 @@ static bool rtw_is_desired_network(struct adapter *adapter, struct wlan_network
>>>  	desired_encmode = psecuritypriv->ndisencryptstatus;
>>>  	privacy = pnetwork->network.privacy;
>>>  
>>> +	u8 *ie_ptr = pnetwork->network.ies + _FIXED_IE_LENGTH_;
>>> +	u32 ie_len_val = pnetwork->network.ie_length - _FIXED_IE_LENGTH_;
>>> +
>>>  	if (check_fwstate(pmlmepriv, WIFI_UNDER_WPS)) {
>>
>> Again you should move two variables declared above here, inside if 
>> statement. Additionally, calculating ie_len_val outside the check is 
>> risky. You should ensure ie_length is actually greater than 
>> _FIXED_IE_LENGTH_ before performing the subtraction to prevent an 
>> unsigned underflow/wrap-around.
>>
>
>Should the guard be a simple early return if ie_length is smaller
>than the offset, or is there a preferred pattern for this?

Sure, you can go with early returns.

>
>>> @@ -1072,10 +1113,13 @@ static void rtw_joinbss_update_network(struct adapter *padapter, struct wlan_net
>>>  			break;
>>>  	}
>>>  
>>> -	rtw_update_protection(padapter, (cur_network->network.ies) + sizeof(struct ndis_802_11_fix_ie),
>>> -									(cur_network->network.ie_length));
>>> +	u8 *ie = cur_network->network.ies + sizeof(struct ndis_802_11_fix_ie);
>>>  
>>> -	rtw_update_ht_cap(padapter, cur_network->network.ies, cur_network->network.ie_length, (u8) cur_network->network.configuration.ds_config);
>>> +	rtw_update_protection(padapter, ie, (cur_network->network.ie_length));
>>
>> The original code had a potential buffer over-read here by passing the 
>> full ie_length with an offset pointer. Since you are refactoring this 
>> line, please fix this bug, in a separate patch, by subtracting the 
>> offset from the length and ensuring the length is actually large enough 
>> first. 1/X of the patch series should be bug fix, 2/X should be this 
>> cleanup. Don't forget to add Fixes and Reported-by tags. Check if this 
>> bug is present in stable tree and if so add Cc: stable@vger.kernel.org 
>> as well.
>>
>
>Should this bug fix be in the same patch that will fix previously
>mentioned subtraction underflow(s)?

No, bug is a separate patch that goes first in the series as it was 
already there. Your subtraction check should be added when you add the
variables as they are directly related to them and we don't want one 
patch to fix the other one. Also how would you add the checks, if 
variables don't exist yet. Bugfix patch should only fix the bug, 
nothing else, and then next patch will introduce variables while 
fixing long lines.

>
>>> @@ -1684,7 +1764,9 @@ static int rtw_check_roaming_candidate(struct mlme_priv *mlme
>>>  	if (jiffies_to_msecs(jiffies - competitor->last_scanned) >= mlme->roam_scanr_exp_ms)
>>>  		goto exit;
>>>  
>>> -	if (competitor->network.rssi - mlme->cur_network_scanned->network.rssi < mlme->roam_rssi_diff_th)
>>> +	long rssi_diff = competitor->network.rssi - mlme->cur_network_scanned->network.rssi;
>>
>> You are declaring a variable (rssi_diff) after executable code again. 
>> All declarations must be at the top of the function. Additionally, why 
>> use long here?
>>
>
>The type of rssi in struct wlan_bssid_ex is long so I matched it.
>Will double check and correct if needed.
>

You should always double-check :).

>Cheers,
>Salman Alghamdi.