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

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

[Marc Zyngier]

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@kernel.org>
Cc: Anup Patel <apatel@ventanamicro.com>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Rob Herring (Arm) <robh@kernel.org>
---
 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);
 		imap += sup_args.args_count + 1;
+
+		of_node_put(sup_args.np);
 		index--;
 	}
 
-- 
2.39.2

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

[Rob Herring (Arm)]

On Tue, 28 May 2024 17:41:32 +0100, Marc Zyngier 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@kernel.org>
> Cc: Anup Patel <apatel@ventanamicro.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/property.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 

Applied, thanks!

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

[Saravana Kannan]

On Tue, May 28, 2024 at 6:41 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> Cc: Anup Patel <apatel@ventanamicro.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Rob Herring (Arm) <robh@kernel.org>
> ---
>  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);
>                 imap += sup_args.args_count + 1;
> +
> +               of_node_put(sup_args.np);
>                 index--;
>         }
>

Thanks Marc! And sorry for not catching this in my earlier review!

Acked-by: Saravana Kannan <saravanak@google.com>

-Saravana

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

[Anup Patel]

On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> Cc: Anup Patel <apatel@ventanamicro.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Rob Herring (Arm) <robh@kernel.org>

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.

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.

The below change on top of this patch worked for me.

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 9adebc63bea9..f54da2989ea9 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1308,7 +1308,7 @@ static struct device_node
*parse_interrupt_map(struct device_node *np,
 {
     const __be32 *imap, *imap_end, *addr;
     struct of_phandle_args sup_args;
-    u32 addrcells, intcells;
+    u32 addrcells, intcells, paddrcells;
     int i, imaplen;

     if (!IS_ENABLED(CONFIG_OF_IRQ))
@@ -1356,7 +1356,8 @@ static struct device_node
*parse_interrupt_map(struct device_node *np,
          * Account for the full parent unit interrupt specifier
          * (address cells, interrupt cells, and phandle).
          */
-        imap += of_bus_n_addr_cells(sup_args.np);
+        if (!of_property_read_u32(sup_args.np, "#address-cells", &paddrcells))
+            imap += paddrcells;
         imap += sup_args.args_count + 1;

         of_node_put(sup_args.np);

<snip>

Regards,
Anup

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

[Marc Zyngier]

On Wed, 29 May 2024 06:15:52 +0100,
Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > Cc: Anup Patel <apatel@ventanamicro.com>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Cc: Rob Herring (Arm) <robh@kernel.org>
> 
> 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.

> 
> 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.

> 
> The below change on top of this patch worked for me.
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 9adebc63bea9..f54da2989ea9 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1308,7 +1308,7 @@ static struct device_node
> *parse_interrupt_map(struct device_node *np,
>  {
>      const __be32 *imap, *imap_end, *addr;
>      struct of_phandle_args sup_args;
> -    u32 addrcells, intcells;
> +    u32 addrcells, intcells, paddrcells;
>      int i, imaplen;
> 
>      if (!IS_ENABLED(CONFIG_OF_IRQ))
> @@ -1356,7 +1356,8 @@ static struct device_node
> *parse_interrupt_map(struct device_node *np,
>           * Account for the full parent unit interrupt specifier
>           * (address cells, interrupt cells, and phandle).
>           */
> -        imap += of_bus_n_addr_cells(sup_args.np);
> +        if (!of_property_read_u32(sup_args.np, "#address-cells", &paddrcells))
> +            imap += paddrcells;

This looks wrong to me for the reason I outlined above: you need to
look for a valid #address-cells all along the parent chain, not just
in the interrupt-controller node.

	M.

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

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

[Anup Patel]

On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 29 May 2024 06:15:52 +0100,
> Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > > Cc: Rob Herring (Arm) <robh@kernel.org>
> >
> > 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.
>
> >
> > 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.

If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
then it would still break for RISC-V.

Using of_bus_n_addr_cells() over here forces interrupt controller
DT nodes to have a "#address-cells" DT property. There are many
interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
DT bindings don't require "#address-cells" DT property and existing
DTS files with such interrupt controllers don't have it either.

In the RISC-V world, there have been quite a few QEMU releases
where the generated DT node of the interrupt controller does not
have the "#address-cells" property. This patch breaks the kernel
for all such QEMU releases.

I think we should align the implementation in parse_interrupt_map()
with of_irq_parse_raw() and use of_property_read_u32() instead of
of_bus_n_addr_cells().

Regards,
Anup

>
> >
> > The below change on top of this patch worked for me.
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 9adebc63bea9..f54da2989ea9 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1308,7 +1308,7 @@ static struct device_node
> > *parse_interrupt_map(struct device_node *np,
> >  {
> >      const __be32 *imap, *imap_end, *addr;
> >      struct of_phandle_args sup_args;
> > -    u32 addrcells, intcells;
> > +    u32 addrcells, intcells, paddrcells;
> >      int i, imaplen;
> >
> >      if (!IS_ENABLED(CONFIG_OF_IRQ))
> > @@ -1356,7 +1356,8 @@ static struct device_node
> > *parse_interrupt_map(struct device_node *np,
> >           * Account for the full parent unit interrupt specifier
> >           * (address cells, interrupt cells, and phandle).
> >           */
> > -        imap += of_bus_n_addr_cells(sup_args.np);
> > +        if (!of_property_read_u32(sup_args.np, "#address-cells", &paddrcells))
> > +            imap += paddrcells;
>
> This looks wrong to me for the reason I outlined above: you need to
> look for a valid #address-cells all along the parent chain, not just
> in the interrupt-controller node.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

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

[Marc Zyngier]

On Wed, 29 May 2024 11:16:30 +0100,
Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 29 May 2024 06:15:52 +0100,
> > Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > Cc: Rob Herring (Arm) <robh@kernel.org>
> > >
> > > 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.
> >
> > >
> > > 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.
> 
> If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
> then it would still break for RISC-V.

I'm not trying to "fix" riscv. I'm merely outlining that you are
relying on both broken DTs and a buggy OS.

> 
> Using of_bus_n_addr_cells() over here forces interrupt controller
> DT nodes to have a "#address-cells" DT property. There are many
> interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
> DT bindings don't require "#address-cells" DT property and existing
> DTS files with such interrupt controllers don't have it either.

It forces you to set #address-cells *if you rely on a different
value in a child node*. It's not like the semantics are new.

> 
> In the RISC-V world, there have been quite a few QEMU releases
> where the generated DT node of the interrupt controller does not
> have the "#address-cells" property. This patch breaks the kernel
> for all such QEMU releases.

Congratulations, you've forked DT. News at 11.

> 
> I think we should align the implementation in parse_interrupt_map()
> with of_irq_parse_raw() and use of_property_read_u32() instead of
> of_bus_n_addr_cells().

I think we should fix the kernel and quirk riscv as broken, just like
PPC or sparc.

	M.

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

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

[Anup Patel]

On Wed, May 29, 2024 at 4:15 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 29 May 2024 11:16:30 +0100,
> Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Wed, 29 May 2024 06:15:52 +0100,
> > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > Cc: Rob Herring (Arm) <robh@kernel.org>
> > > >
> > > > 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.
> > >
> > > >
> > > > 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.
> >
> > If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
> > then it would still break for RISC-V.
>
> I'm not trying to "fix" riscv. I'm merely outlining that you are
> relying on both broken DTs and a buggy OS.
>
> >
> > Using of_bus_n_addr_cells() over here forces interrupt controller
> > DT nodes to have a "#address-cells" DT property. There are many
> > interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
> > DT bindings don't require "#address-cells" DT property and existing
> > DTS files with such interrupt controllers don't have it either.
>
> It forces you to set #address-cells *if you rely on a different
> value in a child node*. It's not like the semantics are new.

We don't have child nodes under the interrupt controller DT node
(for both RISC-V PLIC and APLIC) so we certainly don't require the
"#address-cells" property in the interrupt controller DT node.

>
> >
> > In the RISC-V world, there have been quite a few QEMU releases
> > where the generated DT node of the interrupt controller does not
> > have the "#address-cells" property. This patch breaks the kernel
> > for all such QEMU releases.
>
> Congratulations, you've forked DT. News at 11.

Can you elaborate how ?

>
> >
> > I think we should align the implementation in parse_interrupt_map()
> > with of_irq_parse_raw() and use of_property_read_u32() instead of
> > of_bus_n_addr_cells().
>
> I think we should fix the kernel and quirk riscv as broken, just like
> PPC or sparc.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Regards,
Anup

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

[Marc Zyngier]

On Wed, 29 May 2024 12:28:34 +0100,
Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Wed, May 29, 2024 at 4:15 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 29 May 2024 11:16:30 +0100,
> > Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Wed, 29 May 2024 06:15:52 +0100,
> > > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > > >
> > > > > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > > > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > > Cc: Rob Herring (Arm) <robh@kernel.org>
> > > > >
> > > > > 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.
> > > >
> > > > >
> > > > > 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.
> > >
> > > If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
> > > then it would still break for RISC-V.
> >
> > I'm not trying to "fix" riscv. I'm merely outlining that you are
> > relying on both broken DTs and a buggy OS.
> >
> > >
> > > Using of_bus_n_addr_cells() over here forces interrupt controller
> > > DT nodes to have a "#address-cells" DT property. There are many
> > > interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
> > > DT bindings don't require "#address-cells" DT property and existing
> > > DTS files with such interrupt controllers don't have it either.
> >
> > It forces you to set #address-cells *if you rely on a different
> > value in a child node*. It's not like the semantics are new.
> 
> We don't have child nodes under the interrupt controller DT node
> (for both RISC-V PLIC and APLIC) so we certainly don't require the
> "#address-cells" property in the interrupt controller DT node.

You keep missing the point.

You *do* require it if the parent node has an #address-cells value
that doesn't apply to its children nodes. Basic property inheritance.
Interrupt controller nodes are not special in this regard (and please
allow me to think that I know a thing or two about those).

So it's not that "you don't need it". It's that "you're relying on
something that is broken".

But in your defence, the DT spec is amusingly self-contradictory:

<quote>
2.3.5. #address-cells and #size-cells

The #address-cells and #size-cells properties may be used in any
device node that has children in the devicetree hierarchy and
describes how child device nodes should be addressed. The
#address-cells property defines the number of <u32> cells used to
encode the address field in a child node’s reg property. The
#size-cells property defines the number of <u32> cells used to encode
the size field in a child node’s reg property.

The #address-cells and #size-cells properties are not inherited from
ancestors in the devicetree. They shall be explicitly defined.
</quote>

Followed by:

<quote>
2.4.3.1. interrupt-map

Note

Both the child node and the interrupt parent node are required to have
#address-cells and #interrupt-cells properties defined. If a unit
address component is not required, #address-cells shall be explicitly
defined to be zero.
</quote>

which says one thing and then the other about property inheritance,
but then asserts that #address-cells isn't optional.

> >
> > >
> > > In the RISC-V world, there have been quite a few QEMU releases
> > > where the generated DT node of the interrupt controller does not
> > > have the "#address-cells" property. This patch breaks the kernel
> > > for all such QEMU releases.
> >
> > Congratulations, you've forked DT. News at 11.
> 
> Can you elaborate how ?

You've stated it yourself. You are relying on a behaviour that
deviates from the standard by having DTs with missing properties

And since we can't travel back it time to fix this, the only solution
I can see is to support both behaviours by quirking it.

	M.

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2929 bytes --]

On Wed, May 29, 2024 at 01:00:07PM +0100, Marc Zyngier wrote:
> > > > In the RISC-V world, there have been quite a few QEMU releases
> > > > where the generated DT node of the interrupt controller does not
> > > > have the "#address-cells" property. This patch breaks the kernel
> > > > for all such QEMU releases.
> > >
> > > Congratulations, you've forked DT. News at 11.
> > 
> > Can you elaborate how ?
> 
> You've stated it yourself. You are relying on a behaviour that
> deviates from the standard by having DTs with missing properties
> 
> And since we can't travel back it time to fix this, the only solution
> I can see is to support both behaviours by quirking it.

I'm not convinced that there is any actual production hardware that
would get broken by your patch, just QEMU, so I think it should get
fixed to output devicetrees that are spec compliant rather than add some
riscv-specific hacks that we can't even gate on the "qemu,aplic"
compatible because QEMU doesn't use the compatible created for it...

Spec violations aside, the QEMU aplic nodes in the DT contain a bunch
of other issues, including using properties that changed in the
upstreaming process. Here's the issues with Alistair's current riscv
tree for QEMU w/ -smp 4 -M virt,aia=aplic,dumpdtb=$(qemu_dtb) -cpu max -m 1G -nographic

qemu.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@c000000: $nodename:0: 'aplic@c000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@c000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@c000000: compatible: ['riscv,aplic'] is too short
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
qemu.dtb: aplic@c000000: Unevaluated properties are not allowed ('compatible', 'riscv,delegate' were unexpected)
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#

I guess noone updated QEMU to comply with the bindings that actually got
upstreamed for the aplic?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Anup Patel]

On Wed, May 29, 2024 at 8:47 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, May 29, 2024 at 01:00:07PM +0100, Marc Zyngier wrote:
> > > > > In the RISC-V world, there have been quite a few QEMU releases
> > > > > where the generated DT node of the interrupt controller does not
> > > > > have the "#address-cells" property. This patch breaks the kernel
> > > > > for all such QEMU releases.
> > > >
> > > > Congratulations, you've forked DT. News at 11.
> > >
> > > Can you elaborate how ?
> >
> > You've stated it yourself. You are relying on a behaviour that
> > deviates from the standard by having DTs with missing properties
> >
> > And since we can't travel back it time to fix this, the only solution
> > I can see is to support both behaviours by quirking it.
>
> I'm not convinced that there is any actual production hardware that
> would get broken by your patch, just QEMU, so I think it should get
> fixed to output devicetrees that are spec compliant rather than add some
> riscv-specific hacks that we can't even gate on the "qemu,aplic"
> compatible because QEMU doesn't use the compatible created for it...

I also did further digging and it turns out the "#address-cells"
is missing only for APLIC DT nodes and this issue only impacts
APLIC DT node creation in QEMU RISC-V virt machine. We
should just go ahead and fix QEMU.

>
> Spec violations aside, the QEMU aplic nodes in the DT contain a bunch
> of other issues, including using properties that changed in the
> upstreaming process. Here's the issues with Alistair's current riscv
> tree for QEMU w/ -smp 4 -M virt,aia=aplic,dumpdtb=$(qemu_dtb) -cpu max -m 1G -nographic
>
> qemu.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: $nodename:0: 'aplic@c000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: compatible: ['riscv,aplic'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: Unevaluated properties are not allowed ('compatible', 'riscv,delegate' were unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
>
> I guess noone updated QEMU to comply with the bindings that actually got
> upstreamed for the aplic?

Yes, we never bothered to update the QEMU DT generation after
AIA DT bindings were accepted. Thanks for catching.

Regards,
Anup

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

[Rob Herring]

On Wed, May 29, 2024 at 6:28 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Wed, May 29, 2024 at 4:15 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 29 May 2024 11:16:30 +0100,
> > Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Wed, 29 May 2024 06:15:52 +0100,
> > > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > > >
> > > > > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > > > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > > Cc: Rob Herring (Arm) <robh@kernel.org>
> > > > >
> > > > > 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.
> > > >
> > > > >
> > > > > 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.
> > >
> > > If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
> > > then it would still break for RISC-V.
> >
> > I'm not trying to "fix" riscv. I'm merely outlining that you are
> > relying on both broken DTs and a buggy OS.
> >
> > >
> > > Using of_bus_n_addr_cells() over here forces interrupt controller
> > > DT nodes to have a "#address-cells" DT property. There are many
> > > interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
> > > DT bindings don't require "#address-cells" DT property and existing
> > > DTS files with such interrupt controllers don't have it either.
> >
> > It forces you to set #address-cells *if you rely on a different
> > value in a child node*. It's not like the semantics are new.
>
> We don't have child nodes under the interrupt controller DT node
> (for both RISC-V PLIC and APLIC) so we certainly don't require the
> "#address-cells" property in the interrupt controller DT node.

interrupt controller nodes which are referred to by interrupt-map
require #address-cells. Period. Child nodes or not.

Really, it should be just interrupt-controller nodes require
#address-cells, but that spewed too many warnings so it's limited to
where it is really needed.

Rob

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

[Rob Herring]

On Wed, May 29, 2024 at 8:51 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, May 29, 2024 at 6:28 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, May 29, 2024 at 4:15 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Wed, 29 May 2024 11:16:30 +0100,
> > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Wed, May 29, 2024 at 12:03 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Wed, 29 May 2024 06:15:52 +0100,
> > > > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > > > >
> > > > > > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > > > > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > > > > > Cc: Saravana Kannan <saravanak@google.com>
> > > > > > > Cc: Rob Herring (Arm) <robh@kernel.org>
> > > > > >
> > > > > > 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.
> > > > >
> > > > > >
> > > > > > 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.
> > > >
> > > > If we change of_irq_parse_raw() to use of_bus_n_addr_cells()
> > > > then it would still break for RISC-V.
> > >
> > > I'm not trying to "fix" riscv. I'm merely outlining that you are
> > > relying on both broken DTs and a buggy OS.
> > >
> > > >
> > > > Using of_bus_n_addr_cells() over here forces interrupt controller
> > > > DT nodes to have a "#address-cells" DT property. There are many
> > > > interrupt controller (e.g. RISC-V PLIC or RISC-V APLIC) where the
> > > > DT bindings don't require "#address-cells" DT property and existing
> > > > DTS files with such interrupt controllers don't have it either.
> > >
> > > It forces you to set #address-cells *if you rely on a different
> > > value in a child node*. It's not like the semantics are new.
> >
> > We don't have child nodes under the interrupt controller DT node
> > (for both RISC-V PLIC and APLIC) so we certainly don't require the
> > "#address-cells" property in the interrupt controller DT node.
>
> interrupt controller nodes which are referred to by interrupt-map
> require #address-cells. Period. Child nodes or not.

Now that I've re-read the code, the kernel will treat missing
#address-cells in the interrupt parent as 0. Looking in the parent
nodes for #address-cells only happens for the input address (i.e. the
address before the phandle). IOW, the first use of_bus_n_addr_cells()
was correct, but the 2nd use is not correct. That's not great, but
that's how this code passed down on stone tablets works...

As I commented on v1 of the original patch. I don't like parsing
interrupt-map in 2 places. It leads to problems exactly as this thread
has shown. The duplication was reduced some, but is still more than
I'd like. So I'm looking at how to refactor of_irq_parse_raw() to make
it work for what's needed here. Maybe I'll have a fix quickly. If not,
I'm going to revert the original commit.

Rob

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@kernel.org> wrote:
>
> On Wed, 29 May 2024 06:15:52 +0100,
> Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Tue, May 28, 2024 at 10:11 PM Marc Zyngier <maz@kernel.org> 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@kernel.org>
> > > Cc: Anup Patel <apatel@ventanamicro.com>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > > Cc: Rob Herring (Arm) <robh@kernel.org>
> >
> > 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

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

[Marc Zyngier]

On Wed, 29 May 2024 14:44:07 +0100,
Rob Herring <robh@kernel.org> wrote:
> 
> On Wed, May 29, 2024 at 1:33 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 29 May 2024 06:15:52 +0100,
> > Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > 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.

Clearly that didn't deter the riscv folks from doing silly things, and
we're now stuck with more random hacks.

Anyhow, I vented enough about this, and I'm going back to doing
semi-useful stuff.

	M.

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