linux-kernel - Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqLfq6U3OORsKE962f2RZ7kQXLNEb2ShzrWqGY+=oVsf=g@mail.gmail.com>
Date:   Fri, 3 Feb 2023 09:06:17 -0600
From:   Rob Herring <robh@...nel.org>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
<hayashi.kunihiko@...ionext.com> wrote:
>
> Hi Rob,
>
> On 2023/02/02 7:00, Rob Herring wrote:
> > "ranges" is a standard property and we have common helper functions for
> > parsing it, so let's use them.
> >
> > Signed-off-by: Rob Herring <robh@...nel.org>
> > ---
> > Compile tested only!
>
> Please fix the driver's name.
>
> s/unifier-system-bus/uniphier-system-bus/

doh!

>
> > ---
> >   drivers/bus/uniphier-system-bus.c | 54 +++++++------------------------
> >   1 file changed, 11 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/bus/uniphier-system-bus.c
> > b/drivers/bus/uniphier-system-bus.c
> > index f70dedace20b..cb5c89ce7b86 100644
> > --- a/drivers/bus/uniphier-system-bus.c
> > +++ b/drivers/bus/uniphier-system-bus.c
> > @@ -176,10 +176,9 @@ static int uniphier_system_bus_probe(struct
> > platform_device *pdev)
> >   {
> >       struct device *dev = &pdev->dev;
> >       struct uniphier_system_bus_priv *priv;
> > -     const __be32 *ranges;
> > -     u32 cells, addr, size;
> > -     u64 paddr;
> > -     int pna, bank, rlen, rone, ret;
> > +     struct of_range_parser parser;
> > +     struct of_range range;
> > +     int ret;
> >
> >       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -191,48 +190,17 @@ static int uniphier_system_bus_probe(struct
> > platform_device *pdev)
> >
> >       priv->dev = dev;
> >
> > -     pna = of_n_addr_cells(dev->of_node);
> > -
> > -     ret = of_property_read_u32(dev->of_node, "#address-cells",
> > &cells);
> > -     if (ret) {
> > -             dev_err(dev, "failed to get #address-cells\n");
> > -             return ret;
> > -     }
> > -     if (cells != 2) {
> > -             dev_err(dev, "#address-cells must be 2\n");
> > -             return -EINVAL;
> > -     }
>
> Don't you need to check the value of "#address-cells"?

Doesn't your schema do that?

It's not the kernel's job to validate the DT. If it is, then it does a
terrible job.

> > -
> > -     ret = of_property_read_u32(dev->of_node, "#size-cells", &cells);
> > -     if (ret) {
> > -             dev_err(dev, "failed to get #size-cells\n");
> > +     ret = of_range_parser_init(&parser, dev->of_node);
> > +     if (ret)
> >               return ret;
> > -     }
> > -     if (cells != 1) {
> > -             dev_err(dev, "#size-cells must be 1\n");
> > -             return -EINVAL;
> > -     }
>
> Same as "#size-cells"

While the address clearly needs cells to hold the chip select, there's
no reason to restrict the size cells.

>
> I confirmed the value of all the arguments of uniphier_system_bus_add_bank()
> match the previous ones.
>
> Tested-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>

Thanks.

Rob