linux-kernel - Re: [PATCH v2] gpio: consumer: new virtual driver

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: <ZNT546dM+7QX98pA@smile.fi.intel.com>
Date:   Thu, 10 Aug 2023 17:53:23 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Kent Gibson <warthog618@...il.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2] gpio: consumer: new virtual driver

On Wed, Aug 09, 2023 at 09:29:14AM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 8, 2023 at 8:11 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Tue, Aug 08, 2023 at 04:56:05PM +0200, Bartosz Golaszewski wrote:

...

> > But again, why not
> >
> >         timer_data->val ^= 1;
> 
> This is not ok in my book. If I need to think for more than a second
> about what it does, then it's worse. I put clarity over brevity.

Yes, this will require bit ops to check.

...

> > This can be avoided by
> >
> >         key = kstrndup(skip_spaces(page), count, GFP_KERNEL);
> >
> > no?
> >
> 
> No, because we also want to remove the trailing spaces and newlines.
> But if you have a different suggestion with existing helpers, let me
> know. I didn't find any.

kstrto*() are newline friendly. The rest as you noted can be covered with
sysfs_streq() / sysfs_match_string().

...

> > > +     ret = kstrtoint(page, 0, &offset);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* Use -1 to indicate lookup by name. */
> >
> > This comment is unclear as offset can be -1 given by the user.
> > What does above mean in that context?
> 
> I added this to the documentation. Negative number means: lookup by
> line name, positive or zero - lookup offset in chip.

Then add a link to the documentation from here? Because reading this comment
and the code is confusing.

> > > +     if (offset > (U16_MAX - 1))
> >
> > And how does it related to this -1 if related at all?
> 
> GPIOLIB interprets U16_MAX as "lookup by line name". So we can allow
> max (U16_MAX - 1). I will add a comment.
> 
> > > +             return -EINVAL;

...

> > > +     struct gpio_consumer_device *dev;
> > > +
> > > +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > > +     if (!dev)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     dev->id = ida_alloc(&gpio_consumer_ida, GFP_KERNEL);
> > > +     if (dev->id < 0) {
> > > +             kfree(dev);
> >
> > Wondering if you can utilize cleanup.h.
> 
> Whooaah! In february this year I suggested basic C RAII during my talk
> at fosdem and here we are? I missed this one. Yeah, I will use it!
> Even better, I will abuse the cr*p out of it in gpio-sim as well!
> Thanks for bringing this to my attention. This may be the best thing
> that happened to kernel C code in years if people widely adopt it.
> (This paragraph was written by a fan of GLib's autopointer paradigm.
> :) )

So let it be!

-- 
With Best Regards,
Andy Shevchenko