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
