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

[Lukas Wunner <lukas@wunner.de>]

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