linux-kernel - Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

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: <CAMRc=MdtuDVnBMWHuTYUC7e65s8GsYrPnRv0zcmWGJwEQfmuSw@mail.gmail.com>
Date:   Fri, 12 Nov 2021 15:48:34 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core
 gpiolib code

On Thu, Nov 11, 2021 at 9:47 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Thu, Nov 11, 2021 at 10:26 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:
> >
> > Several drivers read the 'ngpios' device property on their own, but
> > since it's defined as a standard GPIO property in the device tree bindings
> > anyway, it's a good candidate for generalization. If the driver didn't
> > set its gc->ngpio, try to read the 'ngpios' property from the GPIO
> > device's firmware node before bailing out.
>
> Thanks!
>
> ...
>
> > +               ret = fwnode_property_read_u32(gdev->dev.fwnode, "ngpios",
> > +                                              &ngpios);
>
> I'm wondering if there is any obstacle to call
>
>                ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
>
> ?
>
> Rationale (the main one) is to avoid direct dereference of fwnode from
> struct device (it might be changed in the future). I really prefer API
> calls here.
>
> > +               if (ret == 0) {
> > +                       gc->ngpio = ngpios;
> > +               } else {
> > +                       chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > +                       ret = -EINVAL;
> > +                       goto err_free_descs;
> > +               }
>
> I would prefer the other way around and without 'else' being involved.
>
> >         }
>
>
> --
> With Best Regards,
> Andy Shevchenko

Will do. Just a note: some drivers that parse the ngpios property will
need to continue doing it themselves as they have additional
requirements from the value (limited max value, needs to be multiple
of 8, etc.) on the read value. For those that are obvious, we can
start converting them after this patch lands.

Bart