Reusing timed out xfers in a loop can lead to issue if completion was not
properly reinitialized.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index bee33f9c2032..759ae4a23e74 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
xfer->hdr.poll_completion);
xfer->hdr.status = SCMI_SUCCESS;
+ reinit_completion(&xfer->done);
ret = info->desc->ops->send_message(cinfo, xfer);
if (ret < 0) {
dev_dbg(dev, "Failed to send message %d\n", ret);
--
2.17.1
On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:
> Reusing timed out xfers in a loop can lead to issue if completion was not
> properly reinitialized.
>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> drivers/firmware/arm_scmi/driver.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index bee33f9c2032..759ae4a23e74 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
> xfer->hdr.poll_completion);
>
> xfer->hdr.status = SCMI_SUCCESS;
> + reinit_completion(&xfer->done);
>
What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
that it needs to be re-initialised again. I don't see any reason for this ?
If there are, please state them explicitly. If this is needed, I would drop
the one in scmi_xfer_get().
--
Regards,
Sudeep
Hi,
On Mon, Jun 07, 2021 at 06:42:57PM +0100, Sudeep Holla wrote:
> On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:
> > Reusing timed out xfers in a loop can lead to issue if completion was not
> > properly reinitialized.
> >
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/driver.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index bee33f9c2032..759ae4a23e74 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
> > xfer->hdr.poll_completion);
> >
> > xfer->hdr.status = SCMI_SUCCESS;
> > + reinit_completion(&xfer->done);
> >
>
> What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
> that it needs to be re-initialised again. I don't see any reason for this ?
> If there are, please state them explicitly. If this is needed, I would drop
> the one in scmi_xfer_get().
>
The reason, like I explained in the other reply in hdr.status, is the
possibility of do_xfer loops and being more defensive.
I agree that if what I blabbed in the other email is acceptable, I could
drop the reinit_completion in xfer_get() and just use it before
send_message().
Thanks,
Cristian
> --
> Regards,
> Sudeep
On Mon, Jun 07, 2021 at 07:04:34PM +0100, Cristian Marussi wrote:
> Hi,
>
> On Mon, Jun 07, 2021 at 06:42:57PM +0100, Sudeep Holla wrote:
> > On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:
> > > Reusing timed out xfers in a loop can lead to issue if completion was not
> > > properly reinitialized.
> > >
> > > Signed-off-by: Cristian Marussi <[email protected]>
> > > ---
> > > drivers/firmware/arm_scmi/driver.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index bee33f9c2032..759ae4a23e74 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > @@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
> > > xfer->hdr.poll_completion);
> > >
> > > xfer->hdr.status = SCMI_SUCCESS;
> > > + reinit_completion(&xfer->done);
> > >
> >
> > What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
> > that it needs to be re-initialised again. I don't see any reason for this ?
> > If there are, please state them explicitly. If this is needed, I would drop
> > the one in scmi_xfer_get().
> >
>
> The reason, like I explained in the other reply in hdr.status, is the
> possibility of do_xfer loops and being more defensive.
>
Understood.
> I agree that if what I blabbed in the other email is acceptable, I could
> drop the reinit_completion in xfer_get() and just use it before
> send_message().
>
I wonder if it is possible to reset only on error if possible and not
expensive of-course ?
--
Regards,
Sudeep
On Sun, 6 Jun 2021 23:12:22 +0100, Cristian Marussi wrote:
> Reusing timed out xfers in a loop can lead to issue if completion was not
> properly reinitialized.
Applied to sudeep.holla/linux (master), thanks!
[02/10] firmware: arm_scmi: Add missing xfer reinit_completion
https://git.kernel.org/sudeep.holla/c/e30d91d4ff
--
Regards,
Sudeep