1. Patchew
  2. linux
  3. [PATCH v3 0/4] PCIe hotplug interrupt related fixes
  4. View patch

[PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command waiting logic to port driver

Feng Tang posted 4 patches 9 months, 3 weeks ago
[PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command waiting logic to port driver
Posted by Feng Tang 9 months, 3 weeks ago 
According to PCIe spec, after sending a hotplug command, software should
wait some time for the command completion. Currently the waiting logic
is implemented in pciehp driver, as the same logic will be reused by
PCIe port driver, move it to port driver, which complies with the logic
of CONFIG_HOTPLUG_PCI_PCIE depending on CONFIG_PCIEPORTBUS.

Also convert the loop wait logic to helper read_poll_timeout() as
suggested by Sathyanarayanan Kuppuswamy.

Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++------------------------
 drivers/pci/pci.h                |  5 +++++
 drivers/pci/pcie/portdrv.c       | 25 +++++++++++++++++++++
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bb5a8d9f03ad..24e346f558db 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -83,32 +83,6 @@ static inline void pciehp_free_irq(struct controller *ctrl)
 		free_irq(ctrl->pcie->irq, ctrl);
 }
 
-static int pcie_poll_cmd(struct controller *ctrl, int timeout)
-{
-	struct pci_dev *pdev = ctrl_dev(ctrl);
-	u16 slot_status;
-
-	do {
-		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-		if (PCI_POSSIBLE_ERROR(slot_status)) {
-			ctrl_info(ctrl, "%s: no response from device\n",
-				  __func__);
-			return 0;
-		}
-
-		if (slot_status & PCI_EXP_SLTSTA_CC) {
-			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-						   PCI_EXP_SLTSTA_CC);
-			ctrl->cmd_busy = 0;
-			smp_mb();
-			return 1;
-		}
-		msleep(10);
-		timeout -= 10;
-	} while (timeout >= 0);
-	return 0;	/* timeout */
-}
-
 static void pcie_wait_cmd(struct controller *ctrl)
 {
 	unsigned int msecs = pciehp_poll_mode ? 2500 : 1000;
@@ -138,10 +112,16 @@ static void pcie_wait_cmd(struct controller *ctrl)
 		timeout = cmd_timeout - now;
 
 	if (ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
-	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
+	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE) {
 		rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
-	else
-		rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));
+	} else {
+		rc = pcie_poll_sltctl_cmd(ctrl_dev(ctrl), jiffies_to_msecs(timeout));
+		if (!rc) {
+			ctrl->cmd_busy = 0;
+			smp_mb();
+			rc = 1;
+		}
+	}
 
 	if (!rc)
 		ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 01e51db8d285..4c94a589de4a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -759,12 +759,17 @@ 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);
+int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms);
 #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 int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
+{
+	return 0;
+}
 #endif
 
 struct pci_dev_reset_methods {
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 02e73099bad0..bb00ba45ee51 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,30 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 	return 0;
 }
 
+/* Return 0 on command completed on time, otherwise return -ETIMEOUT */
+int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
+{
+	u16 slot_status = 0;
+	u32 slot_cap;
+	int ret = 0;
+	int __maybe_unused ret1;
+
+	/* Don't wait if the command complete event is not well supported */
+	pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slot_cap);
+	if (!(slot_cap & PCI_EXP_SLTCAP_HPC) || slot_cap & PCI_EXP_SLTCAP_NCCS)
+		return ret;
+
+	ret = read_poll_timeout(pcie_capability_read_word, ret1,
+				(slot_status & PCI_EXP_SLTSTA_CC), 10000,
+				timeout_ms * 1000, true, dev, PCI_EXP_SLTSTA,
+				&slot_status);
+	if (!ret)
+		pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
+						PCI_EXP_SLTSTA_CC);
+
+	return  ret;
+}
+
 /**
  * get_port_device_capability - discover capabilities of a PCI Express port
  * @dev: PCI Express port to examine
-- 
2.43.5
Re: [PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command waiting logic to port driver
Posted by Ilpo Järvinen 9 months, 3 weeks ago 
On Mon, 24 Feb 2025, Feng Tang wrote:

> According to PCIe spec, after sending a hotplug command, software should
> wait some time for the command completion. Currently the waiting logic

Where is it in the spec, please put a more precise reference.

> is implemented in pciehp driver, as the same logic will be reused by
> PCIe port driver, move it to port driver, which complies with the logic
> of CONFIG_HOTPLUG_PCI_PCIE depending on CONFIG_PCIEPORTBUS.
> 
> Also convert the loop wait logic to helper read_poll_timeout() as
> suggested by Sathyanarayanan Kuppuswamy.

You could express the second part of this with a tag:

Suggested-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> # Use to read_poll_timeout()

> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++------------------------
>  drivers/pci/pci.h                |  5 +++++
>  drivers/pci/pcie/portdrv.c       | 25 +++++++++++++++++++++
>  3 files changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bb5a8d9f03ad..24e346f558db 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -83,32 +83,6 @@ static inline void pciehp_free_irq(struct controller *ctrl)
>  		free_irq(ctrl->pcie->irq, ctrl);
>  }
>  
> -static int pcie_poll_cmd(struct controller *ctrl, int timeout)
> -{
> -	struct pci_dev *pdev = ctrl_dev(ctrl);
> -	u16 slot_status;
> -
> -	do {
> -		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
> -		if (PCI_POSSIBLE_ERROR(slot_status)) {
> -			ctrl_info(ctrl, "%s: no response from device\n",
> -				  __func__);
> -			return 0;
> -		}
> -
> -		if (slot_status & PCI_EXP_SLTSTA_CC) {
> -			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> -						   PCI_EXP_SLTSTA_CC);
> -			ctrl->cmd_busy = 0;
> -			smp_mb();
> -			return 1;
> -		}
> -		msleep(10);
> -		timeout -= 10;
> -	} while (timeout >= 0);
> -	return 0;	/* timeout */
> -}
> -
>  static void pcie_wait_cmd(struct controller *ctrl)
>  {
>  	unsigned int msecs = pciehp_poll_mode ? 2500 : 1000;
> @@ -138,10 +112,16 @@ static void pcie_wait_cmd(struct controller *ctrl)
>  		timeout = cmd_timeout - now;
>  
>  	if (ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
> -	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
> +	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE) {
>  		rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
> -	else
> -		rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));
> +	} else {
> +		rc = pcie_poll_sltctl_cmd(ctrl_dev(ctrl), jiffies_to_msecs(timeout));
> +		if (!rc) {
> +			ctrl->cmd_busy = 0;
> +			smp_mb();
> +			rc = 1;
> +		}
> +	}
>  
>  	if (!rc)
>  		ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..4c94a589de4a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -759,12 +759,17 @@ 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);
> +int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms);
>  #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 int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
> +{
> +	return 0;
> +}
>  #endif
>  
>  struct pci_dev_reset_methods {
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 02e73099bad0..bb00ba45ee51 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,30 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	return 0;
>  }
>  
> +/* Return 0 on command completed on time, otherwise return -ETIMEOUT */

Since you're making this visible outside of the file, please document this 
properly using a kerneldoc compliant comment.

> +int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
> +{
> +	u16 slot_status = 0;
> +	u32 slot_cap;
> +	int ret = 0;

Unnecessary initialization.

> +	int __maybe_unused ret1;
> +
> +	/* Don't wait if the command complete event is not well supported */
> +	pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slot_cap);
> +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC) || slot_cap & PCI_EXP_SLTCAP_NCCS)
> +		return ret;
> +
> +	ret = read_poll_timeout(pcie_capability_read_word, ret1,
> +				(slot_status & PCI_EXP_SLTSTA_CC), 10000,
> +				timeout_ms * 1000, true, dev, PCI_EXP_SLTSTA,

Replace:
        10000 -> 10 * USEC_PER_MSEC
        timeout_ms * 1000 -> USEC_PER_SEC (the variable can be dropped)

Please also check you have linux/units.h included for those defines.

> +				&slot_status);
> +	if (!ret)

Use the normal error handling logic by reversing the condition.

> +		pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
> +						PCI_EXP_SLTSTA_CC);
> +
> +	return  ret;

Remove extra space but this will become return 0; once the error handling 
is done with the usual pattern.

> +}
> +
>  /**
>   * get_port_device_capability - discover capabilities of a PCI Express port
>   * @dev: PCI Express port to examine
> 

-- 
 i.
Re: [PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command waiting logic to port driver
Posted by Feng Tang 9 months, 3 weeks ago 
Hi Ilpo,

On Mon, Feb 24, 2025 at 05:06:26PM +0200, Ilpo Järvinen wrote:
> On Mon, 24 Feb 2025, Feng Tang wrote:
> 
> > According to PCIe spec, after sending a hotplug command, software should
> > wait some time for the command completion. Currently the waiting logic
> 
> Where is it in the spec, please put a more precise reference.

Will do, if the patch is kept.
> 
> > is implemented in pciehp driver, as the same logic will be reused by
> > PCIe port driver, move it to port driver, which complies with the logic
> > of CONFIG_HOTPLUG_PCI_PCIE depending on CONFIG_PCIEPORTBUS.
> > 
> > Also convert the loop wait logic to helper read_poll_timeout() as
> > suggested by Sathyanarayanan Kuppuswamy.
> 
> You could express the second part of this with a tag:
> 
> Suggested-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> # Use to read_poll_timeout()

Thanks for the suggestion!

> 
> > Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> > ---
> >  drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++------------------------
> >  drivers/pci/pci.h                |  5 +++++
> >  drivers/pci/pcie/portdrv.c       | 25 +++++++++++++++++++++
> >  3 files changed, 39 insertions(+), 29 deletions(-)
[...]

> >  
> > +/* Return 0 on command completed on time, otherwise return -ETIMEOUT */
> 
> Since you're making this visible outside of the file, please document this 
> properly using a kerneldoc compliant comment.

OK.

> 
> > +int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
> > +{
> > +	u16 slot_status = 0;
> > +	u32 slot_cap;
> > +	int ret = 0;
> 
> Unnecessary initialization.

The initialization is actually used below.
> 
> > +	int __maybe_unused ret1;
> > +
> > +	/* Don't wait if the command complete event is not well supported */
> > +	pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slot_cap);
> > +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC) || slot_cap & PCI_EXP_SLTCAP_NCCS)
> > +		return ret;

Used here to return early if the event is not supported.

> > +
> > +	ret = read_poll_timeout(pcie_capability_read_word, ret1,
> > +				(slot_status & PCI_EXP_SLTSTA_CC), 10000,
> > +				timeout_ms * 1000, true, dev, PCI_EXP_SLTSTA,
> 
> Replace:
>         10000 -> 10 * USEC_PER_MSEC
>         timeout_ms * 1000 -> USEC_PER_SEC (the variable can be dropped)
> 
> Please also check you have linux/units.h included for those defines.

Will use the macros, which are indeed self-describing. 

If you are referring 'timeout_ms', I don't think it can be dropped as
it is needed in the logic of pcie_poll_cmd().

> > +				&slot_status);
> > +	if (!ret)
> 
> Use the normal error handling logic by reversing the condition.
> 
> > +		pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
> > +						PCI_EXP_SLTSTA_CC);
> > +
> > +	return  ret;
> 
> Remove extra space but this will become return 0; once the error handling 
> is done with the usual pattern.

This is not error handling, but rather "normal" and "expected", that
the command-completed bit is set and will be cleared here.

Thanks,
Feng

> 
> > +}
> > +
> >  /**
> >   * get_port_device_capability - discover capabilities of a PCI Express port
> >   * @dev: PCI Express port to examine
> > 
> 
> -- 
>  i.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.