Received-SPF: pass (google.com: domain of linux-kernel+bounces-212095-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240527144054.155503-1-brgl@bgdev.pl> <ZleXc6tLbiWQ59i-@surfacebook.localdomain>
 <CAMRc=MftW0y7GicBy4vwABomUYuMndsJBUTdsQzZijDtgX1ohQ@mail.gmail.com> <ZmcUbe1aQfezZy5B@surfacebook.localdomain>
In-Reply-To: <ZmcUbe1aQfezZy5B@surfacebook.localdomain>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Wed, 12 Jun 2024 20:38:14 +0200
Message-ID: <CAMRc=McjAAFX1R4wAbLu5bcUkpPuy8rf=fwDOqCiUY1hbjmhMg@mail.gmail.com>
Subject: Re: [PATCH v7] gpio: virtuser: new virtual driver
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Kent Gibson <warthog618@gmail.com>, Linus Walleij <linus.walleij@linaro.org>, 
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 10, 2024 at 4:57=E2=80=AFPM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Mon, Jun 10, 2024 at 03:22:32PM +0200, Bartosz Golaszewski kirjoitti:
> > On Wed, May 29, 2024 at 11:00=E2=80=AFPM Andy Shevchenko
> > <andy.shevchenko@gmail.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 we=
re
> > > 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 t=
he
> 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 *, c=
har *);
> > > > +     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_attribut=
e.
>

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 *va=
lues,
> > > > +                                                 size_t num_values=
)
> > > > +{
> > > > +     ssize_t len =3D 0;
> > > > +     size_t i;
> > > > +
> > > > +     for (i =3D 0; i < num_values; i++)
> > > > +             len +=3D 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 th=
at
> 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 *value=
s)
> > > > +{
> > > > +     size_t i;
> > > > +
> > > > +     for (i =3D 0; i < len; i++) {
> > >
> > > Perhaps
> > >
> > >                 bool val;
> > >                 int ret;
> > >
> > >                 ret =3D kstrtobool(...);
> >
> > kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]als=
e.
>
> Yes, see below.
>
> > >                 if (ret)
> > >                         return ret;
> > >
> > >                 assign_bit(...); // btw, why atomic?
> > >
> > > > +             if (buf[i] =3D=3D '0')
> > > > +                     clear_bit(i, values);
> > > > +             else if (buf[i] =3D=3D '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 t=
he
> knowledge they familiar with (and I believe the group of the users who kn=
ow
> about bitmaps is much bigger than those who will use this driver).
>
> > > > +     return 0;
> > > > +}
>
> ...
>
> > > > +     return sysfs_emit(buf, "%s\n",
> > > > +                       dir =3D=3D GPIO_LINE_DIRECTION_IN ? "input"=
 : "output");
> > >
> > > I think this maybe transformed to something like str_input_output() i=
n
> > > string_choices.h (and you don't even need to include that as it's imp=
lied 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 =3D GPIO_LINE_DIRECTION_IN;
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     if (sysfs_streq(buf, "output-high"))
> > > > +             *val =3D 1;
> > > > +     else if (sysfs_streq(buf, "output-low"))
> > > > +             *val =3D 0;
> > > > +     else
> > > > +             return -EINVAL;
> > > > +
> > > > +     *dir =3D GPIO_LINE_DIRECTION_OUT;
> > >
> > > This can be transformed to use sysfs_match_string() with
> > >
> > > static const char * const dirs[] =3D { "output-low", "output-high", "=
input" };
> > >
> > >         int ret;
> > >
> > >         ret =3D sysfs_match_string(...);
> > >         if (ret < 0)
> > >                 return ret;
> > >
> > >         *val =3D ret;
> > >         *dir =3D ret =3D=3D 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DI=
RECTION_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 on=
e.

> > >         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 =3D sysfs_match_string(gpio_virtuser_sysfs_value_string=
s, buf);
> > > > +     if (value < 0) {
> > > > +             /* Can be 0 or 1 too. */
> > > > +             ret =3D kstrtoint(buf, 0, &value);
> > > > +             if (ret)
> > > > +                     return ret;
> > >
> > > > +             if (value !=3D 0 && value !=3D 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 =3D 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 some=
thing
> 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") =3D=3D 0;
> > >
> > > Can't we reuse the suffix from the array from the gpiolib internal he=
ader?
> > > Also I don't like the form of '-' in the line. "gpios" is good and ch=
ance
> > > that linker deduplicates the same string if it occurs somewhere else =
in the
> > > binary (in case this goes with =3Dy 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 addi=
tional string
> > > > + * array property - "gpio-virtuser,ids" - containing the list of a=
ll 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 unificati=
on.
>
> > > > + */
>
> ...
>
> > > > +                     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 =3D strpbrk(prop->name, "-");
>
> Btw, don't you want strrchr() here? (Note 'r' variant).

Already changed.

>
> > > > +                     diff =3D dash - prop->name;
> > > > +
> > > > +                     tmp =3D devm_kmemdup(dev, prop->name, diff + =
1,
> > > > +                                        GFP_KERNEL);
> > >
> > > devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at le=
ast 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] =3D '\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 =3D 0;
> > >
> > > Why signed? And in all this kind of case, I would split assignment...
>
> (1)
>
> > > > +     memset(properties, 0, sizeof(properties));
> > > > +
> > > > +     num_ids =3D list_count_nodes(&dev->lookup_list);
> > > > +     char **ids __free(kfree) =3D 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 =3D 0;
>
> instead of the above (1).
>

Why? I see no good reason.

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

Bart