* Re: [PATCH v3] rtlwifi: rtl8723be: Disable ASPM if RTL8723BE connects to Intel PCI bridge
2023-11-16 4:18 ` [PATCH v3] rtlwifi: rtl8723be: Disable ASPM if RTL8723BE connects to Intel PCI bridge Jian-Hong Pan
@ 2023-11-16 16:41 ` Ilpo Järvinen
2023-11-16 17:52 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Ilpo Järvinen @ 2023-11-16 16:41 UTC (permalink / raw)
To: Jian-Hong Pan
Cc: Jonathan Bither, Larry Finger, Ping-Ke Shih, Kalle Valo,
linux-wireless, linux-pci, LKML, linux
On Thu, 16 Nov 2023, Jian-Hong Pan wrote:
> Disable RTL8723BE's ASPM in a PCI quirk and rtl8723be module if the
> Realtek RTL8723BE PCIe Wireless adapter connects to some Intel PCI
> bridges, such as Skylake and Kabylake. Otherwise, the PCI AER flood
> hangs system:
>
> pcieport 0000:00:1c.5: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> pcieport 0000:00:1c.5: device [8086:9d15] error status/mask=00000001/00002000
> pcieport 0000:00:1c.5: [ 0] RxErr (First)
> pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> pcieport 0000:00:1c.5: AER: Multiple Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218127
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> ---
> v2: Add the switch case's default condition with comment:
> "The ASPM has already been enabled by initializing
> rtl8723be_mod_params' aspm_support as 1."
>
> v3: Rework with a PCI qurik which disables RTL8723BE PCIE's ASPM, if it
> connects to some Intel bridges, such as Skylake and Kabylake. Then,
> rtl8723be checks the PCIE ASPM is enabled, or not. If it is not,
> disables rtl8723be's aspm_support parameter.
>
> .../wireless/realtek/rtlwifi/rtl8723be/sw.c | 6 ++++
> drivers/pci/quirks.c | 36 +++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> index 43b611d5288d..fe9acbaa879b 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> @@ -26,6 +26,12 @@ static void rtl8723be_init_aspm_vars(struct ieee80211_hw *hw)
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>
> + /* Disable ASPM if the link control disables it */
> + if (!pcie_aspm_enabled(rtlpci->pdev)) {
> + pci_info(rtlpci->pdev, "PCIE ASPM is disabled\n");
> + rtlpriv->cfg->mod_params->aspm_support = 0;
> + }
> +
> /*close ASPM for AMD defaultly */
> rtlpci->const_amdpci_aspm = 0;
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..170321f4b23b 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3606,6 +3606,42 @@ DECLARE_PCI_FIXUP_FINAL(0x1b7c, 0x0004, /* Ceton InfiniTV4 */
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
> quirk_broken_intx_masking);
>
> +
Remove extra new line.
> +static void quirk_disable_int_bridge_sub_pci_aspm(struct pci_dev *dev)
> +{
> + struct pci_dev *pdev;
> + u16 val;
> +
> + if (dev->bus && dev->bus->self)
> + pdev = dev->bus->self;
> + else
> + return;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + switch (pdev->device) {
> + case 0x9d15:
> + /* PCI bridges on Skylake */
> + case 0xa110 ... 0xa11f:
> + case 0xa167 ... 0xa16a:
> + /* PCI bridges on Kabylake */
> + case 0xa290 ... 0xa29f:
> + case 0xa2e7 ... 0xa2ee:
> + pci_info(dev, "quirk: disable the device's ASPM\n");
> + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &val);
> + val &= ~PCI_EXP_LNKCTL_ASPMC;
> + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, val);
Never do RMW ops on LNKCTL like that but use a RMW-safe accessor:
pcie_capability_clear_word(dev, PCI_EXP_LNKCTL,
PCI_EXP_LNKCTL_ASPMC);
However, isn't there quirk_disable_aspm_l0s_l1() already which this
function could call? Why not use it?
--
i.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] rtlwifi: rtl8723be: Disable ASPM if RTL8723BE connects to Intel PCI bridge
2023-11-16 4:18 ` [PATCH v3] rtlwifi: rtl8723be: Disable ASPM if RTL8723BE connects to Intel PCI bridge Jian-Hong Pan
2023-11-16 16:41 ` Ilpo Järvinen
@ 2023-11-16 17:52 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Helgaas @ 2023-11-16 17:52 UTC (permalink / raw)
To: Jian-Hong Pan
Cc: Jonathan Bither, Larry Finger, Ping-Ke Shih, Kalle Valo,
linux-wireless, linux-pci, linux-kernel, linux
On Thu, Nov 16, 2023 at 12:18:35PM +0800, Jian-Hong Pan wrote:
> Disable RTL8723BE's ASPM in a PCI quirk and rtl8723be module if the
> Realtek RTL8723BE PCIe Wireless adapter connects to some Intel PCI
> bridges, such as Skylake and Kabylake. Otherwise, the PCI AER flood
> hangs system:
"Kaby Lake". Annoying but seems to be the case that "Skylake" has no
space while "Kaby Lake" does.
I don't feel good about this fix because we don't have a root cause
for the issue, we don't know if Skylake and Kaby Lake are the only
affected bridges, and we're giving up some power savings on a lot of
systems.
I *assume* ASPM probably works OK under Windows on these platforms?
Can anybody confirm/deny that?
Do we know whether Realtek parts other than rtl8723be have the same
issue? There are about a dozen rtlwifi drivers that look basically
identical so I assume there's a lot of shared IP there.
The flood seems to start when rtl8723be probes the device, and I
assume it fiddles with the ASPM config then. My guess is it's doing
something wrong then, and if we could figure it out, we could fix it.
Theoretically, drivers shouldn't need to deal with ASPM at all because
the PCI core does that already. But these parts seem to have issues
that require special handling.
Bjorn
> pcieport 0000:00:1c.5: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> pcieport 0000:00:1c.5: device [8086:9d15] error status/mask=00000001/00002000
> pcieport 0000:00:1c.5: [ 0] RxErr (First)
> pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> pcieport 0000:00:1c.5: AER: Multiple Corrected error received: 0000:00:1c.5
> pcieport 0000:00:1c.5: AER: can't find device of ID00e5
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218127
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> ---
> v2: Add the switch case's default condition with comment:
> "The ASPM has already been enabled by initializing
> rtl8723be_mod_params' aspm_support as 1."
>
> v3: Rework with a PCI qurik which disables RTL8723BE PCIE's ASPM, if it
> connects to some Intel bridges, such as Skylake and Kabylake. Then,
> rtl8723be checks the PCIE ASPM is enabled, or not. If it is not,
> disables rtl8723be's aspm_support parameter.
>
> .../wireless/realtek/rtlwifi/rtl8723be/sw.c | 6 ++++
> drivers/pci/quirks.c | 36 +++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> index 43b611d5288d..fe9acbaa879b 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c
> @@ -26,6 +26,12 @@ static void rtl8723be_init_aspm_vars(struct ieee80211_hw *hw)
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>
> + /* Disable ASPM if the link control disables it */
> + if (!pcie_aspm_enabled(rtlpci->pdev)) {
> + pci_info(rtlpci->pdev, "PCIE ASPM is disabled\n");
> + rtlpriv->cfg->mod_params->aspm_support = 0;
> + }
> +
> /*close ASPM for AMD defaultly */
> rtlpci->const_amdpci_aspm = 0;
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..170321f4b23b 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3606,6 +3606,42 @@ DECLARE_PCI_FIXUP_FINAL(0x1b7c, 0x0004, /* Ceton InfiniTV4 */
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
> quirk_broken_intx_masking);
>
> +
> +static void quirk_disable_int_bridge_sub_pci_aspm(struct pci_dev *dev)
> +{
> + struct pci_dev *pdev;
> + u16 val;
> +
> + if (dev->bus && dev->bus->self)
> + pdev = dev->bus->self;
> + else
> + return;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + switch (pdev->device) {
> + case 0x9d15:
> + /* PCI bridges on Skylake */
> + case 0xa110 ... 0xa11f:
> + case 0xa167 ... 0xa16a:
> + /* PCI bridges on Kabylake */
> + case 0xa290 ... 0xa29f:
> + case 0xa2e7 ... 0xa2ee:
> + pci_info(dev, "quirk: disable the device's ASPM\n");
> + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &val);
> + val &= ~PCI_EXP_LNKCTL_ASPMC;
> + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, val);
> + break;
> + }
> + }
> +}
> +
> +/*
> + * Disable Realtek RTL8723BE PCIE's ASPM, if it connects to some Intel bridges,
> + * such as Skylake and Kabylake. Otherwise, the PCI AER flood hangs system.
> + */
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_REALTEK, 0xb723,
> + quirk_disable_int_bridge_sub_pci_aspm);
> +
> /*
> * Intel i40e (XL710/X710) 10/20/40GbE NICs all have broken INTx masking,
> * DisINTx can be set but the interrupt status bit is non-functional.
> --
> 2.42.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread