From: Aaron Kling <webgeek1234@gmail.com>

Debugfs cleanup is moved to a new shutdown callback to ensure the
debugfs nodes are properly cleaned up on shutdown and reboot.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 drivers/pci/controller/pci-tegra.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 1539d172d708c11c3d085721ab9416be3dea6b12..cc9ca4305ea2072b7395ee1f1e979c24fdea3433 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2674,27 +2674,12 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
-static void tegra_pcie_remove(struct platform_device *pdev)
+static void tegra_pcie_shutdown(struct platform_device *pdev)
 {
 	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
-	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
-	struct tegra_pcie_port *port, *tmp;
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_pcie_debugfs_exit(pcie);
-
-	pci_stop_root_bus(host->bus);
-	pci_remove_root_bus(host->bus);
-	pm_runtime_put_sync(pcie->dev);
-	pm_runtime_disable(pcie->dev);
-
-	if (IS_ENABLED(CONFIG_PCI_MSI))
-		tegra_pcie_msi_teardown(pcie);
-
-	tegra_pcie_put_resources(pcie);
-
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
-		tegra_pcie_port_free(port);
 }
 
 static int tegra_pcie_pm_suspend(struct device *dev)
@@ -2800,7 +2785,7 @@ static struct platform_driver tegra_pcie_driver = {
 		.pm = &tegra_pcie_pm_ops,
 	},
 	.probe = tegra_pcie_probe,
-	.remove = tegra_pcie_remove,
+	.shutdown = tegra_pcie_shutdown,
 };
 builtin_platform_driver(tegra_pcie_driver);
 MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");

-- 
2.48.1

On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.com>
> 
> Debugfs cleanup is moved to a new shutdown callback to ensure the
> debugfs nodes are properly cleaned up on shutdown and reboot.
> 

Both are separate changes. You should remove the .remove() callback in the
previous patch itself and add .shutdown() callback in this patch.

And the shutdown callback should quiesce the device by putting it in L2/L3 state
and turn off the supplies. It is not intended to perform resource cleanup.

- Mani

> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>  drivers/pci/controller/pci-tegra.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 1539d172d708c11c3d085721ab9416be3dea6b12..cc9ca4305ea2072b7395ee1f1e979c24fdea3433 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2674,27 +2674,12 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> -static void tegra_pcie_remove(struct platform_device *pdev)
> +static void tegra_pcie_shutdown(struct platform_device *pdev)
>  {
>  	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> -	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> -	struct tegra_pcie_port *port, *tmp;
>  
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_pcie_debugfs_exit(pcie);
> -
> -	pci_stop_root_bus(host->bus);
> -	pci_remove_root_bus(host->bus);
> -	pm_runtime_put_sync(pcie->dev);
> -	pm_runtime_disable(pcie->dev);
> -
> -	if (IS_ENABLED(CONFIG_PCI_MSI))
> -		tegra_pcie_msi_teardown(pcie);
> -
> -	tegra_pcie_put_resources(pcie);
> -
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> -		tegra_pcie_port_free(port);
>  }
>  
>  static int tegra_pcie_pm_suspend(struct device *dev)
> @@ -2800,7 +2785,7 @@ static struct platform_driver tegra_pcie_driver = {
>  		.pm = &tegra_pcie_pm_ops,
>  	},
>  	.probe = tegra_pcie_probe,
> -	.remove = tegra_pcie_remove,
> +	.shutdown = tegra_pcie_shutdown,
>  };
>  builtin_platform_driver(tegra_pcie_driver);
>  MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> 
> -- 
> 2.48.1
> 
> 

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

On Mon, May 5, 2025 at 11:32 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> > From: Aaron Kling <webgeek1234@gmail.com>
> >
> > Debugfs cleanup is moved to a new shutdown callback to ensure the
> > debugfs nodes are properly cleaned up on shutdown and reboot.
> >
>
> Both are separate changes. You should remove the .remove() callback in the
> previous patch itself and add .shutdown() callback in this patch.
>
> And the shutdown callback should quiesce the device by putting it in L2/L3 state
> and turn off the supplies. It is not intended to perform resource cleanup.

Then where would resource cleanup go?

Sincerely,
Aaron Kling

On Mon, May 05, 2025 at 11:43:26AM -0500, Aaron Kling wrote:
> On Mon, May 5, 2025 at 11:32 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> > > From: Aaron Kling <webgeek1234@gmail.com>
> > >
> > > Debugfs cleanup is moved to a new shutdown callback to ensure the
> > > debugfs nodes are properly cleaned up on shutdown and reboot.
> > >
> >
> > Both are separate changes. You should remove the .remove() callback in the
> > previous patch itself and add .shutdown() callback in this patch.
> >
> > And the shutdown callback should quiesce the device by putting it in L2/L3 state
> > and turn off the supplies. It is not intended to perform resource cleanup.
> 
> Then where would resource cleanup go?
> 

.remove(), but it is not useful here since the driver is not getting removed.

- Mani

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

On Mon, May 5, 2025 at 11:48 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, May 05, 2025 at 11:43:26AM -0500, Aaron Kling wrote:
> > On Mon, May 5, 2025 at 11:32 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> > > > From: Aaron Kling <webgeek1234@gmail.com>
> > > >
> > > > Debugfs cleanup is moved to a new shutdown callback to ensure the
> > > > debugfs nodes are properly cleaned up on shutdown and reboot.
> > > >
> > >
> > > Both are separate changes. You should remove the .remove() callback in the
> > > previous patch itself and add .shutdown() callback in this patch.
> > >
> > > And the shutdown callback should quiesce the device by putting it in L2/L3 state
> > > and turn off the supplies. It is not intended to perform resource cleanup.
> >
> > Then where would resource cleanup go?
> >
>
> .remove(), but it is not useful here since the driver is not getting removed.

I may be misunderstanding how stuff works, but don't those resources
still need cleaned up on shutdown/reboot even if the driver can't be
unloaded? I recall seeing shutdown errors in the past when higher
level debugfs nodes tried to clean themselves up, but bad drivers had
left their nodes behind.

In any case, do you want me to just not add .shutdown() at all, or try
to set the proper power state? Prior to the half-baked attempt to make
this driver a loadable module several years ago, there was no such
cleanup.

Sincerely,
Aaron Kling

On Mon, May 05, 2025 at 11:59:27AM -0500, Aaron Kling wrote:
> On Mon, May 5, 2025 at 11:48 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Mon, May 05, 2025 at 11:43:26AM -0500, Aaron Kling wrote:
> > > On Mon, May 5, 2025 at 11:32 AM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> > > > > From: Aaron Kling <webgeek1234@gmail.com>
> > > > >
> > > > > Debugfs cleanup is moved to a new shutdown callback to ensure the
> > > > > debugfs nodes are properly cleaned up on shutdown and reboot.
> > > > >
> > > >
> > > > Both are separate changes. You should remove the .remove() callback in the
> > > > previous patch itself and add .shutdown() callback in this patch.
> > > >
> > > > And the shutdown callback should quiesce the device by putting it in L2/L3 state
> > > > and turn off the supplies. It is not intended to perform resource cleanup.
> > >
> > > Then where would resource cleanup go?
> > >
> >
> > .remove(), but it is not useful here since the driver is not getting removed.
> 
> I may be misunderstanding how stuff works, but don't those resources
> still need cleaned up on shutdown/reboot even if the driver can't be
> unloaded? I recall seeing shutdown errors in the past when higher
> level debugfs nodes tried to clean themselves up, but bad drivers had
> left their nodes behind.
> 

Each callback has its own purpose. The cleanup is only performed by the
.remove() callback, but it will only get called when the driver gets removed.

> In any case, do you want me to just not add .shutdown() at all, or try
> to set the proper power state? Prior to the half-baked attempt to make
> this driver a loadable module several years ago, there was no such
> cleanup.
> 

As a first step, you can ignore .shutdown() callback and just remove the
.remove(). Shutdown callback implementation should follow the steps I mentioned
above, so it can be done later.

- Mani

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

On Sat, May 10, 2025 at 1:24 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, May 05, 2025 at 11:59:27AM -0500, Aaron Kling wrote:
> > On Mon, May 5, 2025 at 11:48 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Mon, May 05, 2025 at 11:43:26AM -0500, Aaron Kling wrote:
> > > > On Mon, May 5, 2025 at 11:32 AM Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > >
> > > > > On Mon, May 05, 2025 at 09:59:01AM -0500, Aaron Kling via B4 Relay wrote:
> > > > > > From: Aaron Kling <webgeek1234@gmail.com>
> > > > > >
> > > > > > Debugfs cleanup is moved to a new shutdown callback to ensure the
> > > > > > debugfs nodes are properly cleaned up on shutdown and reboot.
> > > > > >
> > > > >
> > > > > Both are separate changes. You should remove the .remove() callback in the
> > > > > previous patch itself and add .shutdown() callback in this patch.
> > > > >
> > > > > And the shutdown callback should quiesce the device by putting it in L2/L3 state
> > > > > and turn off the supplies. It is not intended to perform resource cleanup.
> > > >
> > > > Then where would resource cleanup go?
> > > >
> > >
> > > .remove(), but it is not useful here since the driver is not getting removed.
> >
> > I may be misunderstanding how stuff works, but don't those resources
> > still need cleaned up on shutdown/reboot even if the driver can't be
> > unloaded? I recall seeing shutdown errors in the past when higher
> > level debugfs nodes tried to clean themselves up, but bad drivers had
> > left their nodes behind.
> >
>
> Each callback has its own purpose. The cleanup is only performed by the
> .remove() callback, but it will only get called when the driver gets removed.
>
> > In any case, do you want me to just not add .shutdown() at all, or try
> > to set the proper power state? Prior to the half-baked attempt to make
> > this driver a loadable module several years ago, there was no such
> > cleanup.
> >
>
> As a first step, you can ignore .shutdown() callback and just remove the
> .remove(). Shutdown callback implementation should follow the steps I mentioned
> above, so it can be done later.

This has already been done in v6 [0].

Sincerely,
Aaron

[0]  https://lore.kernel.org/r/20250507-pci-tegra-module-v6-0-5fe363eaa302@gmail.com