| Message ID | 20250505-pci-tegra-module-v4-4-088b552c4b1a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: tegra: Allow building as a module | expand |
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
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>");