Re: [PATCH v2 2/3] pmdomain: mediatek: Add support for secure modem power domain control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scpsys_power_off_internal breaks LIFO hardware sequencing by powering off the modem MTCMOS domain before disabling subsystem clocks and restoring external isolation.
- [High] The error path in scpsys_power_on erroneously calls scpsys_sram_disable for secure modem domains, attempting to write to firmware-protected control registers.
--

--- Patch [2]: [PATCH v2 2/3] pmdomain: mediatek: Add support for secure modem power domain control ---
commit 9becf79ed55f0246ad67aeb4808362788efdf915
Author: Nikolai Burov <nikolai.burov@jolla.com>

pmdomain: mediatek: Add support for secure modem power domain control

On recent MediaTek SoCs such as MT6858, the kernel is required to use
a secure monitor call (SMC) to enable or disable the modem power domain.
This patch adds a flag to enable this new power sequence for SoCs that
need it.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 8309a4b46afb0..ada9ea8b45b79 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -717,7 +751,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  			goto err_pwr_ack;
>  	}
>  
> -	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ) &&
> +	    !MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_SECURE_PWRSEQ)) {
>  		ret = scpsys_sram_enable(pd);
>  		if (ret < 0)
>  			goto err_disable_subsys_clks;

[Severity: High]
Since the MTK_SCPD_MODEM_SECURE_PWRSEQ flag is now used to skip
scpsys_sram_enable(), should the error path also be updated?

If a failure occurs later in scpsys_power_on(), the code jumps to the
err_disable_sram label:

err_disable_sram:
        if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
                scpsys_sram_disable(pd);

Could this attempt to invoke scpsys_sram_disable() for the secure domain?

The commit message notes that firmware prevents modifying the control
register directly. Writing to ctl_offs on the error path might trigger
a secure-world exception.

[ ... ]
> @@ -761,7 +796,11 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
> +	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_SECURE_PWRSEQ)) {
> +		ret = scpsys_modem_sec_power_on(false);
> +		if (ret)
> +			return ret;
> +	} else if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
>  		ret = scpsys_sram_disable(pd);
>  		if (ret < 0)
>  			return ret;

[Severity: High]
Does placing scpsys_modem_sec_power_on(false) at the beginning of this
function break the hardware power-down sequence?

If the MTCMOS domain is powered off here, won't the subsequent call to
clk_bulk_disable_unprepare() attempt to access clock control registers
in an unpowered domain?

Typically, this causes an asynchronous external abort or bus fault. Should
this secure power-off command be placed at the end of the function,
parallel to scpsys_ctl_pwrseq_off(pd), to ensure clocks and external
isolation are disabled first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-mt6858-pmdomain-v2-0-6293e87fc093@jolla.com?part=2