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

[Peng Fan <peng.fan@oss.nxp.com>]

On Wed, Jun 10, 2026 at 01:34:28PM +0000, sashiko-bot@kernel.org wrote:
>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?

Pre-existing issue, I could use a new patch to cover this.

This patch is only for VC8000E errata fix.

>
>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?

Ditto.

Thanks,
Peng.

>
>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