diff mbox series

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

Message ID 20250303023630.78397-1-feng.tang@linux.alibaba.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series [v4] PCI/portdrv: Only disable hotplug interrupts early when needed. | expand

Commit Message

Feng Tang March 3, 2025, 2:36 a.m. UTC 
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@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(-)

Comments

Sathyanarayanan Kuppuswamy March 3, 2025, 3:35 a.m. UTC | #1 
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@linux.intel.com>

>
> 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
Feng Tang March 3, 2025, 5:55 a.m. UTC | #2 
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
Lukas Wunner March 3, 2025, 8:54 a.m. UTC | #3 
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@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>

Reviewed-by: Lukas Wunner <lukas@wunner.de>
Bjorn Helgaas March 4, 2025, 11:17 p.m. UTC | #4 
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@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>

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
>
diff mbox series

Patch

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