Re: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence

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

On 7/9/26 10:58, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incomplete error recovery in `scpsys_power_off_internal` leaves hardware paralyzed and causes clock refcount underflows.
> - [High] Unbalanced lifecycle operations in `scpsys_power_on` fail to revert hardware state on error.

Unfortunately there's no way of fixing those two briefly; the hardware may randomly
lockup if changes are reverted at that point.

I'm not even sure if there's any way at all of fixing this, but research will
be done a bit later. Not relevant for this series.

> - [Medium] Malformed device trees lacking access controllers guarantee a NULL pointer dereference in `scpsys_bus_protect_get_regmap`.

No, that's not true - only HFRPSYS HWV lack access controllers, and will use
different ops, so that's not going to create any NULL pointer dereference.

> --
> 
> --- Patch [4]: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
> commit 0a6955778a8abcdf284811fe9a9089e2c1cff6d6
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> pmdomain: mediatek: Add support for Direct CTL simple power sequence
> 
> Some new SoCs like MT8196, MT6991, and others, have got one
> additional power controller (usually in the HFRP Multimedia
> block) which needs a simplified power on/off sequence while
> using Direct Control strategy.
> 
> Domains using the "simple power sequence" are not backed by
> the RTFF hardware, have no Bus Protection mechanism, lacks
> the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
> automatically after getting out of reset.
> 
> This simple power sequence is then a subset of the full one
> as only needs the enablement of the specific power domain's
> clock input and reset (where, again, after getting out of
> reset, the ISO and PWR_ON bits are automatically internally
> getting flipped) to enable or disable (power on or off).
> 
> Moreover, the simple power sequence power domains guarantee
> that they always get enabled/disabled after executing the
> relevant power sequence (on/off) so, differently from the
> others, there is also no need to poll for a PWR_ACK.
> 
>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> index 8d49d9ee3cd29..632c5dcdcedaf 100644
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> [ ... ]
>> @@ -688,7 +739,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>   err_enable_bus_protect:
>>   	scpsys_bus_protect_enable(pd, 0);
>>   err_disable_sram:
>> -	scpsys_sram_disable(pd);
>> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		scpsys_sram_disable(pd);
>>   err_disable_subsys_clks:
>>   	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>>   		clk_bulk_disable_unprepare(pd->num_subsys_clks,
>>   					   pd->subsys_clks);
> 
> [Severity: High]
> This is a pre-existing issue, but does this error path fail to revert
> the hardware state if an error occurs?
> 
> If scpsys_bus_protect_disable() or clk_bulk_prepare_enable() fails after
> *_pwrseq_on() has successfully executed, the error path jumps to err_pwr_ack.
> 
> This path disables the domain's regulator and clocks but fails to execute
> *_pwrseq_off() or revert ext_buck_iso:
> 
> err_pwr_ack:
> 	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> err_reg:
> 	scpsys_regulator_disable(pd->supply);
> 	return ret;
> 
> Could this sever the supply while leaving the domain's internal logic active,
> resulting in hardware lockup and state machine desynchronization?
> 
> [ ... ]
>> @@ -725,15 +778,21 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
>>   		return ret;
>>   
>>   	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
>> -		scpsys_modem_pwrseq_off(pd);
>> +		ret = scpsys_modem_pwrseq_off(pd);
>> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		ret = scpsys_simple_pwrseq_off(pd);
>>   	else
>> -		scpsys_ctl_pwrseq_off(pd);
>> +		ret = scpsys_ctl_pwrseq_off(pd);
>> +
>> +	if (ret < 0) {
>> +		/* Re-enable clocks so that next power off doesn't break the refcount */
>> +		int r = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
>> +
>> +		if (r)
>> +			dev_warn(scpsys->dev, "Could not re-enable clocks: %d\n", r);
>>   
>> -	/* wait until PWR_ACK = 0 */
>> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
>> -				 MTK_POLL_TIMEOUT);
>> -	if (ret < 0)
>>   		return ret;
>> +	}
>>   
>>   	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the error recovery in
> scpsys_power_off_internal() leave the hardware in a paralyzed state?
> 
> If pwrseq_off fails, this block restores subsys_clks but leaves the domain's
> SRAM disabled, isolation applied, and bus protection disabled, while
> returning an error to genpd.
> 
> Because an error is returned, genpd treats the domain as still ON, allowing
> active consumers to interact with a paralyzed hardware domain.
> 
> Additionally, if scpsys_bus_protect_enable() fails earlier in this function:
> 
> 	ret = scpsys_bus_protect_enable(pd, BUS_PROT_IGNORE_SUBCLK);
> 	if (ret < 0)
> 		return ret;
> 
> It returns early and bypasses clock recovery entirely, causing a permanent
> clock refcount underflow on the next failure.
> 
> Can we ensure consistent state on power off errors?
> 
> [ ... ]
>> @@ -1109,6 +1168,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>>   		regmap[2] = NULL;
>>   	}
>>   
>> +	/* If no access controllers are needed, don't allocate and don't fail */
>> +	if (num_regmaps == 0) {
>> +		scpsys->bus_prot = NULL;
>> +		return 0;
>> +	}
>> +
>>   	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
>>   					      sizeof(*scpsys->bus_prot), GFP_KERNEL);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this result in a NULL pointer
> dereference if the device tree is malformed?
> 
> If an older SoC's device tree is missing required access controllers,
> num_regmaps becomes 0. The probe function succeeds and explicitly sets
> scpsys->bus_prot = NULL.
> 
> When the domain later attempts to power on, scpsys_bus_protect_enable() sees
> that the hardcoded SoC domain data still requests bus protection and
> unconditionally dereferences the NULL array in
> scpsys_bus_protect_get_regmap():
> 
> 	unsigned short block_idx = scpsys->bus_prot_index[bpd->bus_prot_block];
> 	return scpsys->bus_prot[block_idx];
> 
> Should we ensure scpsys->bus_prot is not dereferenced when NULL, or perhaps
> fail the probe if required controllers are missing?
>