[PATCH] firmware: arm_scmi: Fix incorrect error propagation

[PATCH] firmware: arm_scmi: Fix incorrect error propagation

[Ludvig Pärsson]

scmi_voltage_descriptors_get() will incorrecly return an
error code if the last iteration of the for loop that
retrieves the descriptors is skipped due to an error.
Skipping an iteration in the loop is not an error, but
the `ret` value from the last iteration will be
propagated when the function returns.

Fix by not saving return values that should not be
propagated. This solution also minimizes the risk of
future patches accidentally reintroducing this bug.

Signed-off-by: Ludvig Pärsson <ludvig.parsson@axis.com>
---
 drivers/firmware/arm_scmi/voltage.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 9d195d8719ab..49b75375d3ff 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -225,9 +225,8 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 
 		/* Retrieve domain attributes at first ... */
 		put_unaligned_le32(dom, td->tx.buf);
-		ret = ph->xops->do_xfer(ph, td);
 		/* Skip domain on comms error */
-		if (ret)
+		if (ph->xops->do_xfer(ph, td))
 			continue;
 
 		v = vinfo->domains + dom;
@@ -249,9 +248,8 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 				v->async_level_set = true;
 		}
 
-		ret = scmi_voltage_levels_get(ph, v);
 		/* Skip invalid voltage descriptors */
-		if (ret)
+		if (scmi_voltage_levels_get(ph, v))
 			continue;
 
 		ph->xops->reset_rx_to_maxsz(ph, td);
-- 
2.20.1

Re: [PATCH] firmware: arm_scmi: Fix incorrect error propagation

[Cristian Marussi]

On Fri, Jun 10, 2022 at 04:00:55PM +0200, Ludvig Pärsson wrote:
> scmi_voltage_descriptors_get() will incorrecly return an
> error code if the last iteration of the for loop that
> retrieves the descriptors is skipped due to an error.
> Skipping an iteration in the loop is not an error, but
> the `ret` value from the last iteration will be
> propagated when the function returns.
> 
> Fix by not saving return values that should not be
> propagated. This solution also minimizes the risk of
> future patches accidentally reintroducing this bug.
> 
> Signed-off-by: Ludvig Pärsson <ludvig.parsson@axis.com>
> ---

Hi Ludvig,

you are right, good catch.

I would also say, reviewing this now, that the following line:

 ph->xops->reset_rx_to_maxsz

it is not called when skipping (even by the original code) BUT
this is not a problem given that VOLTAGE_DOMAIN_ATTRIBUTES is not
a command returning a variable length reply, so it really never
needed to reset the buffer size to max when called in a loop.
(almost all of these command are now embedded in the iterator
helpers that take care to call reset_to_maxsz on their own
internally)

I'll post a fix to remove that last unneeded line soon-ish.

In the meantime, regarding this patch instead:

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

Thanks for this,
Cristian

>  drivers/firmware/arm_scmi/voltage.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> index 9d195d8719ab..49b75375d3ff 100644
> --- a/drivers/firmware/arm_scmi/voltage.c
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -225,9 +225,8 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
>  
>  		/* Retrieve domain attributes at first ... */
>  		put_unaligned_le32(dom, td->tx.buf);
> -		ret = ph->xops->do_xfer(ph, td);
>  		/* Skip domain on comms error */
> -		if (ret)
> +		if (ph->xops->do_xfer(ph, td))
>  			continue;
>  
>  		v = vinfo->domains + dom;
> @@ -249,9 +248,8 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
>  				v->async_level_set = true;
>  		}
>  
> -		ret = scmi_voltage_levels_get(ph, v);
>  		/* Skip invalid voltage descriptors */
> -		if (ret)
> +		if (scmi_voltage_levels_get(ph, v))
>  			continue;
>  
>  		ph->xops->reset_rx_to_maxsz(ph, td);
> -- 
> 2.20.1
> 

Re: [PATCH] firmware: arm_scmi: Fix incorrect error propagation

[Sudeep Holla]

On Fri, 10 Jun 2022 16:00:55 +0200, Ludvig Pärsson wrote:
> scmi_voltage_descriptors_get() will incorrecly return an
> error code if the last iteration of the for loop that
> retrieves the descriptors is skipped due to an error.
> Skipping an iteration in the loop is not an error, but
> the `ret` value from the last iteration will be
> propagated when the function returns.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/1] firmware: arm_scmi: Fix incorrect error propagation
      https://git.kernel.org/sudeep.holla/c/44dbdf3bb3
--
Regards,
Sudeep