Re: [PATCH v4] PCI/portdrv: Only disable hotplug interrupts early when needed.

[Bjorn Helgaas]

On Mon, Mar 03, 2025 at 10:36:30AM +0800, Feng Tang wrote:
> There was problem reported by firmware developers that they received
> two PCIe hotplug 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 command or sending a new command.
> 
> In the failure case, the first PCIe hotplug command firmware received
> is from get_port_device_capability(), which sends command to disable
> PCIe hotplug interrupts without waiting for its completion, and the
> second command comes from pcie_enable_notification() of pciehp driver,
> which enables hotplug interrupts again.
> 
> One solution is to add the necessary delay after the first command [1],
> while Lukas proposed an optimization that if the pciehp driver will be
> loaded soon and handle the interrupts, then the hotplug and the wait
> are not needed and can be saved, for every root port.
> 
> So fix it by only disabling the hotplug interrupts when pciehp driver
> is not enabled.
> 
> [1]. https://lore.kernel.org/lkml/20250224034500.23024-1-feng.tang@xxxxxxxxxxxxxxxxx/t/#u
> 
> Fixes: 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization")
> Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> Signed-off-by: Feng Tang <feng.tang@xxxxxxxxxxxxxxxxx>

Applied to pci/hotplug for v6.15, thanks!

> ---
> Changelog:
> 
>   since v3:
>     * Separate this patch from patches dealing with irq storm in nomsi case
>     * Take Lukas's suggestion (Lukas Wunner)  
> 
>   since v2:
>     * Add patch 0001, which move the waiting logic of pcie_poll_cmd from pciehp
>       driver to PCIe port driver for code reuse (Bjorn Helgaas)
>     * Separate Lucas' suggestion out as patch 0003 (Bjorn and Sathyanarayanan)  
>     * Avoid hotplug command waiting for HW without command-complete
>       event support (Bjorn Helgaas)
>     * Fix spell issue in commit log (Bjorn and Markus)
>     * Add cover-letter for whole patchset (Markus Elfring)
>     * Handle a set-but-unused build warning (0Day lkp bot)
> 
>   since v1:
>     * Add the Originally-by for Liguang for patch 0002. The issue was found on
>       a 5.10 kernel, then 6.6. I was initially given a 5.10 kernel tar ball
>       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)
>     * Fix wrong email address (Markus Elfring)
>     * Add logic to skip irq disabling if it is already disabled.
> 
> 
>  drivers/pci/pcie/portdrv.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 02e73099bad0..e8318fd5f6ed 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -228,10 +228,12 @@ 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.
> +		 * by the BIOS and the hot-plug service driver won't be loaded
> +		 * to handle them.
>  		 */
> -		pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> -			  PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> +		if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> +			pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> +				PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
>  	}
>  
>  #ifdef CONFIG_PCIEAER
> -- 
> 2.43.5
>