Re: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Resource leak on error cleanup due to dummy `sta_mask` in simple power sequence domains causing `scpsys_domain_is_on()` to incorrectly return false.

Pre-existing issues:
- [High] Kernel panic during probe error cleanup due to unconditional dereference of `pd->data` for `HW_VOTER` domains in `scpsys_domain_is_on()`.
--

--- Patch [4]: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
commit 6f2aa6326ce2b3105bc55383da9b0e0d8b25e2d6
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

Add support for the power domains provided by the HFRPSYS Power Controller of
the MT8196 SoC. Those control power to the eDP and DP Transmitter IPs.

> diff --git a/drivers/pmdomain/mediatek/mt8196-pm-domains.h b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> index 2e4b287206596..d704c9fa93374 100644
> --- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> +++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> @@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
>  	},
>  };
>  
> +static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
> +	[MT8196_POWER_DOMAIN_EDPTX] = {
> +		.name = "edp-tx",
> +		.sta_mask = MT8196_PWR_ACK,

[Severity: Medium]
Can this lead to a resource leak during error cleanup?

The MT8196_POWER_DOMAIN_EDPTX and DPTX domains use simple power sequence,
bypassing standard PWR_ACK polling. To pass validation during initialization,
they are assigned MT8196_PWR_ACK as a sta_mask.

If this bit is a dummy and not automatically driven high by hardware for these
specific domains (since it is never polled during power_on),
scpsys_domain_is_on() will perpetually read 0 and return false.

If a failure triggers probe error cleanup, scpsys_remove_one_domain() would
skip scpsys_power_off() for these domains because it incorrectly believes they
are already off. Does this leave the domain permanently powered on in a
corrupted state and leak prepared clocks and regulators?

> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
> +		.ctl_offs = 0x74,
> +		.pwr_sta_offs = 0x74,
> +		.pwr_sta2nd_offs = 0x74,
> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
> +	},

[ ... ]

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index feba436279c54..55e7478804236 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -1258,6 +1258,10 @@ static const struct of_device_id scpsys_of_match[] = {
>  		.compatible = "mediatek,mt8196-power-controller",
>  		.data = &mt8196_scpsys_data,
>  	},
> +	{
> +		.compatible = "mediatek,mt8196-hfrp-power-controller",
> +		.data = &mt8196_hfrpsys_data,
> +	},

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a kernel
panic during probe error cleanup?

If a hardware voter domain is probed successfully, it is added to the probed
list. If a subsequently probed domain fails, scpsys_domain_cleanup()
initiates rollback, calling scpsys_remove_one_domain() on the hardware
voter domain.

scpsys_remove_one_domain() unconditionally calls scpsys_domain_is_on(), which
attempts to read pd->data->sta_mask:

scpsys_remove_one_domain()
    scpsys_domain_is_on()
        u32 mask = pd->data->sta_mask;

For hardware voter domains, pd->data is explicitly set to NULL, which appears
to result in a guaranteed NULL pointer dereference. Could this bring the
system down during boot on unsupported or failing hardware configurations?

>  	{
>  		.compatible = "mediatek,mt8196-hwv-hfrp-power-controller",
>  		.data = &mt8196_hfrpsys_hwv_data,

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