Re: [PATCH v2 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: missing state update on shutdown in rockchip_pd_power()
- [High] pmdomain: rockchip: broken state rollback in rockchip_pd_power()
--

commit 5eed5597863e9ca30f1c34684fba00455e4388c3
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 firewall status polling, inverted chain status handling,
and state tracking across power-on and shutdown retries.

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

[ ... ]

> @@ -644,14 +661,29 @@ static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>  	return 0;
>  }
>  
> +static int rockchip_pmu_wait_firewall(struct rockchip_pm_domain *pd)
> +{
> +	struct rockchip_pmu *pmu = pd->pmu;
> +	u32 mask = pd->info->firewall_mask;
> +	u32 val;
> +
> +	if (!mask)
> +		return 0;
> +
> +	return regmap_read_poll_timeout(pmu->grf, pmu->info->firewall_status_offset,
> +				       val, (val & mask) == mask, 0, 10000);
> +}
> +
>  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  {
>  	struct rockchip_pmu *pmu = pd->pmu;
> +	bool is_on;
>  	int ret;
>  
>  	guard(mutex)(&pmu->mutex);
>  
> -	if (rockchip_pmu_domain_is_on(pd) == power_on)
> +	is_on = rockchip_pmu_domain_is_on(pd);
> +	if (is_on == power_on && pd->power_on_state == ROCKCHIP_PD_ON_IDLE)
>  		return 0;
>  
>  	ret = clk_bulk_enable(pd->num_clks, pd->clks);
> @@ -662,18 +694,37 @@ 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_on_state == ROCKCHIP_PD_ON_IDLE ||
> +		    (pd->power_on_state == ROCKCHIP_PD_ON_RESTORE && !is_on)) {
> +			/* Record the reset before a failed attempt can turn memory off. */
> +			pd->mem_reset_pending = pd->info->mem_status_mask &&
> +				rockchip_pmu_domain_is_mem_on(pd);
> +			pd->power_on_state = ROCKCHIP_PD_ON_HW;
> +		}
> +
> +		/* A status bit alone does not prove the low-level sequence finished. */
> +		if (pd->power_on_state == ROCKCHIP_PD_ON_HW) {
> +			ret = rockchip_do_pmu_set_power_domain(pd, true);
> +			if (ret < 0)
> +				goto out;
> +			pd->power_on_state = ROCKCHIP_PD_ON_RESTORE;
> +		}
> +	} else {
> +		if (pd->power_on_state == ROCKCHIP_PD_ON_IDLE)
> +			rockchip_pmu_save_qos(pd);
>  
>  		/* 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;
> +		if (is_on) {
> +			ret = rockchip_do_pmu_set_power_domain(pd, false);
> +			if (ret < 0)
> +				goto out;
> +		}
> +	}

[Severity: High]
When rockchip_pd_power(pd, false) is called on an active domain with
pd->power_on_state == ROCKCHIP_PD_ON_IDLE, it saves QoS via
rockchip_pmu_save_qos(pd), requests NIU bus idle via
rockchip_pmu_set_idle_request(pd, true), and powers down the domain via
rockchip_do_pmu_set_power_domain(pd, false).

However, the !power_on branch never transitions pd->power_on_state away from
ROCKCHIP_PD_ON_IDLE before performing these operations. If either
rockchip_pmu_set_idle_request(pd, true) or
rockchip_do_pmu_set_power_domain(pd, false) fails and jumps to out,
pd->power_on_state remains ROCKCHIP_PD_ON_IDLE.

Could this leave the domain and its saved QoS in a broken state on subsequent
calls?

1. If rockchip_pd_power(pd, false) is retried while is_on is still true,
   pd->power_on_state == ROCKCHIP_PD_ON_IDLE is still true, so
   rockchip_pmu_save_qos(pd) runs a second time while the NIU bus is already
   idled and power-down has been requested, overwriting pd->qos_save_regs.

2. If rockchip_pd_power(pd, true) is called while is_on is still true after
   rockchip_pmu_set_idle_request(pd, true) idled the NIU bus, the early exit:

       if (is_on == power_on && pd->power_on_state == ROCKCHIP_PD_ON_IDLE)
           return 0;

   returns 0 immediately without calling rockchip_pmu_set_idle_request(pd,
   false) or rockchip_pmu_restore_qos(pd), leaving the NIU bus stuck in idle.

3. If hardware finishes powering down (so is_on becomes false) shortly after
   rockchip_do_pmu_set_power_domain(pd, false) timed out before reaching the
   ROCKCHIP_SLEEP_PD_CONFIG SMC call, a retried rockchip_pd_power(pd, false)
   hits the same early return and skips notifying firmware.

>  
>  	if (power_on) {
>  		/* if powering up, leave idle mode */
> @@ -681,8 +732,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->power_on_state = ROCKCHIP_PD_ON_IDLE;
> +	pd->mem_reset_pending = false;

[Severity: High]
What happens if rockchip_pd_power(pd, true) fails during
rockchip_do_pmu_set_power_domain(pd, true) while in ROCKCHIP_PD_ON_HW, and is
then followed by either a shutdown via rockchip_pd_power(pd, false) or a
retry of rockchip_pd_power(pd, true)?

In rockchip_do_pmu_set_power_domain(pd, true)->rockchip_pmu_domain_mem_reset(pd):

	regmap_write(pmu->regmap, pmu->info->mem_pwr_offset + pd->info->pwr_offset,
		     (pd->info->pwr_mask | pd->info->pwr_w_mask));
	wmb();

	ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_mem_on, pd, is_on,
					is_on == false, 0, 10000);
	if (ret) {
		...
		goto error;
	}

1. If rockchip_pmu_domain_mem_reset(pd) fails after writing mem_pwr_offset to
   gate memory off, mem_pwr_offset continues to force memory off. If
   rockchip_pd_power(pd, false) is then called, it does not clear
   mem_pwr_offset, yet unconditionally resets pd->power_on_state to
   ROCKCHIP_PD_ON_IDLE and pd->mem_reset_pending to false. On the next
   rockchip_pd_power(pd, true), rockchip_pmu_domain_is_mem_on(pd) reads false,
   so pd->mem_reset_pending is computed as false, rockchip_pmu_domain_mem_reset(pd)
   is skipped, and mem_pwr_offset is never cleared, leaving the domain memory
   permanently gated off.

2. If rockchip_do_pmu_set_power_domain(pd, true) wrote pwr_offset to power on
   the domain and timed out while rockchip_pmu_domain_is_on(pd) was still
   false, a subsequent rockchip_pd_power(pd, false) sees is_on == false and
   skips rockchip_do_pmu_set_power_domain(pd, false), leaving the power-on
   write active in pwr_offset while resetting pd->power_on_state to
   ROCKCHIP_PD_ON_IDLE. When the hardware transition finishes and is_on
   becomes true, the next rockchip_pd_power(pd, true) returns 0 early without
   un-idling the NIU bus, waiting for the firewall, or restoring QoS.

3. Conversely, if the domain and its memory were initially off (is_mem_on ==
   false) when rockchip_pd_power(pd, true) first ran, pd->mem_reset_pending is
   set to false and pd->power_on_state is set to ROCKCHIP_PD_ON_HW. After
   rockchip_do_pmu_set_power_domain(pd, true) writes pwr_offset, memory powers
   on (so is_mem_on becomes true), and if polling rockchip_pmu_domain_is_on(pd)
   times out, a direct retry of rockchip_pd_power(pd, true) enters with
   pd->power_on_state == ROCKCHIP_PD_ON_HW, skips re-evaluating
   rockchip_pmu_domain_is_mem_on(pd), and calls
   rockchip_do_pmu_set_power_domain(pd, true) while memory is on without
   performing the memory reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-add-board-ok3572-c-pmdomain-v2-0-79ddd42e92c1@qq.com?part=2