| Message ID | 20260902164313.2901302-1-hanzhijian1991@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] PCI: imx6: fix resource leaks in probe error paths | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Replacing DL_FLAG_STATELESS with DL_FLAG_AUTOREMOVE_CONSUMER for device links to virtual PM domain devices creates managed links that violate the driver core state machine, triggering a WARN_ON during successful probe. -- commit 36162fbf1662cbc9312c01c7d2c2113d6c77e624 Author: Zhijian Han <hanzhijian1991@gmail.com> PCI: imx6: fix resource leaks in probe error paths This commit fixes resource leaks involving pwrctrl devices and power domains in the imx_pcie_probe() error paths. It adds a devm action to detach the power domains and replaces the stateless device link flag with an autoremove consumer flag so links are cleaned up automatically. Link: https://lore.kernel.org/all/20260822013640.182C01F000E9@smtp.kernel.org/ > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 39790e66b98dc..ec8339e03d584 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c [ ... ] > @@ -655,24 +672,30 @@ static int imx_pcie_attach_pd(struct device *dev) > if (!imx_pcie->pd_pcie) > return 0; > link = device_link_add(dev, imx_pcie->pd_pcie, > - DL_FLAG_STATELESS | > DL_FLAG_PM_RUNTIME | > - DL_FLAG_RPM_ACTIVE); > + DL_FLAG_RPM_ACTIVE | > + DL_FLAG_AUTOREMOVE_CONSUMER); [Severity: High] Does changing this link from stateless to managed violate the driver core state machine for virtual PM domain devices? Virtual PM domain devices created on the genpd bus never bind to drivers, so their driver status remains DL_DEV_NO_DRIVER. Because of this, device_link_add() will initialize a managed link to DL_STATE_DORMANT. When the consumer device successfully completes its probe, the driver core calls device_links_driver_bound(). That function iterates over all managed supplier links and evaluates: WARN_ON(link->status != DL_STATE_CONSUMER_PROBE) Since this link is dormant, won't this trigger the WARN_ON and turn a successful normal boot sequence into a kernel panic on systems with panic_on_warn enabled? > if (!link) { > dev_err(dev, "Failed to add device_link to pcie pd\n"); > + imx_pcie_detach_pd(imx_pcie); > return -EINVAL; > } > > imx_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy"); > - if (IS_ERR(imx_pcie->pd_pcie_phy)) > - return PTR_ERR(imx_pcie->pd_pcie_phy); > + if (IS_ERR(imx_pcie->pd_pcie_phy)) { > + int ret = PTR_ERR(imx_pcie->pd_pcie_phy); > + > + imx_pcie_detach_pd(imx_pcie); > + return ret; > + } > > link = device_link_add(dev, imx_pcie->pd_pcie_phy, > - DL_FLAG_STATELESS | > DL_FLAG_PM_RUNTIME | > - DL_FLAG_RPM_ACTIVE); > + DL_FLAG_RPM_ACTIVE | > + DL_FLAG_AUTOREMOVE_CONSUMER); [Severity: High] This has the same issue as the pcie power domain link creation above. Since DL_FLAG_AUTOREMOVE_CONSUMER implies a managed link, should these links for virtual PM domain devices remain DL_FLAG_STATELESS to avoid the WARN_ON during device_links_driver_bound()?
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 39790e66b98d..ec8339e03d58 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -639,6 +639,23 @@ static int imx6q_pcie_abort_handler(unsigned long addr, } #endif +static void imx_pcie_detach_pd(struct imx_pcie *imx_pcie) +{ + if (!IS_ERR_OR_NULL(imx_pcie->pd_pcie_phy)) { + dev_pm_domain_detach(imx_pcie->pd_pcie_phy, true); + imx_pcie->pd_pcie_phy = NULL; + } + if (!IS_ERR_OR_NULL(imx_pcie->pd_pcie)) { + dev_pm_domain_detach(imx_pcie->pd_pcie, true); + imx_pcie->pd_pcie = NULL; + } +} + +static void imx_pcie_detach_pd_action(void *data) +{ + imx_pcie_detach_pd(data); +} + static int imx_pcie_attach_pd(struct device *dev) { struct imx_pcie *imx_pcie = dev_get_drvdata(dev); @@ -655,24 +672,30 @@ static int imx_pcie_attach_pd(struct device *dev) if (!imx_pcie->pd_pcie) return 0; link = device_link_add(dev, imx_pcie->pd_pcie, - DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | - DL_FLAG_RPM_ACTIVE); + DL_FLAG_RPM_ACTIVE | + DL_FLAG_AUTOREMOVE_CONSUMER); if (!link) { dev_err(dev, "Failed to add device_link to pcie pd\n"); + imx_pcie_detach_pd(imx_pcie); return -EINVAL; } imx_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy"); - if (IS_ERR(imx_pcie->pd_pcie_phy)) - return PTR_ERR(imx_pcie->pd_pcie_phy); + if (IS_ERR(imx_pcie->pd_pcie_phy)) { + int ret = PTR_ERR(imx_pcie->pd_pcie_phy); + + imx_pcie_detach_pd(imx_pcie); + return ret; + } link = device_link_add(dev, imx_pcie->pd_pcie_phy, - DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | - DL_FLAG_RPM_ACTIVE); + DL_FLAG_RPM_ACTIVE | + DL_FLAG_AUTOREMOVE_CONSUMER); if (!link) { dev_err(dev, "Failed to add device_link to pcie_phy pd\n"); + imx_pcie_detach_pd(imx_pcie); return -EINVAL; } @@ -1955,6 +1978,10 @@ static int imx_pcie_probe(struct platform_device *pdev) if (ret) return ret; + ret = devm_add_action_or_reset(dev, imx_pcie_detach_pd_action, imx_pcie); + if (ret) + return ret; + ret = pci_pwrctrl_create_devices(dev); if (ret) return dev_err_probe(dev, ret, "failed to create pwrctrl devices\n"); @@ -1975,7 +2002,7 @@ static int imx_pcie_probe(struct platform_device *pdev) pm_runtime_no_callbacks(dev); ret = devm_pm_runtime_set_active_enabled(dev); if (ret < 0) - return ret; + goto err_pwrctrl_destroy; } if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_SKIP_L23_READY))
imx_pcie_probe() leaks both pwrctrl devices and power domains on failure: - imx_pcie_attach_pd() attaches the "pcie" and "pcie_phy" power domains and adds device links to them, but nothing detaches the domains on probe failure or deferral, so they leak. - A failure of devm_pm_runtime_set_active_enabled() returns directly without destroying the pwrctrl devices. - A partial failure inside imx_pcie_attach_pd() leaks the power domains that were already attached. Add imx_pcie_detach_pd() to detach the power domains in reverse order of acquisition and register it with devm_add_action_or_reset() so the power domains are released automatically on probe failure. Add DL_FLAG_AUTOREMOVE_CONSUMER to the device links so the driver core removes them automatically when probe fails, instead of tracking and deleting them manually. Reported-by: sashiko-bot@kernel.org Link: https://lore.kernel.org/all/20260822013640.182C01F000E9@smtp.kernel.org/ Fixes: 2c5768344f88 ("PCI: imx6: Move pci_pwrctrl_create_devices() to imx_pcie_probe()") Signed-off-by: Zhijian Han <hanzhijian1991@gmail.com> --- Changes in v5: - Register imx_pcie_detach_pd() with devm_add_action_or_reset() instead of calling it manually on the error paths Changes in v4: - Use DL_FLAG_AUTOREMOVE_CONSUMER so the driver core removes the device links automatically, instead of tracking and deleting them manually - Add a Fixes tag drivers/pci/controller/dwc/pci-imx6.c | 41 ++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 7 deletions(-)