Re: Sashiko review (Re: [PATCH v4 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Jassi Brar]

On Thu, Apr 2, 2026 at 10:08 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> Answering Sashiko:
>
> > > +static irqreturn_t mfis_mb_iicr_interrupt(int irq, void *data)
> > > +{
> > > +   struct mbox_chan *chan = data;
> > > +   struct mfis_priv *priv = mfis_mb_mbox_to_priv(chan->mbox);
> > > +   struct mfis_chan_priv *chan_priv = chan->con_priv;
> > > +
> > > +   mbox_chan_received_data(chan, NULL);
> > > +   /* Stop remote(!) doorbell */
> > > +   mfis_write(&priv->mbox_reg, chan_priv->reg, 0);
> > Does this clear the correct register for bidirectional channels?
> > In mfis_mb_of_xlate(), for bidirectional channels (!mb_channels_are_unidir),
> > a single register is stored in chan_priv->reg.
> > When the receive interrupt triggers, this writes 0 to chan_priv->reg. If
> > this points to the transmit register, it looks like it would fail to clear
> > the receive interrupt and inadvertently cancel pending outgoing
> > transmissions.
> > Could this lead to an interrupt storm?
>
> While technically correct, this does not matter in practice. The HW with
> bidirectional channels looks quite different and will need a different
> interrupt handler. Sashiko cannot detect this because the part for
> bidirectional channels will be added incrementally once HW is available.
>
> > > +static int mfis_mb_startup(struct mbox_chan *chan)
> > > +{
> > > +   struct mfis_chan_priv *chan_priv = chan->con_priv;
> > > +
> > > +   if (!chan_priv->irq)
> > > +           return 0;
> > > +
> > > +   return request_irq(chan_priv->irq, mfis_mb_iicr_interrupt, 0,
> > > +                      dev_name(chan->mbox->dev), chan);
> > > +}
> > > +
> > > +static void mfis_mb_shutdown(struct mbox_chan *chan)
> > > +{
> > > +   struct mfis_chan_priv *chan_priv = chan->con_priv;
> > > +
> > > +   if (chan_priv->irq)
> > > +           free_irq(chan_priv->irq, chan);
> > > +}
> > What happens if request_irq() fails in mfis_mb_startup()?
> > If request_irq() returns an error, the mailbox framework handles the startup
> > failure by calling the shutdown() callback.
> > Since chan_priv->irq is populated earlier during of_xlate(), it will be
> > non-zero here. Will this cause free_irq() to be called on an IRQ that was
> > never successfully allocated, triggering a warning?
>
> Uuuhh, yes! But this is not a problem of this driver but more of the
> subsystem. It is definitely not intuitive that shutdown() is called when
> startup() failed. There are more mailbox drivers which fell into this
> trap, mostly by freeing an irq they never got. I will have a look at
> this, but as said, I think it should be solved on subsystem level.
>
> While this adds another item to my todo-list, it also means that none of
> the above issues are related to the driver itself \o/ Looks like it is
> good to go upstream, finally!
>
Honestly, I'd treat this as a cosmetic issue. If we fail to get the
IRQ, the channel is already dead in the water. Seeing a warning during
the subsequent cleanup is just a symptom of missing that critical
resource.
How often does your client acquire/release the channel and how
probable is request_irq() to fail in your platform?

Cheers!
-Jassi