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

From: Andy Shevchenko
Date: Fri Sep 25 2020 - 06:06:22 EST


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