1. Patchew
  2. linux
  3. [PATCH 0/2] PCI: tegra: Allow building as a module
  4. View patch

[PATCH 2/2] PCI: tegra: Allow building as a module

Aaron Kling via B4 Relay posted 2 patches 7 months, 3 weeks ago
There is a newer version of this series
[PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Aaron Kling via B4 Relay 7 months, 3 weeks ago 
From: Aaron Kling <webgeek1234@gmail.com>

The driver works fine as a module, so allow building as such.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 drivers/pci/controller/Kconfig     | 2 +-
 drivers/pci/controller/pci-tegra.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 9800b768105402d6dd1ba4b134c2ec23da6e4201..a9164dd2eccaead5ae9348c24a5ad75fcb40f507 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -224,7 +224,7 @@ config PCI_HYPERV_INTERFACE
 	  driver.
 
 config PCI_TEGRA
-	bool "NVIDIA Tegra PCIe controller"
+	tristate "NVIDIA Tegra PCIe controller"
 	depends on ARCH_TEGRA || COMPILE_TEST
 	depends on PCI_MSI
 	help
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index b3cdbc5927de3742161310610dc5dcb836f5dd69..c260842695f2e983ae48fd52b43f62dbb9fb5dd3 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2803,3 +2803,6 @@ static struct platform_driver tegra_pcie_driver = {
 	.remove = tegra_pcie_remove,
 };
 module_platform_driver(tegra_pcie_driver);
+MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
+MODULE_DESCRIPTION("NVIDIA PCI host controller driver");
+MODULE_LICENSE("GPL");

-- 
2.48.1
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by kernel test robot 7 months, 3 weeks ago 
Hi Aaron,

kernel test robot noticed the following build errors:

[auto build test ERROR on e3a854b577cb05ceb77c0eba54bfef98a03278fa]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Kling-via-B4-Relay/irqdomain-Export-irq_domain_free_irqs/20250421-110400
base:   e3a854b577cb05ceb77c0eba54bfef98a03278fa
patch link:    https://lore.kernel.org/r/20250420-pci-tegra-module-v1-2-c0a1f831354a%40gmail.com
patch subject: [PATCH 2/2] PCI: tegra: Allow building as a module
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20250421/202504212046.SmbccNZH-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250421/202504212046.SmbccNZH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504212046.SmbccNZH-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in arch/arm/probes/kprobes/test-kprobes.o
ERROR: modpost: "__aeabi_uldivmod" [fs/bcachefs/bcachefs.ko] undefined!
>> ERROR: modpost: "tegra_cpuidle_pcie_irqs_in_use" [drivers/pci/controller/pci-tegra.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Manivannan Sadhasivam 7 months, 3 weeks ago 
On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.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).

- Mani

[1] https://lore.kernel.org/linux-pci/20240715114854.4792-1-kabel@kernel.org

> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>  drivers/pci/controller/Kconfig     | 2 +-
>  drivers/pci/controller/pci-tegra.c | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 9800b768105402d6dd1ba4b134c2ec23da6e4201..a9164dd2eccaead5ae9348c24a5ad75fcb40f507 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -224,7 +224,7 @@ config PCI_HYPERV_INTERFACE
>  	  driver.
>  
>  config PCI_TEGRA
> -	bool "NVIDIA Tegra PCIe controller"
> +	tristate "NVIDIA Tegra PCIe controller"
>  	depends on ARCH_TEGRA || COMPILE_TEST
>  	depends on PCI_MSI
>  	help
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index b3cdbc5927de3742161310610dc5dcb836f5dd69..c260842695f2e983ae48fd52b43f62dbb9fb5dd3 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2803,3 +2803,6 @@ static struct platform_driver tegra_pcie_driver = {
>  	.remove = tegra_pcie_remove,
>  };
>  module_platform_driver(tegra_pcie_driver);
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA PCI host controller driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.48.1
> 
> 

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Aaron Kling 7 months, 3 weeks ago 
On Mon, Apr 21, 2025 at 2:52 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > From: Aaron Kling <webgeek1234@gmail.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.

Sincerely,
Aaron

>
> - Mani
>
> [1] https://lore.kernel.org/linux-pci/20240715114854.4792-1-kabel@kernel.org
>
> > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > ---
> >  drivers/pci/controller/Kconfig     | 2 +-
> >  drivers/pci/controller/pci-tegra.c | 3 +++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 9800b768105402d6dd1ba4b134c2ec23da6e4201..a9164dd2eccaead5ae9348c24a5ad75fcb40f507 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -224,7 +224,7 @@ config PCI_HYPERV_INTERFACE
> >         driver.
> >
> >  config PCI_TEGRA
> > -     bool "NVIDIA Tegra PCIe controller"
> > +     tristate "NVIDIA Tegra PCIe controller"
> >       depends on ARCH_TEGRA || COMPILE_TEST
> >       depends on PCI_MSI
> >       help
> > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > index b3cdbc5927de3742161310610dc5dcb836f5dd69..c260842695f2e983ae48fd52b43f62dbb9fb5dd3 100644
> > --- a/drivers/pci/controller/pci-tegra.c
> > +++ b/drivers/pci/controller/pci-tegra.c
> > @@ -2803,3 +2803,6 @@ static struct platform_driver tegra_pcie_driver = {
> >       .remove = tegra_pcie_remove,
> >  };
> >  module_platform_driver(tegra_pcie_driver);
> > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> > +MODULE_DESCRIPTION("NVIDIA PCI host controller driver");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.48.1
> >
> >
>
> --
> மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Manivannan Sadhasivam 7 months, 3 weeks ago 
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@linaro.org> wrote:
> >
> > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > From: Aaron Kling <webgeek1234@gmail.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).

- Mani

> Sincerely,
> Aaron
> 
> >
> > - Mani
> >
> > [1] https://lore.kernel.org/linux-pci/20240715114854.4792-1-kabel@kernel.org
> >
> > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > ---
> > >  drivers/pci/controller/Kconfig     | 2 +-
> > >  drivers/pci/controller/pci-tegra.c | 3 +++
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index 9800b768105402d6dd1ba4b134c2ec23da6e4201..a9164dd2eccaead5ae9348c24a5ad75fcb40f507 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -224,7 +224,7 @@ config PCI_HYPERV_INTERFACE
> > >         driver.
> > >
> > >  config PCI_TEGRA
> > > -     bool "NVIDIA Tegra PCIe controller"
> > > +     tristate "NVIDIA Tegra PCIe controller"
> > >       depends on ARCH_TEGRA || COMPILE_TEST
> > >       depends on PCI_MSI
> > >       help
> > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > > index b3cdbc5927de3742161310610dc5dcb836f5dd69..c260842695f2e983ae48fd52b43f62dbb9fb5dd3 100644
> > > --- a/drivers/pci/controller/pci-tegra.c
> > > +++ b/drivers/pci/controller/pci-tegra.c
> > > @@ -2803,3 +2803,6 @@ static struct platform_driver tegra_pcie_driver = {
> > >       .remove = tegra_pcie_remove,
> > >  };
> > >  module_platform_driver(tegra_pcie_driver);
> > > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> > > +MODULE_DESCRIPTION("NVIDIA PCI host controller driver");
> > > +MODULE_LICENSE("GPL");
> > >
> > > --
> > > 2.48.1
> > >
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Aaron Kling 7 months, 3 weeks ago 
On Mon, Apr 21, 2025 at 3:54 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.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@linaro.org> wrote:
> > >
> > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > From: Aaron Kling <webgeek1234@gmail.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?

Sincerely,
Aaron
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Manivannan Sadhasivam 7 months, 3 weeks ago 
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@linaro.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@linaro.org> wrote:
> > > >
> > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > > From: Aaron Kling <webgeek1234@gmail.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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Aaron Kling 7 months, 2 weeks ago 
On Sun, Apr 27, 2025 at 10:57 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.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@linaro.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@linaro.org> wrote:
> > > > >
> > > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > > > From: Aaron Kling <webgeek1234@gmail.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
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Manivannan Sadhasivam 7 months, 2 weeks ago 
On Mon, Apr 28, 2025 at 05:39:23PM -0500, Aaron Kling wrote:
> On Sun, Apr 27, 2025 at 10:57 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.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@linaro.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@linaro.org> wrote:
> > > > > >
> > > > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > > > > From: Aaron Kling <webgeek1234@gmail.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?
> 

Yeah, that commit was simply wrong and pointless. But the revert won't be
straightforward. So I'd recommend to simply drop the .remove() callback and the
associated code and finally make the driver tristate.

- Mani

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/2] PCI: tegra: Allow building as a module
Posted by Aaron Kling 7 months, 2 weeks ago 
On Mon, Apr 28, 2025 at 5:39 PM Aaron Kling <webgeek1234@gmail.com> wrote:
>
> On Sun, Apr 27, 2025 at 10:57 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.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@linaro.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@linaro.org> wrote:
> > > > > >
> > > > > > On Sun, Apr 20, 2025 at 09:59:06PM -0500, Aaron Kling via B4 Relay wrote:
> > > > > > > From: Aaron Kling <webgeek1234@gmail.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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.