RE: [PATCH rtw-next 1/3] wifi: rtlwifi: fix disabling of ASPM for RTL8723BE with AER flooding

[Ping-Ke Shih <pkshih@realtek.com>]

William Hansen-Baird <william.hansen.baird@gmail.com> wrote:
> Commit 77a6407c6ab2 ("wifi: rtlwifi: disable ASPM for RTL8723BE with subsystem ID 11ad:1723")
> adds code which sets ppsc->support_aspm to false in
> _rtl_pci_update_default_setting() in order to disable ASPM.
> This does not, however, disable ASPM. Rather, it disables driver
> control of ASPM, and blocks calls to rtl_pci_enable_aspm()
> and rtl_pci_disable_aspm().
> 
> In some cases, the pci device supplied to the probe function has
> ASPM enabled. The code would therefore not disable ASPM, as it means to,
> but rather just leave it enabled.
> This was discovered through testing on a Razer Blade 14 2017, where ASPM
> was enabled by default for the pci device.
> 
> Move the code added in the previous commit to rtl_pci_init_aspm() to
> allow the adding of a call to rtl_pci_disable_aspm(hw) prior to disabling
> ppsc->pci_support. This makes sure ASPM is disabled before disabling
> driver control of ASPM to block it from being enabled later.
> 
> Fixes: 77a6407c6ab2 ("wifi: rtlwifi: disable ASPM for RTL8723BE with subsystem ID 11ad:1723")
> Signed-off-by: William Hansen-Baird <william.hansen.baird@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/pci.c | 23 +++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c
> b/drivers/net/wireless/realtek/rtlwifi/pci.c
> index 73018a0498b4..4ef1faf649e9 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/pci.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
> @@ -156,15 +156,6 @@ static void _rtl_pci_update_default_setting(struct ieee80211_hw *hw)
>                                 PCI_EXP_LNKCTL_ASPM_L1 | PCI_EXP_LNKCTL_CCC))
>                 ppsc->support_aspm = false;
> 
> -       /* RTL8723BE found on some ASUSTek laptops, such as F441U and
> -        * X555UQ with subsystem ID 11ad:1723 are known to output large
> -        * amounts of PCIe AER errors during and after boot up, causing
> -        * heavy lags, poor network throughput, and occasional lock-ups.
> -        */
> -       if (rtlpriv->rtlhal.hw_type == HARDWARE_TYPE_RTL8723BE &&
> -           (rtlpci->pdev->subsystem_vendor == 0x11ad &&
> -            rtlpci->pdev->subsystem_device == 0x1723))
> -               ppsc->support_aspm = false;
>  }
> 
>  static bool _rtl_pci_platform_switch_device_pci_aspm(
> @@ -330,10 +321,24 @@ static void rtl_pci_parse_configuration(struct pci_dev *pdev,
> 
>  static void rtl_pci_init_aspm(struct ieee80211_hw *hw)
>  {
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
>         struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
> +       struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> 
>         _rtl_pci_update_default_setting(hw);
> 
> +       /* RTL8723BE found on some ASUSTek laptops, such as F441U and
> +        * X555UQ with subsystem ID 11ad:1723 are known to output large
> +        * amounts of PCIe AER errors during and after boot up, causing
> +        * heavy lags, poor network throughput, and occasional lock-ups.
> +        */
> +       if (rtlpriv->rtlhal.hw_type == HARDWARE_TYPE_RTL8723BE &&
> +           (rtlpci->pdev->subsystem_vendor == 0x11ad &&
> +            rtlpci->pdev->subsystem_device == 0x1723)) {
> +               rtl_pci_disable_aspm(hw);

The rtl_pci_disable_aspm() check condition of ppsc->support_aspm, so we
should consider the order seriously. I'd introduce a __rtl_pci_disable_aspm()
without checking, like:

--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -203,7 +203,7 @@ static void _rtl_pci_switch_clk_req(struct ieee80211_hw *hw, u16 value)
 }

 /*Disable RTL8192SE ASPM & Disable Pci Bridge ASPM*/
-static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
+static void __rtl_pci_disable_aspm(struct ieee80211_hw *hw)
 {
        struct rtl_priv *rtlpriv = rtl_priv(hw);
        struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw);
@@ -215,9 +215,6 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
        u16 aspmlevel = 0;
        u16 tmp_u1b = 0;

-       if (!ppsc->support_aspm)
-               return;
-
        if (pcibridge_vendor == PCI_BRIDGE_VENDOR_UNKNOWN) {
                rtl_dbg(rtlpriv, COMP_POWER, DBG_TRACE,
                        "PCI(Bridge) UNKNOWN\n");
@@ -240,6 +237,14 @@ static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
        _rtl_pci_platform_switch_device_pci_aspm(hw, linkctrl_reg);
 }

+static void rtl_pci_disable_aspm(struct ieee80211_hw *hw)
+{
+       if (!ppsc->support_aspm)
+               return;
+
+       __rtl_pci_disable_aspm(hw);
+}
+

Here, rtl_pci_init_aspm() can call

	__rtl_pci_disable_aspm(hw);

> +               ppsc->support_aspm = false;
> +       }
> +
>         if (ppsc->reg_rfps_level & RT_RF_PS_LEVEL_ALWAYS_ASPM) {
>                 /*Always enable ASPM & Clock Req. */
>                 rtl_pci_enable_aspm(hw);
> --
> 2.54.0