Re: [PATCH v2] firewire: ohci: prevent leak of left-over IRQ on unbind

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

(C.C.ed to the list of PCI SUBSYSTEM.)

On Sat, Mar 02, 2024 at 04:52:06PM +0000, Edmund Raile wrote:
> > In my opinion, the devres mechanism releases the allocated memory when
> > releasing the data of associated device structure.
> > 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()
> 
> Looking at __device_release_driver() in drivers/base/dd.c,
> device_remove() gets called, leading to dev->bus->remove(dev),
> which likely calls our good old friend from the call trace:
> pci_device_remove().
> 
> > > 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
> 
> Then in ohci.c's pci_remove(), we kill the MSIs, which leads to
> the removal of the IRQ, etc.
> Back in __device_release_driver(), after device_remove(),
> device_unbind_cleanup() is called, leading to free_irq(), but too late.
> 
> I think the order of these calls may be our issue but I doubt it
> has been done like this without good reason.
> That code is 8 years old, someone would have noticed if it had an error.

Now I got the point. Before optimizing to device managing resource, the
1394 OHCI driver called `free_irq()` then `pci_disable_msi()` in the
.remove() callback. So the issue did not occur. At present, the order is
reversed, as you find.

To be honestly, I have little knowledge about current implementation of
PCIe MSI operation and the current best-practice in Linux PCI subsystem.
I've just replaced the old implementation of the driver with the
relevant APIs, so I need to consult someone about the issue.

> I could be entirely wrong but the function description in
> /kernel/irq/devres.c tells me that function is meant to be used:
> 
> > Except for the extra @dev argument, this function takes the
> > same arguments and performs the same function as free_irq().
> > This function instead of free_irq() should be used to manually
> > free IRQs allocated with devm_request_irq().
> 
> And while devm_request_irq() has no function description of its own, its
> sister devm_request_threaded_irq() mentions this:
> 
> > IRQs requested with this function will be
> > automatically freed on driver detach.
> > 
> > If an IRQ allocated with this function needs to be freed
> > separately, devm_free_irq() must be used.
> 
> Should we pull in the maintainers of dd.c for their opinion?
> 
> Thank you very much for all the very hard work you do Sakamoto-Sensei!

Indeed. If the current implementation of PCIe MSI requires the call of
`free_irq()` (or something) before calling `pci_disable_msi()`, it
should be documented. But we can also see the `pci_disable_msi()` is
legacy API in PCIe MSI implementation[1]. I guess that the extra care of
order to call these two functions would be useless nowadays by some
enhancement.


[1] https://docs.kernel.org/PCI/msi-howto.html#legacy-apis

Thanks

Takashi Sakamoto