Messages in this thread | | | Date | Fri, 10 Jun 2022 15:47:34 +0100 | From | Cristian Marussi <> | Subject | Re: [PATCH] firmware: arm_scmi: Fix incorrect error propagation |
| |
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 >
| |