Re: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete error recovery in `scpsys_power_off_internal` leaves hardware paralyzed and causes clock refcount underflows.
- [High] Unbalanced lifecycle operations in `scpsys_power_on` fail to revert hardware state on error.
- [Medium] Malformed device trees lacking access controllers guarantee a NULL pointer dereference in `scpsys_bus_protect_get_regmap`.
--

--- Patch [4]: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
commit 0a6955778a8abcdf284811fe9a9089e2c1cff6d6
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for Direct CTL simple power sequence

Some new SoCs like MT8196, MT6991, and others, have got one
additional power controller (usually in the HFRP Multimedia
block) which needs a simplified power on/off sequence while
using Direct Control strategy.

Domains using the "simple power sequence" are not backed by
the RTFF hardware, have no Bus Protection mechanism, lacks
the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
automatically after getting out of reset.

This simple power sequence is then a subset of the full one
as only needs the enablement of the specific power domain's
clock input and reset (where, again, after getting out of
reset, the ISO and PWR_ON bits are automatically internally
getting flipped) to enable or disable (power on or off).

Moreover, the simple power sequence power domains guarantee
that they always get enabled/disabled after executing the
relevant power sequence (on/off) so, differently from the
others, there is also no need to poll for a PWR_ACK.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 8d49d9ee3cd29..632c5dcdcedaf 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -688,7 +739,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  err_enable_bus_protect:
>  	scpsys_bus_protect_enable(pd, 0);
>  err_disable_sram:
> -	scpsys_sram_disable(pd);
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		scpsys_sram_disable(pd);
>  err_disable_subsys_clks:
>  	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>  		clk_bulk_disable_unprepare(pd->num_subsys_clks,
>  					   pd->subsys_clks);

[Severity: High]
This is a pre-existing issue, but does this error path fail to revert
the hardware state if an error occurs?

If scpsys_bus_protect_disable() or clk_bulk_prepare_enable() fails after
*_pwrseq_on() has successfully executed, the error path jumps to err_pwr_ack.

This path disables the domain's regulator and clocks but fails to execute
*_pwrseq_off() or revert ext_buck_iso:

err_pwr_ack:
	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
err_reg:
	scpsys_regulator_disable(pd->supply);
	return ret;

Could this sever the supply while leaving the domain's internal logic active,
resulting in hardware lockup and state machine desynchronization?

[ ... ]
> @@ -725,15 +778,21 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
>  		return ret;
>  
>  	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
> -		scpsys_modem_pwrseq_off(pd);
> +		ret = scpsys_modem_pwrseq_off(pd);
> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		ret = scpsys_simple_pwrseq_off(pd);
>  	else
> -		scpsys_ctl_pwrseq_off(pd);
> +		ret = scpsys_ctl_pwrseq_off(pd);
> +
> +	if (ret < 0) {
> +		/* Re-enable clocks so that next power off doesn't break the refcount */
> +		int r = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> +
> +		if (r)
> +			dev_warn(scpsys->dev, "Could not re-enable clocks: %d\n", r);
>  
> -	/* wait until PWR_ACK = 0 */
> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
> -				 MTK_POLL_TIMEOUT);
> -	if (ret < 0)
>  		return ret;
> +	}
>  
>  	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);

[Severity: High]
This isn't a bug introduced by this patch, but does the error recovery in
scpsys_power_off_internal() leave the hardware in a paralyzed state?

If pwrseq_off fails, this block restores subsys_clks but leaves the domain's
SRAM disabled, isolation applied, and bus protection disabled, while
returning an error to genpd.

Because an error is returned, genpd treats the domain as still ON, allowing
active consumers to interact with a paralyzed hardware domain.

Additionally, if scpsys_bus_protect_enable() fails earlier in this function:

	ret = scpsys_bus_protect_enable(pd, BUS_PROT_IGNORE_SUBCLK);
	if (ret < 0)
		return ret;

It returns early and bypasses clock recovery entirely, causing a permanent
clock refcount underflow on the next failure.

Can we ensure consistent state on power off errors?

[ ... ]
> @@ -1109,6 +1168,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>  		regmap[2] = NULL;
>  	}
>  
> +	/* If no access controllers are needed, don't allocate and don't fail */
> +	if (num_regmaps == 0) {
> +		scpsys->bus_prot = NULL;
> +		return 0;
> +	}
> +
>  	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
>  					      sizeof(*scpsys->bus_prot), GFP_KERNEL);

[Severity: Medium]
This is a pre-existing issue, but does this result in a NULL pointer
dereference if the device tree is malformed?

If an older SoC's device tree is missing required access controllers,
num_regmaps becomes 0. The probe function succeeds and explicitly sets
scpsys->bus_prot = NULL.

When the domain later attempts to power on, scpsys_bus_protect_enable() sees
that the hardcoded SoC domain data still requests bus protection and
unconditionally dereferences the NULL array in
scpsys_bus_protect_get_regmap():

	unsigned short block_idx = scpsys->bus_prot_index[bpd->bus_prot_block];
	return scpsys->bus_prot[block_idx];

Should we ensure scpsys->bus_prot is not dereferenced when NULL, or perhaps
fail the probe if required controllers are missing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=4