* [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
@ 2021-10-26 13:42 Kushal Kothari
2021-10-26 13:54 ` [Outreachy kernel] " Praveen Kumar
2021-10-26 14:19 ` Mike Rapoport
0 siblings, 2 replies; 5+ messages in thread
From: Kushal Kothari @ 2021-10-26 13:42 UTC (permalink / raw)
To: gregkh, fabioaiuto83, ross.schm.dev, hdegoede, marcocesati,
fmdefrancesco, linux-staging, linux-kernel, outreachy-kernel,
mike.rapoport, kushalkothari2850
Cc: Kushal Kothari
Refactor nested if-else to avoid deep indentations. There is no change
in the logic of the new code, however, now it is simple because it gets
rid of five unnecessary else conditionals and it combines nested if into
single if-else-if. This refactor also leads to fix warning detected by
checkpatch.pl:
WARNING: Too many leading tabs - consider code refactoring
Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com>
---
Changes in v2: Fix the bug of not handling properly the else logic
when p is not null in else-if. Also, reword the subject line and break
it up at 72 columns.
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
1 file changed, 29 insertions(+), 40 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 0f82f5031c43..267d853b1514 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
for (;;) {
p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
- if (p) {
- if (!memcmp(p+2, WMM_IE, 6)) {
-
- pstat->flags |= WLAN_STA_WME;
-
- pstat->qos_option = 1;
- pstat->qos_info = *(p+8);
-
- pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
-
- if ((pstat->qos_info&0xf) != 0xf)
- pstat->has_legacy_ac = true;
- else
- pstat->has_legacy_ac = false;
-
- if (pstat->qos_info&0xf) {
- if (pstat->qos_info&BIT(0))
- pstat->uapsd_vo = BIT(0)|BIT(1);
- else
- pstat->uapsd_vo = 0;
-
- if (pstat->qos_info&BIT(1))
- pstat->uapsd_vi = BIT(0)|BIT(1);
- else
- pstat->uapsd_vi = 0;
-
- if (pstat->qos_info&BIT(2))
- pstat->uapsd_bk = BIT(0)|BIT(1);
- else
- pstat->uapsd_bk = 0;
-
- if (pstat->qos_info&BIT(3))
- pstat->uapsd_be = BIT(0)|BIT(1);
- else
- pstat->uapsd_be = 0;
-
- }
-
- break;
+ if (p && memcmp(p+2, WMM_IE, 6)) {
+ p = p + ie_len + 2;
+ } else if (p && !memcmp(p+2, WMM_IE, 6)) {
+ pstat->flags |= WLAN_STA_WME;
+ pstat->qos_option = 1;
+ pstat->qos_info = *(p+8);
+ pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
+
+ pstat->has_legacy_ac = false;
+ if ((pstat->qos_info&0xf) != 0xf)
+ pstat->has_legacy_ac = true;
+
+ if (pstat->qos_info&0xf) {
+ pstat->uapsd_vo = 0;
+ if (pstat->qos_info&BIT(0))
+ pstat->uapsd_vo = BIT(0)|BIT(1);
+
+ pstat->uapsd_vi = 0;
+ if (pstat->qos_info&BIT(1))
+ pstat->uapsd_vi = BIT(0)|BIT(1);
+
+ pstat->uapsd_bk = 0;
+ if (pstat->qos_info&BIT(2))
+ pstat->uapsd_bk = BIT(0)|BIT(1);
+
+ pstat->uapsd_be = 0;
+ if (pstat->qos_info&BIT(3))
+ pstat->uapsd_be = BIT(0)|BIT(1);
}
+ break;
} else {
break;
}
- p = p + ie_len + 2;
}
}
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
2021-10-26 13:42 [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else Kushal Kothari
@ 2021-10-26 13:54 ` Praveen Kumar
[not found] ` <CALtHPQsh=0LyM3K9dn8d0X+n4gbx2TApF6k_WhpypHXGj6Hj-w@mail.gmail.com>
2021-10-26 14:19 ` Mike Rapoport
1 sibling, 1 reply; 5+ messages in thread
From: Praveen Kumar @ 2021-10-26 13:54 UTC (permalink / raw)
To: Kushal Kothari, gregkh, fabioaiuto83, ross.schm.dev, hdegoede,
marcocesati, fmdefrancesco, linux-staging, linux-kernel,
outreachy-kernel, mike.rapoport, kushalkothari2850
On 26-10-2021 19:12, Kushal Kothari wrote:
> Refactor nested if-else to avoid deep indentations. There is no change
> in the logic of the new code, however, now it is simple because it gets
> rid of five unnecessary else conditionals and it combines nested if into
> single if-else-if. This refactor also leads to fix warning detected by
> checkpatch.pl:
> WARNING: Too many leading tabs - consider code refactoring
>
> Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com>
> ---
>
> Changes in v2: Fix the bug of not handling properly the else logic
> when p is not null in else-if. Also, reword the subject line and break
> it up at 72 columns.
>
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
> 1 file changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 0f82f5031c43..267d853b1514 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
> p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
> for (;;) {
> p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> - if (p) {
> - if (!memcmp(p+2, WMM_IE, 6)) {
> -
> - pstat->flags |= WLAN_STA_WME;
> -
> - pstat->qos_option = 1;
> - pstat->qos_info = *(p+8);
> -
> - pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> -
> - if ((pstat->qos_info&0xf) != 0xf)
> - pstat->has_legacy_ac = true;
> - else
> - pstat->has_legacy_ac = false;
> -
> - if (pstat->qos_info&0xf) {
> - if (pstat->qos_info&BIT(0))
> - pstat->uapsd_vo = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_vo = 0;
> -
> - if (pstat->qos_info&BIT(1))
> - pstat->uapsd_vi = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_vi = 0;
> -
> - if (pstat->qos_info&BIT(2))
> - pstat->uapsd_bk = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_bk = 0;
> -
> - if (pstat->qos_info&BIT(3))
> - pstat->uapsd_be = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_be = 0;
> -
> - }
> -
> - break;
> + if (p && memcmp(p+2, WMM_IE, 6)) {
> + p = p + ie_len + 2;
> + } else if (p && !memcmp(p+2, WMM_IE, 6)) {
I would think of something,
for(;;) {
p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
if (p) {
if (memcmp(p+2, WMM_IE, 6)) {
p = p + ie_len + 2;
continue;
}
pstat->flags |= WLAN_STA_WME;
...
}
/* Here we will reach only if p is NULL or its required entry */
break;
}
Can you please check if this works well. Thanks.
Also, wanted to check how are you testing these changes ?
> + pstat->flags |= WLAN_STA_WME;
> + pstat->qos_option = 1;
> + pstat->qos_info = *(p+8);
> + pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> +
> + pstat->has_legacy_ac = false;
> + if ((pstat->qos_info&0xf) != 0xf)
> + pstat->has_legacy_ac = true;
> +
> + if (pstat->qos_info&0xf) {
> + pstat->uapsd_vo = 0;
> + if (pstat->qos_info&BIT(0))
> + pstat->uapsd_vo = BIT(0)|BIT(1);
> +
> + pstat->uapsd_vi = 0;
> + if (pstat->qos_info&BIT(1))
> + pstat->uapsd_vi = BIT(0)|BIT(1);
> +
> + pstat->uapsd_bk = 0;
> + if (pstat->qos_info&BIT(2))
> + pstat->uapsd_bk = BIT(0)|BIT(1);
> +
> + pstat->uapsd_be = 0;
> + if (pstat->qos_info&BIT(3))
> + pstat->uapsd_be = BIT(0)|BIT(1);
> }
> + break;
> } else {
> break;
> }
> - p = p + ie_len + 2;
> }
> }
>
>
Regards,
~Praveen.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
2021-10-26 13:42 [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else Kushal Kothari
2021-10-26 13:54 ` [Outreachy kernel] " Praveen Kumar
@ 2021-10-26 14:19 ` Mike Rapoport
[not found] ` <CALtHPQuKDa23Ui4wBVed=7FOkf6u+-MjL1VU-4CD6hXSjmsq6g@mail.gmail.com>
1 sibling, 1 reply; 5+ messages in thread
From: Mike Rapoport @ 2021-10-26 14:19 UTC (permalink / raw)
To: Kushal Kothari
Cc: gregkh, fabioaiuto83, ross.schm.dev, hdegoede, marcocesati,
fmdefrancesco, linux-staging, linux-kernel, outreachy-kernel,
kushalkothari2850
On Tue, Oct 26, 2021 at 07:12:53PM +0530, Kushal Kothari wrote:
> Refactor nested if-else to avoid deep indentations. There is no change
> in the logic of the new code, however, now it is simple because it gets
> rid of five unnecessary else conditionals and it combines nested if into
> single if-else-if. This refactor also leads to fix warning detected by
> checkpatch.pl:
> WARNING: Too many leading tabs - consider code refactoring
>
> Signed-off-by: Kushal Kothari <kushalkothari285@gmail.com>
> ---
>
> Changes in v2: Fix the bug of not handling properly the else logic
> when p is not null in else-if. Also, reword the subject line and break
> it up at 72 columns.
>
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
> 1 file changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 0f82f5031c43..267d853b1514 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
> p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
> for (;;) {
> p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> - if (p) {
> - if (!memcmp(p+2, WMM_IE, 6)) {
> -
> - pstat->flags |= WLAN_STA_WME;
> -
> - pstat->qos_option = 1;
> - pstat->qos_info = *(p+8);
> -
> - pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> -
> - if ((pstat->qos_info&0xf) != 0xf)
> - pstat->has_legacy_ac = true;
> - else
> - pstat->has_legacy_ac = false;
> -
> - if (pstat->qos_info&0xf) {
> - if (pstat->qos_info&BIT(0))
> - pstat->uapsd_vo = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_vo = 0;
> -
> - if (pstat->qos_info&BIT(1))
> - pstat->uapsd_vi = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_vi = 0;
> -
> - if (pstat->qos_info&BIT(2))
> - pstat->uapsd_bk = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_bk = 0;
> -
> - if (pstat->qos_info&BIT(3))
> - pstat->uapsd_be = BIT(0)|BIT(1);
> - else
> - pstat->uapsd_be = 0;
> -
> - }
> -
> - break;
> + if (p && memcmp(p+2, WMM_IE, 6)) {
> + p = p + ie_len + 2;
> + } else if (p && !memcmp(p+2, WMM_IE, 6)) {
> + pstat->flags |= WLAN_STA_WME;
> + pstat->qos_option = 1;
> + pstat->qos_info = *(p+8);
> + pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> +
> + pstat->has_legacy_ac = false;
> + if ((pstat->qos_info&0xf) != 0xf)
> + pstat->has_legacy_ac = true;
> +
> + if (pstat->qos_info&0xf) {
> + pstat->uapsd_vo = 0;
This variable and other variables below are set to 0 just before the loop,
so the initialization here can be removed.
> + if (pstat->qos_info&BIT(0))
> + pstat->uapsd_vo = BIT(0)|BIT(1);
> +
> + pstat->uapsd_vi = 0;
> + if (pstat->qos_info&BIT(1))
> + pstat->uapsd_vi = BIT(0)|BIT(1);
> +
> + pstat->uapsd_bk = 0;
> + if (pstat->qos_info&BIT(2))
> + pstat->uapsd_bk = BIT(0)|BIT(1);
> +
> + pstat->uapsd_be = 0;
> + if (pstat->qos_info&BIT(3))
> + pstat->uapsd_be = BIT(0)|BIT(1);
> }
> + break;
> } else {
> break;
> }
> - p = p + ie_len + 2;
> }
> }
>
> --
> 2.25.1
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-26 17:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-26 13:42 [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else Kushal Kothari
2021-10-26 13:54 ` [Outreachy kernel] " Praveen Kumar
[not found] ` <CALtHPQsh=0LyM3K9dn8d0X+n4gbx2TApF6k_WhpypHXGj6Hj-w@mail.gmail.com>
2021-10-26 17:36 ` Praveen Kumar
2021-10-26 14:19 ` Mike Rapoport
[not found] ` <CALtHPQuKDa23Ui4wBVed=7FOkf6u+-MjL1VU-4CD6hXSjmsq6g@mail.gmail.com>
2021-10-26 15:15 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®