Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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