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

[Geert Uytterhoeven]

On Mon, 13 Apr 2026 at 08:51, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > > 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.

+1

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds