Re: [PATCH] irqchip/renesas-irqc: Use platform_get_irq_optional() to get the interrupt

[Andy Shevchenko]

On Mon, Dec 27, 2021 at 12:19 PM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Andy,
>
> On Mon, Dec 27, 2021 at 11:06 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Mon, Dec 27, 2021 at 12:02 PM Geert Uytterhoeven
> > <geert@xxxxxxxxxxxxxx> wrote:
> > > On Mon, Dec 27, 2021 at 10:57 AM Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Mon, Dec 27, 2021 at 11:45 AM Geert Uytterhoeven
> > > > <geert@xxxxxxxxxxxxxx> wrote:
> > > > > On Sun, Dec 26, 2021 at 9:49 AM Andy Shevchenko
> > > > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > > > On Sun, Dec 26, 2021 at 1:59 AM Lad, Prabhakar
> > > > > > <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > > > > On Sat, Dec 25, 2021 at 5:40 PM Andy Shevchenko
> > > > > > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > > > > > On Sat, Dec 25, 2021 at 7:28 PM Lad, Prabhakar
> > > > > > > > <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > > > > > > On Sat, Dec 25, 2021 at 4:46 PM Andy Shevchenko
> > > > > > > > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > > > > > > > On Thu, Dec 16, 2021 at 9:52 AM Lad Prabhakar
> > > > > > > > > > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > > > ret = platform_get_irq_optional(...);
> > > > > > > > > > if (ret < 0 && ret != -ENXIO)
> > > > > > > > > >   return ret;
> > > > > > > > > > if (ret > 0)
> > > > > > > > > >   ...we got it...
> > > > > > > > > >
> > > > > > > > > > It will allow the future API fix of platform_get_irq_optional() to be
> > > > > > > > > > really optional.
> > > > > > > > > >
> > > > > > > > > Later patch [0] (merged into -next) does check for -ENXIO first.
> > > > > > > > >
> > > > > > > > > [0] https://lore.kernel.org/lkml/20211216182121.5323-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/t/
> > > > > > > >
> > > > > > > > The problem is that it doesn't consider 0 as no IRQ.
> > > > > > > >
> > > > > > > Can you please point me to the discussion/patch where this API change
> > > > > > > is considered/discussed. Just to clarify now the new API for
> > > > > > > platform_get_irq_optional() will return "0" in case there is no
> > > > > > > interrupt and not not -ENXIO anymore?
> > > > > >
> > > > > > The longest one happened here:
> > > > > > https://lore.kernel.org/linux-ide/20211209145937.77719-1-andriy.shevchenko@xxxxxxxxxxxxxxx/T/#u
> > > > > >
> > > > > > It has links to some other discussions on the topic.
> > > > > >
> > > > > > > When will this patch be merged for the new api, so that I can base my
> > > > > > > patches on top of it to avoid more changes?
> > > > > >
> > > > > > You can simply imply that, I dunno when it gets merged (from my point
> > > > > > of view the users should be fixed first, and since you are adding
> > > > > > users, the burden is increasing).
> > > > >
> > > > > Not only users (drivers), but also providers (architecture-specific code).
> > > > > IRQ zero is still valid on some architectures, e.g. on SH[1].
> > > >
> > > > Are we talking about vIRQ?
> > > > And users are fine with a big warning?
> > >
> > > The warning is only seen when a driver uses platorm_get_irq{,_optional}().
> > > There are several other ways to obtain interrupts, avoiding the
> > > big warning.
> > >
> > > > My understanding is that the architecture code there is broken. It
> > > > needs to be fixed to use IRQ domains and all that machinery instead of
> > > > what it does.
> > > >
> > > > 0 is "no IRQ" in Linux.
> > > >
> > > > > [1] https://lore.kernel.org/linux-renesas-soc/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@xxxxxxxxxxxxxx/
> > >
> > > BTW, perhaps the IRQ subsystem should switch from integer IRQ numbers
> > > to an opaque object, like was done for GPIO before?
> >
> > Hasn't it been done a long time ago?
> > IIRC somewhere in ~2014 timeframe.
> >
> > > The IRQ subsystem
> > > is one of the few (only?) subsystems still using plain integers.
> >
> > It uses opaque objects for which the plain integer is a unique key.
>
> And the key "cannot" be zero.
>
> "cannot": that was the end goal, which is AFAIK still not reached,

What do you mean by that? The IRQ 0 is no IRQ in Linux, but the goal is reached.

> as we
> still have IRQ zero is struct resource in platform files.

This is not related to the above. The problem is that some (broken)
code is still in use. Maybe instead of obfuscating platform_get_irq*()
APIs we start fixing the root cause?

> > > That
> > > way we don't need this two-step platform_get_irq_optional() conversion
> > > (step 1: check for both -ENXIO and zero, step 2: drop the check for
> > > -ENXIO).

-- 
With Best Regards,
Andy Shevchenko