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@xxxxxxxxxxxxxxx> # Use to read_poll_timeout()
> Signed-off-by: Feng Tang <feng.tang@xxxxxxxxxxxxxxxxx>
> ---
> 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.
