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 Thu, Jan 30, 2025 at 09:47:47PM GMT, Bartosz Golaszewski wrote: > On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > (Small reply to the previous comment:) Yes, I understand your point about why you think 'num_lines' is unnecessary. What I meant was more of a UX (User eXperience) consideration. > > 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. [...] You're right, that's exactly the intention of the strict naming, 'line0', 'line1', ..., 'line<Y>'. > [...] 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? As Maciej pointed out: (https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > [...] if free form names were for e.g. [1, 02, 10]. we would need a well-defined rule to avoid ambiguity, which could potentially unnecessarily impose burden on users to understand how to properly use the interface. Regardless, the point is that we need to make it clear to users which GPIO line a specific line<Y> of an aggregator forwards operations to. Since requiring users to explicitly set the offset within the aggregator for each virtual line (e.g. besides 'key'/'offset'/'name' attributes, by adding 'idx' attribute, which users would need to set explicitly) would be cumbersome, this RFC implementation instead just makes use of directory naming. I believe we agree on this approach (i.e., using directory naming to establish ordering). Correct me if I'm wrong. So, to move forward, let me outline the possible approaches we can take: Option (a). Drop 'num_lines' attribute and: (a-1). Impose strict naming rule for line directories Users can only create directories with a predefined naming convention. This could be 'line0', 'line1', ... 'line<Y>' (as in the RFC implementation), or simply '0', '1', ..., '<Y>'. (a-2). Allow arbitrary naming for line directories This would require a well-defined rule to avoid ambiguity. As Maciej pointed out: (from https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > if free form names were for e.g. [1, 02, 10] Users would need to understand these rules, which might impose unnecessary burden on users. Option (b). Keep 'num_lines' attribute but: (b-1). Prohibit manual creation of line directories Users would no longer run 'mkdir line0', etc. Instead, writing <Y+1> (Y >= 0) to 'num_lines' would automatically set up the required directories. convention. This could be 'line0', 'line1', ... 'line<Y>' (as in this RFC implementation), or simply '0', '1', ..., '<Y>'. (b-2). Keep manual 'mkdir' for each line, in the same manner as (a-1) (as in the RFC implementation) or (a-2). Seems that no-one is favor of this option. Note: (b-1) is a new idea. Considering what really needs to be configured by users, this could be the least burdensome and simplest, especially when configuring many lines. I'm including it here for broader discussion. Personally, now I'm inclined towards (a-1) with the simplest naming scheme: non-zero-padded integers ('./0', './1', './2', ..., './<Y>'). Or even (b-1). Let me know your thoughts. Thanks. -Koichiro > > Bart
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
On Fri, Jan 31, 2025 at 3:36 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > On Thu, Jan 30, 2025 at 09:47:47PM GMT, Bartosz Golaszewski wrote: > > On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > (Small reply to the previous comment:) > Yes, I understand your point about why you think 'num_lines' is > unnecessary. What I meant was more of a UX (User eXperience) consideration. > If anything this sounds like worse user experience - having to provide duplicate information. > > > 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. [...] > > You're right, that's exactly the intention of the strict naming, 'line0', > 'line1', ..., 'line<Y>'. > > > [...] 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? > > As Maciej pointed out: > > (https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > > [...] if free form names were for e.g. [1, 02, 10]. > > we would need a well-defined rule to avoid ambiguity, which could > potentially unnecessarily impose burden on users to understand how to > properly use the interface. > > > Regardless, the point is that we need to make it clear to users which GPIO > line a specific line<Y> of an aggregator forwards operations to. Since > requiring users to explicitly set the offset within the aggregator for each > virtual line (e.g. besides 'key'/'offset'/'name' attributes, by adding > 'idx' attribute, which users would need to set explicitly) would be > cumbersome, this RFC implementation instead just makes use of directory > naming. I believe we agree on this approach (i.e., using directory naming > to establish ordering). Correct me if I'm wrong. > > So, to move forward, let me outline the possible approaches we can take: > > Option (a). Drop 'num_lines' attribute and: > > (a-1). Impose strict naming rule for line directories > > Users can only create directories with a predefined naming > convention. This could be 'line0', 'line1', ... 'line<Y>' (as in > the RFC implementation), or simply '0', '1', ..., '<Y>'. > > (a-2). Allow arbitrary naming for line directories > > This would require a well-defined rule to avoid ambiguity. As > Maciej pointed out: > > (from https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > > if free form names were for e.g. [1, 02, 10] > > Users would need to understand these rules, which might impose > unnecessary burden on users. > > Option (b). Keep 'num_lines' attribute but: > > (b-1). Prohibit manual creation of line directories > > Users would no longer run 'mkdir line0', etc. Instead, writing > <Y+1> (Y >= 0) to 'num_lines' would automatically set up the > required directories. convention. This could be 'line0', > 'line1', ... 'line<Y>' (as in this RFC implementation), or > simply '0', '1', ..., '<Y>'. > > (b-2). Keep manual 'mkdir' for each line, in the same manner as (a-1) > (as in the RFC implementation) or (a-2). Seems that no-one is > favor of this option. > > > Note: (b-1) is a new idea. Considering what really needs to be > configured by users, this could be the least burdensome and simplest, > especially when configuring many lines. I'm including it here for > broader discussion. > > Personally, now I'm inclined towards (a-1) with the simplest naming scheme: > non-zero-padded integers ('./0', './1', './2', ..., './<Y>'). Or even (b-1). > I too think a-1 is the best option. However, I'd go for line0, line1 etc. convention as for computers it doesn't make any difference while for humans it's more readable. Bartosz > Let me know your thoughts. > > Thanks. > > -Koichiro > > > > > Bart
On Fri, Jan 31, 2025 at 06:22:50PM GMT, Bartosz Golaszewski wrote: > On Fri, Jan 31, 2025 at 3:36 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > On Thu, Jan 30, 2025 at 09:47:47PM GMT, Bartosz Golaszewski wrote: > > > On Thu, Jan 30, 2025 at 7:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > > (Small reply to the previous comment:) > > Yes, I understand your point about why you think 'num_lines' is > > unnecessary. What I meant was more of a UX (User eXperience) consideration. > > > > If anything this sounds like worse user experience - having to provide > duplicate information. > > > > > 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. [...] > > > > You're right, that's exactly the intention of the strict naming, 'line0', > > 'line1', ..., 'line<Y>'. > > > > > [...] 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? > > > > As Maciej pointed out: > > > > (https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > > > [...] if free form names were for e.g. [1, 02, 10]. > > > > we would need a well-defined rule to avoid ambiguity, which could > > potentially unnecessarily impose burden on users to understand how to > > properly use the interface. > > > > > > Regardless, the point is that we need to make it clear to users which GPIO > > line a specific line<Y> of an aggregator forwards operations to. Since > > requiring users to explicitly set the offset within the aggregator for each > > virtual line (e.g. besides 'key'/'offset'/'name' attributes, by adding > > 'idx' attribute, which users would need to set explicitly) would be > > cumbersome, this RFC implementation instead just makes use of directory > > naming. I believe we agree on this approach (i.e., using directory naming > > to establish ordering). Correct me if I'm wrong. > > > > So, to move forward, let me outline the possible approaches we can take: > > > > Option (a). Drop 'num_lines' attribute and: > > > > (a-1). Impose strict naming rule for line directories > > > > Users can only create directories with a predefined naming > > convention. This could be 'line0', 'line1', ... 'line<Y>' (as in > > the RFC implementation), or simply '0', '1', ..., '<Y>'. > > > > (a-2). Allow arbitrary naming for line directories > > > > This would require a well-defined rule to avoid ambiguity. As > > Maciej pointed out: > > > > (from https://lore.kernel.org/all/CAFGk_a0U=jSQD85UKC1e=pSWr8W9y_MMAFyPVFOcE-fUZry7-Q@mail.gmail.com/#t) > > > if free form names were for e.g. [1, 02, 10] > > > > Users would need to understand these rules, which might impose > > unnecessary burden on users. > > > > Option (b). Keep 'num_lines' attribute but: > > > > (b-1). Prohibit manual creation of line directories > > > > Users would no longer run 'mkdir line0', etc. Instead, writing > > <Y+1> (Y >= 0) to 'num_lines' would automatically set up the > > required directories. convention. This could be 'line0', > > 'line1', ... 'line<Y>' (as in this RFC implementation), or > > simply '0', '1', ..., '<Y>'. > > > > (b-2). Keep manual 'mkdir' for each line, in the same manner as (a-1) > > (as in the RFC implementation) or (a-2). Seems that no-one is > > favor of this option. > > > > > > Note: (b-1) is a new idea. Considering what really needs to be > > configured by users, this could be the least burdensome and simplest, > > especially when configuring many lines. I'm including it here for > > broader discussion. > > > > Personally, now I'm inclined towards (a-1) with the simplest naming scheme: > > non-zero-padded integers ('./0', './1', './2', ..., './<Y>'). Or even (b-1). > > > > I too think a-1 is the best option. However, I'd go for line0, line1 > etc. convention as for computers it doesn't make any difference while > for humans it's more readable. Thank you for the comments. I’ll address your feedbacks and send "v2" later, Since we seem to agree on the overall approach, I’ll send it as [PATCH v1] (i.e., without the "RFC"). -Koichiro > > Bartosz > > > Let me know your thoughts. > > > > Thanks. > > > > -Koichiro > > > > > > > > Bart
On Sat, Feb 1, 2025 at 1:26 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > On Fri, Jan 31, 2025 at 06:22:50PM GMT, Bartosz Golaszewski wrote: > > > > I too think a-1 is the best option. However, I'd go for line0, line1 > > etc. convention as for computers it doesn't make any difference while > > for humans it's more readable. > > Thank you for the comments. I’ll address your feedbacks and send "v2" > later, Since we seem to agree on the overall approach, I’ll send it as > [PATCH v1] (i.e., without the "RFC"). > No, please don't. It'll be confusing. The RFC WAS the v1. Next iteration must be v2 with a changelog. Bartosz