From: Hans de Goede <hdegoede@redhat.com>
To: Fabio Aiuto <fabioaiuto83@gmail.com>
Cc: gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code
Date: Tue, 22 Jun 2021 11:19:36 +0200 [thread overview]
Message-ID: <4f128bd2-b84b-132e-c75f-0030701a8a9b@redhat.com> (raw)
In-Reply-To: <20210622091602.GA1426@agape.jhs>
Hi Fabio,
On 6/22/21 11:16 AM, Fabio Aiuto wrote:
> Hello Hans,
>
> On Mon, Jun 21, 2021 at 11:45:39AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 6/19/21 12:47 PM, Fabio Aiuto wrote:
>>> remove VHT dead code, as the device doesn't support
>>> VHT (which is a 802.11ac capability).
>>>
>>> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
>>> ---
>>> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 1 -
>>> drivers/staging/rtl8723bs/hal/hal_com.c | 241 -----------
>>> .../staging/rtl8723bs/hal/hal_com_phycfg.c | 383 +-----------------
>>> drivers/staging/rtl8723bs/include/hal_com.h | 62 +--
>>> .../rtl8723bs/include/hal_com_phycfg.h | 4 -
>>> drivers/staging/rtl8723bs/include/ieee80211.h | 45 --
>>> .../staging/rtl8723bs/include/rtl8723b_xmit.h | 21 -
>>> drivers/staging/rtl8723bs/include/rtw_ht.h | 4 -
>>> 8 files changed, 9 insertions(+), 752 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>>> index 285acd3d843b..c128d462c6c7 100644
>>> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>>> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>>> @@ -46,7 +46,6 @@ static struct action_handler OnAction_tbl[] = {
>>> {RTW_WLAN_CATEGORY_UNPROTECTED_WNM, "ACTION_UNPROTECTED_WNM", &DoReserved},
>>> {RTW_WLAN_CATEGORY_SELF_PROTECTED, "ACTION_SELF_PROTECTED", &DoReserved},
>>> {RTW_WLAN_CATEGORY_WMM, "ACTION_WMM", &DoReserved},
>>> - {RTW_WLAN_CATEGORY_VHT, "ACTION_VHT", &DoReserved},
>>> {RTW_WLAN_CATEGORY_P2P, "ACTION_P2P", &DoReserved},
>>> };
>>>
>>> diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
>>> index 7a88447f8294..eebd48438733 100644
>>> --- a/drivers/staging/rtl8723bs/hal/hal_com.c
>>> +++ b/drivers/staging/rtl8723bs/hal/hal_com.c
>>> @@ -295,126 +295,6 @@ u8 MRateToHwRate(u8 rate)
>>> case MGN_MCS31:
>>> ret = DESC_RATEMCS31;
>>> break;
>>> - case MGN_VHT1SS_MCS0:
>>> - ret = DESC_RATEVHTSS1MCS0;
>>> - break;
>>> - case MGN_VHT1SS_MCS1:
>>> - ret = DESC_RATEVHTSS1MCS1;
>>> - break;
>>> - case MGN_VHT1SS_MCS2:
>>> - ret = DESC_RATEVHTSS1MCS2;
>>> - break;
>>> - case MGN_VHT1SS_MCS3:
>>> - ret = DESC_RATEVHTSS1MCS3;
>>> - break;
>>> - case MGN_VHT1SS_MCS4:
>>> - ret = DESC_RATEVHTSS1MCS4;
>>> - break;
>>> - case MGN_VHT1SS_MCS5:
>>> - ret = DESC_RATEVHTSS1MCS5;
>>> - break;
>>> - case MGN_VHT1SS_MCS6:
>>> - ret = DESC_RATEVHTSS1MCS6;
>>> - break;
>>> - case MGN_VHT1SS_MCS7:
>>> - ret = DESC_RATEVHTSS1MCS7;
>>> - break;
>>> - case MGN_VHT1SS_MCS8:
>>> - ret = DESC_RATEVHTSS1MCS8;
>>> - break;
>>> - case MGN_VHT1SS_MCS9:
>>> - ret = DESC_RATEVHTSS1MCS9;
>>> - break;
>>> - case MGN_VHT2SS_MCS0:
>>> - ret = DESC_RATEVHTSS2MCS0;
>>> - break;
>>> - case MGN_VHT2SS_MCS1:
>>> - ret = DESC_RATEVHTSS2MCS1;
>>> - break;
>>> - case MGN_VHT2SS_MCS2:
>>> - ret = DESC_RATEVHTSS2MCS2;
>>> - break;
>>> - case MGN_VHT2SS_MCS3:
>>> - ret = DESC_RATEVHTSS2MCS3;
>>> - break;
>>> - case MGN_VHT2SS_MCS4:
>>> - ret = DESC_RATEVHTSS2MCS4;
>>> - break;
>>> - case MGN_VHT2SS_MCS5:
>>> - ret = DESC_RATEVHTSS2MCS5;
>>> - break;
>>> - case MGN_VHT2SS_MCS6:
>>> - ret = DESC_RATEVHTSS2MCS6;
>>> - break;
>>> - case MGN_VHT2SS_MCS7:
>>> - ret = DESC_RATEVHTSS2MCS7;
>>> - break;
>>> - case MGN_VHT2SS_MCS8:
>>> - ret = DESC_RATEVHTSS2MCS8;
>>> - break;
>>> - case MGN_VHT2SS_MCS9:
>>> - ret = DESC_RATEVHTSS2MCS9;
>>> - break;
>>> - case MGN_VHT3SS_MCS0:
>>> - ret = DESC_RATEVHTSS3MCS0;
>>> - break;
>>> - case MGN_VHT3SS_MCS1:
>>> - ret = DESC_RATEVHTSS3MCS1;
>>> - break;
>>> - case MGN_VHT3SS_MCS2:
>>> - ret = DESC_RATEVHTSS3MCS2;
>>> - break;
>>> - case MGN_VHT3SS_MCS3:
>>> - ret = DESC_RATEVHTSS3MCS3;
>>> - break;
>>> - case MGN_VHT3SS_MCS4:
>>> - ret = DESC_RATEVHTSS3MCS4;
>>> - break;
>>> - case MGN_VHT3SS_MCS5:
>>> - ret = DESC_RATEVHTSS3MCS5;
>>> - break;
>>> - case MGN_VHT3SS_MCS6:
>>> - ret = DESC_RATEVHTSS3MCS6;
>>> - break;
>>> - case MGN_VHT3SS_MCS7:
>>> - ret = DESC_RATEVHTSS3MCS7;
>>> - break;
>>> - case MGN_VHT3SS_MCS8:
>>> - ret = DESC_RATEVHTSS3MCS8;
>>> - break;
>>> - case MGN_VHT3SS_MCS9:
>>> - ret = DESC_RATEVHTSS3MCS9;
>>> - break;
>>> - case MGN_VHT4SS_MCS0:
>>> - ret = DESC_RATEVHTSS4MCS0;
>>> - break;
>>> - case MGN_VHT4SS_MCS1:
>>> - ret = DESC_RATEVHTSS4MCS1;
>>> - break;
>>> - case MGN_VHT4SS_MCS2:
>>> - ret = DESC_RATEVHTSS4MCS2;
>>> - break;
>>> - case MGN_VHT4SS_MCS3:
>>> - ret = DESC_RATEVHTSS4MCS3;
>>> - break;
>>> - case MGN_VHT4SS_MCS4:
>>> - ret = DESC_RATEVHTSS4MCS4;
>>> - break;
>>> - case MGN_VHT4SS_MCS5:
>>> - ret = DESC_RATEVHTSS4MCS5;
>>> - break;
>>> - case MGN_VHT4SS_MCS6:
>>> - ret = DESC_RATEVHTSS4MCS6;
>>> - break;
>>> - case MGN_VHT4SS_MCS7:
>>> - ret = DESC_RATEVHTSS4MCS7;
>>> - break;
>>> - case MGN_VHT4SS_MCS8:
>>> - ret = DESC_RATEVHTSS4MCS8;
>>> - break;
>>> - case MGN_VHT4SS_MCS9:
>>> - ret = DESC_RATEVHTSS4MCS9;
>>> - break;
>>> default:
>>> break;
>>> }
>>> @@ -559,127 +439,6 @@ u8 HwRateToMRate(u8 rate)
>>> case DESC_RATEMCS31:
>>> ret_rate = MGN_MCS31;
>>> break;
>>> - case DESC_RATEVHTSS1MCS0:
>>> - ret_rate = MGN_VHT1SS_MCS0;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS1:
>>> - ret_rate = MGN_VHT1SS_MCS1;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS2:
>>> - ret_rate = MGN_VHT1SS_MCS2;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS3:
>>> - ret_rate = MGN_VHT1SS_MCS3;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS4:
>>> - ret_rate = MGN_VHT1SS_MCS4;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS5:
>>> - ret_rate = MGN_VHT1SS_MCS5;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS6:
>>> - ret_rate = MGN_VHT1SS_MCS6;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS7:
>>> - ret_rate = MGN_VHT1SS_MCS7;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS8:
>>> - ret_rate = MGN_VHT1SS_MCS8;
>>> - break;
>>> - case DESC_RATEVHTSS1MCS9:
>>> - ret_rate = MGN_VHT1SS_MCS9;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS0:
>>> - ret_rate = MGN_VHT2SS_MCS0;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS1:
>>> - ret_rate = MGN_VHT2SS_MCS1;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS2:
>>> - ret_rate = MGN_VHT2SS_MCS2;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS3:
>>> - ret_rate = MGN_VHT2SS_MCS3;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS4:
>>> - ret_rate = MGN_VHT2SS_MCS4;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS5:
>>> - ret_rate = MGN_VHT2SS_MCS5;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS6:
>>> - ret_rate = MGN_VHT2SS_MCS6;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS7:
>>> - ret_rate = MGN_VHT2SS_MCS7;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS8:
>>> - ret_rate = MGN_VHT2SS_MCS8;
>>> - break;
>>> - case DESC_RATEVHTSS2MCS9:
>>> - ret_rate = MGN_VHT2SS_MCS9;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS0:
>>> - ret_rate = MGN_VHT3SS_MCS0;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS1:
>>> - ret_rate = MGN_VHT3SS_MCS1;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS2:
>>> - ret_rate = MGN_VHT3SS_MCS2;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS3:
>>> - ret_rate = MGN_VHT3SS_MCS3;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS4:
>>> - ret_rate = MGN_VHT3SS_MCS4;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS5:
>>> - ret_rate = MGN_VHT3SS_MCS5;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS6:
>>> - ret_rate = MGN_VHT3SS_MCS6;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS7:
>>> - ret_rate = MGN_VHT3SS_MCS7;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS8:
>>> - ret_rate = MGN_VHT3SS_MCS8;
>>> - break;
>>> - case DESC_RATEVHTSS3MCS9:
>>> - ret_rate = MGN_VHT3SS_MCS9;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS0:
>>> - ret_rate = MGN_VHT4SS_MCS0;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS1:
>>> - ret_rate = MGN_VHT4SS_MCS1;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS2:
>>> - ret_rate = MGN_VHT4SS_MCS2;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS3:
>>> - ret_rate = MGN_VHT4SS_MCS3;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS4:
>>> - ret_rate = MGN_VHT4SS_MCS4;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS5:
>>> - ret_rate = MGN_VHT4SS_MCS5;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS6:
>>> - ret_rate = MGN_VHT4SS_MCS6;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS7:
>>> - ret_rate = MGN_VHT4SS_MCS7;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS8:
>>> - ret_rate = MGN_VHT4SS_MCS8;
>>> - break;
>>> - case DESC_RATEVHTSS4MCS9:
>>> - ret_rate = MGN_VHT4SS_MCS9;
>>> - break;
>>> -
>>> default:
>>> break;
>>> }
>>> diff --git a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
>>> index 395eb3b5af71..bb7941aee0c4 100644
>>> --- a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
>>> +++ b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
>>> @@ -39,18 +39,6 @@ u8 PHY_GetTxPowerByRateBase(struct adapter *Adapter, u8 RfPath,
>>> case HT_MCS24_MCS31:
>>> value = pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][5];
>>> break;
>>> - case VHT_1SSMCS0_1SSMCS9:
>>> - value = pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][6];
>>> - break;
>>> - case VHT_2SSMCS0_2SSMCS9:
>>> - value = pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][7];
>>> - break;
>>> - case VHT_3SSMCS0_3SSMCS9:
>>> - value = pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][8];
>>> - break;
>>> - case VHT_4SSMCS0_4SSMCS9:
>>> - value = pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][9];
>>> - break;
>>> default:
>>> break;
>>> }
>>> @@ -91,18 +79,6 @@ phy_SetTxPowerByRateBase(
>>> case HT_MCS24_MCS31:
>>> pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][5] = Value;
>>> break;
>>> - case VHT_1SSMCS0_1SSMCS9:
>>> - pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][6] = Value;
>>> - break;
>>> - case VHT_2SSMCS0_2SSMCS9:
>>> - pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][7] = Value;
>>> - break;
>>> - case VHT_3SSMCS0_3SSMCS9:
>>> - pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][8] = Value;
>>> - break;
>>> - case VHT_4SSMCS0_4SSMCS9:
>>> - pHalData->TxPwrByRateBase2_4G[RfPath][TxNum][9] = Value;
>>> - break;
>>> default:
>>> break;
>>> }
>>> @@ -131,14 +107,6 @@ struct adapter *padapter
>>> base = PHY_GetTxPowerByRate(padapter, path, RF_3TX, MGN_MCS23);
>>> phy_SetTxPowerByRateBase(padapter, path, HT_MCS16_MCS23, RF_3TX, base);
>>>
>>> - base = PHY_GetTxPowerByRate(padapter, path, RF_1TX, MGN_VHT1SS_MCS7);
>>> - phy_SetTxPowerByRateBase(padapter, path, VHT_1SSMCS0_1SSMCS9, RF_1TX, base);
>>> -
>>> - base = PHY_GetTxPowerByRate(padapter, path, RF_2TX, MGN_VHT2SS_MCS7);
>>> - phy_SetTxPowerByRateBase(padapter, path, VHT_2SSMCS0_2SSMCS9, RF_2TX, base);
>>> -
>>> - base = PHY_GetTxPowerByRate(padapter, path, RF_3TX, MGN_VHT3SS_MCS7);
>>> - phy_SetTxPowerByRateBase(padapter, path, VHT_3SSMCS0_3SSMCS9, RF_3TX, base);
>>
>> You are removing a bunch of register writes from a block here, while keeping others,
>> so this does not seem like it is just dead-code removal it feels like you are
>> actually making a functional change.
>
> I looked deeply at what I deed, and I just removed some functions updating tables
> in memory, no register write here. Tables are used to load Tx Power Limit. So
> removing these function calls according with the removal of the VHT enum items
> grants that wrong Power Limits will never be fetched.
>
> Moreover I have been quite conservative, for I left untouched HT indexes above
> 7 which rtl8723bs doesn't support.
>
> So IMO I think this patch is fine as is...
>> Perhaps this entire block can never be executed ?
>
> the block is executed but there's no register write happening. Just
> updating of values which will never be fetched.
Ack, my bad I was under the impression that phy_SetTxPowerByRateBase()
would actually do a register write, but I checked and it just updates
some unused table values, so dropping this code is fine and you can
keep this patch for v2 of the patch set.
Regards,
Hans
next prev parent reply other threads:[~2021-06-22 9:19 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-19 10:47 [PATCH 00/18] staging: rtl8723bs: remove 5Ghz code Fabio Aiuto
2021-06-19 10:47 ` [PATCH 01/18] staging: rtl8723bs: remove all 5Ghz network types Fabio Aiuto
2021-06-21 9:40 ` Hans de Goede
2021-06-21 11:23 ` Fabio Aiuto
2021-06-19 10:47 ` [PATCH 02/18] staging: rtl8723bs: remove code related to unsupported channel bandwidth Fabio Aiuto
2021-06-19 10:47 ` [PATCH 03/18] staging: rtl8723bs: remove unused enum items related to channel bonding Fabio Aiuto
2021-06-19 10:47 ` [PATCH 04/18] staging: rtl8723bs: rename " Fabio Aiuto
2021-06-19 10:47 ` [PATCH 05/18] staging: rtl8723bs: remove 5Ghz field in struct registry_priv Fabio Aiuto
2021-06-19 10:47 ` [PATCH 06/18] staging: rtl8723bs: remove struct rt_channel_plan_5g Fabio Aiuto
2021-06-19 10:47 ` [PATCH 07/18] staging: rtl8723bs: remove all branchings between 2.4Ghz and 5Ghz band types Fabio Aiuto
2021-06-19 10:47 ` [PATCH 08/18] staging: rtl8723bs: beautify prototypes in include/hal_com_phycfg.h Fabio Aiuto
2021-06-19 10:47 ` [PATCH 09/18] staging: rtl8723bs: remove 5Ghz code related to channel plan definition Fabio Aiuto
2021-06-19 10:47 ` [PATCH 10/18] staging: rtl8723bs: remove some unused 5Ghz macro definitions Fabio Aiuto
2021-06-19 10:47 ` [PATCH 11/18] staging: rtl8723bs: remove 5Ghz code related to RF power calibration Fabio Aiuto
2021-06-19 10:47 ` [PATCH 12/18] staging: rtl8723bs: remove VHT dead code Fabio Aiuto
2021-06-21 9:45 ` Hans de Goede
2021-06-21 12:41 ` Fabio Aiuto
2021-06-21 12:45 ` Hans de Goede
2021-06-21 12:51 ` Fabio Aiuto
2021-06-22 9:16 ` Fabio Aiuto
2021-06-22 9:19 ` Hans de Goede [this message]
2021-06-22 9:57 ` Fabio Aiuto
2021-06-22 11:15 ` Hans de Goede
2021-06-19 10:47 ` [PATCH 13/18] staging: rtl8723bs: remove unused ODM_CMNINFO_BOARD_TYPE enum item Fabio Aiuto
2021-06-19 10:47 ` [PATCH 14/18] staging: rtl8723bs: fix macro value for 2.4Ghz only device Fabio Aiuto
2021-06-19 10:47 ` [PATCH 15/18] staging: rtl8723bs: remove register initializations tied to 802.11ac standard Fabio Aiuto
2021-06-21 9:48 ` Hans de Goede
2021-06-21 12:45 ` Fabio Aiuto
2021-06-19 10:47 ` [PATCH 16/18] staging: rtl8723bs: remove obsolete 5Ghz comments Fabio Aiuto
2021-06-19 10:47 ` [PATCH 17/18] staging: rtl8723bs: fix check allowing 5Ghz settings Fabio Aiuto
2021-06-19 10:47 ` [PATCH 18/18] staging: rtl8723bs: remove item from TODO list Fabio Aiuto
2021-06-21 9:49 ` [PATCH 00/18] staging: rtl8723bs: remove 5Ghz code Hans de Goede
2021-06-21 12:47 ` Fabio Aiuto
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4f128bd2-b84b-132e-c75f-0030701a8a9b@redhat.com \
--to=hdegoede@redhat.com \
--cc=Larry.Finger@lwfinger.net \
--cc=fabioaiuto83@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).