[V7,2/4] PCI: tegra: Add loadable kernel module support

Message ID 1517289851-14111-3-git-send-email-mmaddireddy@nvidia.com
State Superseded
Headers show
Series
  • Add loadable kernel module and power management support
Related show

Commit Message

Manikanta Maddireddy Jan. 30, 2018, 5:24 a.m.
Implement remove callback function for Tegra PCIe driver to add
loadable kernel module support. Change PCI_TEGRA config to tristate to
allow pci-tegra driver to be build as a module.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
V2:
* no change in this patch
V3:
* use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
V4:
* no change in this patch
V5:
* Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
rebased on linux-next
V6:
* no change in this patch
V7:
* no change in this patch

 drivers/pci/host/Kconfig     |  2 +-
 drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

Comments

Thierry Reding Feb. 13, 2018, 11:39 a.m. | #1
On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
> Implement remove callback function for Tegra PCIe driver to add
> loadable kernel module support. Change PCI_TEGRA config to tristate to
> allow pci-tegra driver to be build as a module.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> V2:
> * no change in this patch
> V3:
> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
> V4:
> * no change in this patch
> V5:
> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
> rebased on linux-next
> V6:
> * no change in this patch
> V7:
> * no change in this patch
> 
>  drivers/pci/host/Kconfig     |  2 +-
>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 38d12980db0f..6fd2a5937804 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -34,7 +34,7 @@ config PCI_FTPCI100
>  	default ARCH_GEMINI
>  
>  config PCI_TEGRA
> -	bool "NVIDIA Tegra PCIe controller"
> +	tristate "NVIDIA Tegra PCIe controller"
>  	depends on ARCH_TEGRA
>  	help
>  	  Say Y here if you want support for the PCIe host controller found
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index e68507f658d8..981f126b14d6 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -35,6 +35,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -2276,6 +2277,12 @@ static const struct file_operations tegra_pcie_ports_ops = {
>  	.release = seq_release,
>  };
>  
> +static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
> +{
> +	debugfs_remove_recursive(pcie->debugfs);
> +	pcie->debugfs = NULL;
> +}
> +
>  static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>  {
>  	struct dentry *file;
> @@ -2292,8 +2299,7 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>  	return 0;
>  
>  remove:
> -	debugfs_remove_recursive(pcie->debugfs);
> -	pcie->debugfs = NULL;
> +	tegra_pcie_debugfs_exit(pcie);
>  	return -ENOMEM;
>  }
>  
> @@ -2311,6 +2317,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  
>  	pcie = pci_host_bridge_priv(host);
>  	host->sysdata = pcie;
> +	platform_set_drvdata(pdev, pcie);
>  
>  	pcie->soc = of_device_get_match_data(dev);
>  	INIT_LIST_HEAD(&pcie->ports);
> @@ -2388,6 +2395,25 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int tegra_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_FS))
> +		tegra_pcie_debugfs_exit(pcie);
> +	pci_stop_root_bus(host->bus);
> +	pci_remove_root_bus(host->bus);
> +	tegra_pcie_disable_ports(pcie);
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		tegra_pcie_disable_msi(pcie);
> +	tegra_pcie_free_resources(pcie);
> +	tegra_pcie_disable_controller(pcie);
> +	tegra_pcie_put_resources(pcie);

I prefer blank lines around conditional blocks for readability, so the
above would be:

	if (IS_ENABLED(CONFIG_DEBUG_FS))
		tegra_pcie_debugfs_exit(pcie);

	pci_stop_root_bus(host->bus);
	pci_remove_root_bus(host->bus);
	tegra_pcie_disable_ports(pcie);

	if (IS_ENABLED(CONFIG_PCI_MSI))
		tegra_pcie_disable_msi(pcie);

	tegra_pcie_free_resources(pcie);
	tegra_pcie_disable_controller(pcie);
	tegra_pcie_put_resources(pcie);

Not sure if Lorenzo or Bjorn have any specific preferences, though. My
Acked-by remains valid, though, since this is nitpicky.
Thierry Reding Feb. 13, 2018, 1:07 p.m. | #2
On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
> Implement remove callback function for Tegra PCIe driver to add
> loadable kernel module support. Change PCI_TEGRA config to tristate to
> allow pci-tegra driver to be build as a module.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> V2:
> * no change in this patch
> V3:
> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
> V4:
> * no change in this patch
> V5:
> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
> rebased on linux-next
> V6:
> * no change in this patch
> V7:
> * no change in this patch
> 
>  drivers/pci/host/Kconfig     |  2 +-
>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)

Trying to build this as a module I get a link failure because
irq_set_msi_desc() is not exported. I thought a patch to do that had
been part of an earlier version of this series. Has that not been merged
yet?

Thierry
Manikanta Maddireddy Feb. 13, 2018, 1:18 p.m. | #3
On 13-Feb-18 6:37 PM, Thierry Reding wrote:
> On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
>> Implement remove callback function for Tegra PCIe driver to add
>> loadable kernel module support. Change PCI_TEGRA config to tristate to
>> allow pci-tegra driver to be build as a module.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> ---
>> V2:
>> * no change in this patch
>> V3:
>> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
>> V4:
>> * no change in this patch
>> V5:
>> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
>> rebased on linux-next
>> V6:
>> * no change in this patch
>> V7:
>> * no change in this patch
>>
>>  drivers/pci/host/Kconfig     |  2 +-
>>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> Trying to build this as a module I get a link failure because
> irq_set_msi_desc() is not exported. I thought a patch to do that had
> been part of an earlier version of this series. Has that not been merged
> yet?
> 
> Thierry
> 
I got a review comment to use generic MSI model instead of deprecated API.
source: https://patchwork.ozlabs.org/patch/841111/

Also arm32 compilation will fail because of missing EXPORT of
tegra_cpuidle_pcie_irqs_in_use()
source: https://patchwork.ozlabs.org/patch/841119/

I need to put some time to prepare patches for these two cases.
Meanwhile I want to get the basic driver structure for LKM to be merged.
I verified locally with these two symbols exported.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Feb. 13, 2018, 1:51 p.m. | #4
On Tue, Feb 13, 2018 at 06:48:49PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 13-Feb-18 6:37 PM, Thierry Reding wrote:
> > On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
> >> Implement remove callback function for Tegra PCIe driver to add
> >> loadable kernel module support. Change PCI_TEGRA config to tristate to
> >> allow pci-tegra driver to be build as a module.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> Acked-by: Thierry Reding <treding@nvidia.com>
> >> ---
> >> V2:
> >> * no change in this patch
> >> V3:
> >> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
> >> V4:
> >> * no change in this patch
> >> V5:
> >> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
> >> rebased on linux-next
> >> V6:
> >> * no change in this patch
> >> V7:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/Kconfig     |  2 +-
> >>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
> >>  2 files changed, 32 insertions(+), 4 deletions(-)
> > 
> > Trying to build this as a module I get a link failure because
> > irq_set_msi_desc() is not exported. I thought a patch to do that had
> > been part of an earlier version of this series. Has that not been merged
> > yet?
> > 
> > Thierry
> > 
> I got a review comment to use generic MSI model instead of deprecated API.
> source: https://patchwork.ozlabs.org/patch/841111/
> 
> Also arm32 compilation will fail because of missing EXPORT of
> tegra_cpuidle_pcie_irqs_in_use()
> source: https://patchwork.ozlabs.org/patch/841119/
> 
> I need to put some time to prepare patches for these two cases.
> Meanwhile I want to get the basic driver structure for LKM to be merged.
> I verified locally with these two symbols exported.

Okay, that's fine. However, you can't request these patches to be merged
if they will break builds. There are people that run automated builders
that will check random configurations, which is bound to eventually get
us a bug report about how this fails to build.

Perhaps in order to get loadable module support merged without these
issues you could omit the Kconfig change making this a tristate symbol.
That way we can get all the structural changes ready and you can work
on the necessary exports or MSI model patches so that we can eventually
flip the symbol to tristate.

By the way, if you do respin for the tristate change, do you mind taking
a look at the blank line changes for readability I mentioned earlier?

Thierry
Manikanta Maddireddy Feb. 13, 2018, 3:26 p.m. | #5
On 13-Feb-18 7:21 PM, Thierry Reding wrote:
> On Tue, Feb 13, 2018 at 06:48:49PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 13-Feb-18 6:37 PM, Thierry Reding wrote:
>>> On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
>>>> Implement remove callback function for Tegra PCIe driver to add
>>>> loadable kernel module support. Change PCI_TEGRA config to tristate to
>>>> allow pci-tegra driver to be build as a module.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>> V2:
>>>> * no change in this patch
>>>> V3:
>>>> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
>>>> V4:
>>>> * no change in this patch
>>>> V5:
>>>> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
>>>> rebased on linux-next
>>>> V6:
>>>> * no change in this patch
>>>> V7:
>>>> * no change in this patch
>>>>
>>>>  drivers/pci/host/Kconfig     |  2 +-
>>>>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>>>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>>
>>> Trying to build this as a module I get a link failure because
>>> irq_set_msi_desc() is not exported. I thought a patch to do that had
>>> been part of an earlier version of this series. Has that not been merged
>>> yet?
>>>
>>> Thierry
>>>
>> I got a review comment to use generic MSI model instead of deprecated API.
>> source: https://patchwork.ozlabs.org/patch/841111/
>>
>> Also arm32 compilation will fail because of missing EXPORT of
>> tegra_cpuidle_pcie_irqs_in_use()
>> source: https://patchwork.ozlabs.org/patch/841119/
>>
>> I need to put some time to prepare patches for these two cases.
>> Meanwhile I want to get the basic driver structure for LKM to be merged.
>> I verified locally with these two symbols exported.
> 
> Okay, that's fine. However, you can't request these patches to be merged
> if they will break builds. There are people that run automated builders
> that will check random configurations, which is bound to eventually get
> us a bug report about how this fails to build.
> 
> Perhaps in order to get loadable module support merged without these
> issues you could omit the Kconfig change making this a tristate symbol.
> That way we can get all the structural changes ready and you can work
> on the necessary exports or MSI model patches so that we can eventually
> flip the symbol to tristate.
> 
> By the way, if you do respin for the tristate change, do you mind taking
> a look at the blank line changes for readability I mentioned earlier?
> 
> Thierry
> 

Taken care of both and published V8.

Manikanta
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Manikanta Maddireddy Feb. 13, 2018, 3:30 p.m. | #6
On 13-Feb-18 7:21 PM, Thierry Reding wrote:
> On Tue, Feb 13, 2018 at 06:48:49PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 13-Feb-18 6:37 PM, Thierry Reding wrote:
>>> On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
>>>> Implement remove callback function for Tegra PCIe driver to add
>>>> loadable kernel module support. Change PCI_TEGRA config to tristate to
>>>> allow pci-tegra driver to be build as a module.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>> V2:
>>>> * no change in this patch
>>>> V3:
>>>> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
>>>> V4:
>>>> * no change in this patch
>>>> V5:
>>>> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
>>>> rebased on linux-next
>>>> V6:
>>>> * no change in this patch
>>>> V7:
>>>> * no change in this patch
>>>>
>>>>  drivers/pci/host/Kconfig     |  2 +-
>>>>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>>>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>>
>>> Trying to build this as a module I get a link failure because
>>> irq_set_msi_desc() is not exported. I thought a patch to do that had
>>> been part of an earlier version of this series. Has that not been merged
>>> yet?
>>>
>>> Thierry
>>>
>> I got a review comment to use generic MSI model instead of deprecated API.
>> source: https://patchwork.ozlabs.org/patch/841111/
>>
>> Also arm32 compilation will fail because of missing EXPORT of
>> tegra_cpuidle_pcie_irqs_in_use()
>> source: https://patchwork.ozlabs.org/patch/841119/
>>
>> I need to put some time to prepare patches for these two cases.
>> Meanwhile I want to get the basic driver structure for LKM to be merged.
>> I verified locally with these two symbols exported.
> 
> Okay, that's fine. However, you can't request these patches to be merged
> if they will break builds. There are people that run automated builders
> that will check random configurations, which is bound to eventually get
> us a bug report about how this fails to build.
> 
> Perhaps in order to get loadable module support merged without these
> issues you could omit the Kconfig change making this a tristate symbol.
> That way we can get all the structural changes ready and you can work
> on the necessary exports or MSI model patches so that we can eventually
> flip the symbol to tristate.
> 
> By the way, if you do respin for the tristate change, do you mind taking
> a look at the blank line changes for readability I mentioned earlier?
> 
> Thierry
> 

Hi Thierry,

Taken care of both and published V8. Since these are simple changes,
I retained your Ack & Tested signoff.

Manikanta
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 38d12980db0f..6fd2a5937804 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -34,7 +34,7 @@  config PCI_FTPCI100
 	default ARCH_GEMINI
 
 config PCI_TEGRA
-	bool "NVIDIA Tegra PCIe controller"
+	tristate "NVIDIA Tegra PCIe controller"
 	depends on ARCH_TEGRA
 	help
 	  Say Y here if you want support for the PCIe host controller found
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e68507f658d8..981f126b14d6 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -35,6 +35,7 @@ 
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -2276,6 +2277,12 @@  static const struct file_operations tegra_pcie_ports_ops = {
 	.release = seq_release,
 };
 
+static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
+{
+	debugfs_remove_recursive(pcie->debugfs);
+	pcie->debugfs = NULL;
+}
+
 static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
 {
 	struct dentry *file;
@@ -2292,8 +2299,7 @@  static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
 	return 0;
 
 remove:
-	debugfs_remove_recursive(pcie->debugfs);
-	pcie->debugfs = NULL;
+	tegra_pcie_debugfs_exit(pcie);
 	return -ENOMEM;
 }
 
@@ -2311,6 +2317,7 @@  static int tegra_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(host);
 	host->sysdata = pcie;
+	platform_set_drvdata(pdev, pcie);
 
 	pcie->soc = of_device_get_match_data(dev);
 	INIT_LIST_HEAD(&pcie->ports);
@@ -2388,6 +2395,25 @@  static int tegra_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int tegra_pcie_remove(struct platform_device *pdev)
+{
+	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		tegra_pcie_debugfs_exit(pcie);
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	tegra_pcie_disable_ports(pcie);
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		tegra_pcie_disable_msi(pcie);
+	tegra_pcie_free_resources(pcie);
+	tegra_pcie_disable_controller(pcie);
+	tegra_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",
@@ -2395,5 +2421,7 @@  static struct platform_driver tegra_pcie_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe = tegra_pcie_probe,
+	.remove = tegra_pcie_remove,
 };
-builtin_platform_driver(tegra_pcie_driver);
+module_platform_driver(tegra_pcie_driver);
+MODULE_LICENSE("GPL");