Re: [PATCH v12 09/14] pmdomain: rockchip: add optional per-domain power-on settle delay

[sashiko-bot@kernel.org]

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

New issues:
- [High] Forcing the NPU parent domain off at probe bypasses the idle sequence for its child domains, risking AXI bus hangs, and can cause pm_genpd_add_subdomain() to fail.
- [Medium] Hijacking the need_regulator flag to force a hardware reset at probe is an API abuse that generates intentional, false kernel warnings on unaffected boards.

Pre-existing issues:
- [High] Error cleanup path fails to unlink subdomains, causing pm_genpd_remove() to fail and leading to a Use-After-Free of devres-managed memory.
--

commit c3ca2f4bcfcc96e94f4eccdb9b209d7e8e746177
Author: Jiaxing Hu <gahing@gahingwoo.com>

pmdomain: rockchip: add optional per-domain power-on settle delay

This patch introduces an optional delay_us to rockchip_domain_info, allowing
domains like the RK3576 NPU to settle before QoS register restoration. It also
adds a DOMAIN_RK3576_R macro to pass the need_regulator flag, applying it to
RK3576_PD_NPU to force a power-off at probe on boards without a domain-supply.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index ba66ae7194289..39988efd86aaa 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -258,8 +262,11 @@ struct rockchip_pmu {
>  #define DOMAIN_RK3568(name, pwr, req, wakeup, regulator)		\
>  	DOMAIN_M_R(name, pwr, pwr, req, req, req, wakeup, regulator)
>  
> -#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, wakeup)	\
> -	DOMAIN_M_O_R_G(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, wakeup)
> +#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
> +	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, false)
> +
> +#define DOMAIN_RK3576_R(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
> +	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, true)

[Severity: Medium]
Is hijacking the need_regulator flag here to force a hardware reset at probe
an abuse of the API? 

Setting this flag on a domain that doesn't physically have a regulator forces
the regulator core to issue a warning. As the commit message mentions, 12 out
of 13 boards do not describe a domain-supply, which means they will fallback 
to a dummy regulator and print a dev_warn on otherwise healthy systems. 

Could a dedicated boolean flag be used instead to force a probe-time
power-off?

[ ... ]

> @@ -1300,25 +1311,26 @@ static const struct rockchip_domain_info rk3568_pm_domains[] = {
>  };
>  
>  static const struct rockchip_domain_info rk3576_pm_domains[] = {
> -	[RK3576_PD_NPU]		= DOMAIN_RK3576("npu",    0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       false),
[ ... ]
> +	/*                                            name    p_offset pwr      status  r_status r_offset req      idle     g_mask   delay wakeup */
> +	[RK3576_PD_NPU]		= DOMAIN_RK3576_R("npu",  0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       0,    false),

[Severity: High]
Does forcing the NPU parent domain off at probe bypass the idle sequence for
its child domains?

If the bootloader leaves the NPU powered on, rockchip_pm_add_one_domain()
forces this parent domain off because need_regulator is true:

    if (pd->info->need_regulator)
        rockchip_pd_power(pd, false);
        
Because the parent RK3576_PD_NPU lacks an idle_mask (set to 0 here), power is
cut abruptly without idling its subdomains (NPUTOP, NPU0, NPU1), which risks
AXI bus hangs. 

Furthermore, if the children's hardware status bits do not instantly clear,
they will be registered as ON, and pm_genpd_add_subdomain() will fail with
-EINVAL because it refuses to link an ON child to an OFF parent.

[Severity: High]
This is a pre-existing issue, but if the probe fails (such as from the
pm_genpd_add_subdomain() failure mentioned above), does the error cleanup
path cause a use-after-free?

In rockchip_pm_domain_cleanup(), the code iteratively attempts to remove
domains but doesn't unlink subdomains first:

    rockchip_pm_remove_one_domain(pd);

Which then calls:

    ret = pm_genpd_remove(&pd->genpd);

pm_genpd_remove() detects the active child links on the parent and returns
-EBUSY, failing to remove the parent domain from the global gpd_list.
Devres then frees the struct rockchip_pm_domain memory, leaving dangling
pointers in the core genpd list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912065053.1519165-1-gahing@gahingwoo.com?part=9