Re: [PATCH v3 2/2] pmdomain: rockchip: Add RK3572 boot peripheral domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pmdomain: rockchip: rockchip_pd_power() resets active memory on power-off error
- [Medium] pmdomain: rockchip: domain left powered on when rockchip_pd_power_on() fails
--

commit fcbe7c2ac54282bfc8d75ca012934011c148704f
Author: Finley Xiao <finley.xiao@rock-chips.com>

pmdomain: rockchip: Add RK3572 boot peripheral domains

This commit adds support for the RK3572 NVM0, NVM1, PHP, and VOP power
domains, along with state tracking for incomplete power transitions and
memory resets across retries and power-off failures.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index e193f978d5f35..668d208dea55b 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -600,16 +621,13 @@ static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>  	struct rockchip_pmu *pmu = pd->pmu;
>  	struct generic_pm_domain *genpd = &pd->genpd;
>  	u32 pd_pwr_offset = pd->info->pwr_offset;
> -	bool is_on, is_mem_on = false;
> +	bool is_on;
>  	struct arm_smccc_res res;
>  	int ret;
>  
>  	if (pd->info->pwr_mask == 0)
>  		return 0;
>  
> -	if (on && pd->info->mem_status_mask)
> -		is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> -
>  	if (pd->info->pwr_w_mask)
>  		regmap_write(pmu->regmap, pmu->info->pwr_offset + pd_pwr_offset,
>  			     on ? pd->info->pwr_w_mask :
> @@ -620,13 +638,13 @@ static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>  
>  	wmb();
>  
> -	if (is_mem_on) {
> +	if (on && pd->mem_reset_pending) {
>  		ret = rockchip_pmu_domain_mem_reset(pd);
>  		if (ret)
>  			return ret;
> +		pd->mem_reset_pending = false;
>  	}

[ ... ]

> @@ -662,18 +695,38 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  
>  	rockchip_pmu_ungate_clk(pd, true);
>  
> -	if (!power_on) {
> -		rockchip_pmu_save_qos(pd);
> +	if (power_on) {
> +		if (pd->power_state == ROCKCHIP_PD_IDLE ||
> +		    pd->power_state == ROCKCHIP_PD_OFF ||
> +		    (pd->power_state == ROCKCHIP_PD_ON_RESTORE && !is_on)) {
> +			/* A failed reset may still be forcing memory off. */
> +			pd->mem_reset_pending |= pd->info->mem_status_mask &&
> +				rockchip_pmu_domain_is_mem_on(pd);
> +			pd->power_state = ROCKCHIP_PD_ON_HW;
> +		}

[Severity: High]
Can this power-cycle and corrupt the memory of an active domain when
recovering from a failed power-off in rockchip_pd_power_off()?

When genpd powers off a domain via
rockchip_pd_power_off() -> rockchip_pd_power(pd, false), pd->power_state is
set to ROCKCHIP_PD_OFF before calling rockchip_pmu_set_idle_request(pd, true)
and rockchip_do_pmu_set_power_domain(pd, false):

	} else {
		if (pd->power_state == ROCKCHIP_PD_IDLE)
			rockchip_pmu_save_qos(pd);
		pd->power_state = ROCKCHIP_PD_OFF;

		/* if powering down, idle request to NIU first */
		ret = rockchip_pmu_set_idle_request(pd, true);
		if (ret < 0)
			goto out;

		/* Cancel a pending power-on even if its status is still off. */
		ret = rockchip_do_pmu_set_power_domain(pd, false);
		if (ret < 0)
			goto out;
	}

If rockchip_pmu_set_idle_request(pd, true) fails (for example, timing out
while waiting for NIU idle ack or idle status), rockchip_pd_power(pd, false)
jumps to out without calling rockchip_do_pmu_set_power_domain(pd, false). The
domain and its memory remain powered on (is_on == true and
rockchip_pmu_domain_is_mem_on(pd) == true), the bus is not idle, and
pd->power_state is left at ROCKCHIP_PD_OFF. The same state occurs if
rockchip_do_pmu_set_power_domain(pd, false) times out while the domain is
still on.

rockchip_pd_power_off() then immediately calls rockchip_pd_power(pd, true) to
restore the domain:

	ret = rockchip_pd_power(pd, false);
	if (ret) {
		/* genpd still considers the domain on after a failed power-off. */
		err = rockchip_pd_power(pd, true);

In rockchip_pd_power(pd, true), because pd->power_state == ROCKCHIP_PD_OFF is
not gated on !is_on (unlike ROCKCHIP_PD_ON_RESTORE && !is_on, or
ROCKCHIP_PD_IDLE where is_on is guaranteed false due to the early return at
the top of rockchip_pd_power()), rockchip_pmu_domain_is_mem_on(pd) evaluates
to true because the domain never powered off.

This sets pd->mem_reset_pending = true and transitions pd->power_state to
ROCKCHIP_PD_ON_HW, causing rockchip_do_pmu_set_power_domain(pd, true) to call
rockchip_pmu_domain_mem_reset(pd) and forcibly power-gate off and reset the
memory via mem_pwr_offset on a live domain that was never powered down.

> +
> +		/* A status bit alone does not prove the low-level sequence finished. */
> +		if (pd->power_state == ROCKCHIP_PD_ON_HW) {
> +			ret = rockchip_do_pmu_set_power_domain(pd, true);
> +			if (ret < 0)
> +				goto out;
> +			pd->power_state = ROCKCHIP_PD_ON_RESTORE;
> +		}
> +	} else {
> +		if (pd->power_state == ROCKCHIP_PD_IDLE)
> +			rockchip_pmu_save_qos(pd);
> +		pd->power_state = ROCKCHIP_PD_OFF;
>  
>  		/* if powering down, idle request to NIU first */
>  		ret = rockchip_pmu_set_idle_request(pd, true);
>  		if (ret < 0)
>  			goto out;
> -	}
>  
> -	ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> -	if (ret < 0)
> -		goto out;
> +		/* Cancel a pending power-on even if its status is still off. */
> +		ret = rockchip_do_pmu_set_power_domain(pd, false);
> +		if (ret < 0)
> +			goto out;
> +	}
>  
>  	if (power_on) {
>  		/* if powering up, leave idle mode */
> @@ -681,8 +734,14 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  		if (ret < 0)
>  			goto out;
>  
> +		ret = rockchip_pmu_wait_firewall(pd);
> +		if (ret)
> +			goto out;
> +
>  		rockchip_pmu_restore_qos(pd);
> +		pd->mem_reset_pending = false;
>  	}
> +	pd->power_state = ROCKCHIP_PD_IDLE;

[Severity: Medium]
What happens to the hardware domain state and external supply regulator if
rockchip_pd_power(pd, true) fails in the restoration stage?

In rockchip_pd_power(pd, true), rockchip_do_pmu_set_power_domain(pd, true)
powers on the domain in the PMU, informs TF-A via ROCKCHIP_SLEEP_PD_CONFIG to
keep the domain on during sleep, and sets pd->power_state to
ROCKCHIP_PD_ON_RESTORE.

If rockchip_pmu_set_idle_request(pd, false) or rockchip_pmu_wait_firewall(pd)
subsequently fails, rockchip_pd_power(pd, true) jumps to out and returns an
error to rockchip_pd_power_on():

static int rockchip_pd_power_on(struct generic_pm_domain *domain)
{
	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
	int ret;

	ret = rockchip_pd_regulator_enable(pd);
	if (ret < 0)
		return ret;

	ret = rockchip_pd_power(pd, true);
	if (ret)
		rockchip_pd_regulator_disable(pd);

	return ret;
}

Because rockchip_pd_power_on() returns an error, genpd keeps the domain in
GENPD_STATE_OFF and will not call rockchip_pd_power_off() unless a consumer
later succeeds in powering the domain on. This leaves the PMU domain powered
on (and un-idled if rockchip_pmu_wait_firewall(pd) failed) while genpd
considers it off, and for domains with need_regulator set,
rockchip_pd_regulator_disable(pd) disables the external supply regulator
while the PMU power switch is still on and pd->power_state is
ROCKCHIP_PD_ON_RESTORE.

Should rockchip_pd_power_on() (or the error path of rockchip_pd_power())
power down the PMU domain before disabling the regulator and returning the
error to genpd?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-add-board-ok3572-c-pmdomain-v3-0-70d614d3d921@qq.com?part=2