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

Salman Alghamdi
2026-04-26T02:33:36Z

> 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?

>> - 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?

>> @@ -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)?

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

Cheers,
Salman Alghamdi.