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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]


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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]