Re: [PATCH v3] rtlwifi: rtl8723be: Disable ASPM if RTL8723BE connects to Intel PCI bridge

[Ilpo Järvinen]

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@xxxxxxxxxxxxx>
> ---
> 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.