linux-kernel - Re: [PATCH v7] gpio: virtuser: 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] [day] [month] [year] [list]
Date: Thu, 13 Jun 2024 20:03:21 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kent Gibson <warthog618@...il.com>, Linus Walleij <linus.walleij@...aro.org>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v7] gpio: virtuser: new virtual driver

On Wed, Jun 12, 2024 at 8:38 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:
> On Mon, Jun 10, 2024 at 4:57 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> > Mon, Jun 10, 2024 at 03:22:32PM +0200, Bartosz Golaszewski kirjoitti:
> > > On Wed, May 29, 2024 at 11:00 PM Andy Shevchenko
> > > <andy.shevchenko@...il.com> wrote:
> > > > Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:

...

> > > > > +static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
> > > > > +                                                 unsigned long *values,
> > > > > +                                                 size_t num_values)
> > > > > +{
> > > > > +     ssize_t len = 0;
> > > > > +     size_t i;
> > > > > +
> > > > > +     for (i = 0; i < num_values; i++)
> > > > > +             len += sysfs_emit_at(buf, len, "%d",
> > > > > +                                  test_bit(i, values) ? 1 : 0);
> > > > > +     return len + sysfs_emit_at(buf, len, "\n");
> > > >
> > > > Why not use %pb?
> > >
> > > Because it outputs hex? I want to output binary, can I do it?
> >
> > But why do you need that? You can also print a list of numbers of bits that
> > set (%pbl).
> >
> > We have a few ABIs in the kernel that works nice and people are familiar with
> > (CPU sets, IRQ affinity masks, etc). Why to reinvent the wheel?
>
> If I see "11001011" as output, I can immediately convert that to pins
> in my head. If I see 0xcb, I need to use a calculator.

>From my experience I see the opposite, as the hex values are
condensed, it will be easy to get lost in binary > 8 bits. Despite
that, I am really against Yet Another Parsers in the kernel. Here I do
not see a good justification for one more. As I pointed out the kernel
has very known bitmap ABIs that users are familiar with. Are you
creating a driver for yourself or for a wider audience?

> > > > > +}

...

> > > > > +static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
> > > > > +                                              unsigned long *values)
> > > > > +{
> > > > > +     size_t i;
> > > > > +
> > > > > +     for (i = 0; i < len; i++) {
> > > >
> > > > Perhaps
> > > >
> > > >                 bool val;
> > > >                 int ret;
> > > >
> > > >                 ret = kstrtobool(...);
> > >
> > > kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.
> >
> > Yes, see below.
> >
> > > >                 if (ret)
> > > >                         return ret;
> > > >
> > > >                 assign_bit(...); // btw, why atomic?
> > > >
> > > > > +             if (buf[i] == '0')
> > > > > +                     clear_bit(i, values);
> > > > > +             else if (buf[i] == '1')
> > > > > +                     set_bit(i, values);
> > > > > +             else
> > > > > +                     return -EINVAL;
> > > >
> > > > > +     }
> > > >
> > > > BUT, why not bitmap_parse()?
> > >
> > > Because it parses hex, not binary.
> >
> > So, why do we reinvent a wheel? Wouldn't be better that users may apply the
> > knowledge they are familiar with (and I believe the group of the users who know
> > about bitmaps is much bigger than those who will use this driver).

You see, you ignored this comment.

> > > > > +     return 0;
> > > > > +}

...

> > At bare minumum you can reduce the range by using kstrtou8().
>
> As opposed to just checking '0' and '1'? Meh...

Okay, can you explain why you need to take bigger numbers when it's
going to be used only in a very reduced range? Esp. negative ones.
Whatever, your choice.

...

> > > > > +     int i = 0;
> > > >
> > > > Why signed? And in all this kind of case, I would split assignment...
> >
> > (1)
> >
> > > > > +     memset(properties, 0, sizeof(properties));
> > > > > +
> > > > > +     num_ids = list_count_nodes(&dev->lookup_list);
> > > > > +     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),
> > > > > +                                        GFP_KERNEL);
> > > > > +     if (!ids)
> > > > > +             return ERR_PTR(-ENOMEM);
> > > > > +
> > > >
> > > > To be here, that the reader will see immediately (close enough) what is the
> > > > initial values. Moreover this code will be robuse against changes in between
> > > > (if i become reusable).
> > >
> > > Sorry, I can't parse it.
> >
> > I meant to see here
> >
> >         i = 0;
> >
> > instead of the above (1).
>
> Why? I see no good reason.

There are two:
1) readability as explained above;
2) maintenance.

The second one is good in case the same variable (that's the very
often case for such as "i" is going to be reused in the future by some
new code. But okay, in this case you probably will be the only one for
maintenance.

> > > > > +     list_for_each_entry(lookup, &dev->lookup_list, siblings)
> > > > > +             ids[i++] = lookup->con_id;


-- 
With Best Regards,
Andy Shevchenko