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

[Feng Tang <feng.tang@linux.alibaba.com>]

Hi Sathyanarayanan,

On Sun, Mar 02, 2025 at 07:35:21PM -0800, Sathyanarayanan Kuppuswamy wrote:
> 
> 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

Sure. Will add.

> Code wise looks fine.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com>
 
Thank you!

- Feng

> > 
> > 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@linux.alibaba.com/t/#u
> > 
> > Fixes: 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization")
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> > ---
> > 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