Re: [PATCH 09/11] PCI: keystone: Switch to devm_request_irq() for "ks-pcie-error-irq" IRQ

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On Thu, Sep 04, 2025 at 09:24:01AM +0200, Jiri Slaby wrote:
> On 03. 09. 25, 14:44, Siddharth Vadapalli wrote:
> > In preparation for enabling loadable module support for the driver,
> > there is motivation to switch to devm_request_irq() to simplify the
> > cleanup on driver removal. Additionally, since the interrupt handler
> > associated with the "ks-pcie-error-irq" namely "ks_pcie_handle_error_irq()
> > is only printing the error and is clearing the interrupt, there is no
> > necessity to prefer devm_request_threaded_irq() over devm_request_irq().
> > Hence, switch from request_irq() to devm_request_irq().
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >   drivers/pci/controller/dwc/pci-keystone.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > index bb93559f6468..02f9a6d0e4a8 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > @@ -1277,8 +1277,8 @@ static int ks_pcie_probe(struct platform_device *pdev)
> >   	if (irq < 0)
> >   		return irq;
> > -	ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED,
> > -			  "ks-pcie-error-irq", ks_pcie);
> > +	ret = devm_request_irq(dev, irq, ks_pcie_err_irq_handler, IRQF_SHARED,
> > +			       "ks-pcie-error-irq", ks_pcie);
> >   	if (ret < 0) {
> >   		dev_err(dev, "failed to request error IRQ %d\n",
> >   			irq);
> 
> Ugh, so you are not removing any free_irq() from anywhere?
> 
> <me checking>
> 
> Because there is none...
> 
> So you are actually fixing an IRQ leak in case something later fails -- I
> guess this needs Fixes and Cc stable tags, right?

Yes! While working on this series and unloading the module, I had seen
the issue and narrowed it down to the absence of a free_irq() in the
driver's .remove callback. Since the driver cannot be unloaded prior to
this series (driver can only be built-in), I had felt that a separate
fix may not be required for adding the missing free_irq() in the .remove
callback. However, after you pointed it out now, I realize that there
are other places during the driver's probe where we may exit in the
event of an error, without freeing the interrupt. Thank you for noticing
this. The commit to be fixed is:
0790eb175ee0 PCI: keystone: Cleanup error_irq configuration
Link to the commit is:
https://github.com/torvalds/linux/commit/0790eb175ee0
I will include the Fixes tag for the above commit and also Cc stable when
I post the v2 series. I will wait for feedback on the remaining patches
in the series before posting the v2 series.

Regards,
Siddharth.