Re: [PATCH] of: property: Fix fw_devlink handling of interrupt-map

[Rob Herring]

On Wed, May 29, 2024 at 1:33 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Wed, 29 May 2024 06:15:52 +0100,
> Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > Commit d976c6f4b32c ("of: property: Add fw_devlink support for
> > > interrupt-map property") tried to do what it says on the tin,
> > > but failed on a couple of points:
> > >
> > > - it confuses bytes and cells. Not a huge deal, except when it
> > >   comes to pointer arithmetic
> > >
> > > - it doesn't really handle anything but interrupt-maps that have
> > >   their parent #address-cells set to 0
> > >
> > > The combinations of the two leads to some serious fun on my M1
> > > box, with plenty of WARN-ON() firing all over the shop, and
> > > amusing values being generated for interrupt specifiers.
> > >
> > > Address both issues so that I can boot my machines again.
> > >
> > > Fixes: d976c6f4b32c ("of: property: Add fw_devlink support for interrupt-map property")
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > Cc: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > > Cc: Saravana Kannan <saravanak@xxxxxxxxxx>
> > > Cc: Rob Herring (Arm) <robh@xxxxxxxxxx>
> >
> > Thanks for the fix patch but unfortunately it breaks for RISC-V.
> >
> > > ---
> > >  drivers/of/property.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 1c83e68f805b..9adebc63bea9 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1322,7 +1322,13 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
> > >         addrcells = of_bus_n_addr_cells(np);
> > >
> > >         imap = of_get_property(np, "interrupt-map", &imaplen);
> > > -       if (!imap || imaplen <= (addrcells + intcells))
> > > +       imaplen /= sizeof(*imap);
> > > +
> > > +       /*
> > > +        * Check that we have enough runway for the child unit interrupt
> > > +        * specifier and a phandle. That's the bare minimum we can expect.
> > > +        */
> > > +       if (!imap || imaplen <= (addrcells + intcells + 1))
> > >                 return NULL;
> > >         imap_end = imap + imaplen;
> > >
> > > @@ -1346,8 +1352,14 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
> > >                 if (!index)
> > >                         return sup_args.np;
> > >
> > > -               of_node_put(sup_args.np);
> > > +               /*
> > > +                * Account for the full parent unit interrupt specifier
> > > +                * (address cells, interrupt cells, and phandle).
> > > +                */
> > > +               imap += of_bus_n_addr_cells(sup_args.np);
> >
> > This breaks for RISC-V because we don't have "#address-cells"
> > property in interrupt controller DT node and of_bus_n_addr_cells()
> > retrieves "#address-cells" from the parent of interrupt controller.
>
> That's a feature, not a bug. #address-cells, AFAICT, applies to all
> child nodes until you set it otherwise.

That may be supported in some places, but only because of buggy DTs
(we're talking 2000 era). Current dtc should warn if an interrupt
controller node doesn't have #address-cells AND is referred to by
interrupt-map.

There's also the notion of default root values, but that's broken as
well. dtc and the kernel don't even agree on the default for some
arches. Fortunately, that's been a dtc warning longer than I've worked
on DT.


> > The of_irq_parse_raw() looks for "#address-cells" property
> > in the interrupt controller DT node only so we should do a
> > similar thing here as well.
>
> This looks more like a of_irq_parse_raw() bug than anything else.

Nope.

Rob