Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code

From: Hans de Goede
Date: Tue Jun 22 2021 - 05:19:43 EST

Next message: David Laight: "RE: [PATCH v2] slab: Use %s instead of function name"
Previous message: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
In reply to: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
Next in thread: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
>>> ---
>>> 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 message: David Laight: "RE: [PATCH v2] slab: Use %s instead of function name"
Previous message: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
In reply to: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
Next in thread: Fabio Aiuto: "Re: [PATCH 12/18] staging: rtl8723bs: remove VHT dead code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]