Re: Query regarding "firmware: arm_scmi: Free mailbox channels if probe fails"

[Shivnandan Kumar]

hi Cristian,

Thanks for your support in providing the patch to try.

I found one race condition in our downstream mbox controller driver 
while accessing con_priv, when I serialized access to this, issue is not 
seen on 3 days of testing.

As you rightly mentioned that your provided patch will impact all the 
other users.

Also if  we take your provided patch, same race still exists while 
accessing con_priv in our downstream mbox controller so this issue will 
still be there.

So, we are planning to merge the patch( serialized access to con_priv) 
in our downstream mbox controller now.


Thanks,

Shivnandan


On 9/22/2022 8:58 PM, Cristian Marussi wrote:
> On Thu, Sep 22, 2022 at 03:54:31PM +0100, Cristian Marussi wrote:
> > On Thu, Sep 22, 2022 at 10:31:47AM +0530, Shivnandan Kumar wrote:
> > > Hi Cristian,
> Hi Shivnandan,
>   
> [snip]
>
> > Looking at the transport layer that you use, mailbox, I see that while
> > setup/free helpers are synchronized on an internal chan->lock, the RX
> > path inside the mailbox core is not, so I tried this:
> >
> >
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 4229b9b5da98..bb6173c0ad54 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -157,9 +157,13 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> >    */
> >   void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
> >   {
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&chan->lock, flags);
> >          /* No buffering the received data */
> >          if (chan->cl->rx_callback)
> >                  chan->cl->rx_callback(chan->cl, mssg);
> > +       spin_unlock_irqrestore(&chan->lock, flags);
> >   }
> >   EXPORT_SYMBOL_GPL(mbox_chan_received_data);
> >   
> ...sorry... a small change on the tentative above fix...
>
> ----8<------
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 4229b9b5da98..6fbe183acdae 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -157,9 +157,13 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>    */
>   void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>   {
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&chan->lock, flags);
>          /* No buffering the received data */
> -       if (chan->cl->rx_callback)
> +       if (chan->cl && chan->cl->rx_callback)
>                  chan->cl->rx_callback(chan->cl, mssg);
> +       spin_unlock_irqrestore(&chan->lock, flags);
>   }
>   EXPORT_SYMBOL_GPL(mbox_chan_received_data);
>
> ------8<-----
>
> Thanks,
> Cristian