mbox series

[v2,00/12] gpio: implement the configfs testing module

Message ID 20210304102452.21726-1-brgl@bgdev.pl
Headers show
Series gpio: implement the configfs testing module | expand

Message

Bartosz Golaszewski March 4, 2021, 10:24 a.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This series adds a new GPIO testing module based on configfs committable items
and sysfs. The goal is to provide a testing driver that will be configurable
at runtime (won't need module reload) and easily extensible. The control over
the attributes is also much more fine-grained than in gpio-mockup.

This series also contains a respin of the patches I sent separately to the
configfs maintainers - these patches implement the concept of committable
items that was well defined for a long time but never actually completed.

Apart from the new driver itself, its selftests and the configfs patches, this
series contains some changes to the bitmap API - most importantly: it adds
devres managed variants of bitmap_alloc() and bitmap_zalloc().

v1 -> v2:
- add selftests for gpio-sim
- add helper programs for selftests
- update the configfs rename callback to work with the new API introduced in
  v5.11
- fix a missing quote in the documentation
- use !! whenever using bits operation that are required to return 0 or 1
- use provided bitmap API instead of reimplementing copy or fill operations
- fix a deadlock in gpio_sim_direction_output()
- add new read-only configfs attributes for mapping of configfs items to GPIO
  device names
- and address other minor issues pointed out in reviews of v1

Bartosz Golaszewski (12):
  configfs: increase the item name length
  configfs: use (1UL << bit) for internal flags
  configfs: implement committable items
  samples: configfs: add a committable group
  lib: bitmap: remove the 'extern' keyword from function declarations
  lib: bitmap: order includes alphabetically
  lib: bitmap: provide devm_bitmap_alloc() and devm_bitmap_zalloc()
  drivers: export device_is_bound()
  gpio: sim: new testing module
  selftests: gpio: provide a helper for reading chip info
  selftests: gpio: add a helper for reading GPIO line names
  selftests: gpio: add test cases for gpio-sim

 Documentation/admin-guide/gpio/gpio-sim.rst   |  72 ++
 Documentation/filesystems/configfs.rst        |   6 +-
 drivers/base/dd.c                             |   1 +
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-sim.c                       | 878 ++++++++++++++++++
 fs/configfs/configfs_internal.h               |  22 +-
 fs/configfs/dir.c                             | 245 ++++-
 include/linux/bitmap.h                        | 129 +--
 include/linux/configfs.h                      |   3 +-
 lib/bitmap.c                                  |  42 +-
 samples/configfs/configfs_sample.c            | 153 +++
 tools/testing/selftests/gpio/.gitignore       |   2 +
 tools/testing/selftests/gpio/Makefile         |   4 +-
 tools/testing/selftests/gpio/config           |   1 +
 tools/testing/selftests/gpio/gpio-chip-info.c |  57 ++
 tools/testing/selftests/gpio/gpio-line-name.c |  55 ++
 tools/testing/selftests/gpio/gpio-sim.sh      | 229 +++++
 18 files changed, 1822 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c
 create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c
 create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c
 create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh

Comments

Geert Uytterhoeven March 5, 2021, 8:18 a.m. UTC | #1
CC Greg

On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

>

> Export the symbol for device_is_bound() so that we can use it in gpio-sim

> to check if the simulated GPIO chip is bound before fetching its driver

> data from configfs callbacks in order to retrieve the name of the GPIO

> chip device.

>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> ---

>  drivers/base/dd.c | 1 +

>  1 file changed, 1 insertion(+)

>

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> index 9179825ff646..c62c02e3490a 100644

> --- a/drivers/base/dd.c

> +++ b/drivers/base/dd.c

> @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

>  {

>         return dev->p && klist_node_attached(&dev->p->knode_driver);

>  }

> +EXPORT_SYMBOL_GPL(device_is_bound);

>

>  static void driver_bound(struct device *dev)

>  {

> --

> 2.29.1
Greg Kroah-Hartman March 5, 2021, 8:33 a.m. UTC | #2
On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:
> CC Greg

> 

> On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> >

> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> >

> > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > to check if the simulated GPIO chip is bound before fetching its driver

> > data from configfs callbacks in order to retrieve the name of the GPIO

> > chip device.

> >

> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > ---

> >  drivers/base/dd.c | 1 +

> >  1 file changed, 1 insertion(+)

> >

> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > index 9179825ff646..c62c02e3490a 100644

> > --- a/drivers/base/dd.c

> > +++ b/drivers/base/dd.c

> > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> >  {

> >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> >  }

> > +EXPORT_SYMBOL_GPL(device_is_bound);


No.  Please no.  Why is this needed?  Feels like someone is doing
something really wrong...

NACK.
Bartosz Golaszewski March 5, 2021, 8:45 a.m. UTC | #3
On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > CC Greg

> >

> > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > >

> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > >

> > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > to check if the simulated GPIO chip is bound before fetching its driver

> > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > chip device.

> > >

> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > ---

> > >  drivers/base/dd.c | 1 +

> > >  1 file changed, 1 insertion(+)

> > >

> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > index 9179825ff646..c62c02e3490a 100644

> > > --- a/drivers/base/dd.c

> > > +++ b/drivers/base/dd.c

> > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > >  {

> > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > >  }

> > > +EXPORT_SYMBOL_GPL(device_is_bound);

>

> No.  Please no.  Why is this needed?  Feels like someone is doing

> something really wrong...

>

> NACK.

>


I should have Cc'ed you the entire series, my bad.

This is the patch that uses this change - it's a new, improved testing
module for GPIO using configfs & sysfs as you (I think) suggested a
while ago:

https://lkml.org/lkml/2021/3/4/355

The story goes like this: committing the configfs item registers a
platform device. As far as I understand - there's no guarantee that
the device will be bound to a driver before the commit callback (or
more specifically platform_device_register_full() in this case)
returns so the user may try to retrieve the name of the device
immediately (normally user-space should wait for the associated uevent
but nobody can force that) by doing:

mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/
cat /sys/kernel/config/gpio-sim/live/foo/dev_name

If the device is not bound at this point, we'll have a crash in the
kernel as opposed to just returning -ENODEV.

Please advise on how to handle it without device_is_bound().

Best Regards,
Bartosz
Greg Kroah-Hartman March 5, 2021, 8:55 a.m. UTC | #4
On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> >

> > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > CC Greg

> > >

> > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > >

> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > >

> > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > chip device.

> > > >

> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > ---

> > > >  drivers/base/dd.c | 1 +

> > > >  1 file changed, 1 insertion(+)

> > > >

> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > index 9179825ff646..c62c02e3490a 100644

> > > > --- a/drivers/base/dd.c

> > > > +++ b/drivers/base/dd.c

> > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > >  {

> > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > >  }

> > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> >

> > No.  Please no.  Why is this needed?  Feels like someone is doing

> > something really wrong...

> >

> > NACK.

> >

> 

> I should have Cc'ed you the entire series, my bad.

> 

> This is the patch that uses this change - it's a new, improved testing

> module for GPIO using configfs & sysfs as you (I think) suggested a

> while ago:

> 

> https://lkml.org/lkml/2021/3/4/355

> 

> The story goes like this: committing the configfs item registers a

> platform device.


Ick, no, stop there, that's not a "real" device, please do not abuse
platform devices like that, you all know I hate this :(

Use the virtbus code instead perhaps?

> As far as I understand - there's no guarantee that

> the device will be bound to a driver before the commit callback (or

> more specifically platform_device_register_full() in this case)

> returns so the user may try to retrieve the name of the device

> immediately (normally user-space should wait for the associated uevent

> but nobody can force that) by doing:

> 

> mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> 

> If the device is not bound at this point, we'll have a crash in the

> kernel as opposed to just returning -ENODEV.


How will the kernel crash?  What has created the dev_name sysfs file
before it is possible to be read from?  That feels like the root
problem.

> Please advise on how to handle it without device_is_bound().


Please do not create sysfs files before they can be read from :)

thanks,

greg k-h
Bartosz Golaszewski March 5, 2021, 9:16 a.m. UTC | #5
On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > >

> > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > CC Greg

> > > >

> > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > >

> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > >

> > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > chip device.

> > > > >

> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > ---

> > > > >  drivers/base/dd.c | 1 +

> > > > >  1 file changed, 1 insertion(+)

> > > > >

> > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > --- a/drivers/base/dd.c

> > > > > +++ b/drivers/base/dd.c

> > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > >  {

> > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > >  }

> > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > >

> > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > something really wrong...

> > >

> > > NACK.

> > >

> >

> > I should have Cc'ed you the entire series, my bad.

> >

> > This is the patch that uses this change - it's a new, improved testing

> > module for GPIO using configfs & sysfs as you (I think) suggested a

> > while ago:

> >

> > https://lkml.org/lkml/2021/3/4/355

> >

> > The story goes like this: committing the configfs item registers a

> > platform device.

>

> Ick, no, stop there, that's not a "real" device, please do not abuse

> platform devices like that, you all know I hate this :(

>

> Use the virtbus code instead perhaps?

>


I have no idea what virtbus is and grepping for it only returns three
hits in: ./drivers/pci/iov.c and it's a function argument.

If it stands for virtual bus then for sure it sounds like the right
thing but I need to find more info on this.

> > As far as I understand - there's no guarantee that

> > the device will be bound to a driver before the commit callback (or

> > more specifically platform_device_register_full() in this case)

> > returns so the user may try to retrieve the name of the device

> > immediately (normally user-space should wait for the associated uevent

> > but nobody can force that) by doing:

> >

> > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> >

> > If the device is not bound at this point, we'll have a crash in the

> > kernel as opposed to just returning -ENODEV.

>

> How will the kernel crash?  What has created the dev_name sysfs file

> before it is possible to be read from?  That feels like the root

> problem.

>


It's not sysfs - it's in configfs. Each chip has a read-only configfs
attribute that returns the name of the device - I don't really have a
better idea to map the configfs items to devices that committing
creates.

> > Please advise on how to handle it without device_is_bound().

>

> Please do not create sysfs files before they can be read from :)

>


Don't worry, I don't.

Bartosz
Greg Kroah-Hartman March 5, 2021, 10:24 a.m. UTC | #6
On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> >

> > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > >

> > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > CC Greg

> > > > >

> > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > >

> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > >

> > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > chip device.

> > > > > >

> > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > ---

> > > > > >  drivers/base/dd.c | 1 +

> > > > > >  1 file changed, 1 insertion(+)

> > > > > >

> > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > --- a/drivers/base/dd.c

> > > > > > +++ b/drivers/base/dd.c

> > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > >  {

> > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > >  }

> > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > >

> > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > something really wrong...

> > > >

> > > > NACK.

> > > >

> > >

> > > I should have Cc'ed you the entire series, my bad.

> > >

> > > This is the patch that uses this change - it's a new, improved testing

> > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > while ago:

> > >

> > > https://lkml.org/lkml/2021/3/4/355

> > >

> > > The story goes like this: committing the configfs item registers a

> > > platform device.

> >

> > Ick, no, stop there, that's not a "real" device, please do not abuse

> > platform devices like that, you all know I hate this :(

> >

> > Use the virtbus code instead perhaps?

> >

> 

> I have no idea what virtbus is and grepping for it only returns three

> hits in: ./drivers/pci/iov.c and it's a function argument.

> 

> If it stands for virtual bus then for sure it sounds like the right

> thing but I need to find more info on this.


Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for
the details.  "virtbus" was what I think about it as that was my
original name for it, but it eventually got merged with a different
name.

> > > As far as I understand - there's no guarantee that

> > > the device will be bound to a driver before the commit callback (or

> > > more specifically platform_device_register_full() in this case)

> > > returns so the user may try to retrieve the name of the device

> > > immediately (normally user-space should wait for the associated uevent

> > > but nobody can force that) by doing:

> > >

> > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > >

> > > If the device is not bound at this point, we'll have a crash in the

> > > kernel as opposed to just returning -ENODEV.

> >

> > How will the kernel crash?  What has created the dev_name sysfs file

> > before it is possible to be read from?  That feels like the root

> > problem.

> >

> 

> It's not sysfs - it's in configfs. Each chip has a read-only configfs

> attribute that returns the name of the device - I don't really have a

> better idea to map the configfs items to devices that committing

> creates.


Same question, why are you exporting a configfs attribute that can not
be read from?  Only export it when your driver is bound to the device.

thanks,

greg k-h
Bartosz Golaszewski March 5, 2021, 10:58 a.m. UTC | #7
On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:

> > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > >

> > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > >

> > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > > CC Greg

> > > > > >

> > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > > >

> > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > >

> > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > > chip device.

> > > > > > >

> > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > ---

> > > > > > >  drivers/base/dd.c | 1 +

> > > > > > >  1 file changed, 1 insertion(+)

> > > > > > >

> > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > > --- a/drivers/base/dd.c

> > > > > > > +++ b/drivers/base/dd.c

> > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > > >  {

> > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > > >  }

> > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > > >

> > > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > > something really wrong...

> > > > >

> > > > > NACK.

> > > > >

> > > >

> > > > I should have Cc'ed you the entire series, my bad.

> > > >

> > > > This is the patch that uses this change - it's a new, improved testing

> > > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > > while ago:

> > > >

> > > > https://lkml.org/lkml/2021/3/4/355

> > > >

> > > > The story goes like this: committing the configfs item registers a

> > > > platform device.

> > >

> > > Ick, no, stop there, that's not a "real" device, please do not abuse

> > > platform devices like that, you all know I hate this :(

> > >

> > > Use the virtbus code instead perhaps?

> > >

> >

> > I have no idea what virtbus is and grepping for it only returns three

> > hits in: ./drivers/pci/iov.c and it's a function argument.

> >

> > If it stands for virtual bus then for sure it sounds like the right

> > thing but I need to find more info on this.

>

> Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for

> the details.  "virtbus" was what I think about it as that was my

> original name for it, but it eventually got merged with a different

> name.

>

> > > > As far as I understand - there's no guarantee that

> > > > the device will be bound to a driver before the commit callback (or

> > > > more specifically platform_device_register_full() in this case)

> > > > returns so the user may try to retrieve the name of the device

> > > > immediately (normally user-space should wait for the associated uevent

> > > > but nobody can force that) by doing:

> > > >

> > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > > >

> > > > If the device is not bound at this point, we'll have a crash in the

> > > > kernel as opposed to just returning -ENODEV.

> > >

> > > How will the kernel crash?  What has created the dev_name sysfs file

> > > before it is possible to be read from?  That feels like the root

> > > problem.

> > >

> >

> > It's not sysfs - it's in configfs. Each chip has a read-only configfs

> > attribute that returns the name of the device - I don't really have a

> > better idea to map the configfs items to devices that committing

> > creates.

>

> Same question, why are you exporting a configfs attribute that can not

> be read from?  Only export it when your driver is bound to the device.

>


The device doesn't know anything about configfs. Why would it? The
configuration of a GPIO chip can't be changed after it's instantiated,
this is why we have committable items.

We export a directory in configfs: gpio-sim -> user creates a new
directory (item) in gpio-sim/pending/foo and it's not tied to any
device yet but exports attributes which we use to configure the device
(label, number of lines, line names etc.), then we mv
gpio-sim/pending/foo gpio-sim/live and this is when the device gets
created and registered with the subsystem. We take all the configured
attributes and put them into device properties for both the driver and
gpiolib core (for standard properties) to read - just like we would
with a regular GPIO driver because this is the goal: test the core
code.

Configfs doesn't even allow to dynamically export and unexport attributes.

Bart
Greg Kroah-Hartman March 5, 2021, 11:27 a.m. UTC | #8
On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> >

> > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:

> > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > >

> > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > >

> > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > > > CC Greg

> > > > > > >

> > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > > > >

> > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > >

> > > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > > > chip device.

> > > > > > > >

> > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > ---

> > > > > > > >  drivers/base/dd.c | 1 +

> > > > > > > >  1 file changed, 1 insertion(+)

> > > > > > > >

> > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > > > --- a/drivers/base/dd.c

> > > > > > > > +++ b/drivers/base/dd.c

> > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > > > >  {

> > > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > > > >  }

> > > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > > > >

> > > > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > > > something really wrong...

> > > > > >

> > > > > > NACK.

> > > > > >

> > > > >

> > > > > I should have Cc'ed you the entire series, my bad.

> > > > >

> > > > > This is the patch that uses this change - it's a new, improved testing

> > > > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > > > while ago:

> > > > >

> > > > > https://lkml.org/lkml/2021/3/4/355

> > > > >

> > > > > The story goes like this: committing the configfs item registers a

> > > > > platform device.

> > > >

> > > > Ick, no, stop there, that's not a "real" device, please do not abuse

> > > > platform devices like that, you all know I hate this :(

> > > >

> > > > Use the virtbus code instead perhaps?

> > > >

> > >

> > > I have no idea what virtbus is and grepping for it only returns three

> > > hits in: ./drivers/pci/iov.c and it's a function argument.

> > >

> > > If it stands for virtual bus then for sure it sounds like the right

> > > thing but I need to find more info on this.

> >

> > Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for

> > the details.  "virtbus" was what I think about it as that was my

> > original name for it, but it eventually got merged with a different

> > name.

> >

> > > > > As far as I understand - there's no guarantee that

> > > > > the device will be bound to a driver before the commit callback (or

> > > > > more specifically platform_device_register_full() in this case)

> > > > > returns so the user may try to retrieve the name of the device

> > > > > immediately (normally user-space should wait for the associated uevent

> > > > > but nobody can force that) by doing:

> > > > >

> > > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > > > >

> > > > > If the device is not bound at this point, we'll have a crash in the

> > > > > kernel as opposed to just returning -ENODEV.

> > > >

> > > > How will the kernel crash?  What has created the dev_name sysfs file

> > > > before it is possible to be read from?  That feels like the root

> > > > problem.

> > > >

> > >

> > > It's not sysfs - it's in configfs. Each chip has a read-only configfs

> > > attribute that returns the name of the device - I don't really have a

> > > better idea to map the configfs items to devices that committing

> > > creates.

> >

> > Same question, why are you exporting a configfs attribute that can not

> > be read from?  Only export it when your driver is bound to the device.

> >

> 

> The device doesn't know anything about configfs. Why would it? The

> configuration of a GPIO chip can't be changed after it's instantiated,

> this is why we have committable items.

> 

> We export a directory in configfs: gpio-sim -> user creates a new

> directory (item) in gpio-sim/pending/foo and it's not tied to any

> device yet but exports attributes which we use to configure the device

> (label, number of lines, line names etc.), then we mv

> gpio-sim/pending/foo gpio-sim/live and this is when the device gets

> created and registered with the subsystem. We take all the configured

> attributes and put them into device properties for both the driver and

> gpiolib core (for standard properties) to read - just like we would

> with a regular GPIO driver because this is the goal: test the core

> code.


Ok, but they why are you trying to have dev_name be an exported thing?
I don't understand an attribute here that is visable but can not be read
from.

And why not just use the default device name function: dev_name(), which
will always return a string that will work no matter if the device is
bound to a driver or not.

thanks,

greg k-h
Bartosz Golaszewski March 5, 2021, 2:20 p.m. UTC | #9
On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:

> > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > >

> > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:

> > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > >

> > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > > >

> > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > > > > CC Greg

> > > > > > > >

> > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > > > > >

> > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > >

> > > > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > > > > chip device.

> > > > > > > > >

> > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > > ---

> > > > > > > > >  drivers/base/dd.c | 1 +

> > > > > > > > >  1 file changed, 1 insertion(+)

> > > > > > > > >

> > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > > > > --- a/drivers/base/dd.c

> > > > > > > > > +++ b/drivers/base/dd.c

> > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > > > > >  {

> > > > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > > > > >  }

> > > > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > > > > >

> > > > > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > > > > something really wrong...

> > > > > > >

> > > > > > > NACK.

> > > > > > >

> > > > > >

> > > > > > I should have Cc'ed you the entire series, my bad.

> > > > > >

> > > > > > This is the patch that uses this change - it's a new, improved testing

> > > > > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > > > > while ago:

> > > > > >

> > > > > > https://lkml.org/lkml/2021/3/4/355

> > > > > >

> > > > > > The story goes like this: committing the configfs item registers a

> > > > > > platform device.

> > > > >

> > > > > Ick, no, stop there, that's not a "real" device, please do not abuse

> > > > > platform devices like that, you all know I hate this :(

> > > > >

> > > > > Use the virtbus code instead perhaps?

> > > > >

> > > >

> > > > I have no idea what virtbus is and grepping for it only returns three

> > > > hits in: ./drivers/pci/iov.c and it's a function argument.

> > > >

> > > > If it stands for virtual bus then for sure it sounds like the right

> > > > thing but I need to find more info on this.

> > >

> > > Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for

> > > the details.  "virtbus" was what I think about it as that was my

> > > original name for it, but it eventually got merged with a different

> > > name.

> > >


Unless I'm not seeing something - it completely doesn't look like the
right solution. This auxiliary bus sounds like MFD with extra steps.
Its aim seems to be to provide virtual devices for sub-modules of real
devices.

What I have here really is a dummy device for which no HW exists.
Also: while the preferred way is to use configfs to instantiate these
simulated devices, then can still be registered from device-tree (this
is a feature that was requested and eventually implemented in
gpio-mockup which we want to phase out so we can't just drop it).
AFAIK only platform devices can be populated from DT.

I guess we could create something like a "virtual bus" that would be
there for devices that don't exist on any physical bus but this would
end up in big part being the same thing as platform devices.

> > > > > > As far as I understand - there's no guarantee that

> > > > > > the device will be bound to a driver before the commit callback (or

> > > > > > more specifically platform_device_register_full() in this case)

> > > > > > returns so the user may try to retrieve the name of the device

> > > > > > immediately (normally user-space should wait for the associated uevent

> > > > > > but nobody can force that) by doing:

> > > > > >

> > > > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > > > > >

> > > > > > If the device is not bound at this point, we'll have a crash in the

> > > > > > kernel as opposed to just returning -ENODEV.

> > > > >

> > > > > How will the kernel crash?  What has created the dev_name sysfs file

> > > > > before it is possible to be read from?  That feels like the root

> > > > > problem.

> > > > >

> > > >

> > > > It's not sysfs - it's in configfs. Each chip has a read-only configfs

> > > > attribute that returns the name of the device - I don't really have a

> > > > better idea to map the configfs items to devices that committing

> > > > creates.

> > >

> > > Same question, why are you exporting a configfs attribute that can not

> > > be read from?  Only export it when your driver is bound to the device.

> > >

> >

> > The device doesn't know anything about configfs. Why would it? The

> > configuration of a GPIO chip can't be changed after it's instantiated,

> > this is why we have committable items.

> >

> > We export a directory in configfs: gpio-sim -> user creates a new

> > directory (item) in gpio-sim/pending/foo and it's not tied to any

> > device yet but exports attributes which we use to configure the device

> > (label, number of lines, line names etc.), then we mv

> > gpio-sim/pending/foo gpio-sim/live and this is when the device gets

> > created and registered with the subsystem. We take all the configured

> > attributes and put them into device properties for both the driver and

> > gpiolib core (for standard properties) to read - just like we would

> > with a regular GPIO driver because this is the goal: test the core

> > code.

>

> Ok, but they why are you trying to have dev_name be an exported thing?

> I don't understand an attribute here that is visable but can not be read

> from.

>


Because once the associated configfs item is committed and the device
created, it will become readable. The list of attributes is fixed in
configfs. I'm not sure what the better approach would be - return
"none" if the device handle is NULL?

> And why not just use the default device name function: dev_name(), which

> will always return a string that will work no matter if the device is

> bound to a driver or not.

>


I can do this but then it's possible that user-space gets the name of
the device which doesn't exist in sysfs. I guess we can mention that
in the documentation.

Bartosz
Greg Kroah-Hartman March 5, 2021, 3:01 p.m. UTC | #10
On Fri, Mar 05, 2021 at 03:20:27PM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:

> >

> > On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:

> > > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > >

> > > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:

> > > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > >

> > > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > > > >

> > > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > > > > > CC Greg

> > > > > > > > >

> > > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > > > > > >

> > > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > > >

> > > > > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > > > > > chip device.

> > > > > > > > > >

> > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > > > ---

> > > > > > > > > >  drivers/base/dd.c | 1 +

> > > > > > > > > >  1 file changed, 1 insertion(+)

> > > > > > > > > >

> > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > > > > > --- a/drivers/base/dd.c

> > > > > > > > > > +++ b/drivers/base/dd.c

> > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > > > > > >  {

> > > > > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > > > > > >  }

> > > > > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > > > > > >

> > > > > > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > > > > > something really wrong...

> > > > > > > >

> > > > > > > > NACK.

> > > > > > > >

> > > > > > >

> > > > > > > I should have Cc'ed you the entire series, my bad.

> > > > > > >

> > > > > > > This is the patch that uses this change - it's a new, improved testing

> > > > > > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > > > > > while ago:

> > > > > > >

> > > > > > > https://lkml.org/lkml/2021/3/4/355

> > > > > > >

> > > > > > > The story goes like this: committing the configfs item registers a

> > > > > > > platform device.

> > > > > >

> > > > > > Ick, no, stop there, that's not a "real" device, please do not abuse

> > > > > > platform devices like that, you all know I hate this :(

> > > > > >

> > > > > > Use the virtbus code instead perhaps?

> > > > > >

> > > > >

> > > > > I have no idea what virtbus is and grepping for it only returns three

> > > > > hits in: ./drivers/pci/iov.c and it's a function argument.

> > > > >

> > > > > If it stands for virtual bus then for sure it sounds like the right

> > > > > thing but I need to find more info on this.

> > > >

> > > > Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for

> > > > the details.  "virtbus" was what I think about it as that was my

> > > > original name for it, but it eventually got merged with a different

> > > > name.

> > > >

> 

> Unless I'm not seeing something - it completely doesn't look like the

> right solution. This auxiliary bus sounds like MFD with extra steps.

> Its aim seems to be to provide virtual devices for sub-modules of real

> devices.

> 

> What I have here really is a dummy device for which no HW exists.


Then just use a "normal" virtual device.  We have loads of them.  But if
you want to bind a "driver" to it, then use the aux bus please.  Do NOT
abuse a platform device for this.

> Also: while the preferred way is to use configfs to instantiate these

> simulated devices, then can still be registered from device-tree (this

> is a feature that was requested and eventually implemented in

> gpio-mockup which we want to phase out so we can't just drop it).

> AFAIK only platform devices can be populated from DT.


If you really are using DT, then ok, a platform device can be used, but
you didn't say that :)

> I guess we could create something like a "virtual bus" that would be

> there for devices that don't exist on any physical bus but this would

> end up in big part being the same thing as platform devices.


That's what the aux bus code is there for.  So maybe you do need to use
it.

> > > > > > > As far as I understand - there's no guarantee that

> > > > > > > the device will be bound to a driver before the commit callback (or

> > > > > > > more specifically platform_device_register_full() in this case)

> > > > > > > returns so the user may try to retrieve the name of the device

> > > > > > > immediately (normally user-space should wait for the associated uevent

> > > > > > > but nobody can force that) by doing:

> > > > > > >

> > > > > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > > > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > > > > > >

> > > > > > > If the device is not bound at this point, we'll have a crash in the

> > > > > > > kernel as opposed to just returning -ENODEV.

> > > > > >

> > > > > > How will the kernel crash?  What has created the dev_name sysfs file

> > > > > > before it is possible to be read from?  That feels like the root

> > > > > > problem.

> > > > > >

> > > > >

> > > > > It's not sysfs - it's in configfs. Each chip has a read-only configfs

> > > > > attribute that returns the name of the device - I don't really have a

> > > > > better idea to map the configfs items to devices that committing

> > > > > creates.

> > > >

> > > > Same question, why are you exporting a configfs attribute that can not

> > > > be read from?  Only export it when your driver is bound to the device.

> > > >

> > >

> > > The device doesn't know anything about configfs. Why would it? The

> > > configuration of a GPIO chip can't be changed after it's instantiated,

> > > this is why we have committable items.

> > >

> > > We export a directory in configfs: gpio-sim -> user creates a new

> > > directory (item) in gpio-sim/pending/foo and it's not tied to any

> > > device yet but exports attributes which we use to configure the device

> > > (label, number of lines, line names etc.), then we mv

> > > gpio-sim/pending/foo gpio-sim/live and this is when the device gets

> > > created and registered with the subsystem. We take all the configured

> > > attributes and put them into device properties for both the driver and

> > > gpiolib core (for standard properties) to read - just like we would

> > > with a regular GPIO driver because this is the goal: test the core

> > > code.

> >

> > Ok, but they why are you trying to have dev_name be an exported thing?

> > I don't understand an attribute here that is visable but can not be read

> > from.

> >

> 

> Because once the associated configfs item is committed and the device

> created, it will become readable. The list of attributes is fixed in

> configfs. I'm not sure what the better approach would be - return

> "none" if the device handle is NULL?


Sounds reasonable, I don't know how configfs works, it's been a decade
since I last touched it.

> > And why not just use the default device name function: dev_name(), which

> > will always return a string that will work no matter if the device is

> > bound to a driver or not.

> >

> 

> I can do this but then it's possible that user-space gets the name of

> the device which doesn't exist in sysfs. I guess we can mention that

> in the documentation.


Device names can change over time, nothing new there.

thanks,

greg k-h
Bartosz Golaszewski March 8, 2021, 10:58 a.m. UTC | #11
On Fri, Mar 5, 2021 at 4:01 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, Mar 05, 2021 at 03:20:27PM +0100, Bartosz Golaszewski wrote:

> > On Fri, Mar 5, 2021 at 12:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:

> > >

> > > On Fri, Mar 05, 2021 at 11:58:18AM +0100, Bartosz Golaszewski wrote:

> > > > On Fri, Mar 5, 2021 at 11:24 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > >

> > > > > On Fri, Mar 05, 2021 at 10:16:10AM +0100, Bartosz Golaszewski wrote:

> > > > > > On Fri, Mar 5, 2021 at 9:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > > >

> > > > > > > On Fri, Mar 05, 2021 at 09:45:41AM +0100, Bartosz Golaszewski wrote:

> > > > > > > > On Fri, Mar 5, 2021 at 9:34 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > > > > > > > >

> > > > > > > > > On Fri, Mar 05, 2021 at 09:18:30AM +0100, Geert Uytterhoeven wrote:

> > > > > > > > > > CC Greg

> > > > > > > > > >

> > > > > > > > > > On Thu, Mar 4, 2021 at 11:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > > > > > > >

> > > > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > > > >

> > > > > > > > > > > Export the symbol for device_is_bound() so that we can use it in gpio-sim

> > > > > > > > > > > to check if the simulated GPIO chip is bound before fetching its driver

> > > > > > > > > > > data from configfs callbacks in order to retrieve the name of the GPIO

> > > > > > > > > > > chip device.

> > > > > > > > > > >

> > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > > > > > > > > > ---

> > > > > > > > > > >  drivers/base/dd.c | 1 +

> > > > > > > > > > >  1 file changed, 1 insertion(+)

> > > > > > > > > > >

> > > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> > > > > > > > > > > index 9179825ff646..c62c02e3490a 100644

> > > > > > > > > > > --- a/drivers/base/dd.c

> > > > > > > > > > > +++ b/drivers/base/dd.c

> > > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)

> > > > > > > > > > >  {

> > > > > > > > > > >         return dev->p && klist_node_attached(&dev->p->knode_driver);

> > > > > > > > > > >  }

> > > > > > > > > > > +EXPORT_SYMBOL_GPL(device_is_bound);

> > > > > > > > >

> > > > > > > > > No.  Please no.  Why is this needed?  Feels like someone is doing

> > > > > > > > > something really wrong...

> > > > > > > > >

> > > > > > > > > NACK.

> > > > > > > > >

> > > > > > > >

> > > > > > > > I should have Cc'ed you the entire series, my bad.

> > > > > > > >

> > > > > > > > This is the patch that uses this change - it's a new, improved testing

> > > > > > > > module for GPIO using configfs & sysfs as you (I think) suggested a

> > > > > > > > while ago:

> > > > > > > >

> > > > > > > > https://lkml.org/lkml/2021/3/4/355

> > > > > > > >

> > > > > > > > The story goes like this: committing the configfs item registers a

> > > > > > > > platform device.

> > > > > > >

> > > > > > > Ick, no, stop there, that's not a "real" device, please do not abuse

> > > > > > > platform devices like that, you all know I hate this :(

> > > > > > >

> > > > > > > Use the virtbus code instead perhaps?

> > > > > > >

> > > > > >

> > > > > > I have no idea what virtbus is and grepping for it only returns three

> > > > > > hits in: ./drivers/pci/iov.c and it's a function argument.

> > > > > >

> > > > > > If it stands for virtual bus then for sure it sounds like the right

> > > > > > thing but I need to find more info on this.

> > > > >

> > > > > Sorry, wrong name, see Documentation/driver-api/auxiliary_bus.rst for

> > > > > the details.  "virtbus" was what I think about it as that was my

> > > > > original name for it, but it eventually got merged with a different

> > > > > name.

> > > > >

> >

> > Unless I'm not seeing something - it completely doesn't look like the

> > right solution. This auxiliary bus sounds like MFD with extra steps.

> > Its aim seems to be to provide virtual devices for sub-modules of real

> > devices.

> >

> > What I have here really is a dummy device for which no HW exists.

>

> Then just use a "normal" virtual device.  We have loads of them.  But if

> you want to bind a "driver" to it, then use the aux bus please.  Do NOT

> abuse a platform device for this.

>

> > Also: while the preferred way is to use configfs to instantiate these

> > simulated devices, then can still be registered from device-tree (this

> > is a feature that was requested and eventually implemented in

> > gpio-mockup which we want to phase out so we can't just drop it).

> > AFAIK only platform devices can be populated from DT.

>

> If you really are using DT, then ok, a platform device can be used, but

> you didn't say that :)

>


My bad. Yes we need to use DT. And platform device does sound like the
best approach.

> > I guess we could create something like a "virtual bus" that would be

> > there for devices that don't exist on any physical bus but this would

> > end up in big part being the same thing as platform devices.

>

> That's what the aux bus code is there for.  So maybe you do need to use

> it.

>


I'm fine with that if it can be instantiated from DT but it doesn't seem so.

> > > > > > > > As far as I understand - there's no guarantee that

> > > > > > > > the device will be bound to a driver before the commit callback (or

> > > > > > > > more specifically platform_device_register_full() in this case)

> > > > > > > > returns so the user may try to retrieve the name of the device

> > > > > > > > immediately (normally user-space should wait for the associated uevent

> > > > > > > > but nobody can force that) by doing:

> > > > > > > >

> > > > > > > > mv /sys/kernel/config/gpio-sim/pending/foo /sys/kernel/config/gpio-sim/live/

> > > > > > > > cat /sys/kernel/config/gpio-sim/live/foo/dev_name

> > > > > > > >

> > > > > > > > If the device is not bound at this point, we'll have a crash in the

> > > > > > > > kernel as opposed to just returning -ENODEV.

> > > > > > >

> > > > > > > How will the kernel crash?  What has created the dev_name sysfs file

> > > > > > > before it is possible to be read from?  That feels like the root

> > > > > > > problem.

> > > > > > >

> > > > > >

> > > > > > It's not sysfs - it's in configfs. Each chip has a read-only configfs

> > > > > > attribute that returns the name of the device - I don't really have a

> > > > > > better idea to map the configfs items to devices that committing

> > > > > > creates.

> > > > >

> > > > > Same question, why are you exporting a configfs attribute that can not

> > > > > be read from?  Only export it when your driver is bound to the device.

> > > > >

> > > >

> > > > The device doesn't know anything about configfs. Why would it? The

> > > > configuration of a GPIO chip can't be changed after it's instantiated,

> > > > this is why we have committable items.

> > > >

> > > > We export a directory in configfs: gpio-sim -> user creates a new

> > > > directory (item) in gpio-sim/pending/foo and it's not tied to any

> > > > device yet but exports attributes which we use to configure the device

> > > > (label, number of lines, line names etc.), then we mv

> > > > gpio-sim/pending/foo gpio-sim/live and this is when the device gets

> > > > created and registered with the subsystem. We take all the configured

> > > > attributes and put them into device properties for both the driver and

> > > > gpiolib core (for standard properties) to read - just like we would

> > > > with a regular GPIO driver because this is the goal: test the core

> > > > code.

> > >

> > > Ok, but they why are you trying to have dev_name be an exported thing?

> > > I don't understand an attribute here that is visable but can not be read

> > > from.

> > >

> >

> > Because once the associated configfs item is committed and the device

> > created, it will become readable. The list of attributes is fixed in

> > configfs. I'm not sure what the better approach would be - return

> > "none" if the device handle is NULL?

>

> Sounds reasonable, I don't know how configfs works, it's been a decade

> since I last touched it.

>

> > > And why not just use the default device name function: dev_name(), which

> > > will always return a string that will work no matter if the device is

> > > bound to a driver or not.

> > >

> >

> > I can do this but then it's possible that user-space gets the name of

> > the device which doesn't exist in sysfs. I guess we can mention that

> > in the documentation.

>

> Device names can change over time, nothing new there.

>


Ok will change in v3. I'll Cc you next time.

Bart