| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 26 Oct 2021 23:06:30 +0530
From: Praveen Kumar <kpraveen.lkml@...il.com>
To: kushal kothari <kushalkothari285@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
fabioaiuto83@...il.com, ross.schm.dev@...il.com,
hdegoede@...hat.com, marcocesati@...il.com,
"Fabio M. De Francesco" <fmdefrancesco@...il.com>,
linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
outreachy-kernel@...glegroups.com,
Mike Rapoport <mike.rapoport@...il.com>,
kushalkothari2850@...il.com
Subject: Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: core: Refactor
nested if-else
On 26-10-2021 20:00, kushal kothari wrote:
>> Can you please check if this works well. Thanks.
>
> checkpatch.pl again warns of
> "WARNING: Too many leading tabs - consider code refactoring"
> when adding another if loop on top .
>
Thanks for checking. The whole intention was to reduce unnecessary memcmp.
These operations are heavy and performing same comparison twice, IMHO needs to be thought well.
But, I'll wait for others to comment on this. Thanks.
Regards,
~Praveen.
> On Tue, Oct 26, 2021 at 7:24 PM Praveen Kumar <kpraveen.lkml@...il.com>
> wrote:
>
>> 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@...il.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.
>>
>>
>
Powered by blists - more mailing lists