[PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[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>
---
 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;
-- 
2.39.5

Re: [PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

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;
> 

-- 
 i.

Re: [PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[Stefan Wahren]

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

Re: [PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[Lukas Wunner]

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

Re: [PATCH 1/1] PCI/PME+pciehp: Request IRQF_ONESHOT because bwctrl shares IRQ

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

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.

-- 
 i.