Re: [PATCH] remoteproc: stm32: fix mbox_send_message call

From: Bjorn Andersson
Date: Wed Jun 23 2021 - 14:45:22 EST


On Tue 22 Jun 02:56 CDT 2021, Arnaud POULIQUEN wrote:

> Hello Bjorn
>
> On 5/28/21 10:03 AM, Arnaud POULIQUEN wrote:
> > Hello Bjorn,
> >
> > On 5/28/21 5:26 AM, Bjorn Andersson wrote:
> >> On Tue 20 Apr 04:19 CDT 2021, Arnaud Pouliquen wrote:
> >>
> >>> mbox_send_message is called by passing a local dummy message or
> >>> a function parameter. As the message is queued, it is dereferenced.
> >>> This works because the message field is not used by the stm32 ipcc
> >>> driver, but it is not clean.
> >>>
> >>> Fix by passing a constant string in all cases.
> >>>
> >>> The associated comments are removed because rproc should not have to
> >>> deal with the behavior of the mailbox frame.
> >>>
> >>
> >> Didn't we conclude that the mailbox driver doesn't actually dereference
> >> the pointer being passed?
> >
> > Right it can store the reference to queue the sent.
> >
> >>
> >> If so I would prefer that you just pass NULL, so that if you in the
> >> future need to pass some actual data it will be easy to distinguish the
> >> old and new case.
> >
> > I can not use NULL pointer in stm32_rproc_attach and stm32_rproc_detach case.
> > The reason is that the tx_done callback is not called if the message is NULL.
> > (https://elixir.bootlin.com/linux/latest/source/drivers/mailbox/mailbox.c#L106)
> >
> > I could use NULL pointer in stm32_rproc_kick, but I would prefer to use the same way
> > of calling mbox_send_message for all use cases and not take into account the
> > mailbox internal behavior.
>
> Do you still have any concern about this patch?
>

I think my concern is now directed at the mailbox api. I think the
change is reasonable given that. Thanks for the explanation. I'm picking
up the patch.

Regards,
Bjorn

> Thanks,
> Arnaud
>
> >
> > Thanks,
> > Arnaud
> >
> >
> >>
> >> Regards,
> >> Bjorn
> >>
> >>> Reported-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> >>> ---
> >>> drivers/remoteproc/stm32_rproc.c | 14 +++++---------
> >>> 1 file changed, 5 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >>> index 7353f9e7e7af..0e8203a432ab 100644
> >>> --- a/drivers/remoteproc/stm32_rproc.c
> >>> +++ b/drivers/remoteproc/stm32_rproc.c
> >>> @@ -474,14 +474,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
> >>> static int stm32_rproc_detach(struct rproc *rproc)
> >>> {
> >>> struct stm32_rproc *ddata = rproc->priv;
> >>> - int err, dummy_data, idx;
> >>> + int err, idx;
> >>>
> >>> /* Inform the remote processor of the detach */
> >>> idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_DETACH);
> >>> if (idx >= 0 && ddata->mb[idx].chan) {
> >>> - /* A dummy data is sent to allow to block on transmit */
> >>> - err = mbox_send_message(ddata->mb[idx].chan,
> >>> - &dummy_data);
> >>> + err = mbox_send_message(ddata->mb[idx].chan, "stop");
> >>> if (err < 0)
> >>> dev_warn(&rproc->dev, "warning: remote FW detach without ack\n");
> >>> }
> >>> @@ -493,15 +491,13 @@ static int stm32_rproc_detach(struct rproc *rproc)
> >>> static int stm32_rproc_stop(struct rproc *rproc)
> >>> {
> >>> struct stm32_rproc *ddata = rproc->priv;
> >>> - int err, dummy_data, idx;
> >>> + int err, idx;
> >>>
> >>> /* request shutdown of the remote processor */
> >>> if (rproc->state != RPROC_OFFLINE) {
> >>> idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_SHUTDOWN);
> >>> if (idx >= 0 && ddata->mb[idx].chan) {
> >>> - /* a dummy data is sent to allow to block on transmit */
> >>> - err = mbox_send_message(ddata->mb[idx].chan,
> >>> - &dummy_data);
> >>> + err = mbox_send_message(ddata->mb[idx].chan, "detach");
> >>> if (err < 0)
> >>> dev_warn(&rproc->dev, "warning: remote FW shutdown without ack\n");
> >>> }
> >>> @@ -556,7 +552,7 @@ static void stm32_rproc_kick(struct rproc *rproc, int vqid)
> >>> continue;
> >>> if (!ddata->mb[i].chan)
> >>> return;
> >>> - err = mbox_send_message(ddata->mb[i].chan, (void *)(long)vqid);
> >>> + err = mbox_send_message(ddata->mb[i].chan, "kick");
> >>> if (err < 0)
> >>> dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
> >>> __func__, ddata->mb[i].name, err);
> >>> --
> >>> 2.17.1
> >>>