Re: [PATCH v8 2/2] misc: gpio-virtuser: new virtual testing driver for the GPIO API

[Bartosz Golaszewski]

On Thu, Jun 13, 2024 at 12:02 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jun 13, 2024 at 11:28:30AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > The GPIO subsystem used to have a serious problem with undefined behavior
> > and use-after-free bugs on hot-unplug of GPIO chips. This can be
> > considered a corner-case by some as most GPIO controllers are enabled
> > early in the boot process and live until the system goes down but most
> > GPIO drivers do allow unbind over sysfs, many are loadable modules that
> > can be (force) unloaded and there are also GPIO devices that can be
> > dynamically detached, for instance CP2112 which is a USB GPIO expender.
> >
> > Bugs can be triggered both from user-space as well as by in-kernel users.
> > We have the means of testing it from user-space via the character device
> > but the issues manifest themselves differently in the kernel.
> >
> > This is a proposition of adding a new virtual driver - a configurable
> > GPIO consumer that can be configured over configfs (similarly to
> > gpio-sim) or described on the device-tree.
> >
> > This driver is aimed as a helper in spotting any regressions in
> > hot-unplug handling in GPIOLIB.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> >  .../admin-guide/gpio/gpio-virtuser.rst        |  176 ++
> >  Documentation/admin-guide/gpio/index.rst      |    1 +
>
> sysfs documentation needs to go in Documentation/ABI/ not in a random
> .rst file where the tools that check this will not catch it.
>

This is a testing driver, not representing real hardware. Do we hold
such modules to the same standard?

> >  MAINTAINERS                                   |    8 +
> >  drivers/misc/Kconfig                          |    8 +
> >  drivers/misc/Makefile                         |    1 +
> >  drivers/misc/gpio-virtuser.c                  | 1790 +++++++++++++++++
>
> Why not put this in drivers/gpio/?  Why misc?
>

Because it's quite... well "misc". It's not a GPIO chip provider
(drivers/gpio/ is for GPIO providers), it's only a GPIO consumer. It
also has an interface that doesn't fit any particular subsystem.

> > +Both attributes allow to read and set arrays of GPIO values. User must pass
> > +exactly the number of values that the array contains in the form of a string
> > +containing zeroes and ones representing inactive and active GPIO states
> > +respectively. In this example: ``echo 11 > values``.
>
> sysfs is "one value per file", so why are there multiple values here?
>
> If you want to just use this for testing, and want to put whatever you
> want in the files, just use debugfs, that's what it is there for, not
> sysfs.
>

Debugfs doesn't allow me to attach attributes to a particular device
which is what I want here.

Bart

> > +config GPIO_VIRTUSER
> > +     tristate "GPIO Virtual User Testing Module"
> > +     select CONFIGFS_FS
> > +     select IRQ_WORK
> > +     help
> > +       This enables the configurable, configfs-based virtual GPIO consumer
> > +       testing driver.
> > +
>
> module name?
>
> And you need more documentation here, I have no idea what this means
> when it shows up in a Kconfig help entry :(
>
> thanks,
>
> greg k-h