[v2,6/6] firmware: arm_scmi: Check if platform has released shmem before using

Sometimes platfom may take too long to respond to the command and OS
might timeout before platform transfer the ownership of the shared
memory region to the OS with the response.

Since the mailbox channel associated with the channel is freed and new
commands are dispatch on the same channel, OS needs to wait until it
gets back the ownership. If not, either OS may end up overwriting the
platform response for the last command(which is fine as OS timed out
that command) or platform might overwrite the payload for the next
command with the response for the old.

The latter is problematic as platform may end up interpretting the
response as the payload. In order to avoid such race, let's wait until
the OS gets back the ownership before we prepare the shared memory with
the payload for the next command.

Reported-by: Jim Quinlan <james.quinlan@broadcom.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/firmware/arm_scmi/driver.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.17.1

Hello Sudeep,

On Fri, 26 Jul 2019 at 15:46, Sudeep Holla <sudeep.holla@arm.com> wrote:
>

> Sometimes platfom may take too long to respond to the command and OS

> might timeout before platform transfer the ownership of the shared

> memory region to the OS with the response.

>

> Since the mailbox channel associated with the channel is freed and new

> commands are dispatch on the same channel, OS needs to wait until it

> gets back the ownership. If not, either OS may end up overwriting the

> platform response for the last command(which is fine as OS timed out

> that command) or platform might overwrite the payload for the next

> command with the response for the old.

>

> The latter is problematic as platform may end up interpretting the

> response as the payload. In order to avoid such race, let's wait until

> the OS gets back the ownership before we prepare the shared memory with

> the payload for the next command.

>

> Reported-by: Jim Quinlan <james.quinlan@broadcom.com>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  drivers/firmware/arm_scmi/driver.c | 8 ++++++++

>  1 file changed, 8 insertions(+)

>

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c

> index 69bf85fea967..765573756987 100644

> --- a/drivers/firmware/arm_scmi/driver.c

> +++ b/drivers/firmware/arm_scmi/driver.c

> @@ -265,6 +265,14 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)

>         struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);

>         struct scmi_shared_mem __iomem *mem = cinfo->payload;

>

> +       /*

> +        * Ideally channel must be free by now unless OS timeout last

> +        * request and platform continued to process the same, wait

> +        * until it releases the shared memory, otherwise we may endup

> +        * overwriting it's response with new command payload or vice-versa


minor typo: s/it's/its/
maybe also s/command/message/

regards,
etienne


> +        */

> +       spin_until_cond(ioread32(&mem->channel_status) &

> +                       SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);

>         /* Mark channel busy + clear error */

>         iowrite32(0x0, &mem->channel_status);

>         iowrite32(t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,

> --

> 2.17.1

>

On Mon, Aug 05, 2019 at 02:33:53PM +0200, Etienne Carriere wrote:
> Hello Sudeep,

>

> On Fri, 26 Jul 2019 at 15:46, Sudeep Holla <sudeep.holla@arm.com> wrote:

> >

> > Sometimes platfom may take too long to respond to the command and OS

> > might timeout before platform transfer the ownership of the shared

> > memory region to the OS with the response.

> >

> > Since the mailbox channel associated with the channel is freed and new

> > commands are dispatch on the same channel, OS needs to wait until it

> > gets back the ownership. If not, either OS may end up overwriting the

> > platform response for the last command(which is fine as OS timed out

> > that command) or platform might overwrite the payload for the next

> > command with the response for the old.

> >

> > The latter is problematic as platform may end up interpretting the

> > response as the payload. In order to avoid such race, let's wait until

> > the OS gets back the ownership before we prepare the shared memory with

> > the payload for the next command.

> >

> > Reported-by: Jim Quinlan <james.quinlan@broadcom.com>

> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > ---

> >  drivers/firmware/arm_scmi/driver.c | 8 ++++++++

> >  1 file changed, 8 insertions(+)

> >

> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c

> > index 69bf85fea967..765573756987 100644

> > --- a/drivers/firmware/arm_scmi/driver.c

> > +++ b/drivers/firmware/arm_scmi/driver.c

> > @@ -265,6 +265,14 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)

> >         struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);

> >         struct scmi_shared_mem __iomem *mem = cinfo->payload;

> >

> > +       /*

> > +        * Ideally channel must be free by now unless OS timeout last

> > +        * request and platform continued to process the same, wait

> > +        * until it releases the shared memory, otherwise we may endup

> > +        * overwriting it's response with new command payload or vice-versa

>

> minor typo: s/it's/its/

> maybe also s/command/message/

>


Thanks for taking a look at this, both are fixed locally now.

--
Regards,
Sudeep