Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200922023151.387447-1-warthog618@gmail.com> <20200922023151.387447-8-warthog618@gmail.com>
 <CAHp75VdQUbDnjQEr5X5q6WdU6rD=uBNznNn5=Vy=pvdwVj_hEA@mail.gmail.com> <20200924080921.GE17562@sol>
In-Reply-To: <20200924080921.GE17562@sol>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 25 Sep 2020 13:06:02 +0300
Message-ID: <CAHp75VehvUTt19sBxgPTZszUmxDGZwqGAV7bgW5jVM8Mf63UJA@mail.gmail.com>
Subject: Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL
 and GPIO_V2_LINE_GET_VALUES_IOCTL
To:     Kent Gibson <warthog618@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Sep 24, 2020 at 11:09 AM Kent Gibson <warthog618@gmail.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@gmail.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