| Message ID | 20211118145142.14519-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| Headers | show |
| Series | gpio-sim: configfs-based GPIO simulator | expand |
On Thu, Nov 18, 2021 at 03:51:38PM +0100, Bartosz Golaszewski wrote: > This is another shot at the gpio-sim testing module. As there was no > reasoning with configfs maintainers for many months, this time the whole > concept of committable items has been dropped. Instead, each configfs > chip item (or rather a group - more on that later) exposes a new > attribute called 'live'. Writing 1 to it brings the chip on-line > (registers the platform device) and writing 0 tears it down. > > There are some caveats to that approach - for example: we can't block > the user-space from deleting chip items when chips are live but is just > handled by silently destroying the chip device in the background. > > Andy (rightfully) pointed out that parsing of the lists of line names is > awkward so in this iteration it's been replaced by a system that is more > elegant and will allow to easily extend configuration options for > specific GPIO lines. This is achieved by turning the chip's configfs > item into a configfs group and allowing the user-space to create > additional items inside it. The items must be called line<offset> (e.g. > line0, line12 etc.) where the offset part indicates to the module the > offset for which given item stores the configuration for. Within each > such line item, there are additional attributes that allow specifying > configuration for specific lines. Currently we only support the 'name' > attribute but I plan to extend that to support GPIO hogging too. One question here. Since you know how the driver looks like in both cases (with and without committable items), would it be possible to modify what you proposed here to the former one in case ConfigFS gains the feature?
On Thu, Nov 18, 2021 at 4:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 18, 2021 at 03:51:38PM +0100, Bartosz Golaszewski wrote: > > This is another shot at the gpio-sim testing module. As there was no > > reasoning with configfs maintainers for many months, this time the whole > > concept of committable items has been dropped. Instead, each configfs > > chip item (or rather a group - more on that later) exposes a new > > attribute called 'live'. Writing 1 to it brings the chip on-line > > (registers the platform device) and writing 0 tears it down. > > > > There are some caveats to that approach - for example: we can't block > > the user-space from deleting chip items when chips are live but is just > > handled by silently destroying the chip device in the background. > > > > Andy (rightfully) pointed out that parsing of the lists of line names is > > awkward so in this iteration it's been replaced by a system that is more > > elegant and will allow to easily extend configuration options for > > specific GPIO lines. This is achieved by turning the chip's configfs > > item into a configfs group and allowing the user-space to create > > additional items inside it. The items must be called line<offset> (e.g. > > line0, line12 etc.) where the offset part indicates to the module the > > offset for which given item stores the configuration for. Within each > > such line item, there are additional attributes that allow specifying > > configuration for specific lines. Currently we only support the 'name' > > attribute but I plan to extend that to support GPIO hogging too. > > One question here. Since you know how the driver looks like in both cases > (with and without committable items), would it be possible to modify what > you proposed here to the former one in case ConfigFS gains the feature? > This would completely change the user interface unfortunately. We could extend it but we would need to keep this one too most likely. TBH I don't see the committable items merged anytime soon, and this is GoodEnough®. Bart
On Thu, Nov 18, 2021 at 05:37:02PM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 18, 2021 at 4:50 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Nov 18, 2021 at 03:51:38PM +0100, Bartosz Golaszewski wrote: > > > This is another shot at the gpio-sim testing module. As there was no > > > reasoning with configfs maintainers for many months, this time the whole > > > concept of committable items has been dropped. Instead, each configfs > > > chip item (or rather a group - more on that later) exposes a new > > > attribute called 'live'. Writing 1 to it brings the chip on-line > > > (registers the platform device) and writing 0 tears it down. > > > > > > There are some caveats to that approach - for example: we can't block > > > the user-space from deleting chip items when chips are live but is just > > > handled by silently destroying the chip device in the background. > > > > > > Andy (rightfully) pointed out that parsing of the lists of line names is > > > awkward so in this iteration it's been replaced by a system that is more > > > elegant and will allow to easily extend configuration options for > > > specific GPIO lines. This is achieved by turning the chip's configfs > > > item into a configfs group and allowing the user-space to create > > > additional items inside it. The items must be called line<offset> (e.g. > > > line0, line12 etc.) where the offset part indicates to the module the > > > offset for which given item stores the configuration for. Within each > > > such line item, there are additional attributes that allow specifying > > > configuration for specific lines. Currently we only support the 'name' > > > attribute but I plan to extend that to support GPIO hogging too. > > > > One question here. Since you know how the driver looks like in both cases > > (with and without committable items), would it be possible to modify what > > you proposed here to the former one in case ConfigFS gains the feature? > > This would completely change the user interface unfortunately. We > could extend it but we would need to keep this one too most likely. > > TBH I don't see the committable items merged anytime soon, and this is > GoodEnough®. Fine with me then! Thanks for doing this all, I know it's a bit delayed in terms of getting into upstream. Btw, gpio-mockup testing scripts have an issue that the number of lines to check overflow is hardcoded and since x86_64 switched to 1024 from 512 it reveals the issue. Does gpio-sim solve this in a better way (like telling to user space the ngpios, etc)?
On Thu, Nov 18, 2021 at 5:59 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 18, 2021 at 05:37:02PM +0100, Bartosz Golaszewski wrote: > > On Thu, Nov 18, 2021 at 4:50 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, Nov 18, 2021 at 03:51:38PM +0100, Bartosz Golaszewski wrote: > > > > This is another shot at the gpio-sim testing module. As there was no > > > > reasoning with configfs maintainers for many months, this time the whole > > > > concept of committable items has been dropped. Instead, each configfs > > > > chip item (or rather a group - more on that later) exposes a new > > > > attribute called 'live'. Writing 1 to it brings the chip on-line > > > > (registers the platform device) and writing 0 tears it down. > > > > > > > > There are some caveats to that approach - for example: we can't block > > > > the user-space from deleting chip items when chips are live but is just > > > > handled by silently destroying the chip device in the background. > > > > > > > > Andy (rightfully) pointed out that parsing of the lists of line names is > > > > awkward so in this iteration it's been replaced by a system that is more > > > > elegant and will allow to easily extend configuration options for > > > > specific GPIO lines. This is achieved by turning the chip's configfs > > > > item into a configfs group and allowing the user-space to create > > > > additional items inside it. The items must be called line<offset> (e.g. > > > > line0, line12 etc.) where the offset part indicates to the module the > > > > offset for which given item stores the configuration for. Within each > > > > such line item, there are additional attributes that allow specifying > > > > configuration for specific lines. Currently we only support the 'name' > > > > attribute but I plan to extend that to support GPIO hogging too. > > > > > > One question here. Since you know how the driver looks like in both cases > > > (with and without committable items), would it be possible to modify what > > > you proposed here to the former one in case ConfigFS gains the feature? > > > > This would completely change the user interface unfortunately. We > > could extend it but we would need to keep this one too most likely. > > > > TBH I don't see the committable items merged anytime soon, and this is > > GoodEnough®. > > Fine with me then! > > Thanks for doing this all, I know it's a bit delayed in terms of getting > into upstream. > > Btw, gpio-mockup testing scripts have an issue that the number of lines to > check overflow is hardcoded and since x86_64 switched to 1024 from 512 it > reveals the issue. Does gpio-sim solve this in a better way (like telling > to user space the ngpios, etc)? > Yeah the selftests need fixing now. No, there's no fix for that in gpio-sim - probe() will just fail. Which makes me think - maybe we should synchronously wait when writing to 'live' for the probe to return (for instance setup a notifier) so that we know if the chip probed correctly. Then we can notify the user-space about the error destroy the device too. Bart
On Fri, Nov 19, 2021 at 11:34:59AM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 18, 2021 at 5:59 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Nov 18, 2021 at 05:37:02PM +0100, Bartosz Golaszewski wrote: > > > On Thu, Nov 18, 2021 at 4:50 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Thu, Nov 18, 2021 at 03:51:38PM +0100, Bartosz Golaszewski wrote: > > > > > This is another shot at the gpio-sim testing module. As there was no > > > > > reasoning with configfs maintainers for many months, this time the whole > > > > > concept of committable items has been dropped. Instead, each configfs > > > > > chip item (or rather a group - more on that later) exposes a new > > > > > attribute called 'live'. Writing 1 to it brings the chip on-line > > > > > (registers the platform device) and writing 0 tears it down. > > > > > > > > > > There are some caveats to that approach - for example: we can't block > > > > > the user-space from deleting chip items when chips are live but is just > > > > > handled by silently destroying the chip device in the background. > > > > > > > > > > Andy (rightfully) pointed out that parsing of the lists of line names is > > > > > awkward so in this iteration it's been replaced by a system that is more > > > > > elegant and will allow to easily extend configuration options for > > > > > specific GPIO lines. This is achieved by turning the chip's configfs > > > > > item into a configfs group and allowing the user-space to create > > > > > additional items inside it. The items must be called line<offset> (e.g. > > > > > line0, line12 etc.) where the offset part indicates to the module the > > > > > offset for which given item stores the configuration for. Within each > > > > > such line item, there are additional attributes that allow specifying > > > > > configuration for specific lines. Currently we only support the 'name' > > > > > attribute but I plan to extend that to support GPIO hogging too. > > > > > > > > One question here. Since you know how the driver looks like in both cases > > > > (with and without committable items), would it be possible to modify what > > > > you proposed here to the former one in case ConfigFS gains the feature? > > > > > > This would completely change the user interface unfortunately. We > > > could extend it but we would need to keep this one too most likely. > > > > > > TBH I don't see the committable items merged anytime soon, and this is > > > GoodEnough®. > > > > Fine with me then! > > > > Thanks for doing this all, I know it's a bit delayed in terms of getting > > into upstream. > > > > Btw, gpio-mockup testing scripts have an issue that the number of lines to > > check overflow is hardcoded and since x86_64 switched to 1024 from 512 it > > reveals the issue. Does gpio-sim solve this in a better way (like telling > > to user space the ngpios, etc)? > > > > Yeah the selftests need fixing now. > No, there need to be new selftests added for your gpio-sim. The existing selftests cover the gpio-mockup itself. > No, there's no fix for that in gpio-sim - probe() will just fail. > Which makes me think - maybe we should synchronously wait when writing > to 'live' for the probe to return (for instance setup a notifier) so > that we know if the chip probed correctly. Then we can notify the > user-space about the error destroy the device too. > +1 - need definite feedback to userspace that the change to the test setup is in place so tests can proceed. No arbitrary waits or waiting for events from other subsystems like udev as we have to do with gpio-mockup. Cheers, Kent.
On Thu, Nov 18, 2021 at 3:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > v8 -> v9: > - dropped the patches implementing committable-items and reworked the > driver to not use them > - reworked the gpio-line-names property and configuring specific lines > in general > - many smaller tweaks here and there The series: Acked-by: Linus Walleij <linus.walleij@linaro.org> Let's go with this. Yours, Linus Walleij
On Mon, Nov 22, 2021 at 1:02 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Nov 18, 2021 at 3:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > v8 -> v9: > > - dropped the patches implementing committable-items and reworked the > > driver to not use them > > - reworked the gpio-line-names property and configuring specific lines > > in general > > - many smaller tweaks here and there > > The series: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Let's go with this. > > Yours, > Linus Walleij Thanks but Kent and Andy are right, I'm about to send another version that synchronously waits during `echo 1 > live` for the device to come on-line. Bart
This is another shot at the gpio-sim testing module. As there was no reasoning with configfs maintainers for many months, this time the whole concept of committable items has been dropped. Instead, each configfs chip item (or rather a group - more on that later) exposes a new attribute called 'live'. Writing 1 to it brings the chip on-line (registers the platform device) and writing 0 tears it down. There are some caveats to that approach - for example: we can't block the user-space from deleting chip items when chips are live but is just handled by silently destroying the chip device in the background. Andy (rightfully) pointed out that parsing of the lists of line names is awkward so in this iteration it's been replaced by a system that is more elegant and will allow to easily extend configuration options for specific GPIO lines. This is achieved by turning the chip's configfs item into a configfs group and allowing the user-space to create additional items inside it. The items must be called line<offset> (e.g. line0, line12 etc.) where the offset part indicates to the module the offset for which given item stores the configuration for. Within each such line item, there are additional attributes that allow specifying configuration for specific lines. Currently we only support the 'name' attribute but I plan to extend that to support GPIO hogging too. 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 v2 -> v3: - use devm_bitmap_alloc() instead of the zalloc variant if we're initializing the bitmap with 1s - drop the patch exporting device_is_bound() - don't return -ENODEV from dev_nam and chip_name configfs attributes, return a string indicating that the device is not available yet ('n/a') - fix indentation where it makes sense - don't protect IDA functions which use their own locking and where it's not needed - use kmemdup() instead of kzalloc() + memcpy() - collected review tags - minor coding style fixes v3 -> v4: - return 'none' instead of 'n/a' from dev_name and chip_name before the device is registered - use sysfs_emit() instead of s*printf() - drop GPIO_SIM_MAX_PROP as it's only used in an array's definition where it's fine to hardcode the value v4 -> v5: - drop lib patches that are already upstream - use BIT() instead of (1UL << bit) for flags - fix refcounting for the configfs_dirent in rename() - drop d_move() from the rename() callback - free memory allocated for the live and pending groups in configfs_d_iput() and not in detach_groups() - make sure that if a group of some name is in the live directory, a new group with the same name cannot be created in the pending directory v5 -> v6: - go back to using (1UL << bit) instead of BIT() - if the live group dentry doesn't exist for whatever reason at the time when mkdir() in the pending group is called (would be a BUG()), return -ENOENT instead of -EEXIST which should only be returned if given subsystem already exists in either live or pending group v6 -> v7: - as detailed by Andy in commit 6fda593f3082 ("gpio: mockup: Convert to use software nodes") removing device properties after the platform device is removed but before the GPIO device gets dropped can lead to a use-after-free bug - use software nodes to manually control the freeing of the properties v7 -> v8: - fixed some minor coding style issues as pointed out by Andy v8 -> v9: - dropped the patches implementing committable-items and reworked the driver to not use them - reworked the gpio-line-names property and configuring specific lines in general - many smaller tweaks here and there Bartosz Golaszewski (4): 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 | 67 ++ drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-sim.c | 990 ++++++++++++++++++ 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 | 266 +++++ 10 files changed, 1449 insertions(+), 2 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