On 3/2/25 6:36 PM, 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.
I think above part of the commit message might need some rewording.
The way you are fixing this issue is to make the first command conditional
on hotplug driver not enabled. That way you can skip one of these
commands in both enable/disable hotplug driver case.
I also recommend adding some info about why making this change
should not affect the original intention of commit # 2bd50dd800b5
Code wise looks fine.
Reviewed-by: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
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>
---
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
I think you can use "is not enabled" instead of 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
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
