Re: 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: 1973 bytes --]

Hi Jassi,

> > > 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.
> >
> 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?

I agree that this is unlikely to happen in practice. I still think this
is more than just a cosmetic issue, however. Because the above code path
breaks an expectation a programmer likely has. If probe() fails,
remove() is not called. If request_irq() fails, free_irq() is not
called. So, the expectation is that if startup() fails, shutdown() is
not called. I surely was surprised about this behaviour. And surprise is
not good, boring is good, I'd say. The free_irq() splat which this
currently causes is just one appearance of the problem. It can be seen
as "not so bad" because it is the follow-up of a previous problem. I am
afraid, though, that more subtle issues might show up in the future
because of this broken expectation. It is easy to fix, so I'd still vote
to go for some fix.

Thanks and happy hacking,

   Wolfram


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