* Re: [PATCH v2 2/2] staging: rtl8723bs: refactor rtw_joinbss_event_prehandle to reduce indentation
2026-03-19 23:13 ` [PATCH v2 2/2] staging: rtl8723bs: refactor rtw_joinbss_event_prehandle to reduce indentation Jose A. Perez de Azpillaga
@ 2026-03-20 8:05 ` Luka Gejak
2026-03-20 9:40 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Luka Gejak @ 2026-03-20 8:05 UTC (permalink / raw)
To: Jose A. Perez de Azpillaga, linux-staging
Cc: Greg Kroah-Hartman, Michael Straube, Hans de Goede,
Khushal Chitturi, Dan Carpenter, Ethan Tidmore,
Vivek BalachandharTN, Artur Stupa, Zhuoheng Li, Nino Zhang,
linux-kernel
On March 20, 2026 12:13:35 AM GMT+01:00, "Jose A. Perez de Azpillaga" <azpijr@gmail.com> 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@gmail.com>
>---
> 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) {
>+ _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");
>+ 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 */
>+ spin_unlock_bh(&pmlmepriv->lock);
>+ timer_delete_sync(&pmlmepriv->assoc_timer);
>+ return;
>+
> ignore_joinbss_callback:
>
> spin_unlock_bh(&pmlmepriv->lock);
One minor note: Since the REJOIN logic is now gone, the static u8 retry
variable at line 1150 is now only assigned to 0 and never used. It
doesn't break the build thanks to __maybe_unused, but it should be
removed in a v3 of the series. Also include everyone from To and Cc for
cover letter email as well.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 2/2] staging: rtl8723bs: refactor rtw_joinbss_event_prehandle to reduce indentation
2026-03-19 23:13 ` [PATCH v2 2/2] staging: rtl8723bs: refactor rtw_joinbss_event_prehandle to reduce indentation Jose A. Perez de Azpillaga
2026-03-20 8:05 ` Luka Gejak
@ 2026-03-20 9:40 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2026-03-20 9:40 UTC (permalink / raw)
To: Jose A. Perez de Azpillaga
Cc: linux-staging, Greg Kroah-Hartman, Michael Straube,
Hans de Goede, Khushal Chitturi, Ethan Tidmore,
Vivek BalachandharTN, Luka Gejak, Artur Stupa, Zhuoheng Li,
Nino Zhang, linux-kernel
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@gmail.com>
> ---
> 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);
^ permalink raw reply [flat|nested] 4+ messages in thread