Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else

[Praveen Kumar]

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