Re: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unconditional power-off of SIMPLE_PWRSEQ domains in error cleanup, masking potentially incorrect status mask configurations.

Pre-existing issues:
- [Critical] Missing error handling for pm_genpd_init() causes kernel panic via NULL pointer dereference during error cleanup.
- [Critical] Missing subdomain unlinking during probe error cleanup leaves freed genpd objects in the global gpd_list, causing Use-After-Free.
- [High] HW_VOTER domains invoke sleepable clock and regulator APIs while holding a spinlock with IRQs disabled.
- [High] Missing GENPD_FLAG_NO_STAY_ON or sync_state callback causes regulator-supplied domains to stay on indefinitely, leading to hardware malfunction on regulator cleanup.
- [High] Unbalanced resource cleanup on the error path for KEEP_DEFAULT_OFF domains left on by the bootloader.
--

--- Patch [5]: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
commit 5fec9c9afff1947a88f0033701c6c01f031caa0d
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.

> --- 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,
> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
> +		.ctl_offs = 0x74,
> +		.pwr_sta_offs = 0x74,
> +		.pwr_sta2nd_offs = 0x74,
> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
> +	},

[ ... ]

> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -1071,7 +1071,7 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  		if (scpsys_hwv_domain_is_enable_done(pd))
>  			scpsys_hwv_power_off_internal(pd);

[Severity: High]
This isn't a bug introduced by this patch, but do HW_VOTER domains invoke
sleepable clock and regulator APIs while holding a spinlock with IRQs
disabled?

Since HW_VOTER domains are registered with GENPD_FLAG_IRQ_SAFE, the genpd core
guarantees their power_on and power_off callbacks run with interrupts
disabled. However, scpsys_hwv_power_on() and scpsys_hwv_power_off()
unconditionally call clk_bulk_prepare_enable() and scpsys_regulator_enable(),
which take sleepable mutexes.

Can this result in a scheduling while atomic BUG or kernel panic?

>  	} else {
> -		if (scpsys_domain_is_on(pd))
> +		if (scpsys_domain_is_on(pd) || MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>  			scpsys_power_off_internal(pd);
>  	}

[Severity: Medium]
Will this unconditionally force power off for SIMPLE_PWRSEQ domains on the
error path?

If a SIMPLE_PWRSEQ domain is ever configured with MTK_SCPD_KEEP_DEFAULT_OFF
and probe fails, it appears the cleanup path unconditionally powers it off
regardless of whether it was active. Could this lead to disabling clocks that
were never enabled, resulting in a clock underflow WARN_ON?

Is it possible this acts as a workaround that masks the fact that
scpsys_domain_is_on() fails to report the true state for these domains due
to their status mask configurations (like MT8196_PWR_ACK for edp-tx)?

[Severity: Critical]
This is a pre-existing issue, but does scpsys_add_one_domain() ignore the
return value of pm_genpd_init()?

If pm_genpd_init() fails, it seems genpd->gpd_list_node remains 0-initialized.
On a subsequent probe failure, the error cleanup loop calls pm_genpd_remove()
which executes list_del(&genpd->gpd_list_node).

Could this dereference a NULL pointer and cause a kernel panic since the node
was never initialized?

[Severity: Critical]
This is a pre-existing issue, but does scpsys_domain_cleanup() iterate
backwards calling scpsys_remove_one_domain() without breaking parent-child
links first?

When this happens, pm_genpd_remove() returns -EBUSY and aborts without
unlinking the domain from gpd_list. Since the driver ignores this error, devm
may free the still-linked genpd objects.

Could background processes like genpd_power_off_unused later iterate gpd_list
and trigger a use-after-free when accessing the freed memory?

[Severity: High]
This is a pre-existing issue, but is there unbalanced resource cleanup on
the error path for KEEP_DEFAULT_OFF domains left on by the bootloader?

If such a domain is left physically ON, the driver warns but skips OS resource
preparation, leaving the enable_count at 0. If probe fails later, this path in
scpsys_remove_one_domain() evaluates scpsys_domain_is_on() as true and calls
scpsys_power_off_internal().

Could this unconditionally disable resources the OS never enabled, triggering
a clock/regulator underflow WARN_ON?

[ ... ]

> @@ -1288,6 +1288,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 is a pre-existing issue, but is the platform driver missing the
.sync_state callback required to clear the stay_on flag for default-ON
domains?

Since the driver initializes default-on domains with is_off = false, the genpd
core sets genpd->stay_on = true. Without a .sync_state callback or
GENPD_FLAG_NO_STAY_ON, stay_on is never cleared, and the framework will refuse
to power off unused domains.

When regulator_late_cleanup() later forces unused regulators off, could this
silently cut physical power to domains the kernel still considers logically
active and cause a hardware malfunction?

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