Re: [PATCH v4 2/2] pmdomain: imx: Fix i.MX8MP VC8000E power up sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A registered PM domain power notifier is not removed if `devm_of_platform_populate()` fails during probe, leading to a Use-After-Free.
- [High] Missing `pm_runtime_put_noidle()` for `domain->power_dev` when `pm_runtime_get_sync()` fails in `imx8m_blk_ctrl_power_on()`.
--

commit 66b12e5f806bfaa002428850f133acef50ed1a55
Author: Peng Fan <peng.fan@nxp.com>

pmdomain: imx: Fix i.MX8MP VC8000E power up sequence

This patch addresses an errata for the i.MX8MP VPU_NOC power down handshake,
which may hang during power cycles due to timing issues between the VC8000E
reset and AXI clock. It introduces a boolean flag to apply a workaround that
clears the clock before powering up the GPC and enables it afterward.

> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> index e13a47eeed75d..99d100e1d923f 100644
> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c

[ ... ]

> @@ -108,7 +117,11 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  		dev_err(bc->dev, "failed to enable clocks\n");
>  		goto bus_put;
>  	}
> -	regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> +
> +	if (data->is_errata_err050531)
> +		regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> +	else
> +		regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
>  
>  	/* power up upstream GPC domain */
>  	ret = pm_runtime_get_sync(domain->power_dev);
> @@ -117,6 +130,9 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  		goto clk_disable;

[Severity: High]
This is a pre-existing issue, but does this error path leak the runtime PM
reference?

When pm_runtime_get_sync(domain->power_dev) fails, it returns a negative
error code but the usage counter remains incremented.

The error path jumps to the clk_disable label:

clk_disable:
	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
bus_put:
	pm_runtime_put(bc->bus_power_dev);

	return ret;

Should there be a call to pm_runtime_put_noidle(domain->power_dev) here to
balance the counter and prevent the device from remaining permanently active?

>  	}
>  
> +	if (data->is_errata_err050531)
> +		regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> +

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk in the
imx8m_blk_ctrl_probe() error path?

If devm_of_platform_populate() fails, the code branches to the
cleanup_provider label:

	ret = devm_of_platform_populate(dev);
	if (ret)
		goto cleanup_provider;

The cleanup path explicitly detaches bc->bus_power_dev but never unregisters
the power_nb notifier from it:

cleanup_provider:
	of_genpd_del_provider(dev->of_node);
cleanup_pds:
	for (i--; i >= 0; i--) {
		pm_genpd_remove(&bc->domains[i].genpd);
		dev_pm_domain_detach(bc->domains[i].power_dev, true);
	}

	dev_pm_domain_detach(bc->bus_power_dev, true);

	return ret;

Since bc is devm-allocated, the memory containing bc->power_nb will be freed
when probe fails. When the PM domain later changes state, the core will
iterate over the notifier chain and dereference the freed memory.

Should dev_pm_genpd_remove_notifier() be called before returning to prevent
the freed notifier block from remaining in the generic PM domain's list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-b4-imx8mp-vc8000e-pm-v4-1-v4-0-ea58ce929c84@nxp.com?part=2