[v3,2/2] PCI: qcom: Add support for modular builds

Allow the Qualcomm PCIe controller driver to be built as a module, which
is useful for multi-platform kernels as well as during development.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/Kconfig     |  2 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> Allow the Qualcomm PCIe controller driver to be built as a module, which
> is useful for multi-platform kernels as well as during development.

There are two different goals here, and there's no real reason to
bundle them together:

  1) Make qcom a loadable module.  This is a hard requirement so
     multi-platform kernels don't need to build in all drivers
     statically.

  2) Make qcom unloadable.  This is a high want, possibly even a
     requirement for developers, but is not really a big issue for
     users.

There are different changes required: 1) requires the Kconfig change;
2) requires .remove() to be implemented.  Since there's no requirement
that these be done together, let's split them into separate patches.

Then we can make sure that at least 1) gets done, and if for any
reason 2) isn't safe or breaks something, we can at least bisect and
if necessary revert it without losing 1).

> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 26 +++++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 62ce3abf0f19..230f56d1a268 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -168,7 +168,7 @@ config PCI_HISI
>  	  Hip05 and Hip06 SoCs
>  
>  config PCIE_QCOM
> -	bool "Qualcomm PCIe controller"
> +	tristate "Qualcomm PCIe controller"
>  	depends on OF && (ARCH_QCOM || COMPILE_TEST)
>  	depends on PCI_MSI_IRQ_DOMAIN
>  	select PCIE_DW_HOST
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 417be4d225ed..699172c22ed4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -17,7 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> -#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pci.h>
> @@ -1820,6 +1820,21 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int qcom_pcie_remove(struct platform_device *pdev)
> +{
> +	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	dw_pcie_host_deinit(&pcie->pci->pp);
> +
> +	phy_exit(pcie->phy);
> +
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> @@ -1841,6 +1856,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, qcom_pcie_match);
>  
>  static void qcom_fixup_class(struct pci_dev *dev)
>  {
> @@ -1856,10 +1872,14 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
>  
>  static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
> +	.remove = qcom_pcie_remove,
>  	.driver = {
>  		.name = "qcom-pcie",
> -		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
>  };
> -builtin_platform_driver(qcom_pcie_driver);
> +module_platform_driver(qcom_pcie_driver);
> +
> +MODULE_AUTHOR("Stanimir Varbanov <svarbanov@mm-sol.com>");
> +MODULE_DESCRIPTION("Qualcomm PCIe root complex driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.37.3
>

On Mon, Oct 17, 2022 at 12:34:22PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > is useful for multi-platform kernels as well as during development.
> 
> There are two different goals here, and there's no real reason to
> bundle them together:
> 
>   1) Make qcom a loadable module.  This is a hard requirement so
>      multi-platform kernels don't need to build in all drivers
>      statically.
> 
>   2) Make qcom unloadable.  This is a high want, possibly even a
>      requirement for developers, but is not really a big issue for
>      users.
> 
> There are different changes required: 1) requires the Kconfig change;
> 2) requires .remove() to be implemented.  Since there's no requirement
> that these be done together, let's split them into separate patches.
> 
> Then we can make sure that at least 1) gets done, and if for any
> reason 2) isn't safe or breaks something, we can at least bisect and
> if necessary revert it without losing 1).

Implementing 1) in itself requires more than simply splitting this
patch. And I don't think we should be making life harder for developers,
as well as users assisting during debugging, by going in that direction.

We have tons of modules in the kernel and very few that cannot be
unloaded. Anyone who doesn't trust root to not unload modules can
always disable unloading completely using CONFIG_MODULE_UNLOAD.

Johan

On Tue, Oct 18, 2022 at 05:10:53PM +0200, Johan Hovold wrote:
> On Mon, Oct 17, 2022 at 12:34:22PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> > > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > > is useful for multi-platform kernels as well as during development.
> > 
> > There are two different goals here, and there's no real reason to
> > bundle them together:
> > 
> >   1) Make qcom a loadable module.  This is a hard requirement so
> >      multi-platform kernels don't need to build in all drivers
> >      statically.
> > 
> >   2) Make qcom unloadable.  This is a high want, possibly even a
> >      requirement for developers, but is not really a big issue for
> >      users.
> > 
> > There are different changes required: 1) requires the Kconfig change;
> > 2) requires .remove() to be implemented.  Since there's no requirement
> > that these be done together, let's split them into separate patches.
> > 
> > Then we can make sure that at least 1) gets done, and if for any
> > reason 2) isn't safe or breaks something, we can at least bisect and
> > if necessary revert it without losing 1).
> 
> Implementing 1) in itself requires more than simply splitting this
> patch. And I don't think we should be making life harder for developers,
> as well as users assisting during debugging, by going in that direction.

If you're saying this patch can't be split, can you elaborate on the
details of *why* it can't be split?

> We have tons of modules in the kernel and very few that cannot be
> unloaded. Anyone who doesn't trust root to not unload modules can
> always disable unloading completely using CONFIG_MODULE_UNLOAD.

This is all true, but IIUC, the issue is about unloading IRQ
controller drivers, and this doesn't address that.  I don't have a
clear understanding of the issue, and it would be nice if a patch that
specifically added unloadability could elaborate on that.  Then we can
decide that "yes, this is a risk, and we're willing to accept it." An
argument that "tons of modules do this" totally avoids the issues of
this particular case.

Bjorn