Received-SPF: pass (google.com: domain of linux-kernel+bounces-213087-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240613092830.15761-1-brgl@bgdev.pl> <20240613092830.15761-3-brgl@bgdev.pl>
 <2024061356-uptake-ideology-e57b@gregkh>
In-Reply-To: <2024061356-uptake-ideology-e57b@gregkh>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Thu, 13 Jun 2024 13:22:58 +0200
Message-ID: <CAMRc=MfjQdFR_8ALGibxQnr5tzoHykCBpkBxjH78c5HuD43rBg@mail.gmail.com>
Subject: Re: [PATCH v8 2/2] misc: gpio-virtuser: new virtual testing driver
 for the GPIO API
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Walleij <linus.walleij@linaro.org>, Jonathan Corbet <corbet@lwn.net>, 
	"Rafael J . Wysocki" <rafael@kernel.org>, Arnd Bergmann <arnd@arndb.de>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, linux-gpio@vger.kernel.org, 
	linux-doc@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 Thu, Jun 13, 2024 at 12:02=E2=80=AFPM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 13, 2024 at 11:28:30AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The GPIO subsystem used to have a serious problem with undefined behavi=
or
> > 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 user=
s.
> > We have the means of testing it from user-space via the character devic=
e
> > 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@linaro.org>
> > ---
> >  .../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 state=
s
> > +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 cons=
umer
> > +       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