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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.