Re: [PATCH] pmdomain: amlogic: Keep Amlogic A9 ETH domain always on

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver ignores the return value of `pm_genpd_init()`, which causes a critical Use-After-Free during system boot if initialization fails.
--

commit e3b7d61b7b7e8838767d2093bcc10638ec8bbd84
Author: Qingpeng Yang <qingpeng.yang@amlogic.com>

pmdomain: amlogic: Keep Amlogic A9 ETH domain always on
    
This commit marks the Amlogic A9 ethernet power domain as always-on.
The change is intended to ensure the domain remains powered to support
ethernet online wakeup functionality.

> diff --git a/drivers/pmdomain/amlogic/meson-secure-pwrc.c b/drivers/pmdomain/amlogic/meson-secure-pwrc.c
> index 133549956efa4..daf09cf86335b 100644
> --- a/drivers/pmdomain/amlogic/meson-secure-pwrc.c
> +++ b/drivers/pmdomain/amlogic/meson-secure-pwrc.c
> @@ -196,7 +196,8 @@ static const struct meson_secure_pwrc_domain_desc a9_pwrc_domains[] = {
>  	SEC_PD(A9_VICP,		0),
>  	SEC_PD(A9_SD_EMMC_A,	0),
>  	SEC_PD(A9_SD_EMMC_B,	0),
> -	SEC_PD(A9_ETH,		0),
> +	/* ETH is for ethernet online wakeup, and should be always on */
> +	SEC_PD(A9_ETH,		GENPD_FLAG_ALWAYS_ON),

[Severity: Critical]
This is a pre-existing issue, but does adding GENPD_FLAG_ALWAYS_ON here risk
a use-after-free during system boot if the ethernet domain fails to power on?

If the A9_ETH hardware state is off and the underlying firmware call fails to
power it on, pm_genpd_init() returns -EINVAL because it strictly enforces the
always-on constraint:

drivers/pmdomain/core.c:pm_genpd_init() {
    ...
    /* Always-on domains must be powered on at initialization. */
    if ((genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd)) &&
            !genpd_status_on(genpd)) {
        pr_err("always-on PM domain %s is not on\n", genpd->name);
        return -EINVAL;
    }
    ...
}

The probe function in the Amlogic secure power controller driver currently
ignores the return value of pm_genpd_init(), leaving the failed domain
uninitialized and out of the global gpd_list:

drivers/pmdomain/amlogic/meson-secure-pwrc.c:meson_secure_pwrc_probe() {
    ...
    for (i = 0; i < match->count; i++) {
        ...
        pm_genpd_init(&dom->base, NULL, match->domains[i].is_off(dom));
        pwrc->xlate.domains[i] = &dom->base;
    }
    ...
}

When the probe function later calls of_genpd_add_provider_onecell(), it
detects that the failed domain is missing from gpd_list and aborts provider
registration.

The probe failure then triggers devres cleanup, which frees the devm_kcalloc
allocated pwrc->domains array. However, the previously successfully initialized
domains (such as A9_DSPA) are never removed from the global gpd_list. 

Later, when genpd_power_off_unused() iterates over gpd_list during the late
initcalls, won't it dereference the freed memory from the pwrc->domains array?

>  	SEC_PD(A9_PCIE_A,	0),
>  	SEC_PD(A9_PCIE_B,	0),
>  	SEC_PD(A9_NNA_4T,	0),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-a9-pwr-eth-v1-1-752005131135@amlogic.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic