netdev - Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array

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]

Date:   Sun, 02 Sep 2018 12:19:11 +0200
From:   Janusz Krzysztofik <jmkrzyszt@...il.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Peter Korsgaard <peter.korsgaard@...co.com>,
        Peter Rosin <peda@...ntia.se>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Jiri Slaby <jslaby@...e.com>, Willy Tarreau <w@....eu>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        linux-i2c@...r.kernel.org, linux-mmc@...r.kernel.org,
        Network Development <netdev@...r.kernel.org>,
        linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-serial@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Janusz Krzysztofik <jmkrzyszt@...il.com>
Subject: Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array

Hi Miguel,

On Thursday, August 30, 2018 1:10:59 PM CEST Miguel Ojeda wrote:
> Hi Janusz,
> 
> On Wed, Aug 29, 2018 at 10:48 PM, Janusz Krzysztofik
> <jmkrzyszt@...il.com> wrote:
> > ...
> >         /* High nibble + RS, RW */
> > -       for (i = 4; i < 8; i++)
> > -               values[PIN_DATA0 + i] = !!(val & BIT(i));
> > -       values[PIN_CTRL_RS] = rs;
> > +       value_bitmap[0] = val;
> > +       __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> >         n = 5;
> >         if (hd->pins[PIN_CTRL_RW]) {
> > -               values[PIN_CTRL_RW] = 0;
> > +               __clear_bit(PIN_CTRL_RW, value_bitmap);
> >                 n++;
> >         }
> > +       value_bitmap[0] >>= PIN_DATA4;
> >
> >         /* Present the data to the port */
> > -       gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4],
> > -                                      &values[PIN_DATA4]);
> > +       gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4], 
value_bitmap);
> >
> >         hd44780_strobe_gpio(hd);
> >
> >         /* Low nibble */
> > -       for (i = 0; i < 4; i++)
> > -               values[PIN_DATA4 + i] = !!(val & BIT(i));
> > +       value_bitmap[0] &= ~((1 << PIN_DATA4) - 1);
> > +       value_bitmap[0] |= val & ~((1 << PIN_DATA4) - 1);
> 
> This is still wrong! What I originally meant in my v4 review is that
> there is an extra ~ in the second line.

Indeed, that's wrong, I missed your original point, sorry.

> Also, a couple of general comments:
> 
>  - Please review the list of CCs (I was not CC'd originally, so maybe
> there are other maintainers that aren't, either)

That's probably because early versions of the series, prior to v4, were not 
touching existing GPIO API so there were no changes to users of gpiod_get/
set_array_value() and their variants. From v4 on, you are in the loop so don't 
worry, you haven't missed anything.
But anyway, thanks for your suggestion to review the Cc; list, I've done that 
for v7 and added still a few people who contributed most to the code being 
changed.

>  - In general, the new code seems harder to read than the original one
> (but that is my impression).

I hope we are slowly approaching acceptable readability in recent iterations.

Thanks,
Janusz