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

New issues:
- [High] Hardware sequence inversion for secure modem teardown
- [Medium] Unwarranted 1-second busy sleep loop on SMC hard errors

Pre-existing issues:
- [High] Missing power-off rollback in hardware bring-up error paths
- [High] Bus protection resource leak on early power-off failure
--

--- Patch [2]: [PATCH v3 2/3] pmdomain: mediatek: Add support for secure modem power domain control ---
commit a98929d49f9662dbe82719e18f3bfc5621c41558
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.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 8309a4b46afb0..5b4d860318a46 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -668,6 +672,34 @@ static int scpsys_modem_pwrseq_off(struct scpsys_domain *pd)
>  	return 0;
>  }
>  
> +static bool scpsys_modem_sec_poll(unsigned long cmd)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(MTK_SIP_KERNEL_CCCI_CONTROL, cmd, 1, 0, 0, 0, 0, 0, &res);
> +
> +	return res.a0 == 0;
> +}
> +
> +static int scpsys_modem_sec_power_on(bool on)
> +{
> +	struct arm_smccc_res res;
> +	unsigned long cmd = on ? MTK_MD_MTCMOS_ENABLE : MTK_MD_MTCMOS_DISABLE;
> +	bool tmp;
> +	int ret;
> +
> +	arm_smccc_smc(MTK_SIP_KERNEL_CCCI_CONTROL, cmd, 0, 0, 0, 0, 0, 0, &res);
> +	if (res.a0 == 0)
> +		return 0;
> +
> +	ret = readx_poll_timeout(scpsys_modem_sec_poll, cmd, tmp, tmp,
> +				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);

[Severity: Medium]
Does this code assume any non-zero res.a0 indicates an 'in-progress'
operation? If the SMC call returns a negative error code (like
SMCCC_RET_NOT_SUPPORTED), will it dive into a 1-second busy sleep loop instead
of failing immediately?

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
[ ... ]
> @@ -739,7 +774,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  err_enable_bus_protect:
>  	scpsys_bus_protect_enable(pd, 0);
>  err_disable_sram:
> -	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))
>  		scpsys_sram_disable(pd);
>  err_disable_subsys_clks:

[Severity: High]
This isn't a bug introduced by this patch, but does this error path block skip
reverting the MTCMOS/SMC power-on state before disabling the regulator?

If a step fails after powering on the MTCMOS domain (for example,
clk_bulk_prepare_enable()), the error paths drop the subsystem clocks and
regulator but fail to explicitly invoke the power-off rollback. Could this
leave the SMC firmware tracking a powered-on domain while physical power is
cut, potentially deadlocking firmware?

>  	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
> @@ -761,7 +797,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);

[Severity: High]
In scpsys_power_on(), the sequence un-isolates the buck, powers on the domain
via SMC, and then enables subsystem clocks. To maintain LIFO symmetry,
shouldn't the teardown sequence disable the clocks and isolate the buck before
cutting MTCMOS power?

Since the SMC power-off call is placed at the very beginning of the function
here, could this cut power while clocks are still actively toggling, causing
hardware bus hangs?

> +		if (ret)
> +			return ret;

[Severity: High]
This is a pre-existing issue, but does returning an error here leave the bus
protection enabled indefinitely?

If the SMC power-off fails and returns early, the genpd framework treats a
failed power-off as the domain remaining active, but the bus is now
permanently blocked. Does this error path need to revert bus protection to
avoid a system hang when accessing devices within the domain?

> +	} else if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
>  		ret = scpsys_sram_disable(pd);
>  		if (ret < 0)
>  			return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-mt6858-pmdomain-v3-0-8966d8de93c8@jolla.com?part=2