On Sun, Apr 27, 2025 at 10:57 AM Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > On Mon, Apr 21, 2025 at 11:33:01AM -0500, Aaron Kling wrote: > > On Mon, Apr 21, 2025 at 3:54 AM Manivannan Sadhasivam > > <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > > > > > On Mon, Apr 21, 2025 at 03:09:42AM -0500, Aaron Kling wrote: > > > > On Mon, Apr 21, 2025 at 2:52 AM Manivannan Sadhasivam > > > > <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > > > > > > > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote: > > > > > > From: Aaron Kling <webgeek1234@xxxxxxxxx> > > > > > > > > > > > > The driver works fine as a module, so allow building as such. > > > > > > > > > > > > > > > > In the past, the former irqchip maintainer raised concerns for allowing the > > > > > irqchip drivers to be removed from the kernel. The concern was mostly (afaik) > > > > > due to not disposing all IRQs before removing the irq_domain. > > > > > > > > > > So Marek submitted a series [1] that added a new API for that. But that series > > > > > didn't progress further. So if you want to make this driver a module, you need > > > > > to do 2 things: > > > > > > > > > > 1. Make sure the cited series gets merged and this driver uses the new API. > > > > > 2. Get an Ack from Thomas (who is the only irqchip maintainer now). > > > > > > > > Should this be a hard blocker for building this one driver as a > > > > module? I did a quick grep of drivers/pci/controller for irq_domain, > > > > then compared several of the hits to the Kconfig. And every single one > > > > is tristate. Tegra is by far not a unique offender here. > > > > > > > > > > Not 'unique', yes. But the situation is a bit worse atm. Some of the patches > > > (making the driver as a module) were merged in the past without addressing the > > > mapping issue. > > > > > > Please take a look at the reply from Marc: > > > https://lkml.iu.edu/hypermail/linux/kernel/2207.2/08367.html > > > > > > Even though Marc said that disposing IRQs is not enough to make sure there are > > > no dangling pointers of the IRQs in the client drivers, I'm inclined to atleast > > > allow modular drivers if they could dispose all the mappings with the new API. > > > This doesn't mean that I'm not cared about the potential issue, but the removing > > > of modules is always an 'experimental' feature in the kernel. So users should be > > > aware of what they are doing. Also, we have not seen any reported issues after > > > disposing the IRQs from the controller drivers. That also adds to my view on > > > this issue. > > > > > > That being said, the safest option would be to get rid of the remove callback > > > and make the module modular. This will allow the driver to be built as a module > > > but never getting removed (make sure .suppress_bind_attrs is also set). > > .suppress_bind_attrs is already set in this driver. But what happens > > cleanup on shutdown if the remove is dropped? Would it be better to > > move remove to shutdown for this case? > > > > remove() won't be called on shutdown path, you need to populate the shutdown() > callback for that. But do note that both remove() and shutdown() serves > different purpose, so do not just rename the function. I did some more looking into this today and came across 662b94c3195654 [0]. That commit stated to add support to the driver to be built as a module, but didn't touch the Kconfig, so I'm unsure how that ever worked. But Manivannan, can you please take a look at that commit and its message? I'm not familiar with pcie and what is required for proper de-initialization. That commit added the remove method for the stated purpose of making the driver a module. Implying that none of that was needed on shutdown when built-in. So if the intent is to make a permanent module which cannot be unloaded, as you're asking for, does that mean it's safe to just fully revert that commit? Sincerely, Aaron [0] https://github.com/torvalds/linux/commit/662b94c3195654c225174c680094555c0d750d41
