linux-kernel - Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES

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: <CAHp75VehvUTt19sBxgPTZszUmxDGZwqGAV7bgW5jVM8Mf63UJA@mail.gmail.com>
Date:   Fri, 25 Sep 2020 13:06:02 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Kent Gibson <warthog618@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL
 and GPIO_V2_LINE_GET_VALUES_IOCTL

On Thu, Sep 24, 2020 at 11:09 AM Kent Gibson <warthog618@...il.com> wrote:
> On Wed, Sep 23, 2020 at 02:11:54PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@...il.com> wrote:

...

> > > +       assign_bit(FLAG_ACTIVE_LOW, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
> >
> > What I meant is to attach also this to the other assign_bit():s below.
> > And just in case a question: why not __asign_bit() do we really need atomicity?
> >
>
> These are initialized as per their order in the flags so it is easier to
> tell if any are missing.
>
> The atomicity is not required here, but it is elsewhere so you are
> oblidged to use it for all accesses, no?

I'm not sure. I think if you are using non-atomic in one place, it
means that all automatically drop the atomicity guarantee. So, it's
all or none for atomicity, for non-atomicity it's rather none or at
least one. That said, code should be carefully checked before doing
such.

> > > +       if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
> > > +               set_bit(FLAG_IS_OUT, flagsp);
> > > +       else if (flags & GPIO_V2_LINE_FLAG_INPUT)
> > > +               clear_bit(FLAG_IS_OUT, flagsp);
> > > +
> > > +       assign_bit(FLAG_OPEN_DRAIN, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
> > > +       assign_bit(FLAG_OPEN_SOURCE, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
> > > +       assign_bit(FLAG_PULL_UP, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
> > > +       assign_bit(FLAG_PULL_DOWN, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
> > > +       assign_bit(FLAG_BIAS_DISABLE, flagsp,
> > > +                  flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);

...

> > > +       /* Make sure this is terminated */
> > > +       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
> > > +       if (strlen(ulr.consumer)) {
> > > +               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
> > > +               if (!lr->label) {
> > > +                       ret = -ENOMEM;
> > > +                       goto out_free_linereq;
> > > +               }
> > > +       }
> >
> > Still don't get why we can\t use kstrndup() here...
> >
>
> I know ;-).
>
> Another one directly from v1, and the behaviour there is to leave
> lr->label nulled if consumer is empty.
> It just avoids a pointless malloc for the null terminator.

Again, similar as for bitmap API usage, if it makes code cleaner and
increases readability, I will go for it.
Also don't forget the army of janitors that won't understand the case
and simply convert everything that can be converted.

-- 
With Best Regards,
Andy Shevchenko