linux-kernel - Re: [PATCH 2/2] PCI: tegra: Allow building as a module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CALHNRZ_r4H=QY8K9L8vWSzDCoym8O9DBmeTnQi5LeVVb+6Vn=w@mail.gmail.com>
Date: Mon, 28 Apr 2025 20:03:05 -0500
From: Aaron Kling <webgeek1234@...il.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Thierry Reding <thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>, 
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, 
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH 2/2] PCI: tegra: Allow building as a module

On Mon, Apr 28, 2025 at 5:39 PM Aaron Kling <webgeek1234@...il.com> wrote:
>
> On Sun, Apr 27, 2025 at 10:57 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@...aro.org> 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@...aro.org> 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@...aro.org> wrote:
> > > > > >
> > > > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > > > > From: Aaron Kling <webgeek1234@...il.com>
> > > > > > >
> > > > > > > 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?

After some actual testing, I note that dropping the remove callback
does not affect whether a module can be unloaded. However, dropping
the module exit routine does. So I've folded that into the v2 that I'm
pushing shortly.

Sincerely,
Aaron