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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@linaro.org>
> >>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>> ---
> >>>  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
> >>>