Message-ID: <CAMRc=McjAAFX1R4wAbLu5bcUkpPuy8rf=fwDOqCiUY1hbjmhMg@mail.gmail.com>
Date: Wed, 12 Jun 2024 20:38:14 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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 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:
>
> ...
>
> > > > User must pass exactly the number of values that the array contains
> > >
> > > Can't we assume non-active values for the rest if less than needed were
> > > provided? For more than that, why do we care?
> >
> > Honestly, what good would it do? It would just be more confusing IMO.
>
> Let's say you can leave documentation as is, but relax the code. That's the
> benefit, less complex checks in the code.
>

Actually this is more ambiguity in the code. I'm against it.

> ...
>
> > > > +#include <linux/atomic.h>
> > > > +#include <linux/bitmap.h>
> > > > +#include <linux/cleanup.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/configfs.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/gpio/driver.h>
> > > > +#include <linux/gpio/machine.h>
> > >
> > > > +#include <linux/idr.h>
> > >
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/irq_work.h>
> > >
> > > > +#include <linux/kernel.h>
> > >
> > > Do you need this?
> >
> > ARRAY_SIZE() used to live here when I first wrote this but it was
> > since moved. I'll drop this.
>
> Rather replace with array_size.h.
>

Yeah this is what I did.

> > > > +#include <linux/limits.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/lockdep.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/notifier.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/overflow.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/printk.h>
> > > > +#include <linux/property.h>
> > > > +#include <linux/slab.h>
> > >
> > > > +#include <linux/string.h>
> > >
> > > Implied by string_helpers.h
> >
> > Yeah, but we still use symbols directly from string.h, we shouldn't
> > depend on implicit includes.
>
> string_helpers.h is and will continue guranteening inclusion of string.h.
> It's the same as we drop bits.h when we include, for instance, bitmap.h.
>

Whatever, ok.

> > > > +#include <linux/string_helpers.h>
> > > > +#include <linux/sysfs.h>
> > > > +#include <linux/types.h>
>
> ...
>
> > > > +struct gpio_virtuser_attr_descr {
> > > > +     const char *name;
> > > > +     ssize_t (*show)(struct device *, struct device_attribute *, char *);
> > > > +     ssize_t (*store)(struct device *, struct device_attribute *,
> > > > +                      const char *, size_t);
> > > > +};
> > >
> > > struct device_attribute ? (Yes, I know that that one is a bit bigger but
> > > benefit is that we have some code that you may reuse)
> >
> > Not sure what you mean here, these are callbacks for sysfs.
>
> I mean to replace your custom data type with the existing device_attribute.
>

Doesn't make sense here. struct device_attribute has lots of cruft we
don't need here. It's just the name and the callback pointers.

> ...
>
> > > > +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.

>
> > > > +}
>
> ...
>
> > > > +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 familiar with (and I believe the group of the users who know
> about bitmaps is much bigger than those who will use this driver).
>
> > > > +     return 0;
> > > > +}
>
> ...
>
> > > > +     return sysfs_emit(buf, "%s\n",
> > > > +                       dir == GPIO_LINE_DIRECTION_IN ? "input" : "output");
> > >
> > > I think this maybe transformed to something like str_input_output() in
> > > string_choices.h (and you don't even need to include that as it's implied by
> > > string_helpers.h)
> >
> > These helpers take bool as argument. Hard to tell whether input or
> > output should correspond to true. I'd leave it as is.
>
> There is a convention: str_TRUE_FALSE().
>
> ...
>
> > > > +static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
> > > > +{
> > > > +     if (sysfs_streq(buf, "input")) {
> > > > +             *dir = GPIO_LINE_DIRECTION_IN;
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     if (sysfs_streq(buf, "output-high"))
> > > > +             *val = 1;
> > > > +     else if (sysfs_streq(buf, "output-low"))
> > > > +             *val = 0;
> > > > +     else
> > > > +             return -EINVAL;
> > > > +
> > > > +     *dir = GPIO_LINE_DIRECTION_OUT;
> > >
> > > This can be transformed to use sysfs_match_string() with
> > >
> > > static const char * const dirs[] = { "output-low", "output-high", "input" };
> > >
> > >         int ret;
> > >
> > >         ret = sysfs_match_string(...);
> > >         if (ret < 0)
> > >                 return ret;
> > >
> > >         *val = ret;
> > >         *dir = ret == 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> > >
> > > And with this approach it even not clear why do you need dir and val to be
> > > separated here (esp. if we add a enum like
> >
> > We do want them to be separated not for better UX but to be able to
> > test all kernel APIs (gpiod_direction_input|output() and
> > gpiod_set_value()).
>
> Still you can do some optimisations I proposed above.
>

IMO they are much less readable and you don't gain anything. NAK on this one.

> > >         GPIO_VIRTUSER_OUT_LOW,
> > >         GPIO_VIRTUSER_OUT_HIGH,
> > >         GPIO_VIRTUSER_IN,
> > >
> > > (with it the string array can also be indexed).
> > >
> > > > +     return 0;
> > > > +}
>
> ...
>
> > > > +static int gpio_virtuser_parse_value(const char *buf)
> > > > +{
> > > > +     int value, ret;
> > > > +
> > > > +     value = sysfs_match_string(gpio_virtuser_sysfs_value_strings, buf);
> > > > +     if (value < 0) {
> > > > +             /* Can be 0 or 1 too. */
> > > > +             ret = kstrtoint(buf, 0, &value);
> > > > +             if (ret)
> > > > +                     return ret;
> > >
> > > > +             if (value != 0 && value != 1)
> > > > +                     return -EINVAL;
> > >
> > > Why not kstrtobool()?
> >
> > I don't want to accept all the other strings kstrtobool() is fine with.
>
> What's wrong with other strings?
>

A line is False? I mean sure, you can map that to inactive but it's
not a naming convention associated with GPIOs very often.

> At bare minumum you can reduce the range by using kstrtou8().
>

As opposed to just checking '0' and '1'? Meh...

> > > > +     }
> > > > +
> > > > +     return value;
> > > > +}
>
> ...
>
> > > > +     ret = kstrtouint(buf, 10, &debounce);
> > >
> > > Why restrict to decimal?
> >
> > Not sure what you gain from passing a period in hex?
>
> For example, if I compare this to the real HW, I might be able to do something
> like 0x1234 (let's say it's debounce step) and shifting it by 4 bits will give
> me something I want. But despite that quite unlikely case the restriction here
> doesn't bring us much.
>

Ok, whatever.

> > > > +     if (ret)
> > > > +             return ret;
>
> ...
>
> > > > +     return dash && strcmp(dash, "-gpios") == 0;
> > >
> > > Can't we reuse the suffix from the array from the gpiolib internal header?
> > > Also I don't like the form of '-' in the line. "gpios" is good and chance
> > > that linker deduplicates the same string if it occurs somewhere else in the
> > > binary (in case this goes with =y in .config).
> >
> > I'm not sure I follow what you're saying here. Please rephrase.
>
> Do strcmp() against one from the gpio_suffixes array.
>

I don't want to accept the "gpio" suffix. Also: I don't want to
include gpiolib.h.

> ...
>
> > > > +/*
> > > > + * If this is an OF-based system, then we iterate over properties and consider
> > > > + * all whose names end in "-gpios". For configfs we expect an additional string
> > > > + * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
> > > > + * to request.
> > >
> > > Why not any other system? What's wrong for having this available for ACPI, for
> > > example? Okay, I see that this is probably due to absence of API.
> > >
> > > OTOH the last call in the function assumes non-OF cases. Why can't we have the
> > > same approach in both?
> >
> > Again: I have no idea what you mean. We support device-tree and
> > configfs as sources of configuration for these virtual consumers. If
> > you want to add something more, be my guest once it's upstream.
> >
> > The reason to use a different approach is to not require the
> > "gpio-virtuser,ids" property in device-tree.
>
> Yes, and I'm asking why can't we unify and require it there as well?
> But okay, I might give up on the trying of the DT/ACPI property unification.
>
> > > > + */
>
> ...
>
> > > > +                     if (gpio_virtuser_prop_is_gpio(prop))
> > > > +                             ++ret;
> > >
> > > Why pre-increment?
> >
> > Why not?
>
> Because we have a pattern. Pre-increment adds into additional questioning
> "why?". I.e. What does make this case special? When I read such a code I need
> more brain power to parse it.
>

Ok whatever.

> ...
>
> > > > +                     dash = strpbrk(prop->name, "-");
>
> Btw, don't you want strrchr() here? (Note 'r' variant).

Already changed.

>
> > > > +                     diff = dash - prop->name;
> > > > +
> > > > +                     tmp = devm_kmemdup(dev, prop->name, diff + 1,
> > > > +                                        GFP_KERNEL);
> > >
> > > devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at least I
> > > would rather expect wrapped kstrndup() than this.
> >
> > Meh, this logic is fine as we know the range exactly. IMO kstrndup()
> > here would be overkill. I'd leave it for now.
> >
> > > > +                     if (!tmp)
> > > > +                             return -ENOMEM;
>
> > > > +                     tmp[diff] = '\0';
>
> This line will gone with kstrndup(). I think we will benefit from it.
>

I'll allow myself to keep my version here.

> ...
>
> > > > +     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.

> > > > +     list_for_each_entry(lookup, &dev->lookup_list, siblings)
> > > > +             ids[i++] = lookup->con_id;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Bart

Powered by blists - more mailing lists