| Message ID | 20220420093449.38054-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: microchip: Allow driver to be built as a module | expand |
On Wednesday 20 April 2022 11:34:49 Uwe Kleine-König wrote: > There are no known reasons to not use this driver as a module, Hello! I think that there are reasons. pcie-microchip-host.c driver uses builtin_platform_driver() and not module_platform_driver(); it does not implement .remove driver callback and also has set suppress_bind_attrs to true. I think that all these parts should be properly implemented otherwise it does not have sane reasons to use driver as loadable and unloadable module. Btw, I implemented proper module support for pci-mvebu.c driver recently, so you can take an inspiration. See: https://lore.kernel.org/linux-pci/20211126144307.7568-1-pali@kernel.org/t/#u > so allow to configure PCIE_MICROCHIP_HOST=m. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pci/controller/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index b8d96d38064d..6eae2289410a 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -301,7 +301,7 @@ config PCI_LOONGSON > Loongson systems. > > config PCIE_MICROCHIP_HOST > - bool "Microchip AXI PCIe host bridge support" > + tristate "Microchip AXI PCIe host bridge support" > depends on PCI_MSI && OF > select PCI_MSI_IRQ_DOMAIN > select GENERIC_MSI_IRQ_DOMAIN > > base-commit: 3123109284176b1532874591f7c81f3837bbdc17 > prerequisite-patch-id: e8aad0ef8193038684bc2e10d387a7b74da1116a > -- > 2.35.1 >
On 20/04/2022 16:41, Pali Rohár wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wednesday 20 April 2022 11:34:49 Uwe Kleine-König wrote: >> There are no known reasons to not use this driver as a module, > > Hello! I think that there are reasons. pcie-microchip-host.c driver uses > builtin_platform_driver() and not module_platform_driver(); it does not > implement .remove driver callback and also has set suppress_bind_attrs > to true. I think that all these parts should be properly implemented > otherwise it does not have sane reasons to use driver as loadable and > unloadable module. > > Btw, I implemented proper module support for pci-mvebu.c driver > recently, so you can take an inspiration. See: > https://lore.kernel.org/linux-pci/20211126144307.7568-1-pali@kernel.org/t/#u Hmm, so what is the way forward here, are you happy to do it yourself or do you not have the hardware/would rather that we did it? If you'd prefer that we did it, do we change the driver & submit that as a series with this patch as patch 2/2? Or should it be a single patch with your suggested-by? Not quite sure what the expectation is with attestation for something like this. Thanks, Conor
On Thursday 21 April 2022 11:31:16 Conor.Dooley@microchip.com wrote: > On 20/04/2022 16:41, Pali Rohár wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Wednesday 20 April 2022 11:34:49 Uwe Kleine-König wrote: > >> There are no known reasons to not use this driver as a module, > > > > Hello! I think that there are reasons. pcie-microchip-host.c driver uses > > builtin_platform_driver() and not module_platform_driver(); it does not > > implement .remove driver callback and also has set suppress_bind_attrs > > to true. I think that all these parts should be properly implemented > > otherwise it does not have sane reasons to use driver as loadable and > > unloadable module. > > > > Btw, I implemented proper module support for pci-mvebu.c driver > > recently, so you can take an inspiration. See: > > https://lore.kernel.org/linux-pci/20211126144307.7568-1-pali@kernel.org/t/#u > > Hmm, so what is the way forward here, are you happy to do it yourself > or do you not have the hardware/would rather that we did it? Hello! It would be needed to implement remove callback. But I do not have hardware for doing and testing it, so I do not feel that I can do it. I think that somebody with hardware and documentation should look at it and decide what is required to do in remove/cleanup procedure. Also it would be needed to investigate if something more is needed to change builtin_platform_driver() to module_platform_driver(). If there are not some other steps which needs to be done in correct sequence and usage of builtin_platform_driver() currently ensures it. > If you'd prefer that we did it, do we change the driver & submit that > as a series with this patch as patch 2/2? Or should it be a single > patch with your suggested-by? Feel free to put it into one patch. It is single change which implements one new feature = module support. > Not quite sure what the expectation is with attestation for something > like this. > > Thanks, > Conor
On 21/04/2022 13:41, Pali Rohár wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thursday 21 April 2022 11:31:16 Conor.Dooley@microchip.com wrote: >> On 20/04/2022 16:41, Pali Rohár wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Wednesday 20 April 2022 11:34:49 Uwe Kleine-König wrote: >>>> There are no known reasons to not use this driver as a module, >>> >>> Hello! I think that there are reasons. pcie-microchip-host.c driver uses >>> builtin_platform_driver() and not module_platform_driver(); it does not >>> implement .remove driver callback and also has set suppress_bind_attrs >>> to true. I think that all these parts should be properly implemented >>> otherwise it does not have sane reasons to use driver as loadable and >>> unloadable module. >>> >>> Btw, I implemented proper module support for pci-mvebu.c driver >>> recently, so you can take an inspiration. See: >>> https://lore.kernel.org/linux-pci/20211126144307.7568-1-pali@kernel.org/t/#u >> >> Hmm, so what is the way forward here, are you happy to do it yourself >> or do you not have the hardware/would rather that we did it? > > Hello! It would be needed to implement remove callback. But I do not > have hardware for doing and testing it, so I do not feel that I can do > it. I think that somebody with hardware and documentation should look at > it and decide what is required to do in remove/cleanup procedure. > > Also it would be needed to investigate if something more is needed to > change builtin_platform_driver() to module_platform_driver(). If there > are not some other steps which needs to be done in correct sequence and > usage of builtin_platform_driver() currently ensures it. Was more wondering if this was something Uwe had hardware for than yourself, since he was poking around at the driver. But (assuming he doesnt either) I'll add this to our todo :) > >> If you'd prefer that we did it, do we change the driver & submit that >> as a series with this patch as patch 2/2? Or should it be a single >> patch with your suggested-by? > > Feel free to put it into one patch. It is single change which implements > one new feature = module support. I probably should have specified, it'd be Uwe's suggested-by once merged into a single patch. Thanks, Conor.
On Thu, Apr 21, 2022 at 01:55:01PM +0000, Conor.Dooley@microchip.com wrote: > On 21/04/2022 13:41, Pali Rohár wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Thursday 21 April 2022 11:31:16 Conor.Dooley@microchip.com wrote: > >> On 20/04/2022 16:41, Pali Rohár wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> > >>> On Wednesday 20 April 2022 11:34:49 Uwe Kleine-König wrote: > >>>> There are no known reasons to not use this driver as a module, > >>> > >>> Hello! I think that there are reasons. pcie-microchip-host.c driver uses > >>> builtin_platform_driver() and not module_platform_driver(); it does not > >>> implement .remove driver callback and also has set suppress_bind_attrs > >>> to true. I think that all these parts should be properly implemented > >>> otherwise it does not have sane reasons to use driver as loadable and > >>> unloadable module. > >>> > >>> Btw, I implemented proper module support for pci-mvebu.c driver > >>> recently, so you can take an inspiration. See: > >>> https://lore.kernel.org/linux-pci/20211126144307.7568-1-pali@kernel.org/t/#u > >> > >> Hmm, so what is the way forward here, are you happy to do it yourself > >> or do you not have the hardware/would rather that we did it? > > > > Hello! It would be needed to implement remove callback. But I do not > > have hardware for doing and testing it, so I do not feel that I can do > > it. I think that somebody with hardware and documentation should look at > > it and decide what is required to do in remove/cleanup procedure. > > > > Also it would be needed to investigate if something more is needed to > > change builtin_platform_driver() to module_platform_driver(). If there > > are not some other steps which needs to be done in correct sequence and > > usage of builtin_platform_driver() currently ensures it. > > Was more wondering if this was something Uwe had hardware for than > yourself, since he was poking around at the driver. But (assuming he > doesnt either) I'll add this to our todo :) FTR: I don't have the hardware, I just touched the driver because I found that missing ; that didn't hurt with the driver =y. Wondering why it was bool was just the obvious next thought. Best regards Uwe
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index b8d96d38064d..6eae2289410a 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -301,7 +301,7 @@ config PCI_LOONGSON Loongson systems. config PCIE_MICROCHIP_HOST - bool "Microchip AXI PCIe host bridge support" + tristate "Microchip AXI PCIe host bridge support" depends on PCI_MSI && OF select PCI_MSI_IRQ_DOMAIN select GENERIC_MSI_IRQ_DOMAIN
There are no known reasons to not use this driver as a module, so allow to configure PCIE_MICROCHIP_HOST=m. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pci/controller/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 3123109284176b1532874591f7c81f3837bbdc17 prerequisite-patch-id: e8aad0ef8193038684bc2e10d387a7b74da1116a