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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 2/3/25 9:37 PM, Feng Tang wrote:
> According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at
> least 1 second for the command-complete event, before resending the cmd
> or sending a new cmd.
>
> Currently get_port_device_capability() sends slot control cmd to disable
> PCIe hotplug interrupts without waiting for its completion and there was
> real problem reported for the lack of waiting.

Can you include the error log associated with this issue? What is the
actual issue you are seeing and in which hardware?

>
> Add the necessary wait to comply with PCIe spec. The waiting logic refers
> existing pcie_poll_cmd().
>
> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> ---
>   drivers/pci/pci.h          |  2 ++
>   drivers/pci/pcie/portdrv.c | 33 +++++++++++++++++++++++++++++++--
>   2 files changed, 33 insertions(+), 2 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..16010973bfe2 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/delay.h>
>   
>   #include "../pci.h"
>   #include "portdrv.h"
> @@ -205,6 +206,35 @@ 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 *pdev)
> +{
> +	u16 slot_status;
> +	/* 1000 ms, according toPCIe spec 6.1, section 6.7.3.2 */
> +	int timeout = 1000;
> +
> +	do {
> +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
> +		if (slot_status & PCI_EXP_SLTSTA_CC) {
> +			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> +						   PCI_EXP_SLTSTA_CC);
> +			return 0;
> +		}
> +		msleep(10);
> +		timeout -= 10;
> +	} while (timeout);
> +
> +	/* Timeout */
> +	return  -1;
> +}

May be this logic can be simplified using readl_poll_timeout()?

> +
> +void pcie_disable_hp_interrupts_early(struct pci_dev *dev)
> +{
> +	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 hot-plug interrupts\n");
> +}
> +
>   /**
>    * get_port_device_capability - discover capabilities of a PCI Express port
>    * @dev: PCI Express port to examine
> @@ -230,8 +260,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>   		 * 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

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer