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

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

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

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