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

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 13, 2024 at 01:22:58PM +0200, Bartosz Golaszewski wrote:
> 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?

Yes.

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

but it's gpio-specific, please put it there.

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

Yes it does, you just have to create the tree yourself.  Many subsystems
do this today just fine.

Please do not abuse sysfs for something that it is not designed for,
please use debugfs, that is EXACTLY what it is designed for.

thanks,

greg k-h