Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Marc Zyngier <maz@kernel.org>]

Hi Vladimir,

On Mon, 13 Dec 2021 19:59:58 +0000,
Vladimir Oltean <olteanv@gmail.com> wrote:

[...]

>
> I am a user of the ls-extirq driver which is responsible for 3 of the 7
> compatible strings mentioned by you here. I have close to zero knowledge
> of the irq subsystem, although I am looking forward to learn.

Unfortunately, this has nothing to do with the IRQ subsystem, which
doesn't really care about the firmware interfaces.

> Could you please spend a few minutes to detail what you see as a possible
> path forward for this driver?

Define "path forward". My preference would be to travel back in time
so that this driver doesn't make it into the tree, but it is an
unlikely outcome. The only other solution is to leave it as is, but
not to allow any further occurrence of the issue.

> I am getting mixed impressions about what it's doing wrong.
>
> On one hand, it was requested by Rob during review that what used to be
> called "fsl,extirq-map" should be named "interrupt-map" instead:
> https://lore.kernel.org/lkml/20190928092331.GB1894@linutronix.de/

I stand by my analysis that this is wrong, by the very letter of what
an interrupt-map means. If the interrupt map points to an interrupt
controller, that's the target for the interrupt. No ifs, no buts.

> Then, you seem to suggest something's wrong with drivers privately using
> that name and parsing a property which used to be ignored by the core,
> due to your "silly-interrupt-map" comment:
> https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/T/#ebae8f9231296dc936cb7c9791218fc6785a03390

And I stand by this comment.

> Then, Rob breaks the ls-extirq driver for platforms that have a GIC ITS*
> defined in the device tree via commit 869f0ec048dc ("arm64: dts:
> freescale: Fix 'interrupt-map' parent address cells") - this is also,
> incidentally, the reason why I'm here.
> * because the driver doesn't parse the "standard" format where the
>   interrupt parent has a non-zero #address-cells - which the "arm,gic-v3"
>   may have when there's a "arm,gic-v3-its" under it (although I don't
>   necessarily see the relevance of the ITS being there to the needs of
>   the ls-extirq - which are just a bijective mapping of IRQs - this
>   driver simply drives a multi-channel logical inverter).

And that's another reason why using interrupt-map is totally
bonkers. You can't have your cake and eat it (in this case: use a
standard property and yet attribute it some other semantics) -- at
some point, these things break. And when they break, we're left with
these stupid quirks to paper over the breakage.

> So if I understand correctly, we keep ignoring the non-standard use of
> the "interrupt-map" property in these abuser drivers, yet we patch their
> device trees to have a more standard format in their non-standard use? :)

I'm happy to drop support for these FSL/NXP machines immediately. Say
the word, and I will merge the patch!

Now, when it comes to Rob's patch, I think this was the logic thing to
do, and that nobody realised how badly broken the whole thing was. I'm
just as guilty to have merged some of these drivers without really
checking what they were doing in their DT parsing (I tend to focus on
the correctness of the runtime behaviour). Expect a lot more scrutiny
for any new patch.

> Since some breakage has already been introduced, for good or bad, I
> think we can start discussing how things should have been done from the
> beginning, and see if we can make those changes now.

If using standard properties, this should have never been an
interrupt-map. A whole collection of 'interrupts', maybe (we have some
other issues with that, but nothing that cannot be fixed without
changing the DT). Or the initially proposed fsl,blah. But if you are
using a standard property, it is handled by the core code, and you
have no business messing with it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.