diff mbox series

[4/5] PCI: aardvark: Implement driver 'remove' function and allow to build it as module

Message ID 20200715142557.17115-5-marek.behun@nic.cz
State New
Headers show
Series PCIe aardvark controller improvements | expand

Commit Message

Marek Behun July 15, 2020, 2:25 p.m. UTC
From: Pali Rohár <pali@kernel.org>

Providing driver's 'remove' function allows kernel to bind and unbind devices
from aardvark driver. It also allows to build aardvark driver as a module.

Compiling aardvark as a module simplifies development and debugging of
this driver as it can be reloaded at runtime without the need to reboot
to new kernel.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/Kconfig        |  2 +-
 drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

Comments

Rob Herring July 29, 2020, 6:48 p.m. UTC | #1
On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Providing driver's 'remove' function allows kernel to bind and unbind devices
> from aardvark driver. It also allows to build aardvark driver as a module.
> 
> Compiling aardvark as a module simplifies development and debugging of
> this driver as it can be reloaded at runtime without the need to reboot
> to new kernel.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/pci/controller/Kconfig        |  2 +-
>  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index adddf21fa381..f9da5ff2c517 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -12,7 +12,7 @@ config PCI_MVEBU
>  	select PCI_BRIDGE_EMUL
>  
>  config PCI_AARDVARK
> -	bool "Aardvark PCIe controller"
> +	tristate "Aardvark PCIe controller"
>  	depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
>  	depends on OF
>  	depends on PCI_MSI_IRQ_DOMAIN
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index d5f58684d962..0a5aa6d77f5d 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -14,6 +14,7 @@
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/init.h>
>  #include <linux/phy/phy.h>
> @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  
>  	pcie = pci_host_bridge_priv(bridge);
>  	pcie->pdev = pdev;
> +	platform_set_drvdata(pdev, pcie);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pcie->base = devm_ioremap_resource(dev, res);
> @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int advk_pcie_remove(struct platform_device *pdev)
> +{
> +	struct advk_pcie *pcie = platform_get_drvdata(pdev);
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +
> +	pci_stop_root_bus(bridge->bus);
> +	pci_remove_root_bus(bridge->bus);

Based on pci_host_common_remove() implementation, doesn't this need a 
lock around it (pci_lock_rescan_remove)?

We should probably have a common function (say pci_bridge_remove) to 
implement this. You could use pci_host_common_remove(), but you'd have 
to adjust drvdata.

> +
> +	advk_pcie_remove_msi_irq_domain(pcie);
> +	advk_pcie_remove_irq_domain(pcie);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id advk_pcie_of_match_table[] = {
>  	{ .compatible = "marvell,armada-3700-pcie", },
>  	{},
>  };
> +MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
>  
>  static struct platform_driver advk_pcie_driver = {
>  	.driver = {
>  		.name = "advk-pcie",
>  		.of_match_table = advk_pcie_of_match_table,
> -		/* Driver unloading/unbinding currently not supported */
> -		.suppress_bind_attrs = true,
>  	},
>  	.probe = advk_pcie_probe,
> +	.remove = advk_pcie_remove,
>  };
> -builtin_platform_driver(advk_pcie_driver);
> +module_platform_driver(advk_pcie_driver);
> +
> +MODULE_DESCRIPTION("Aardvark PCIe controller");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.26.2
>
Pali Rohár Aug. 3, 2020, 2:46 p.m. UTC | #2
On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Providing driver's 'remove' function allows kernel to bind and unbind devices
> > from aardvark driver. It also allows to build aardvark driver as a module.
> > 
> > Compiling aardvark as a module simplifies development and debugging of
> > this driver as it can be reloaded at runtime without the need to reboot
> > to new kernel.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/pci/controller/Kconfig        |  2 +-
> >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index adddf21fa381..f9da5ff2c517 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -12,7 +12,7 @@ config PCI_MVEBU
> >  	select PCI_BRIDGE_EMUL
> >  
> >  config PCI_AARDVARK
> > -	bool "Aardvark PCIe controller"
> > +	tristate "Aardvark PCIe controller"
> >  	depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
> >  	depends on OF
> >  	depends on PCI_MSI_IRQ_DOMAIN
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index d5f58684d962..0a5aa6d77f5d 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> > +#include <linux/module.h>
> >  #include <linux/pci.h>
> >  #include <linux/init.h>
> >  #include <linux/phy/phy.h>
> > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >  
> >  	pcie = pci_host_bridge_priv(bridge);
> >  	pcie->pdev = pdev;
> > +	platform_set_drvdata(pdev, pcie);
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	pcie->base = devm_ioremap_resource(dev, res);
> > @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static int advk_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct advk_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > +
> > +	pci_stop_root_bus(bridge->bus);
> > +	pci_remove_root_bus(bridge->bus);
> 
> Based on pci_host_common_remove() implementation, doesn't this need a 
> lock around it (pci_lock_rescan_remove)?

Well, I'm not sure. I looked into other pci drivers and none of
following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
pci_lock_rescan_remove() and pci_unlock_rescan_remove().

Only pci-host-common.c and pci-hyperv.c protect pci_stop_root_bus() and
pci_remove_root_bus() by locks.

> We should probably have a common function (say pci_bridge_remove) to 
> implement this. You could use pci_host_common_remove(), but you'd have 
> to adjust drvdata.

Ok, I agree that some common function could be useful as the same code
(pci_stop_root_bus(); pci_remove_root_bus();) is called in more pci
drivers.

But first we need to know if locks are required or not.

> > +
> > +	advk_pcie_remove_msi_irq_domain(pcie);
> > +	advk_pcie_remove_irq_domain(pcie);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct of_device_id advk_pcie_of_match_table[] = {
> >  	{ .compatible = "marvell,armada-3700-pcie", },
> >  	{},
> >  };
> > +MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
> >  
> >  static struct platform_driver advk_pcie_driver = {
> >  	.driver = {
> >  		.name = "advk-pcie",
> >  		.of_match_table = advk_pcie_of_match_table,
> > -		/* Driver unloading/unbinding currently not supported */
> > -		.suppress_bind_attrs = true,
> >  	},
> >  	.probe = advk_pcie_probe,
> > +	.remove = advk_pcie_remove,
> >  };
> > -builtin_platform_driver(advk_pcie_driver);
> > +module_platform_driver(advk_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("Aardvark PCIe controller");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.26.2
> >
Rob Herring Aug. 3, 2020, 8 p.m. UTC | #3
On Mon, Aug 3, 2020 at 8:46 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> > On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > Providing driver's 'remove' function allows kernel to bind and unbind devices
> > > from aardvark driver. It also allows to build aardvark driver as a module.
> > >
> > > Compiling aardvark as a module simplifies development and debugging of
> > > this driver as it can be reloaded at runtime without the need to reboot
> > > to new kernel.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  drivers/pci/controller/Kconfig        |  2 +-
> > >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index adddf21fa381..f9da5ff2c517 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -12,7 +12,7 @@ config PCI_MVEBU
> > >     select PCI_BRIDGE_EMUL
> > >
> > >  config PCI_AARDVARK
> > > -   bool "Aardvark PCIe controller"
> > > +   tristate "Aardvark PCIe controller"
> > >     depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
> > >     depends on OF
> > >     depends on PCI_MSI_IRQ_DOMAIN
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index d5f58684d962..0a5aa6d77f5d 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/irq.h>
> > >  #include <linux/irqdomain.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/module.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/init.h>
> > >  #include <linux/phy/phy.h>
> > > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > >
> > >     pcie = pci_host_bridge_priv(bridge);
> > >     pcie->pdev = pdev;
> > > +   platform_set_drvdata(pdev, pcie);
> > >
> > >     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >     pcie->base = devm_ioremap_resource(dev, res);
> > > @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > >     return 0;
> > >  }
> > >
> > > +static int advk_pcie_remove(struct platform_device *pdev)
> > > +{
> > > +   struct advk_pcie *pcie = platform_get_drvdata(pdev);
> > > +   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > +
> > > +   pci_stop_root_bus(bridge->bus);
> > > +   pci_remove_root_bus(bridge->bus);
> >
> > Based on pci_host_common_remove() implementation, doesn't this need a
> > lock around it (pci_lock_rescan_remove)?
>
> Well, I'm not sure. I looked into other pci drivers and none of
> following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
> pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
> pci_lock_rescan_remove() and pci_unlock_rescan_remove().

The mutex protects the bus->devices list, so yes I believe it is needed.

Rob
Pali Rohár Aug. 4, 2020, 7:24 a.m. UTC | #4
On Monday 03 August 2020 14:00:37 Rob Herring wrote:
> On Mon, Aug 3, 2020 at 8:46 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> > > On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > >
> > > > Providing driver's 'remove' function allows kernel to bind and unbind devices
> > > > from aardvark driver. It also allows to build aardvark driver as a module.
> > > >
> > > > Compiling aardvark as a module simplifies development and debugging of
> > > > this driver as it can be reloaded at runtime without the need to reboot
> > > > to new kernel.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > >  drivers/pci/controller/Kconfig        |  2 +-
> > > >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> > > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > > index adddf21fa381..f9da5ff2c517 100644
> > > > --- a/drivers/pci/controller/Kconfig
> > > > +++ b/drivers/pci/controller/Kconfig
> > > > @@ -12,7 +12,7 @@ config PCI_MVEBU
> > > >     select PCI_BRIDGE_EMUL
> > > >
> > > >  config PCI_AARDVARK
> > > > -   bool "Aardvark PCIe controller"
> > > > +   tristate "Aardvark PCIe controller"
> > > >     depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
> > > >     depends on OF
> > > >     depends on PCI_MSI_IRQ_DOMAIN
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index d5f58684d962..0a5aa6d77f5d 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include <linux/irq.h>
> > > >  #include <linux/irqdomain.h>
> > > >  #include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/init.h>
> > > >  #include <linux/phy/phy.h>
> > > > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > > >
> > > >     pcie = pci_host_bridge_priv(bridge);
> > > >     pcie->pdev = pdev;
> > > > +   platform_set_drvdata(pdev, pcie);
> > > >
> > > >     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >     pcie->base = devm_ioremap_resource(dev, res);
> > > > @@ -1204,18 +1206,35 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > > >     return 0;
> > > >  }
> > > >
> > > > +static int advk_pcie_remove(struct platform_device *pdev)
> > > > +{
> > > > +   struct advk_pcie *pcie = platform_get_drvdata(pdev);
> > > > +   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > > +
> > > > +   pci_stop_root_bus(bridge->bus);
> > > > +   pci_remove_root_bus(bridge->bus);
> > >
> > > Based on pci_host_common_remove() implementation, doesn't this need a
> > > lock around it (pci_lock_rescan_remove)?
> >
> > Well, I'm not sure. I looked into other pci drivers and none of
> > following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
> > pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
> > pci_lock_rescan_remove() and pci_unlock_rescan_remove().
> 
> The mutex protects the bus->devices list, so yes I believe it is needed.
> 
> Rob

Thank you Rob! It means that all above pci drivers should be fixed too.

I will prepare a new version of aardvark patches with protecting pci
stop/remove functions. And later I can look at some common bridge remove
function for fixing those other pci drivers.
diff mbox series

Patch

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index adddf21fa381..f9da5ff2c517 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -12,7 +12,7 @@  config PCI_MVEBU
 	select PCI_BRIDGE_EMUL
 
 config PCI_AARDVARK
-	bool "Aardvark PCIe controller"
+	tristate "Aardvark PCIe controller"
 	depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d5f58684d962..0a5aa6d77f5d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -14,6 +14,7 @@ 
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/phy/phy.h>
@@ -1114,6 +1115,7 @@  static int advk_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->pdev = pdev;
+	platform_set_drvdata(pdev, pcie);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pcie->base = devm_ioremap_resource(dev, res);
@@ -1204,18 +1206,35 @@  static int advk_pcie_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int advk_pcie_remove(struct platform_device *pdev)
+{
+	struct advk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(bridge->bus);
+	pci_remove_root_bus(bridge->bus);
+
+	advk_pcie_remove_msi_irq_domain(pcie);
+	advk_pcie_remove_irq_domain(pcie);
+
+	return 0;
+}
+
 static const struct of_device_id advk_pcie_of_match_table[] = {
 	{ .compatible = "marvell,armada-3700-pcie", },
 	{},
 };
+MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
 
 static struct platform_driver advk_pcie_driver = {
 	.driver = {
 		.name = "advk-pcie",
 		.of_match_table = advk_pcie_of_match_table,
-		/* Driver unloading/unbinding currently not supported */
-		.suppress_bind_attrs = true,
 	},
 	.probe = advk_pcie_probe,
+	.remove = advk_pcie_remove,
 };
-builtin_platform_driver(advk_pcie_driver);
+module_platform_driver(advk_pcie_driver);
+
+MODULE_DESCRIPTION("Aardvark PCIe controller");
+MODULE_LICENSE("GPL v2");