[PATCH] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[PATCH] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
driver") added the "pcie_hp=nomsi" kernel parameter to work around this
error on shutdown:

  irq 16: nobody cared (try booting with the "irqpoll" option)
  Pid: 1081, comm: reboot Not tainted 3.2.0 #1
  ...
  Disabling IRQ #16

This happened on an unspecified system (possibly involving the Integrated
Device Technology, Inc. Device 807f bridge).  There is no automated way to
set this parameter, so it's not very useful for distributions or end users.

I suspect the root cause of the underlying "irq 16: nobody cared" issue was
fixed by Prarit Bhargava <prarit@redhat.com> with fda78d7a0ead ("PCI/MSI:
Stop disabling MSI/MSI-X in pci_device_shutdown()") and we probably don't
need "pcie_hp=nomsi" any more.

Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>
CC: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
CC: Prarit Bhargava <prarit@redhat.com>
---
 Documentation/admin-guide/kernel-parameters.txt |    4 ----
 drivers/pci/pcie/portdrv.h                      |   12 ------------
 drivers/pci/pcie/portdrv_core.c                 |   20 +++-----------------
 3 files changed, 3 insertions(+), 33 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 78cdf6a637fc..ed0d26d26c26 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3101,10 +3101,6 @@
 		force	Enable ASPM even on devices that claim not to support it.
 			WARNING: Forcing ASPM on may cause system lockups.
 
-	pcie_hp=	[PCIE] PCI Express Hotplug driver options:
-		nomsi	Do not use MSI for PCI Express Native Hotplug (this
-			makes all PCIe ports use INTx for hotplug services).
-
 	pcie_ports=	[PCIE] PCIe ports handling:
 		auto	Ask the BIOS whether or not to use native PCIe services
 			associated with PCIe ports (PME, hot-plug, AER).  Use
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index a854bc569117..b00764bc684d 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -36,18 +36,6 @@ struct pci_dev;
 
 void pcie_clear_root_pme_status(struct pci_dev *dev);
 
-#ifdef CONFIG_HOTPLUG_PCI_PCIE
-extern bool pciehp_msi_disabled;
-
-static inline bool pciehp_no_msi(void)
-{
-	return pciehp_msi_disabled;
-}
-
-#else  /* !CONFIG_HOTPLUG_PCI_PCIE */
-static inline bool pciehp_no_msi(void) { return false; }
-#endif /* !CONFIG_HOTPLUG_PCI_PCIE */
-
 #ifdef CONFIG_PCIE_PME
 extern bool pcie_pme_msi_disabled;
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index ef3bad4ad010..219f0f47f6fb 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -21,17 +21,6 @@
 #include "../pci.h"
 #include "portdrv.h"
 
-bool pciehp_msi_disabled;
-
-static int __init pciehp_setup(char *str)
-{
-	if (!strncmp(str, "nomsi", 5))
-		pciehp_msi_disabled = true;
-
-	return 1;
-}
-__setup("pcie_hp=", pciehp_setup);
-
 /**
  * release_pcie_device - free PCI Express port service device structure
  * @dev: Port service device to release
@@ -169,16 +158,13 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 		irqs[i] = -1;
 
 	/*
-	 * If we support PME or hotplug, but we can't use MSI/MSI-X for
-	 * them, we have to fall back to INTx or other interrupts, e.g., a
-	 * system shared interrupt.
+	 * If we support PME but can't use MSI/MSI-X for it, we have to
+	 * fall back to INTx or other interrupts, e.g., a system shared
+	 * interrupt.
 	 */
 	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
 		goto legacy_irq;
 
-	if ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())
-		goto legacy_irq;
-
 	/* Try to use MSI-X or MSI if supported */
 	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
 		return 0;

Re: [PATCH] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Lukas Wunner]

On Sat, Feb 10, 2018 at 08:48:15PM -0600, Bjorn Helgaas wrote:
> 7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
> driver") added the "pcie_hp=nomsi" kernel parameter to work around this
> error on shutdown:
> 
>   irq 16: nobody cared (try booting with the "irqpoll" option)
>   Pid: 1081, comm: reboot Not tainted 3.2.0 #1
>   ...
>   Disabling IRQ #16
> 
> This happened on an unspecified system (possibly involving the Integrated
> Device Technology, Inc. Device 807f bridge).  There is no automated way to
> set this parameter, so it's not very useful for distributions or end users.
> 
> I suspect the root cause of the underlying "irq 16: nobody cared" issue was
> fixed by Prarit Bhargava <prarit@redhat.com> with fda78d7a0ead ("PCI/MSI:
> Stop disabling MSI/MSI-X in pci_device_shutdown()") and we probably don't
> need "pcie_hp=nomsi" any more.
> 
> Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.

This has been useful when bringing up broken hardware which claims to
support MSI but really doesn't, such as in commit 19bf4d4f909d
("thunderbolt: Support 1st gen Light Ridge controller").  In this case
I didn't know why the controller wasn't sending interrupts, on a hunch
I tried "pcie_hp=nomsi" and that worked, and looking in the macOS source
code I was able to verify that they disable MSI for this particular
hardware as well.  So please leave it in to ease developers' lives.

Thanks,

Lukas

Re: [PATCH] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Bjorn Helgaas]

On Sun, Feb 11, 2018 at 10:25:25AM +0100, Lukas Wunner wrote:
> On Sat, Feb 10, 2018 at 08:48:15PM -0600, Bjorn Helgaas wrote:
> > 7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
> > driver") added the "pcie_hp=nomsi" kernel parameter to work around this
> > error on shutdown:
> > 
> >   irq 16: nobody cared (try booting with the "irqpoll" option)
> >   Pid: 1081, comm: reboot Not tainted 3.2.0 #1
> >   ...
> >   Disabling IRQ #16
> > 
> > This happened on an unspecified system (possibly involving the Integrated
> > Device Technology, Inc. Device 807f bridge).  There is no automated way to
> > set this parameter, so it's not very useful for distributions or end users.
> > 
> > I suspect the root cause of the underlying "irq 16: nobody cared" issue was
> > fixed by Prarit Bhargava <prarit@redhat.com> with fda78d7a0ead ("PCI/MSI:
> > Stop disabling MSI/MSI-X in pci_device_shutdown()") and we probably don't
> > need "pcie_hp=nomsi" any more.
> > 
> > Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.
> 
> This has been useful when bringing up broken hardware which claims to
> support MSI but really doesn't, such as in commit 19bf4d4f909d
> ("thunderbolt: Support 1st gen Light Ridge controller").  In this case
> I didn't know why the controller wasn't sending interrupts, on a hunch
> I tried "pcie_hp=nomsi" and that worked, and looking in the macOS source
> code I was able to verify that they disable MSI for this particular
> hardware as well.  So please leave it in to ease developers' lives.

Wouldn't "pci=nomsi" be sufficient for that sort of bringup
experimentation?  We don't need to be super specific in that
situation.

The reason I want to remove it is that the port driver
(drivers/pci/pcie/portdrv*) has become a rat's nest of switches and
special cases, and I'm trying to simplify it.  Things like
"pcie_hp=nomsi" are trivial individually, but collectively it's
getting unmanageable.

Bjorn

Re: [PATCH] PCI/portdrv: Remove "pcie_hp=nomsi" kernel parameter

[Lukas Wunner]

On Mon, Feb 12, 2018 at 09:01:46AM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 11, 2018 at 10:25:25AM +0100, Lukas Wunner wrote:
> > On Sat, Feb 10, 2018 at 08:48:15PM -0600, Bjorn Helgaas wrote:
> > > 7570a333d8b0 ("PCI: Add pcie_hp=nomsi to disable MSI/MSI-X for pciehp
> > > driver") added the "pcie_hp=nomsi" kernel parameter to work around this
> > > error on shutdown:
> > > 
> > >   irq 16: nobody cared (try booting with the "irqpoll" option)
> > >   Pid: 1081, comm: reboot Not tainted 3.2.0 #1
> > >   ...
> > >   Disabling IRQ #16
> > > 
> > > This happened on an unspecified system (possibly involving the Integrated
> > > Device Technology, Inc. Device 807f bridge).  There is no automated way to
> > > set this parameter, so it's not very useful for distributions or end users.
> > > 
> > > I suspect the root cause of the underlying "irq 16: nobody cared" issue was
> > > fixed by Prarit Bhargava <prarit@redhat.com> with fda78d7a0ead ("PCI/MSI:
> > > Stop disabling MSI/MSI-X in pci_device_shutdown()") and we probably don't
> > > need "pcie_hp=nomsi" any more.
> > > 
> > > Revert 7570a333d8b0 to remove the "pcie_hp=nomsi" parameter.
> > 
> > This has been useful when bringing up broken hardware which claims to
> > support MSI but really doesn't, such as in commit 19bf4d4f909d
> > ("thunderbolt: Support 1st gen Light Ridge controller").  In this case
> > I didn't know why the controller wasn't sending interrupts, on a hunch
> > I tried "pcie_hp=nomsi" and that worked, and looking in the macOS source
> > code I was able to verify that they disable MSI for this particular
> > hardware as well.  So please leave it in to ease developers' lives.
> 
> Wouldn't "pci=nomsi" be sufficient for that sort of bringup
> experimentation?  We don't need to be super specific in that
> situation.

I'm not sure if that would work as it also disables MSI for
anything not a hotplug bridge.


> The reason I want to remove it is that the port driver
> (drivers/pci/pcie/portdrv*) has become a rat's nest of switches and
> special cases, and I'm trying to simplify it.  Things like
> "pcie_hp=nomsi" are trivial individually, but collectively it's
> getting unmanageable.

I see your point and don't want to stand in the way of progress.
It just was a useful tool for me once, that's all I'm saying,
I probably won't need it again, but maybe someone else will.

Thanks,

Lukas