[v2] PCI: imx6: fix resource leaks in probe error paths

[PATCH v2] PCI: imx6: fix resource leaks in probe error paths

Posted by hanzhijian 1 month ago

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 creates device links to them, but the device links are only kept
  in a local variable and nothing detaches the domains on probe failure
  or deferral, so all of them 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
  and device links that were already attached.

Save the device links, add imx_pcie_detach_pd() to release the power
domains and device links in reverse order of acquisition, call it from
the probe error paths, and make imx_pcie_attach_pd() release everything
it has attached so far on failure.

Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/all/20260822013640.182C01F000E9@smtp.kernel.org/
Signed-off-by: hanzhijian <hanzhijian1991@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 42 ++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39790e66b..a2bae4a2c 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -182,6 +182,9 @@ struct imx_pcie {
 	struct device		*pd_pcie;
 	/* power domain for pcie phy */
 	struct device		*pd_pcie_phy;
+	/* device links for the power domains */
+	struct device_link	*pd_link;
+	struct device_link	*pd_phy_link;
 	struct phy		*phy;
 	const struct imx_pcie_drvdata *drvdata;
 
@@ -639,6 +642,26 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
 }
 #endif
 
+static void imx_pcie_detach_pd(struct imx_pcie *imx_pcie)
+{
+	if (imx_pcie->pd_phy_link) {
+		device_link_del(imx_pcie->pd_phy_link);
+		imx_pcie->pd_phy_link = NULL;
+	}
+	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 (imx_pcie->pd_link) {
+		device_link_del(imx_pcie->pd_link);
+		imx_pcie->pd_link = 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 int imx_pcie_attach_pd(struct device *dev)
 {
 	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
@@ -660,12 +683,18 @@ static int imx_pcie_attach_pd(struct device *dev)
 			DL_FLAG_RPM_ACTIVE);
 	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_link = link;
 
 	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 |
@@ -673,8 +702,10 @@ static int imx_pcie_attach_pd(struct device *dev)
 			DL_FLAG_RPM_ACTIVE);
 	if (!link) {
 		dev_err(dev, "Failed to add device_link to pcie_phy pd\n");
+		imx_pcie_detach_pd(imx_pcie);
 		return -EINVAL;
 	}
+	imx_pcie->pd_phy_link = link;
 
 	return 0;
 }
@@ -1956,8 +1987,10 @@ static int imx_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = pci_pwrctrl_create_devices(dev);
-	if (ret)
+	if (ret) {
+		imx_pcie_detach_pd(imx_pcie);
 		return dev_err_probe(dev, ret, "failed to create pwrctrl devices\n");
+	}
 
 	pci->use_parent_dt_ranges = true;
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
@@ -1975,7 +2008,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))
@@ -1999,6 +2032,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 err_pwrctrl_destroy:
+	imx_pcie_detach_pd(imx_pcie);
 	if (ret != -EPROBE_DEFER)
 		pci_pwrctrl_destroy_devices(dev);
 	return ret;
-- 
2.43.0

[PATCH v3] PCI: imx6: fix resource leaks in probe error paths

Posted by hanzhijian 1 month ago

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 creates device links to them, but the device links are only kept
  in a local variable and nothing detaches the domains on probe failure
  or deferral, so all of them 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
  and device links that were already attached.

Save the device links, add imx_pcie_detach_pd() to release the power
domains and device links in reverse order of acquisition, call it from
the probe error paths, and make imx_pcie_attach_pd() release everything
it has attached so far on failure.

Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/all/20260822013640.182C01F000E9@smtp.kernel.org/
Signed-off-by: hanzhijian <hanzhijian1991@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 42 ++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39790e66b..467006d83 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -182,6 +182,9 @@ struct imx_pcie {
 	struct device		*pd_pcie;
 	/* power domain for pcie phy */
 	struct device		*pd_pcie_phy;
+	/* device links for the power domains */
+	struct device_link	*pd_link;
+	struct device_link	*pd_phy_link;
 	struct phy		*phy;
 	const struct imx_pcie_drvdata *drvdata;
 
@@ -639,6 +642,26 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
 }
 #endif
 
+static void imx_pcie_detach_pd(struct imx_pcie *imx_pcie)
+{
+	if (imx_pcie->pd_phy_link) {
+		device_link_del(imx_pcie->pd_phy_link);
+		imx_pcie->pd_phy_link = NULL;
+	}
+	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 (imx_pcie->pd_link) {
+		device_link_del(imx_pcie->pd_link);
+		imx_pcie->pd_link = 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 int imx_pcie_attach_pd(struct device *dev)
 {
 	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
@@ -660,12 +683,18 @@ static int imx_pcie_attach_pd(struct device *dev)
 			DL_FLAG_RPM_ACTIVE);
 	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_link = link;
 
 	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 |
@@ -673,8 +702,10 @@ static int imx_pcie_attach_pd(struct device *dev)
 			DL_FLAG_RPM_ACTIVE);
 	if (!link) {
 		dev_err(dev, "Failed to add device_link to pcie_phy pd\n");
+		imx_pcie_detach_pd(imx_pcie);
 		return -EINVAL;
 	}
+	imx_pcie->pd_phy_link = link;
 
 	return 0;
 }
@@ -1956,8 +1987,10 @@ static int imx_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = pci_pwrctrl_create_devices(dev);
-	if (ret)
+	if (ret) {
+		imx_pcie_detach_pd(imx_pcie);
 		return dev_err_probe(dev, ret, "failed to create pwrctrl devices\n");
+	}
 
 	pci->use_parent_dt_ranges = true;
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
@@ -1975,7 +2008,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))
@@ -2001,6 +2034,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
 err_pwrctrl_destroy:
 	if (ret != -EPROBE_DEFER)
 		pci_pwrctrl_destroy_devices(dev);
+	imx_pcie_detach_pd(imx_pcie);
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH v3] PCI: imx6: fix resource leaks in probe error paths

Posted by Frank Li 1 month ago

On Mon, Aug 24, 2026 at 03:55:12PM +0800, hanzhijian wrote:

Don't post new version patch at old thread.

>
> 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 creates device links to them, but the device links are only kept
>   in a local variable and nothing detaches the domains on probe failure
>   or deferral, so all of them 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
>   and device links that were already attached.
>
> Save the device links, add imx_pcie_detach_pd() to release the power
> domains and device links in reverse order of acquisition, call it from
> the probe error paths, and make imx_pcie_attach_pd() release everything
> it has attached so far on failure.
>
> Reported-by: sashiko-bot@kernel.org
> Link: https://lore.kernel.org/all/20260822013640.182C01F000E9@smtp.kernel.org/
> Signed-off-by: hanzhijian <hanzhijian1991@gmail.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 42 ++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 39790e66b..467006d83 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -182,6 +182,9 @@ struct imx_pcie {
>         struct device           *pd_pcie;
>         /* power domain for pcie phy */
>         struct device           *pd_pcie_phy;
> +       /* device links for the power domains */
> +       struct device_link      *pd_link;
> +       struct device_link      *pd_phy_link;
>         struct phy              *phy;
>         const struct imx_pcie_drvdata *drvdata;
>
> @@ -639,6 +642,26 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
>  }
>  #endif
>
> +static void imx_pcie_detach_pd(struct imx_pcie *imx_pcie)
> +{
> +       if (imx_pcie->pd_phy_link) {
> +               device_link_del(imx_pcie->pd_phy_link);
> +               imx_pcie->pd_phy_link = NULL;
> +       }
> +       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 (imx_pcie->pd_link) {
> +               device_link_del(imx_pcie->pd_link);
> +               imx_pcie->pd_link = 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 int imx_pcie_attach_pd(struct device *dev)
>  {
>         struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
> @@ -660,12 +683,18 @@ static int imx_pcie_attach_pd(struct device *dev)
>                         DL_FLAG_RPM_ACTIVE);
>         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_link = link;
>
>         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 |

Does DL_FLAG_AUTOREMOVE_CONSUMER work?

Frank

> @@ -673,8 +702,10 @@ static int imx_pcie_attach_pd(struct device *dev)
>                         DL_FLAG_RPM_ACTIVE);
>         if (!link) {
>                 dev_err(dev, "Failed to add device_link to pcie_phy pd\n");
> +               imx_pcie_detach_pd(imx_pcie);
>                 return -EINVAL;
>         }
> +       imx_pcie->pd_phy_link = link;
>
>         return 0;
>  }
> @@ -1956,8 +1987,10 @@ static int imx_pcie_probe(struct platform_device *pdev)
>                 return ret;
>
>         ret = pci_pwrctrl_create_devices(dev);
> -       if (ret)
> +       if (ret) {
> +               imx_pcie_detach_pd(imx_pcie);
>                 return dev_err_probe(dev, ret, "failed to create pwrctrl devices\n");
> +       }
>
>         pci->use_parent_dt_ranges = true;
>         if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
> @@ -1975,7 +2008,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))
> @@ -2001,6 +2034,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  err_pwrctrl_destroy:
>         if (ret != -EPROBE_DEFER)
>                 pci_pwrctrl_destroy_devices(dev);
> +       imx_pcie_detach_pd(imx_pcie);
>         return ret;
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH v3] PCI: imx6: fix resource leaks in probe error paths

Posted by Markus Elfring 1 month ago

> imx_pcie_probe() leaks both pwrctrl devices and power domains on
> failure:
…

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

See also:
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n145
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2#n34


Would you like to avoid a bit of duplicate source code here?


> Signed-off-by: hanzhijian <hanzhijian1991@gmail.com>

Can an other representation be more desirable for the “personal name”
according to the requirements for the Developer's Certificate of Origin?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n396


> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 42 ++++++++++++++++++++++++---
…

Some contributors would appreciate patch version descriptions.
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n310

Regards,
Markus