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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALHNRZ-vVzNzfJRMM+i044qwvuv-bm0hB8fTZu0XQJA_qT9Mow@mail.gmail.com>
Date: Mon, 28 Apr 2025 17:39:23 -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 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?

Sincerely,
Aaron

[0] https://github.com/torvalds/linux/commit/662b94c3195654c225174c680094555c0d750d41