Re: [PATCH v2 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 4 potential issue(s) to consider:

New issues:
- [Low] Ignoring the return value of `clk_bulk_prepare_enable` in the error recovery path of `scpsys_power_off` violates its `__must_check` attribute.

Pre-existing issues:
- [High] If `scpsys_bus_protect_enable()` fails in `scpsys_power_off()`, it returns early without re-enabling `subsys_clks`, bypassing the newly added clock recovery block and breaking the clock refcount.
- [High] Error paths in `scpsys_power_off()` leave the hardware in a corrupted state (SRAM disabled, bus protection active, and buck isolated) because they fail to revert these states before returning an error.
- [High] Error paths in `scpsys_power_on()` disable clocks and regulators without reverting the power-on hardware sequence, leaving the domain hardware partially ON while genpd considers it OFF.
--

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

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

This commit adds a simplified power sequence for certain new MediaTek SoCs 
(like MT8196 and MT6991). The new sequence avoids using RTFF hardware and 
certain control bits, handling power domain enablement automatically after 
exiting reset.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index db543d4b1813c..feba436279c54 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -682,7 +733,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,

[Severity: High]
This is a pre-existing issue, but if a step fails during scpsys_power_on()
after the initial MTCMOS power-on sequence, the function jumps to these 
error labels. It disables the clocks and regulators, but does this leave 
the domain's MTCMOS physically ON since it doesn't call a corresponding 
power-off sequence to revert it? Could this hardware inconsistency cause 
issues while the framework considers the domain OFF?

> @@ -698,16 +750,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>  {
>  	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
>  	struct scpsys *scpsys = pd->scpsys;
> -	bool tmp;
>  	int ret;
>  
>  	ret = scpsys_bus_protect_enable(pd, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = scpsys_sram_disable(pd);
> -	if (ret < 0)
> -		return ret;
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
> +		ret = scpsys_sram_disable(pd);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
This is a pre-existing issue, but if scpsys_sram_disable() or 
scpsys_bus_protect_enable() fail here and return early, the framework will 
still treat the domain as ON. Does this early return leave the hardware 
physically isolated, potentially causing active consumer devices to hit an 
active bus protector or unpowered SRAM?

[ ... ]
> @@ -720,15 +773,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>  		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);
>  
> -	/* 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)
> +	if (ret < 0) {
> +		/* Re-enable clocks so that next power off doesn't break the refcount */
> +		clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);

[Severity: High]
This is a pre-existing issue, but if scpsys_bus_protect_enable() fails 
earlier in scpsys_power_off() when called with BUS_PROT_IGNORE_SUBCLK, it 
returns early. Does that early return completely bypass this newly added 
clock recovery block, leaving the subsystem clocks disabled while the 
domain is logically considered ON?

[Severity: Low]
Does this error recovery block ignore the return value of 
clk_bulk_prepare_enable()? Since the function is declared with 
__must_check in the clk API, could this cause a -Wunused-result compiler 
warning and break builds using -Werror?

>  		return ret;
> +	}

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