| Message ID | 20211201114102.13446-1-maz@kernel.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Marc, On Wed, Dec 1, 2021 at 12:41 PM Marc Zyngier <maz@kernel.org> wrote: > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local > to an interrupt controller"), a handful of interrupt controllers have > stopped working correctly. This is due to the DT exposing a non-sensical > interrupt-map property, and their drivers relying on the kernel ignoring > this property. > > Since we cannot realistically fix this terrible behaviour, add a quirk > for the limited set of devices that have implemented this monster, > and document that this is a pretty bad practice. > > Cc: Rob Herring <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Sander Vanheule <sander@svanheule.net> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > > Notes: > v2: Switched over to of_device_compatible_match() as per Rob's > request. Thanks for the update! > --- a/drivers/of/irq.c > +++ b/drivers/of/irq.c > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) > /* > * Now check if cursor is an interrupt-controller and > * if it is then we are done, unless there is an > - * interrupt-map which takes precedence. > + * interrupt-map which takes precedence if we're not > + * in presence of once of these broken platform that one > + * want to parse interrupt-map themselves for $reason. > */ > bool intc = of_property_read_bool(ipar, "interrupt-controller"); > + bool imap_abuse; > > imap = of_get_property(ipar, "interrupt-map", &imaplen); > - if (imap == NULL && intc) { > + imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers); ... = intc && imap && of_device_compatible_match(...) > + if (intc && (imap == NULL || imap_abuse)) { > pr_debug(" -> got it !\n"); > return 0; > } Still working fine on RZ/A1, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Thu, Dec 02, 2021 at 04:06:21PM +0100, Geert Uytterhoeven wrote: > Hi Marc, > > On Wed, Dec 1, 2021 at 12:41 PM Marc Zyngier <maz@kernel.org> wrote: > > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local > > to an interrupt controller"), a handful of interrupt controllers have > > stopped working correctly. This is due to the DT exposing a non-sensical > > interrupt-map property, and their drivers relying on the kernel ignoring > > this property. > > > > Since we cannot realistically fix this terrible behaviour, add a quirk > > for the limited set of devices that have implemented this monster, > > and document that this is a pretty bad practice. > > > > Cc: Rob Herring <robh@kernel.org> > > Cc: John Crispin <john@phrozen.org> > > Cc: Biwen Li <biwen.li@nxp.com> > > Cc: Chris Brandt <chris.brandt@renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Cc: Sander Vanheule <sander@svanheule.net> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > > > Notes: > > v2: Switched over to of_device_compatible_match() as per Rob's > > request. > > Thanks for the update! > > > --- a/drivers/of/irq.c > > +++ b/drivers/of/irq.c > > > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) > > /* > > * Now check if cursor is an interrupt-controller and > > * if it is then we are done, unless there is an > > - * interrupt-map which takes precedence. > > + * interrupt-map which takes precedence if we're not > > + * in presence of once of these broken platform that > > one and 'platforms'. Will fixup. > > > + * want to parse interrupt-map themselves for $reason. > > */ > > bool intc = of_property_read_bool(ipar, "interrupt-controller"); > > + bool imap_abuse; > > > > imap = of_get_property(ipar, "interrupt-map", &imaplen); > > - if (imap == NULL && intc) { > > + imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers); > > ... = intc && imap && of_device_compatible_match(...) Why? Then we are comparing intc twice because we still need it for the intc && !imap case. > > > + if (intc && (imap == NULL || imap_abuse)) { > > pr_debug(" -> got it !\n"); > > return 0; > > } > > Still working fine on RZ/A1, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > 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 >
On Wed, 01 Dec 2021 11:41:02 +0000, Marc Zyngier wrote: > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local > to an interrupt controller"), a handful of interrupt controllers have > stopped working correctly. This is due to the DT exposing a non-sensical > interrupt-map property, and their drivers relying on the kernel ignoring > this property. > > Since we cannot realistically fix this terrible behaviour, add a quirk > for the limited set of devices that have implemented this monster, > and document that this is a pretty bad practice. > > Cc: Rob Herring <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Sander Vanheule <sander@svanheule.net> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > > Notes: > v2: Switched over to of_device_compatible_match() as per Rob's > request. > > drivers/of/irq.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > Applied, thanks!
On Fri, Dec 3, 2021 at 11:17 AM Rob Herring <robh@kernel.org> wrote: > > On Thu, Dec 02, 2021 at 04:06:21PM +0100, Geert Uytterhoeven wrote: > > Hi Marc, > > > > On Wed, Dec 1, 2021 at 12:41 PM Marc Zyngier <maz@kernel.org> wrote: > > > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local > > > to an interrupt controller"), a handful of interrupt controllers have > > > stopped working correctly. This is due to the DT exposing a non-sensical > > > interrupt-map property, and their drivers relying on the kernel ignoring > > > this property. > > > > > > Since we cannot realistically fix this terrible behaviour, add a quirk > > > for the limited set of devices that have implemented this monster, > > > and document that this is a pretty bad practice. > > > > > > Cc: Rob Herring <robh@kernel.org> > > > Cc: John Crispin <john@phrozen.org> > > > Cc: Biwen Li <biwen.li@nxp.com> > > > Cc: Chris Brandt <chris.brandt@renesas.com> > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > > Cc: Sander Vanheule <sander@svanheule.net> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > --- > > > > > > Notes: > > > v2: Switched over to of_device_compatible_match() as per Rob's > > > request. > > > > Thanks for the update! > > > > > --- a/drivers/of/irq.c > > > +++ b/drivers/of/irq.c > > > > > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) > > > /* > > > * Now check if cursor is an interrupt-controller and > > > * if it is then we are done, unless there is an > > > - * interrupt-map which takes precedence. > > > + * interrupt-map which takes precedence if we're not > > > + * in presence of once of these broken platform that > > > > one > > and 'platforms'. Will fixup. > > > > > > + * want to parse interrupt-map themselves for $reason. > > > */ > > > bool intc = of_property_read_bool(ipar, "interrupt-controller"); > > > + bool imap_abuse; > > > > > > imap = of_get_property(ipar, "interrupt-map", &imaplen); > > > - if (imap == NULL && intc) { > > > + imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers); > > > > ... = intc && imap && of_device_compatible_match(...) > > Why? Then we are comparing intc twice because we still need it for the > intc && !imap case. I ended up rewriting it like this: - if (imap == NULL && intc) { + if (intc && + (!imap || of_device_compatible_match(ipar, of_irq_imap_abusers))) { Rob
Hello Marc, On Wed, Dec 01, 2021 at 11:41:02AM +0000, Marc Zyngier wrote: > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local > to an interrupt controller"), a handful of interrupt controllers have > stopped working correctly. This is due to the DT exposing a non-sensical > interrupt-map property, and their drivers relying on the kernel ignoring > this property. > > Since we cannot realistically fix this terrible behaviour, add a quirk > for the limited set of devices that have implemented this monster, > and document that this is a pretty bad practice. > > Cc: Rob Herring <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Sander Vanheule <sander@svanheule.net> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > > Notes: > v2: Switched over to of_device_compatible_match() as per Rob's > request. > > drivers/of/irq.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c > index b10f015b2e37..65a325aad984 100644 > --- a/drivers/of/irq.c > +++ b/drivers/of/irq.c > @@ -76,6 +76,26 @@ struct device_node *of_irq_find_parent(struct device_node *child) > } > EXPORT_SYMBOL_GPL(of_irq_find_parent); > > +/* > + * These interrupt controllers abuse interrupt-map for unspeakable > + * reasons and rely on the core code to *ignore* it (the drivers do > + * their own parsing of the property). > + * > + * If you think of adding to the list for something *new*, think > + * again. There is a high chance that you will be sent back to the > + * drawing board. > + */ > +static const char * const of_irq_imap_abusers[] = { > + "CBEA,platform-spider-pic", > + "sti,platform-spider-pic", > + "realtek,rtl-intc", > + "fsl,ls1021a-extirq", > + "fsl,ls1043a-extirq", > + "fsl,ls1088a-extirq", > + "renesas,rza1-irqc", > + NULL, > +}; > + > /** > * of_irq_parse_raw - Low level interrupt tree parsing > * @addr: address specifier (start of "reg" property of the device) in be32 format > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) > /* > * Now check if cursor is an interrupt-controller and > * if it is then we are done, unless there is an > - * interrupt-map which takes precedence. > + * interrupt-map which takes precedence if we're not > + * in presence of once of these broken platform that > + * want to parse interrupt-map themselves for $reason. > */ > bool intc = of_property_read_bool(ipar, "interrupt-controller"); > + bool imap_abuse; > > imap = of_get_property(ipar, "interrupt-map", &imaplen); > - if (imap == NULL && intc) { > + imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers); > + if (intc && (imap == NULL || imap_abuse)) { > pr_debug(" -> got it !\n"); > return 0; > } > -- > 2.30.2 > 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. Could you please spend a few minutes to detail what you see as a possible path forward for this driver? 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/ 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 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). 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? :) 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.
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.
diff --git a/drivers/of/irq.c b/drivers/of/irq.c index b10f015b2e37..65a325aad984 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -76,6 +76,26 @@ struct device_node *of_irq_find_parent(struct device_node *child) } EXPORT_SYMBOL_GPL(of_irq_find_parent); +/* + * These interrupt controllers abuse interrupt-map for unspeakable + * reasons and rely on the core code to *ignore* it (the drivers do + * their own parsing of the property). + * + * If you think of adding to the list for something *new*, think + * again. There is a high chance that you will be sent back to the + * drawing board. + */ +static const char * const of_irq_imap_abusers[] = { + "CBEA,platform-spider-pic", + "sti,platform-spider-pic", + "realtek,rtl-intc", + "fsl,ls1021a-extirq", + "fsl,ls1043a-extirq", + "fsl,ls1088a-extirq", + "renesas,rza1-irqc", + NULL, +}; + /** * of_irq_parse_raw - Low level interrupt tree parsing * @addr: address specifier (start of "reg" property of the device) in be32 format @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) /* * Now check if cursor is an interrupt-controller and * if it is then we are done, unless there is an - * interrupt-map which takes precedence. + * interrupt-map which takes precedence if we're not + * in presence of once of these broken platform that + * want to parse interrupt-map themselves for $reason. */ bool intc = of_property_read_bool(ipar, "interrupt-controller"); + bool imap_abuse; imap = of_get_property(ipar, "interrupt-map", &imaplen); - if (imap == NULL && intc) { + imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers); + if (intc && (imap == NULL || imap_abuse)) { pr_debug(" -> got it !\n"); return 0; }
Since 041284181226 ("of/irq: Allow matching of an interrupt-map local to an interrupt controller"), a handful of interrupt controllers have stopped working correctly. This is due to the DT exposing a non-sensical interrupt-map property, and their drivers relying on the kernel ignoring this property. Since we cannot realistically fix this terrible behaviour, add a quirk for the limited set of devices that have implemented this monster, and document that this is a pretty bad practice. Cc: Rob Herring <robh@kernel.org> Cc: John Crispin <john@phrozen.org> Cc: Biwen Li <biwen.li@nxp.com> Cc: Chris Brandt <chris.brandt@renesas.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Sander Vanheule <sander@svanheule.net> Signed-off-by: Marc Zyngier <maz@kernel.org> --- Notes: v2: Switched over to of_device_compatible_match() as per Rob's request. drivers/of/irq.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)