| Message ID | 20241114142034.4388-1-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ | expand |
On Thu, 14 Nov 2024, Ilpo Järvinen wrote: > PCIe BW controller uses IRQF_ONESHOT to solve the problem fixed by the > commit 3e82a7f9031f ("PCI/LINK: Supply IRQ handler so level-triggered > IRQs are acked"). The IRQ is shared with PME and PCIe hotplug. Due to > probe order, PME and hotplug can request IRQ first without IRQF_ONESHOT > and when BW controller requests IRQ later with IRQF_ONESHOT, the IRQ > request fails. The problem is seen at least on Rasperry Pi 4: > > pcieport 0000:00:00.0: PME: Signaling with IRQ 39 > pcieport 0000:00:00.0: AER: enabled with IRQ 39 > genirq: Flags mismatch irq 39. 00002084 (PCIe bwctrl) vs.00200084 (PCIe PME) > pcie_bwctrl 0000:00:00.0:pcie010: probe with driver pcie_bwctrl failed with error -16 > > BW controller is always enabled so change PME and pciehp too to use > IRQF_ONESHOT. > > Fixes: 470b218c2bdf ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller") This should be: Fixes: 058a4cb11620 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller") (I wasn't thinking and used the local commit id instead of the one from pci repo.) > Reported-by: Stefan Wahren <wahrenst@gmx.net> > Link: https://lore.kernel.org/linux-pci/dcd660fd-a265-4f47-8696-776a85e097a0@gmx.net/ > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/hotplug/pciehp_hpc.c | 3 ++- > drivers/pci/pcie/pme.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 736ad8baa2a5..0778305cff9d 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -68,7 +68,8 @@ static inline int pciehp_request_irq(struct controller *ctrl) > > /* Installs the interrupt handler */ > retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, > - IRQF_SHARED, "pciehp", ctrl); > + IRQF_SHARED | IRQF_ONESHOT, > + "pciehp", ctrl); > if (retval) > ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", > irq); > diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c > index a2daebd9806c..04f0e5a7b74c 100644 > --- a/drivers/pci/pcie/pme.c > +++ b/drivers/pci/pcie/pme.c > @@ -347,7 +347,8 @@ static int pcie_pme_probe(struct pcie_device *srv) > pcie_pme_interrupt_enable(port, false); > pcie_clear_root_pme_status(port); > > - ret = request_irq(srv->irq, pcie_pme_irq, IRQF_SHARED, "PCIe PME", srv); > + ret = request_irq(srv->irq, pcie_pme_irq, IRQF_SHARED | IRQF_ONESHOT, > + "PCIe PME", srv); > if (ret) { > kfree(data); > return ret; >
Hi Ilpo, Am 14.11.24 um 15:20 schrieb Ilpo Järvinen: > PCIe BW controller uses IRQF_ONESHOT to solve the problem fixed by the > commit 3e82a7f9031f ("PCI/LINK: Supply IRQ handler so level-triggered > IRQs are acked"). The IRQ is shared with PME and PCIe hotplug. Due to > probe order, PME and hotplug can request IRQ first without IRQF_ONESHOT > and when BW controller requests IRQ later with IRQF_ONESHOT, the IRQ > request fails. The problem is seen at least on Rasperry Pi 4: > > pcieport 0000:00:00.0: PME: Signaling with IRQ 39 > pcieport 0000:00:00.0: AER: enabled with IRQ 39 > genirq: Flags mismatch irq 39. 00002084 (PCIe bwctrl) vs.00200084 (PCIe PME) > pcie_bwctrl 0000:00:00.0:pcie010: probe with driver pcie_bwctrl failed with error -16 > > BW controller is always enabled so change PME and pciehp too to use > IRQF_ONESHOT. > > Fixes: 470b218c2bdf ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller") > Reported-by: Stefan Wahren <wahrenst@gmx.net> > Link: https://lore.kernel.org/linux-pci/dcd660fd-a265-4f47-8696-776a85e097a0@gmx.net/ > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > I tested this patch on my Raspberry Pi 4B (linux-next, arm64/defconfig), but unfortunately this doesn't fix the issue completely: [ 6.635741] pcieport 0000:00:00.0: PME: Signaling with IRQ 39 [ 6.638845] genirq: Flags mismatch irq 39. 00000084 (aerdrv) vs. 00202084 (PCIe PME) [ 6.638954] pcieport 0000:00:00.0: AER: request AER IRQ 39 failed [ 6.638970] aer 0000:00:00.0:pcie002: probe with driver aer failed with error -16 Regards
On Thu, Nov 14, 2024 at 04:20:34PM +0200, Ilpo Järvinen wrote: > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -68,7 +68,8 @@ static inline int pciehp_request_irq(struct controller *ctrl) > > /* Installs the interrupt handler */ > retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, > - IRQF_SHARED, "pciehp", ctrl); > + IRQF_SHARED | IRQF_ONESHOT, > + "pciehp", ctrl); > if (retval) > ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", > irq); I don't think this will work. The IRQ thread pciehp_ist() may write to the Slot Control register and await a Command Completed event, e.g. when turning Slot Power on/off, changing LEDs, etc. What happens then is, the hardware sets the Command Completed bit in the Slot Status register and signals an interrupt. The hardirq handler pciehp_isr() reads the Slot Status register, acknowledges the Command Completed event, sets "ctrl->cmd_busy = 0" and wakes up the waiting IRQ thread. In other words, pciehp does need the interrupt to stay enabled while the IRQ thread is running so that the hardirq handler can receive Command Completed interrupts. Note that DPC also does not use IRQF_ONESHOT, so you'd have to change that as well in this patch. The Raspberry Pi happens to not support DPC, so Stefan didn't see an error related to it. I'm afraid you need to amend bwctrl to work without IRQF_ONESHOT rather than changing all the others. Thanks, Lukas
On Fri, 15 Nov 2024, Lukas Wunner wrote: > On Thu, Nov 14, 2024 at 04:20:34PM +0200, Ilpo Järvinen wrote: > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -68,7 +68,8 @@ static inline int pciehp_request_irq(struct controller *ctrl) > > > > /* Installs the interrupt handler */ > > retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, > > - IRQF_SHARED, "pciehp", ctrl); > > + IRQF_SHARED | IRQF_ONESHOT, > > + "pciehp", ctrl); > > if (retval) > > ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", > > irq); > > I don't think this will work. The IRQ thread pciehp_ist() may write > to the Slot Control register and await a Command Completed event, > e.g. when turning Slot Power on/off, changing LEDs, etc. > > What happens then is, the hardware sets the Command Completed bit in > the Slot Status register and signals an interrupt. The hardirq handler > pciehp_isr() reads the Slot Status register, acknowledges the > Command Completed event, sets "ctrl->cmd_busy = 0" and wakes up the > waiting IRQ thread. > > In other words, pciehp does need the interrupt to stay enabled while > the IRQ thread is running so that the hardirq handler can receive > Command Completed interrupts. > > Note that DPC also does not use IRQF_ONESHOT, so you'd have to change > that as well in this patch. The Raspberry Pi happens to not support > DPC, so Stefan didn't see an error related to it. > > I'm afraid you need to amend bwctrl to work without IRQF_ONESHOT rather > than changing all the others. That isn't complicated. The current irq thread handler is simple enough that it will just work as hardirq handler without any changes.
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 736ad8baa2a5..0778305cff9d 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -68,7 +68,8 @@ static inline int pciehp_request_irq(struct controller *ctrl) /* Installs the interrupt handler */ retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, - IRQF_SHARED, "pciehp", ctrl); + IRQF_SHARED | IRQF_ONESHOT, + "pciehp", ctrl); if (retval) ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", irq); diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c index a2daebd9806c..04f0e5a7b74c 100644 --- a/drivers/pci/pcie/pme.c +++ b/drivers/pci/pcie/pme.c @@ -347,7 +347,8 @@ static int pcie_pme_probe(struct pcie_device *srv) pcie_pme_interrupt_enable(port, false); pcie_clear_root_pme_status(port); - ret = request_irq(srv->irq, pcie_pme_irq, IRQF_SHARED, "PCIe PME", srv); + ret = request_irq(srv->irq, pcie_pme_irq, IRQF_SHARED | IRQF_ONESHOT, + "PCIe PME", srv); if (ret) { kfree(data); return ret;
PCIe BW controller uses IRQF_ONESHOT to solve the problem fixed by the commit 3e82a7f9031f ("PCI/LINK: Supply IRQ handler so level-triggered IRQs are acked"). The IRQ is shared with PME and PCIe hotplug. Due to probe order, PME and hotplug can request IRQ first without IRQF_ONESHOT and when BW controller requests IRQ later with IRQF_ONESHOT, the IRQ request fails. The problem is seen at least on Rasperry Pi 4: pcieport 0000:00:00.0: PME: Signaling with IRQ 39 pcieport 0000:00:00.0: AER: enabled with IRQ 39 genirq: Flags mismatch irq 39. 00002084 (PCIe bwctrl) vs.00200084 (PCIe PME) pcie_bwctrl 0000:00:00.0:pcie010: probe with driver pcie_bwctrl failed with error -16 BW controller is always enabled so change PME and pciehp too to use IRQF_ONESHOT. Fixes: 470b218c2bdf ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller") Reported-by: Stefan Wahren <wahrenst@gmx.net> Link: https://lore.kernel.org/linux-pci/dcd660fd-a265-4f47-8696-776a85e097a0@gmx.net/ Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/hotplug/pciehp_hpc.c | 3 ++- drivers/pci/pcie/pme.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)