Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 10 Jun 2022 15:47:34 +0100
From:   Cristian Marussi <cristian.marussi@arm.com>
To:     Ludvig =?iso-8859-1?Q?P=E4rsson?= <ludvig.parsson@axis.com>
Cc:     Sudeep Holla <sudeep.holla@arm.com>, kernel@axis.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firmware: arm_scmi: Fix incorrect error propagation
Message-ID: <YqNZhmeol/JvEvsB@e120937-lin>
References: <20220610140055.31491-1-ludvig.parsson@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220610140055.31491-1-ludvig.parsson@axis.com>
Precedence: bulk

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
>