linux-kernel - Re: [PATCH v2] of/irq: Support #msi-cells=<0> in of_msi_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240817-c1d2345fcc50ade60ce09449@orel>
Date: Sat, 17 Aug 2024 09:42:01 +0200
From: Andrew Jones <ajones@...tanamicro.com>
To: Rob Herring <robh@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, loongarch@...ts.linux.dev, 
	linux-mips@...r.kernel.org, maz@...nel.org, mark.rutland@....com, saravanak@...gle.com, 
	paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	Anup Patel <apatel@...tanamicro.com>
Subject: Re: [PATCH v2] of/irq: Support #msi-cells=<0> in of_msi_get_domain

On Fri, Aug 16, 2024 at 02:18:41PM GMT, Rob Herring wrote:
> On Fri, Aug 16, 2024 at 6:50 AM Andrew Jones <ajones@...tanamicro.com> wrote:
> >
> > An 'msi-parent' property with a single entry and no accompanying
> > '#msi-cells' property is considered the legacy definition as opposed
> > to its definition after being expanded with commit 126b16e2ad98
> > ("Docs: dt: add generic MSI bindings"). However, the legacy
> > definition is completely compatible with the current definition and,
> > since of_phandle_iterator_next() tolerates missing and present-but-
> > zero *cells properties since commit e42ee61017f5 ("of: Let
> > of_for_each_phandle fallback to non-negative cell_count"), there's no
> > need anymore to special case the legacy definition in
> > of_msi_get_domain().
> >
> > Indeed, special casing has turned out to be harmful, because, as of
> > commit 7c025238b47a ("dt-bindings: irqchip: Describe the IMX MU block
> > as a MSI controller"), MSI controller DT bindings have started
> > specifying '#msi-cells' as a required property (even when the value
> > must be zero) as an effort to make the bindings more explicit. But,
> > since the special casing of 'msi-parent' only uses the existence of
> > '#msi-cells' for its heuristic, and not whether or not it's also
> > nonzero, the legacy path is not taken. Furthermore, the path to
> > support the new, broader definition isn't taken either since that
> > path has been restricted to the platform-msi bus.
> >
> > But, neither the definition of 'msi-parent' nor the definition of
> > '#msi-cells' is platform-msi-specific (the platform-msi bus was just
> > the first bus that needed '#msi-cells'), so remove both the special
> > casing and the restriction. The code removal also requires changing
> > to of_parse_phandle_with_optional_args() in order to ensure the
> > legacy (but compatible) use of 'msi-parent' remains supported. This
> > not only simplifies the code but also resolves an issue with PCI
> > devices finding their MSI controllers on riscv, as the riscv,imsics
> > binding requires '#msi-cells=<0>'.
> >
> > Signed-off-by: Andrew Jones <ajones@...tanamicro.com>
> > ---
> > v2:
> >  - switch to of_parse_phandle_with_optional_args() to ensure the
> >    absence of #msi-cells means count=0
> >
> >  drivers/of/irq.c | 37 +++++++++++--------------------------
> >  1 file changed, 11 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index c94203ce65bb..690df4b71ab9 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -709,8 +709,7 @@ struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> >   * @np: device node for @dev
> >   * @token: bus type for this domain
> >   *
> > - * Parse the msi-parent property (both the simple and the complex
> > - * versions), and returns the corresponding MSI domain.
> > + * Parse the msi-parent property and returns the corresponding MSI domain.
> >   *
> >   * Returns: the MSI domain for this device (or NULL on failure).
> >   */
> > @@ -718,33 +717,19 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
> >                                      struct device_node *np,
> >                                      enum irq_domain_bus_token token)
> >  {
> > -       struct device_node *msi_np;
> > +       struct of_phandle_args args;
> >         struct irq_domain *d;
> > +       int index = 0;
> >
> > -       /* Check for a single msi-parent property */
> > -       msi_np = of_parse_phandle(np, "msi-parent", 0);
> > -       if (msi_np && !of_property_read_bool(msi_np, "#msi-cells")) {
> > -               d = irq_find_matching_host(msi_np, token);
> > -               if (!d)
> > -                       of_node_put(msi_np);
> > -               return d;
> > -       }
> > -
> > -       if (token == DOMAIN_BUS_PLATFORM_MSI) {
> > -               /* Check for the complex msi-parent version */
> > -               struct of_phandle_args args;
> > -               int index = 0;
> > +       while (!of_parse_phandle_with_optional_args(np, "msi-parent",
> > +                                                   "#msi-cells",
> > +                                                   index, &args)) {
> 
> I guess you just maintained essentially what was here, but
> of_for_each_phandle() should work here. It's a bit more efficient too
> because of_parse_phandle_with_optional_args() is implemented using
> of_for_each_phandle().

Thanks, Rob. I just sent v3.

drew