[PATCH] pmdomain: arm: scmi_pm_domain: Remove redundant state verification

[PATCH] pmdomain: arm: scmi_pm_domain: Remove redundant state verification

[Sudeep Holla]

Currently, scmi_pd_power() explicitly verifies whether the requested
power state was applied by calling state_get(). While this check could
detect failures where the state was not properly updated, ensuring
correctness is the responsibility of the SCMI firmware.

Removing this redundant state_get() call eliminates an unnecessary
round-trip to the firmware, improving efficiency. Any mismatches
between the requested and actual states should be handled by the SCMI
firmware, which must return a failure if state_set() is unsuccessful.

Additionally, in some cases, checking the state after powering off a
domain may be unreliable or unsafe, depending on the firmware
implementation.

This patch removes the redundant verification, simplifying the function
without compromising correctness.

Cc: Peng Fan <peng.fan@nxp.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Cristian Marussi <cristian.marussi@arm.com>
Reported-and-tested-by: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index 86b531e15b85..2a213c218126 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -24,8 +24,7 @@ struct scmi_pm_domain {
 
 static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
 {
-	int ret;
-	u32 state, ret_state;
+	u32 state;
 	struct scmi_pm_domain *pd = to_scmi_pd(domain);
 
 	if (power_on)
@@ -33,13 +32,7 @@ static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
 	else
 		state = SCMI_POWER_STATE_GENERIC_OFF;
 
-	ret = power_ops->state_set(pd->ph, pd->domain, state);
-	if (!ret)
-		ret = power_ops->state_get(pd->ph, pd->domain, &ret_state);
-	if (!ret && state != ret_state)
-		return -EIO;
-
-	return ret;
+	return power_ops->state_set(pd->ph, pd->domain, state);
 }
 
 static int scmi_pd_power_on(struct generic_pm_domain *domain)
-- 
2.34.1

RE: [PATCH] pmdomain: arm: scmi_pm_domain: Remove redundant state verification

[Peng Fan]

> Subject: [PATCH] pmdomain: arm: scmi_pm_domain: Remove
> redundant state verification
> 
> Currently, scmi_pd_power() explicitly verifies whether the requested
> power state was applied by calling state_get(). While this check could
> detect failures where the state was not properly updated, ensuring
> correctness is the responsibility of the SCMI firmware.
> 
> Removing this redundant state_get() call eliminates an unnecessary
> round-trip to the firmware, improving efficiency. Any mismatches
> between the requested and actual states should be handled by the
> SCMI firmware, which must return a failure if state_set() is unsuccessful.
> 
> Additionally, in some cases, checking the state after powering off a
> domain may be unreliable or unsafe, depending on the firmware
> implementation.
> 
> This patch removes the redundant verification, simplifying the function
> without compromising correctness.
> 
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Reported-and-tested-by: Ranjani Vaidyanathan
> <ranjani.vaidyanathan@nxp.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH] pmdomain: arm: scmi_pm_domain: Remove redundant state verification

[Cristian Marussi]

On Fri, Mar 14, 2025 at 09:58:51AM +0000, Sudeep Holla wrote:
> Currently, scmi_pd_power() explicitly verifies whether the requested
> power state was applied by calling state_get(). While this check could
> detect failures where the state was not properly updated, ensuring
> correctness is the responsibility of the SCMI firmware.
> 
> Removing this redundant state_get() call eliminates an unnecessary
> round-trip to the firmware, improving efficiency. Any mismatches
> between the requested and actual states should be handled by the SCMI
> firmware, which must return a failure if state_set() is unsuccessful.
> 
> Additionally, in some cases, checking the state after powering off a
> domain may be unreliable or unsafe, depending on the firmware
> implementation.
> 
> This patch removes the redundant verification, simplifying the function
> without compromising correctness.
> 
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Reported-and-tested-by: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/pmdomain/arm/scmi_pm_domain.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index 86b531e15b85..2a213c218126 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -24,8 +24,7 @@ struct scmi_pm_domain {
>  
>  static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
>  {
> -	int ret;
> -	u32 state, ret_state;
> +	u32 state;
>  	struct scmi_pm_domain *pd = to_scmi_pd(domain);
>  
>  	if (power_on)
> @@ -33,13 +32,7 @@ static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
>  	else
>  		state = SCMI_POWER_STATE_GENERIC_OFF;
>  
> -	ret = power_ops->state_set(pd->ph, pd->domain, state);
> -	if (!ret)
> -		ret = power_ops->state_get(pd->ph, pd->domain, &ret_state);
> -	if (!ret && state != ret_state)
> -		return -EIO;
> -
> -	return ret;
> +	return power_ops->state_set(pd->ph, pd->domain, state);
>  }

...not sure about the history of this but it would have also definitely
failed consistently on any systen where the SCMI Server exposes resources
physical states (an IMPDEF behaviour), so that after a successfull set_OFF
on a shared resource a subsequent get() could return that the resource is
still physically ON if it was still needed by the other agennts sharing it...

LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH] pmdomain: arm: scmi_pm_domain: Remove redundant state verification

[Ulf Hansson]

On Fri, 14 Mar 2025 at 10:58, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Currently, scmi_pd_power() explicitly verifies whether the requested
> power state was applied by calling state_get(). While this check could
> detect failures where the state was not properly updated, ensuring
> correctness is the responsibility of the SCMI firmware.
>
> Removing this redundant state_get() call eliminates an unnecessary
> round-trip to the firmware, improving efficiency. Any mismatches
> between the requested and actual states should be handled by the SCMI
> firmware, which must return a failure if state_set() is unsuccessful.
>
> Additionally, in some cases, checking the state after powering off a
> domain may be unreliable or unsafe, depending on the firmware
> implementation.
>
> This patch removes the redundant verification, simplifying the function
> without compromising correctness.
>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Reported-and-tested-by: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/pmdomain/arm/scmi_pm_domain.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index 86b531e15b85..2a213c218126 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -24,8 +24,7 @@ struct scmi_pm_domain {
>
>  static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
>  {
> -       int ret;
> -       u32 state, ret_state;
> +       u32 state;
>         struct scmi_pm_domain *pd = to_scmi_pd(domain);
>
>         if (power_on)
> @@ -33,13 +32,7 @@ static int scmi_pd_power(struct generic_pm_domain *domain, bool power_on)
>         else
>                 state = SCMI_POWER_STATE_GENERIC_OFF;
>
> -       ret = power_ops->state_set(pd->ph, pd->domain, state);
> -       if (!ret)
> -               ret = power_ops->state_get(pd->ph, pd->domain, &ret_state);
> -       if (!ret && state != ret_state)
> -               return -EIO;
> -
> -       return ret;
> +       return power_ops->state_set(pd->ph, pd->domain, state);
>  }
>
>  static int scmi_pd_power_on(struct generic_pm_domain *domain)
> --
> 2.34.1
>