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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/9/26 11:00, sashiko-bot@kernel.org wrote:
> 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.
> 

That's not an issue. It's done on purpose.

> 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.

I have already fixed whatever I could, can't do the rest in this cycle; maybe in
september I can.

Also, this shall not block this series from being picked (as it also contains
important fixes inside), because those are all pre-existing issues.

> --
> 
> --- 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?
>