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!