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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The return values of the power-off sequence functions (`scpsys_modem_pwrseq_off`, `scpsys_ctl_pwrseq_off`, and `scpsys_simple_pwrseq_off`) are ignored in `scpsys_power_off()`, silently dropping timeouts and masking hardware failures.

Pre-existing issues:
- [High] Incomplete error handling in `scpsys_power_on` leaves the hardware domain powered on if subsequent initialization steps fail.
- [High] Subsystem clocks are left permanently disabled if the bus protection enable step fails during the power-off sequence.
- [High] Missing GENPD_FLAG_NO_STAY_ON for domains supplied by regulators risks delayed regulator cleanup while domains remain active.
- [High] The function `scpsys_power_on()` fails to restore the ext_buck_iso hardware state on its error path.
--

commit 893d0afc69c059985945081fa1e299e1362c2611
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: pmdomain: mediatek: Add support for Direct CTL simple power sequence

This commit introduces support for a Direct CTL simple power sequence for
new MediaTek SoCs. The simple power sequence applies to domains without RTFF
hardware or bus protection, simplifying the power on/off process. It enables
the specific power domain's clock input and reset without needing to poll
for a power acknowledge signal.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index db543d4b1813c..5276adea1d041 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -721,15 +774,11 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>  
>  	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
>  		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);
>  
> -	/* 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]
Does this code silently ignore the return values of the power-off sequence
functions?

By moving the timeout check into the individual off functions and changing
them to return an error code, if readx_poll_timeout() fails, they return the
error.

However, scpsys_power_off() does not check these return values (nor the
assigned ret variable for the simple sequence), meaning the timeout is
completely ignored.

Could this cause the driver to unconditionally tear down clocks and regulators
despite a physical power-off failure, potentially causing a system hang?

[ ... ]

While reviewing this patch, I noticed a few pre-existing issues in the same
file.

[Severity: High]
This is a pre-existing issue, but does the error path in scpsys_power_on()
fail to call the corresponding power-off sequence before disabling clocks and
regulators?

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_power_on() {
    ...
err_pwr_ack:
	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
err_reg:
	scpsys_regulator_disable(pd->supply);
	return ret;
}

If a step like bus protection or SRAM enablement fails during power-on, the
framework considers the domain off, but the physical hardware may remain
powered on without its necessary clocks and regulators.

Could this lead to state desynchronization and potential crashes?

[Severity: High]
This is a pre-existing issue, but are subsystem clocks left permanently
disabled if the bus protection enable step fails during the power-off sequence?

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_power_off() {
    ...
	clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);

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

If scpsys_bus_protect_enable() fails, the operation aborts and returns an
error, leaving the domain considered ON by the genpd framework.

However, because subsystem clocks were already disabled and are never
re-enabled, could subsequent accesses to the domain cause a system crash or
lockup?

[Severity: High]
This is a pre-existing issue, but does scpsys_add_one_domain() implicitly
activate the stay_on mechanism without an opt-out flag?

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_add_one_domain() {
    ...
	if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF))
		pm_genpd_init(&pd->genpd, NULL, true);
	else
		pm_genpd_init(&pd->genpd, NULL, false);
    ...
}

For domains supplied by regulators, if a consumer device does not probe
successfully, the genpd stay_on flag is never cleared because sync_state never
fires.

The regulator core will asynchronously disable unused regulators after a delay,
causing the hardware to lose power while its power domain registers are still
active.

Should this include GENPD_FLAG_NO_STAY_ON to prevent this?

[Severity: High]
This is a pre-existing issue, but does the error path in scpsys_power_on()
fail to restore the ext_buck_iso hardware state?

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_power_on() {
    ...
	if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, MTK_SCPD_EXT_BUCK_ISO))
		regmap_clear_bits(scpsys->base, pd->data->ext_buck_iso_offs,
				  pd->data->ext_buck_iso_mask);
    ...
err_pwr_ack:
	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
err_reg:
	scpsys_regulator_disable(pd->supply);
	return ret;
}

If a step fails after the ext_buck_iso bits are cleared, the function jumps to
err_pwr_ack which disables clocks and regulators but fails to set the bits back
to restore isolation.

Could this lead to hardware state inconsistency and potential leakage current?

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