Re: [PATCH v2 1/2] PCI/portdrv: Add necessary wait for disabling hotplug events

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Feb 18, 2025 at 11:48:58AM +0800, Feng Tang wrote:
> There was problem reported by firmware developers that they received
> 2 pcie link control commands in very short intervals on an ARM server,
> which doesn't comply with pcie spec, and broke their state machine and
> work flow. According to PCIe 6.1 spec, section 6.7.3.2, software needs
> to wait at least 1 second for the command-complete event, before
> resending the cmd or sending a new cmd.

s/link control/hotplug/ (also below)
s/2/two/
s/pcie/PCIe/ (also below)

> And the first link control command firmware received is from
> get_port_device_capability(), which sends cmd to disable pcie hotplug
> interrupts without waiting for its completion.
> 
> Fix it by adding the necessary wait to comply with PCIe spec, referring
> pcie_poll_cmd().
> 
> Also make the interrupt disabling not dependent on whether pciehp
> service driver will be loaded as suggested by Lukas.

This sounds like maybe it should be two separate patches.

> Fixes: 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization")
> Originally-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> Suggested-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> ---
> Changlog:
> 
>   since v1:
>     * Add the Originally-by for Liguang. The issue was found on a 5.10 kernel,
>       then 6.6. I was initially given a 5.10 kernel tar bar without git info to
>       debug the issue, and made the patch. Thanks to Guanghui who recently pointed
>       me to tree https://gitee.com/anolis/cloud-kernel which show the wait logic
>       in 5.10 was originally from Liguang, and never hit mainline.
>     * Make the irq disabling not dependent on wthether pciehp service driver
>       will be loaded (Lukas Wunner) 
>     * Use read_poll_timeout() API to simply the waiting logic (Sathyanarayanan
>       Kuppuswamy)
>     * Add logic to skip irq disabling if it is already disabled.
> 
>  drivers/pci/pci.h          |  2 ++
>  drivers/pci/pcie/portdrv.c | 44 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..c1e234d1b81d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -759,12 +759,14 @@ static inline void pcie_ecrc_get_policy(char *str) { }
>  #ifdef CONFIG_PCIEPORTBUS
>  void pcie_reset_lbms_count(struct pci_dev *port);
>  int pcie_lbms_count(struct pci_dev *port, unsigned long *val);
> +void pcie_disable_hp_interrupts_early(struct pci_dev *dev);
>  #else
>  static inline void pcie_reset_lbms_count(struct pci_dev *port) {}
>  static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val)
>  {
>  	return -EOPNOTSUPP;
>  }
> +static inline void pcie_disable_hp_interrupts_early(struct pci_dev *dev) {}
>  #endif
>  
>  struct pci_dev_reset_methods {
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 02e73099bad0..2470333bba2f 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -18,6 +18,7 @@
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  #include <linux/aer.h>
> +#include <linux/iopoll.h>
>  
>  #include "../pci.h"
>  #include "portdrv.h"
> @@ -205,6 +206,40 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	return 0;
>  }
>  
> +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *dev)
> +{
> +	u16 slot_status = 0;
> +	int ret, ret1, timeout_us;
> +
> +	/* 1 second, according to PCIe spec 6.1, section 6.7.3.2 */
> +	timeout_us = 1000000;
> +	ret = read_poll_timeout(pcie_capability_read_word, ret1,
> +				(slot_status & PCI_EXP_SLTSTA_CC), 10000,
> +				timeout_us, true, dev, PCI_EXP_SLTSTA,
> +				&slot_status);
> +	if (!ret)
> +		pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
> +						PCI_EXP_SLTSTA_CC);
> +
> +	return  ret;

Ugh.  I really don't like the way this basically duplicates
pcie_poll_cmd().  I don't have a great suggestion to fix it; maybe we
need a way to build part of pciehp unconditionally.  At the very least
we need a comment here pointing to pcie_poll_cmd().

And IIUC this will add a one second delay for ports that don't need
command completed events.  I don't think that's fair to those ports.

> +}
> +
> +void pcie_disable_hp_interrupts_early(struct pci_dev *dev)
> +{
> +	u16 slot_ctrl = 0;
> +
> +	pcie_capability_read_word(dev, PCI_EXP_SLTCTL, &slot_ctrl);
> +	/* Bail out early if it is already disabled */
> +	if (!(slot_ctrl & (PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE)))
> +		return;
> +
> +	pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> +		  PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> +
> +	if (pcie_wait_sltctl_cmd_raw(dev))
> +		pci_info(dev, "Timeout on disabling PCIE hot-plug interrupt\n");

s/PCIE/PCIe/

> +}
> +
>  /**
>   * get_port_device_capability - discover capabilities of a PCI Express port
>   * @dev: PCI Express port to examine
> @@ -222,16 +257,15 @@ static int get_port_device_capability(struct pci_dev *dev)
>  
>  	if (dev->is_hotplug_bridge &&
>  	    (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> -	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) &&
> -	    (pcie_ports_native || host->native_pcie_hotplug)) {
> -		services |= PCIE_PORT_SERVICE_HP;
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> +		if (pcie_ports_native || host->native_pcie_hotplug)
> +			services |= PCIE_PORT_SERVICE_HP;
>  
>  		/*
>  		 * Disable hot-plug interrupts in case they have been enabled
>  		 * by the BIOS and the hot-plug service driver is not loaded.
>  		 */
> -		pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> -			  PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> +		pcie_disable_hp_interrupts_early(dev);
>  	}
>  
>  #ifdef CONFIG_PCIEAER
> -- 
> 2.43.5
>