| Message ID | 20250129155525.663780-1-koichiro.den@canonical.com |
|---|---|
| Headers | show |
| Series | Introduce configfs-based interface for gpio-aggregator | expand |
On Wed, Jan 29, 2025 at 4:56 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > This RFC patch series proposes adding a configfs-based interface to > gpio-aggregator to address limitations in the existing 'new_device' > interface. > > The existing 'new_device' interface has several limitations: > > #1. No way to determine when GPIO aggregator creation is complete. > #2. No way to retrieve errors when creating a GPIO aggregator. > #3. No way to trace a GPIO line of an aggregator back to its > corresponding physical device. > #4. The 'new_device' echo does not indicate which virtual gpiochip.<N> > was created. > #5. No way to assign names to GPIO lines exported through an aggregator. > > Although issues #1 to #3 could technically be resolved easily without > configfs, using configfs offers a streamlined, modern, and extensible > approach, especially since gpio-sim and gpio-virtuser already utilize > configfs. > > This RFC patch series includes two commits: > > * [PATCH 1/2] implements the configfs interface and resolves the above > issues: > - #1, Wait for probe completion using a platform bus notifier, > in the same manner as gpio-virtuser. > - #2, Introduce a 'live' attribute (like gpio-virtuser/gpio-sim), > returning -ENXIO when probe fails. > - #3, Structure configfs directories to clearly map virtual lines to > physical ones. > - #4, Add a read-only 'dev_name' attribute exposing the platform bus > device name. > - #5, Allow users to set custom line names via a 'name' attribute. > > * [PATCH 2/2] provides documentation on using the new interface. > > > Koichiro Den (2): > gpio: aggregator: Introduce configfs interface > Documentation: gpio: document configfs interface for gpio-aggregator > > .../admin-guide/gpio/gpio-aggregator.rst | 86 +++ > drivers/gpio/gpio-aggregator.c | 673 +++++++++++++++++- > 2 files changed, 757 insertions(+), 2 deletions(-) > > -- > 2.45.2 > Hi! I love the idea! In fact I think I floated it in a discussion with Geert some time ago but never got around to working on it. I just glanced at the code and it looks nice and clean. I'd love to see some more improvements like using a common prefix for all internal symbols but it can be addressed in a separate series. I played a bit with the module and this is where I noticed some issues: 1. The sysfs interface must keep on working. The same command that works with mainline, fails for me with your patch. There's no error propagated to user-space, write() returns success and I only see: gpio-aggregator.0: probe with driver gpio-aggregator failed with error -12 in the kernel log. 2. I couldn't verify that it's not the case already but the code does not suggest it: IMO devices created with sysfs should appear in configfs. 3. I don't think the user should need to specify the number of lines to aggregate. That information should be automatically inferred from the number of lineX attributes they created instead. Also: if I create a line attribute without setting num_lines, the driver just crashes. In fact it seems any discrepancy between the number of lines specified and the naming convention of the line attribute causes a crash. 4. Writing 1 to live, when no lines to aggregate were specified, should fail. There's probably more but I haven't had a lot of time. In short: I'm very much in favor of adding this but it will require some work. Thanks, Bartosz
On Thu, Jan 30, 2025 at 11:30:59AM GMT, Bartosz Golaszewski wrote: > On Wed, Jan 29, 2025 at 4:56 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > This RFC patch series proposes adding a configfs-based interface to > > gpio-aggregator to address limitations in the existing 'new_device' > > interface. > > > > The existing 'new_device' interface has several limitations: > > > > #1. No way to determine when GPIO aggregator creation is complete. > > #2. No way to retrieve errors when creating a GPIO aggregator. > > #3. No way to trace a GPIO line of an aggregator back to its > > corresponding physical device. > > #4. The 'new_device' echo does not indicate which virtual gpiochip.<N> > > was created. > > #5. No way to assign names to GPIO lines exported through an aggregator. > > > > Although issues #1 to #3 could technically be resolved easily without > > configfs, using configfs offers a streamlined, modern, and extensible > > approach, especially since gpio-sim and gpio-virtuser already utilize > > configfs. > > > > This RFC patch series includes two commits: > > > > * [PATCH 1/2] implements the configfs interface and resolves the above > > issues: > > - #1, Wait for probe completion using a platform bus notifier, > > in the same manner as gpio-virtuser. > > - #2, Introduce a 'live' attribute (like gpio-virtuser/gpio-sim), > > returning -ENXIO when probe fails. > > - #3, Structure configfs directories to clearly map virtual lines to > > physical ones. > > - #4, Add a read-only 'dev_name' attribute exposing the platform bus > > device name. > > - #5, Allow users to set custom line names via a 'name' attribute. > > > > * [PATCH 2/2] provides documentation on using the new interface. > > > > > > Koichiro Den (2): > > gpio: aggregator: Introduce configfs interface > > Documentation: gpio: document configfs interface for gpio-aggregator > > > > .../admin-guide/gpio/gpio-aggregator.rst | 86 +++ > > drivers/gpio/gpio-aggregator.c | 673 +++++++++++++++++- > > 2 files changed, 757 insertions(+), 2 deletions(-) > > > > -- > > 2.45.2 > > > > Hi! Hi, thank you for reviewing. > > I love the idea! In fact I think I floated it in a discussion with > Geert some time ago but never got around to working on it. > > I just glanced at the code and it looks nice and clean. I'd love to > see some more improvements like using a common prefix for all internal > symbols but it can be addressed in a separate series. > > I played a bit with the module and this is where I noticed some issues: > > 1. The sysfs interface must keep on working. The same command that > works with mainline, fails for me with your patch. There's no error > propagated to user-space, write() returns success and I only see: > > gpio-aggregator.0: probe with driver gpio-aggregator failed with error -12 It looks like the issue is caused by gpiochip_fwd_line_names(). I'll fix it. > > in the kernel log. > > 2. I couldn't verify that it's not the case already but the code does > not suggest it: IMO devices created with sysfs should appear in > configfs. That makes sense, I'll add the implementation. > > 3. I don't think the user should need to specify the number of lines > to aggregate. That information should be automatically inferred from > the number of lineX attributes they created instead. [...] I agree that it's essentially unnecessary, but considering the current state of gpio-sim's configfs, having the user set num_lines doesn't seem too unnatural to me. What do you think? > [...] Also: if I create > a line attribute without setting num_lines, the driver just crashes. > In fact it seems any discrepancy between the number of lines specified > and the naming convention of the line attribute causes a crash. My bad.. thanks for pointing it out. > > 4. Writing 1 to live, when no lines to aggregate were specified, should fail. Agreed, I'll address this. > > There's probably more but I haven't had a lot of time. > > In short: I'm very much in favor of adding this but it will require some work. > > Thanks, > Bartosz I'd appreciate a response to one question above. I'll prepare v2 after. Thanks again! -Koichiro
On Thu, Jan 30, 2025 at 5:04 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > On Thu, Jan 30, 2025 at 11:30:59AM GMT, Bartosz Golaszewski wrote: > > > > 3. I don't think the user should need to specify the number of lines > > to aggregate. That information should be automatically inferred from > > the number of lineX attributes they created instead. [...] > > I agree that it's essentially unnecessary, but considering the current > state of gpio-sim's configfs, having the user set num_lines doesn't seem > too unnatural to me. What do you think? > No, this is completely different. We cannot figure out how many lines the user wants gpio-sim to have without explicitly providing that information over the num_lines attribute. For the aggregator, we know exactly how many lines the user wants - it's determined by the number of line groups. We can simply wait for the live attribute to be set to 1 and then count them. While at it: there's no reason to impose a naming convention of lineX, lineY etc., the names don't matter for the aggregator setup (unlike gpio-sim where they indicate the offset of the line they concern). Bart
On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > While at it: there's no reason to impose a > naming convention of lineX, lineY etc., the names don't matter for the > aggregator setup (unlike gpio-sim where they indicate the offset of > the line they concern). > Scratch that part. There's a good reason for that - the ordering of lines within the aggregator. I'm just not sure whether we should impose a strict naming where - for an aggregator of 3 lines total - we expect there to exist groups named line0, line1 and line2 or if we should be more lenient and possibly sort whatever names the user provides alphabetically? Bart
On Thu, 30 Jan 2025 at 21:48, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > While at it: there's no reason to impose a > > naming convention of lineX, lineY etc., the names don't matter for the > > aggregator setup (unlike gpio-sim where they indicate the offset of > > the line they concern). > > > > Scratch that part. There's a good reason for that - the ordering of > lines within the aggregator. I'm just not sure whether we should > impose a strict naming where - for an aggregator of 3 lines total - we > expect there to exist groups named line0, line1 and line2 or if we > should be more lenient and possibly sort whatever names the user > provides alphabetically? If I may jump in quickly (I provided some initial feedback on the configfs interfaces for the first internal patches). I think it's preferable to have strict and explicit, even If more verbose, line ordering in the aggregator.The motivator for this is that whoever sets up a new device through the aggregator does not have to second guess what the driver will do. Implicit ordering could perhaps be fine if the consumers were to impose some set of rules themselves, but I fear there would still be some ambiguity left if free form names were for e.g. [1, 02, 10]. In the end they would probably settle on some mechanism which would mimic what we could already do in the driver itself and avoid any further confusion for the user. Cheers, Maciej
On Fri, Jan 31, 2025 at 8:26 AM Maciej Borzęcki <maciej.borzecki@canonical.com> wrote: > > On Thu, 30 Jan 2025 at 21:48, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > While at it: there's no reason to impose a > > > naming convention of lineX, lineY etc., the names don't matter for the > > > aggregator setup (unlike gpio-sim where they indicate the offset of > > > the line they concern). > > > > > > > Scratch that part. There's a good reason for that - the ordering of > > lines within the aggregator. I'm just not sure whether we should > > impose a strict naming where - for an aggregator of 3 lines total - we > > expect there to exist groups named line0, line1 and line2 or if we > > should be more lenient and possibly sort whatever names the user > > provides alphabetically? > > If I may jump in quickly (I provided some initial feedback on the > configfs interfaces > for the first internal patches). I think it's preferable to have > strict and explicit, even > If more verbose, line ordering in the aggregator.The motivator for > this is that whoever > sets up a new device through the aggregator does not have to second guess what > the driver will do. Implicit ordering could perhaps be fine if the > consumers were to > impose some set of rules themselves, but I fear there would still be > some ambiguity > left if free form names were for e.g. [1, 02, 10]. In the end they > would probably settle > on some mechanism which would mimic what we could already do in the > driver itself > and avoid any further confusion for the user. > > Cheers, > Maciej Fair enough, I was thinking that just sorting the entries alphabetically would both allow the lineX scheme AND leave some leeway for non-standard naming but it may indeed end up being confusing with no apparent advantage. Bart
This RFC patch series proposes adding a configfs-based interface to gpio-aggregator to address limitations in the existing 'new_device' interface. The existing 'new_device' interface has several limitations: #1. No way to determine when GPIO aggregator creation is complete. #2. No way to retrieve errors when creating a GPIO aggregator. #3. No way to trace a GPIO line of an aggregator back to its corresponding physical device. #4. The 'new_device' echo does not indicate which virtual gpiochip.<N> was created. #5. No way to assign names to GPIO lines exported through an aggregator. Although issues #1 to #3 could technically be resolved easily without configfs, using configfs offers a streamlined, modern, and extensible approach, especially since gpio-sim and gpio-virtuser already utilize configfs. This RFC patch series includes two commits: * [PATCH 1/2] implements the configfs interface and resolves the above issues: - #1, Wait for probe completion using a platform bus notifier, in the same manner as gpio-virtuser. - #2, Introduce a 'live' attribute (like gpio-virtuser/gpio-sim), returning -ENXIO when probe fails. - #3, Structure configfs directories to clearly map virtual lines to physical ones. - #4, Add a read-only 'dev_name' attribute exposing the platform bus device name. - #5, Allow users to set custom line names via a 'name' attribute. * [PATCH 2/2] provides documentation on using the new interface. Koichiro Den (2): gpio: aggregator: Introduce configfs interface Documentation: gpio: document configfs interface for gpio-aggregator .../admin-guide/gpio/gpio-aggregator.rst | 86 +++ drivers/gpio/gpio-aggregator.c | 673 +++++++++++++++++- 2 files changed, 757 insertions(+), 2 deletions(-)