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

From: Shivnandan Kumar
Date: Fri Sep 30 2022 - 08:59:35 EST


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