Re: [PATCH 06/11] pmdomain: st: ux500: Control DB8500 EPODs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Probe error path leaks dynamically allocated memory, fails to unregister PM domains, and lacks rollback for partially added subdomains.
- [High] Hardware RAM retention capabilities are entirely lost, rendering related state handling logic dead code.
- [Medium] Bisection breakage: Disabling REGULATOR_DB8500_PRCMU causes consumer devices expecting `v-ape-supply` to permanently fail to probe.
--

commit 26ccd884add2c876cdf4c876688df7b868a2fa06
Author: Linus Walleij <linusw@kernel.org>

pmdomain: st: ux500: Control DB8500 EPODs

Move the DB8500 EPOD state handling into the Ux500 power-domain driver.
Keep the old regulator driver mutually exclusive with the pmdomain driver.

> diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> --- a/arch/arm/mach-ux500/Kconfig
> +++ b/arch/arm/mach-ux500/Kconfig
[ ... ]
> @@ -26,7 +26,7 @@ menuconfig ARCH_U8500
>  	select PL310_ERRATA_753970 if CACHE_L2X0
>  	select PM_GENERIC_DOMAINS if PM
>  	select REGULATOR
> -	select REGULATOR_DB8500_PRCMU
> +	select UX500_PM_DOMAIN
>  	select REGULATOR_FIXED_VOLTAGE
>  	select SOC_BUS
>  	select RESET_CONTROLLER

[Severity: Medium]
Does this break git bisect by disabling the regulator provider before consumer
devices are updated?

Consumer devices depending on v-ape-supply may receive -EPROBE_DEFER
indefinitely if the regulator driver is disabled here. I see this is fixed in
a later commit ("regulator: db8500: Add power domain regulators"), but is it
possible to reorder the changes to avoid breaking bisection?

> diff --git a/drivers/pmdomain/st/ste-ux500-pm-domain.c b/drivers/pmdomain/st/ste-ux500-pm-domain.c
> --- a/drivers/pmdomain/st/ste-ux500-pm-domain.c
> +++ b/drivers/pmdomain/st/ste-ux500-pm-domain.c
[ ... ]
> +static int disable_epod(u16 epod_id, bool ramret)
> +{
> +	int ret;
> +
> +	if (ramret) {
> +		if (!epod_on[epod_id]) {
> +			ret = prcmu_set_epod(epod_id, EPOD_STATE_OFF);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		epod_ramret[epod_id] = false;
> +	} else {
> +		if (epod_ramret[epod_id]) {
> +			ret = prcmu_set_epod(epod_id, EPOD_STATE_RAMRET);

[Severity: High]
Is the hardware RAM retention capability effectively lost here?

The struct dbx500_powerdomain_info defines is_ramret, but none of the
newly defined domains initialize it to true (e.g., for SVAMMDSP or SIAMMDSP).
Because ramret is always false, epod_ramret[] is never set, making
the EPOD_STATE_RAMRET state unreachable. Does this dead code need to
be activated by setting is_ramret on the appropriate domains?

[ ... ]
> +static int ux500_pm_domains_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct genpd_onecell_data *genpd_data;
>  	int i;
> +	int ret;
>  
>  	if (!np)
>  		return -ENODEV;
[ ... ]
>  	for (i = 0; i < ARRAY_SIZE(ux500_pm_domains); ++i)
> -		pm_genpd_init(ux500_pm_domains[i], NULL, false);
> +		pm_genpd_init(ux500_pm_domains[i], NULL, true);
> +
> +	ret = ux500_pm_domains_add_subdomains();
> +	if (ret)
> +		return ret;

[Severity: High]
Does this error path leak memory and leave the power domains registered?

If ux500_pm_domains_add_subdomains() fails, it returns an error directly.
This leaves genpd_data allocated and the domains initialized and linked into
the global gpd_list via pm_genpd_init().

If a driver unbind/rebind occurs, could INIT_LIST_HEAD be invoked on elements
already in the list, potentially corrupting the doubly linked list and causing
a kernel panic?

Should pm_genpd_remove() be called for initialized domains, and the added
subdomains be rolled back with pm_genpd_remove_subdomain() before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-ux500-power-domains-v7-1-v1-0-eb5e50b1a588@kernel.org?part=6