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

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

[Aaron Kling via B4 Relay]

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
Aaron Kling (2):
      irqdomain: Export irq_domain_free_irqs
      PCI: tegra: Allow building as a module

 drivers/pci/controller/Kconfig     | 2 +-
 drivers/pci/controller/pci-tegra.c | 3 +++
 kernel/irq/irqdomain.c             | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)
---
base-commit: e3a854b577cb05ceb77c0eba54bfef98a03278fa
change-id: 20250313-pci-tegra-module-7cbd1c5e70af

Best regards,
-- 
Aaron Kling <webgeek1234@gmail.com>

[PATCH 1/2] irqdomain: Export irq_domain_free_irqs

[Aaron Kling via B4 Relay]

From: Aaron Kling <webgeek1234@gmail.com>

Add export for irq_domain_free_irqs() so that we can allow drivers like
the pci-tegra driver to be loadable as a module.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 kernel/irq/irqdomain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ec6d8e72d980f604ded2bfa2143420e0e0095920..36cd79a8a2ce960b07b03c40067343ec8f632452 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1890,6 +1890,7 @@ void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs)
 	irq_domain_free_irq_data(virq, nr_irqs);
 	irq_free_descs(virq, nr_irqs);
 }
+EXPORT_SYMBOL_GPL(irq_domain_free_irqs);
 
 static void irq_domain_free_one_irq(struct irq_domain *domain, unsigned int virq)
 {

-- 
2.48.1

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

[Aaron Kling via B4 Relay]

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

[Manivannan Sadhasivam]

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

[Aaron Kling]

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

[Manivannan Sadhasivam]

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

[kernel test robot]

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

[Aaron Kling]

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

[Aaron Kling]

On Mon, Apr 21, 2025 at 9:20 AM Nicolas Chauvet <kwizart@gmail.com> wrote:
>
> Le lun. 21 avr. 2025 à 04:59, Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org> a écrit :
> >
> > 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");
>
> Thanks for looking into this.
>
> Last time I checked, building PCI as a module didn't work on Trimslice (Tegra20) at runtime. PCI ethernet failed to bring-up.
> It might be because of the tegra_cpuidle_pcie_irqs_in_use in drivers/cpuidle/cpuidle-tegra.c, see the comments...
> At least this function would need EXPORT_SYMBOL_GPL IIRC.

Ah, I've only been building and testing on arm64, tegra210 and
tegra186. So I missed the arm issues. Not sure I can easily get my
tegra114 or tegra124 setups booting again right now either. Don't have
anything older. But I can at least make sure it builds. Will fix for
v2.

Sincerely,
Aaron

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

[Manivannan Sadhasivam]

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

[Aaron Kling]

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

[Aaron Kling]

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

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்