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

From: Fabio Aiuto
Date: Mon Jun 21 2021 - 08:51:07 EST

Next message: Srinivas Kandagatla: "Re: [PATCH v3] ASoC: qcom: Fix for DMA interrupt clear reg overwriting"
Previous message: Fabio Aiuto: "Re: [PATCH 00/18] staging: rtl8723bs: remove 5Ghz code"
In reply to: Hans de Goede: "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 ]

On Mon, Jun 21, 2021 at 02:45:37PM +0200, Hans de Goede wrote:

Hello Hans,

> Hi,
>
> On 6/21/21 2:41 PM, Fabio Aiuto wrote:
> > 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.
> >
> > You are right, I will fix the changelog in next v2.
> >
> >>
> >> Perhaps this entire block can never be executed ?
> >
> > It think it's executed during hal initialization
> >
> >>
> >> Or maybe the VHT name used here is misleading and this is also relevant for 2.4 GHZ
> >> 40Mhz use ? Did you test with an access point configured to use 40 MHz channels
> >> and then checked that you were actually using a 40MHz channel ? Note these are
> >> quite rare AFAIK because 40 MHz tends to cause even more collosion issues in the
> >> already quite full 2.4G spectrum, still if possible you should test this and make
> >> sure that it does not regress (with out specifically testing this we might not
> >> note regressions for a long time because of 40MHz usage being rare).
> >>
> >
> > The MCS section involved is from 0 to 9 in a set of up to 4 spatial streams,
> > but rtl8723bs works only with 0-7 and 32 MCS rates in both 20 and 40 Mhz
> > and multiple spatial streams are not supported.
> >
> > So setting something like VHT_{1,2,3}SSMCS0_{1,2,3}SSMCS9 in our case does
> > something odd.
>
> Right, but since we don't know what it is actually doing, it would be best
> IMHO to just leave this as is. Esp. when the changes cannot be tested
> with 40MHZ channel width.
>
> Regards,
>
> Hans
>

you are right, will drop this patch as well.

Thank you,

fabio.

Next message: Srinivas Kandagatla: "Re: [PATCH v3] ASoC: qcom: Fix for DMA interrupt clear reg overwriting"
Previous message: Fabio Aiuto: "Re: [PATCH 00/18] staging: rtl8723bs: remove 5Ghz code"
In reply to: Hans de Goede: "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 ]