Re: [PATCH v2] firewire: ohci: prevent leak of left-over IRQ on unbind
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-03-01 04:40:42
|
Hi, Thanks for your taking the issue, and sending the patch. But I have a concern about the change, since the allocated IRQ should be released as the part of managed device resource[1]: (include/linux/interrupt.h) devm_request_irq() (kernel/irq/devres.c) ->devm_request_threaded_irq() ->devres_alloc(devm_irq_release) (kernel/irq/devres.c) devrm_irq_release() ->free_irq() In my opinion, the devres mechanism releases the allocated memory when releasing the data of associated device structure. In our case, it is the data of pci_dev structure (precisely, the data of device structure embedded in it). In the call trace of your commit comment, the release should be done in: (drivers/base/dd.c) device_release_driver_internal() ->__device_release_driver() ->device_unbind_cleanup() (drivers/base/devres.c) ->devres_release_all(dev); ->release_nodes() (kernel/irq/devres.c) ->free_irq() However, you encounter the leak actually. I think we have another cause for the leak, but never figured it out. Anyway, I'll further investigate the issue. [1] https://docs.kernel.org/driver-api/driver-model/devres.html On Thu, Feb 29, 2024 at 02:47:59PM +0000, Edmund Raile wrote: > > Commit 5a95f1ded28691e6 ("firewire: ohci: use devres for requested IRQ") > also removed the call to free_irq() in pci_remove(), leading to a > leftover irq of devm_request_irq() at pci_disable_msi() in pci_remove() > when unbinding the driver from the device > > remove_proc_entry: removing non-empty directory 'irq/136', leaking at > least 'firewire_ohci' > Call Trace: > ? remove_proc_entry+0x19c/0x1c0 > ? __warn+0x81/0x130 > ? remove_proc_entry+0x19c/0x1c0 > ? report_bug+0x171/0x1a0 > ? console_unlock+0x78/0x120 > ? handle_bug+0x3c/0x80 > ? exc_invalid_op+0x17/0x70 > ? asm_exc_invalid_op+0x1a/0x20 > ? remove_proc_entry+0x19c/0x1c0 > unregister_irq_proc+0xf4/0x120 > free_desc+0x3d/0xe0 > ? kfree+0x29f/0x2f0 > irq_free_descs+0x47/0x70 > msi_domain_free_locked.part.0+0x19d/0x1d0 > msi_domain_free_irqs_all_locked+0x81/0xc0 > pci_free_msi_irqs+0x12/0x40 > pci_disable_msi+0x4c/0x60 > pci_remove+0x9d/0xc0 [firewire_ohci > 01b483699bebf9cb07a3d69df0aa2bee71db1b26] > pci_device_remove+0x37/0xa0 > device_release_driver_internal+0x19f/0x200 > unbind_store+0xa1/0xb0 > > remove irq with devm_free_irq() before pci_disable_msi() > also remove it in fail_msi: of pci_probe() as this would lead to > an identical leak > > Fixes: 5a95f1ded28691e6 ("firewire: ohci: use devres for requested IRQ") > > Signed-off-by: Edmund Raile <edm...@pr...> > > --- > > Using FW643 with vfio-pci required unbinding from firewire_ohci, > doing so currently produces a memory leak due to a leftover irq > which this patch removes. > > The irq can be observed while the driver is loaded and bound: > find /proc/irq -type d -name "firewire_ohci" > > Is it a good idea to submit a patch to devm_request_irq() in > include/linux/interrupt.h to add the function comment > /* > * counterpart: devm_free_irq() > */ > so LSPs show that hint? > > v2 change: corrected patch title > > drivers/firewire/ohci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c > index 9db9290c3269..7bc71f4be64a 100644 > --- a/drivers/firewire/ohci.c > +++ b/drivers/firewire/ohci.c > @@ -3773,6 +3773,7 @@ static int pci_probe(struct pci_dev *dev, > return 0; > > fail_msi: > + devm_free_irq(&dev->dev, dev->irq, ohci); > pci_disable_msi(dev); > > return err; > @@ -3800,6 +3801,7 @@ static void pci_remove(struct pci_dev *dev) > > software_reset(ohci); > > + devm_free_irq(&dev->dev, dev->irq, ohci); > pci_disable_msi(dev); > > dev_notice(&dev->dev, "removing fw-ohci device\n"); > -- > 2.43.0 Thanks Takashi Sakamoto |