Received-SPF: pass (google.com: domain of linux-kernel+bounces-193519-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 29 May 2024 07:33:54 +0100
Message-ID: <86bk4pm8j1.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org>
To: Anup Patel <apatel@ventanamicro.com>
Cc: devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-riscv@lists.infradead.org,
	Saravana Kannan <saravanak@google.com>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH] of: property: Fix fw_devlink handling of interrupt-map
In-Reply-To: <CAK9=C2XNPJP0X=pg5TSrQbsuouDD3jP-gy2Sm4BXNJp0ZiWp+A@mail.gmail.com>
References: <20240528164132.2451685-1-maz@kernel.org>
	<CAK9=C2XNPJP0X=pg5TSrQbsuouDD3jP-gy2Sm4BXNJp0ZiWp+A@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, 29 May 2024 06:15:52 +0100,
Anup Patel <apatel@ventanamicro.com> wrote:
>=20
> On Tue, May 28, 2024 at 10:11=E2=80=AFPM Marc Zyngier <maz@kernel.org> wr=
ote:
> >
> > 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 interrup=
t-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>
>=20
> Thanks for the fix patch but unfortunately it breaks for RISC-V.
>=20
> > ---
> >  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(s=
truct device_node *np,
> >         addrcells =3D of_bus_n_addr_cells(np);
> >
> >         imap =3D of_get_property(np, "interrupt-map", &imaplen);
> > -       if (!imap || imaplen <=3D (addrcells + intcells))
> > +       imaplen /=3D sizeof(*imap);
> > +
> > +       /*
> > +        * Check that we have enough runway for the child unit interrupt
> > +        * specifier and a phandle. That's the bare minimum we can expe=
ct.
> > +        */
> > +       if (!imap || imaplen <=3D (addrcells + intcells + 1))
> >                 return NULL;
> >         imap_end =3D imap + imaplen;
> >
> > @@ -1346,8 +1352,14 @@ static struct device_node *parse_interrupt_map(s=
truct 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 +=3D of_bus_n_addr_cells(sup_args.np);
>=20
> 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.

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

>=20
> The below change on top of this patch worked for me.
>=20
> 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;
>=20
>      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 +=3D of_bus_n_addr_cells(sup_args.np);
> +        if (!of_property_read_u32(sup_args.np, "#address-cells", &paddrc=
ells))
> +            imap +=3D 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.

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