| Message ID | 20240908192303.151562-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: Add new "charge_types" property | expand |
Hello Hans, On Sun, Sep 08, 2024 at 09:23:00PM GMT, Hans de Goede wrote: > Some enum style power-supply properties have text-values / labels for some > of the enum values containing a space, e.g. "Long Life" for > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. > > Make power_supply_show_enum_with_available() surround these label with "" > when the label is not for the active enum value to make it clear that this > is a single label and not 2 different labels for 2 different enum values. > > After this the output for a battery which support "Long Life" will be e.g.: > > Fast [Standard] "Long Life" > > or: > > Fast Standard [Long Life] > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- This looks annoying from parsing point of view. Maybe we can just replace " " with "_" and guarantee that space is a value separator at the cost of the values not being exactly the same as the existing charge_type sysfs file? Do you know if there is prior examples for this in the kernel by chance? Greetings, -- Sebastian > drivers/power/supply/power_supply_sysfs.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index 16b3c5880cd8..ac42784eab11 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -253,7 +253,10 @@ static ssize_t power_supply_show_enum_with_available( > count += sysfs_emit_at(buf, count, "[%s] ", labels[i]); > match = true; > } else if (available) { > - count += sysfs_emit_at(buf, count, "%s ", labels[i]); > + if (strchr(labels[i], ' ')) > + count += sysfs_emit_at(buf, count, "\"%s\" ", labels[i]); > + else > + count += sysfs_emit_at(buf, count, "%s ", labels[i]); > } > } > > -- > 2.46.0 > >
Hi Sebastian, On 9/17/24 9:38 AM, Sebastian Reichel wrote: > Hello Hans, > > On Sun, Sep 08, 2024 at 09:23:00PM GMT, Hans de Goede wrote: >> Some enum style power-supply properties have text-values / labels for some >> of the enum values containing a space, e.g. "Long Life" for >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. >> >> Make power_supply_show_enum_with_available() surround these label with "" >> when the label is not for the active enum value to make it clear that this >> is a single label and not 2 different labels for 2 different enum values. >> >> After this the output for a battery which support "Long Life" will be e.g.: >> >> Fast [Standard] "Long Life" >> >> or: >> >> Fast Standard [Long Life] >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- > > This looks annoying from parsing point of view. Maybe we can just > replace " " with "_" and guarantee that space is a value separator > at the cost of the values not being exactly the same as the existing > charge_type sysfs file? My thinking here was that if a parser wants to see if a certain option is available it can just do a strstr() and parsing for the active value does not change. But yes a parser which wants to tokenize the string to get all possible values as separate tokens will become harder to write with this. I did consider moving to using a "_" one problem there is that this means that echo-ing "Long_Life" to set the value should then work. Which would require special handling in the generic store() function. I guess we could makle I guess an easy solution here would be to define a second set of POWER_SUPPLY_CHARGE_TYPE_TEXT[] strings aptly named POWER_SUPPLY_CHARGE_TYPES_TEXT[] (with the extra s). This can then simply contain Long_Life instead of Long Life, downside of this would be that writing "Long Life" will then not work. So charge_type then takes "Long Life" and charge_types "Long_Life" which is less then ideal. The best I can come up with is to replace " " with _ when showing and in power_supply_store_property() add some special handling for charge_types like this: /* Accept "Long_Life" as alt for "Long Life" for "charge_types" */ if (dev_attr_psp(attr) == POWER_SUPPLY_PROP_CHARGE_TYPES && sysfs_streq(buf, "Long_Life")) buf = "Long Life"; ret = -EINVAL; if (ps_attr->text_values_len > 0) { ret = __sysfs_match_string(ps_attr->text_values, ps_attr->text_values_len, buf); } This isn't pretty, but this way we don't need to define a second set of POWER_SUPPLY_CHARGE_TYPES_TEXT[] values, duplicating those (and needing to manually keep them in sync), while accepting both "Long Life" and "Long_Life". Note that a generic replacing of _ with space or the other way around in store() will not work because we already allow/use "Not Charging" but also "PD_DRP" so replacing either _ or space with the other will break one of those. > Do you know if there is prior examples for this in the kernel by > chance? I'm not aware of any examples where a sysfs attr show() uses the show all available values with the active one surrounding by [ ] where one of the values has a space in it. It is quite easy to avoid the values having spaces if this format is used from the start. Regards, Hans
Hi, On Tue, Sep 17, 2024 at 11:06:42AM GMT, Hans de Goede wrote: > Hi Sebastian, > > On 9/17/24 9:38 AM, Sebastian Reichel wrote: > > Hello Hans, > > > > On Sun, Sep 08, 2024 at 09:23:00PM GMT, Hans de Goede wrote: > >> Some enum style power-supply properties have text-values / labels for some > >> of the enum values containing a space, e.g. "Long Life" for > >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. > >> > >> Make power_supply_show_enum_with_available() surround these label with "" > >> when the label is not for the active enum value to make it clear that this > >> is a single label and not 2 different labels for 2 different enum values. > >> > >> After this the output for a battery which support "Long Life" will be e.g.: > >> > >> Fast [Standard] "Long Life" > >> > >> or: > >> > >> Fast Standard [Long Life] > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > > > > This looks annoying from parsing point of view. Maybe we can just > > replace " " with "_" and guarantee that space is a value separator > > at the cost of the values not being exactly the same as the existing > > charge_type sysfs file? > > My thinking here was that if a parser wants to see if a certain option > is available it can just do a strstr() and parsing for the active > value does not change. But yes a parser which wants to tokenize > the string to get all possible values as separate tokens will > become harder to write with this. > > I did consider moving to using a "_" one problem there is that > this means that echo-ing "Long_Life" to set the value should then > work. Which would require special handling in the generic store() > function. I guess we could makle > > I guess an easy solution here would be to define a second set > of POWER_SUPPLY_CHARGE_TYPE_TEXT[] strings aptly named > POWER_SUPPLY_CHARGE_TYPES_TEXT[] (with the extra s). > > This can then simply contain Long_Life instead of Long Life, > downside of this would be that writing "Long Life" will then > not work. So charge_type then takes "Long Life" and charge_types > "Long_Life" which is less then ideal. Also having two lists sounds error prone regarding going out of sync. > The best I can come up with is to replace " " with _ when showing > and in power_supply_store_property() add some special handling for > charge_types like this: > > /* Accept "Long_Life" as alt for "Long Life" for "charge_types" */ > if (dev_attr_psp(attr) == POWER_SUPPLY_PROP_CHARGE_TYPES && > sysfs_streq(buf, "Long_Life")) > buf = "Long Life"; > > ret = -EINVAL; > if (ps_attr->text_values_len > 0) { > ret = __sysfs_match_string(ps_attr->text_values, > ps_attr->text_values_len, buf); > } That's ugly, but better than maintaining a second list IMHO. > This isn't pretty, but this way we don't need to define a second set of > POWER_SUPPLY_CHARGE_TYPES_TEXT[] values, duplicating those (and needing > to manually keep them in sync), while accepting both "Long Life" and > "Long_Life". > > Note that a generic replacing of _ with space or the other way around > in store() will not work because we already allow/use "Not Charging" > but also "PD_DRP" so replacing either _ or space with the other will > break one of those. We only replace " " with "_" on the output side for the code doing multi-value printing. I think on the write side accepting "Not_Charging" in addition to "Not Charging" is probably fine as long as we document that? > > Do you know if there is prior examples for this in the kernel by > > chance? > > I'm not aware of any examples where a sysfs attr show() uses the show > all available values with the active one surrounding by [ ] where > one of the values has a space in it. It is quite easy to avoid the > values having spaces if this format is used from the start. Then let's please avoid that :) P.S.: Last message before my 3 week vacation on this patchset. Thomas Weißschuh said, that he might have some ideas and will sync with you next week. Greetings, -- Sebastian
Hi, On 20-Sep-24 3:52 PM, Sebastian Reichel wrote: > Hi, > > On Tue, Sep 17, 2024 at 11:06:42AM GMT, Hans de Goede wrote: >> Hi Sebastian, >> >> On 9/17/24 9:38 AM, Sebastian Reichel wrote: >>> Hello Hans, >>> >>> On Sun, Sep 08, 2024 at 09:23:00PM GMT, Hans de Goede wrote: >>>> Some enum style power-supply properties have text-values / labels for some >>>> of the enum values containing a space, e.g. "Long Life" for >>>> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. >>>> >>>> Make power_supply_show_enum_with_available() surround these label with "" >>>> when the label is not for the active enum value to make it clear that this >>>> is a single label and not 2 different labels for 2 different enum values. >>>> >>>> After this the output for a battery which support "Long Life" will be e.g.: >>>> >>>> Fast [Standard] "Long Life" >>>> >>>> or: >>>> >>>> Fast Standard [Long Life] >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>> >>> This looks annoying from parsing point of view. Maybe we can just >>> replace " " with "_" and guarantee that space is a value separator >>> at the cost of the values not being exactly the same as the existing >>> charge_type sysfs file? >> >> My thinking here was that if a parser wants to see if a certain option >> is available it can just do a strstr() and parsing for the active >> value does not change. But yes a parser which wants to tokenize >> the string to get all possible values as separate tokens will >> become harder to write with this. >> >> I did consider moving to using a "_" one problem there is that >> this means that echo-ing "Long_Life" to set the value should then >> work. Which would require special handling in the generic store() >> function. I guess we could makle >> >> I guess an easy solution here would be to define a second set >> of POWER_SUPPLY_CHARGE_TYPE_TEXT[] strings aptly named >> POWER_SUPPLY_CHARGE_TYPES_TEXT[] (with the extra s). >> >> This can then simply contain Long_Life instead of Long Life, >> downside of this would be that writing "Long Life" will then >> not work. So charge_type then takes "Long Life" and charge_types >> "Long_Life" which is less then ideal. > > Also having two lists sounds error prone regarding going out of > sync. > >> The best I can come up with is to replace " " with _ when showing >> and in power_supply_store_property() add some special handling for >> charge_types like this: >> >> /* Accept "Long_Life" as alt for "Long Life" for "charge_types" */ >> if (dev_attr_psp(attr) == POWER_SUPPLY_PROP_CHARGE_TYPES && >> sysfs_streq(buf, "Long_Life")) >> buf = "Long Life"; >> >> ret = -EINVAL; >> if (ps_attr->text_values_len > 0) { >> ret = __sysfs_match_string(ps_attr->text_values, >> ps_attr->text_values_len, buf); >> } > > That's ugly, but better than maintaining a second list IMHO. > >> This isn't pretty, but this way we don't need to define a second set of >> POWER_SUPPLY_CHARGE_TYPES_TEXT[] values, duplicating those (and needing >> to manually keep them in sync), while accepting both "Long Life" and >> "Long_Life". >> >> Note that a generic replacing of _ with space or the other way around >> in store() will not work because we already allow/use "Not Charging" >> but also "PD_DRP" so replacing either _ or space with the other will >> break one of those. > > We only replace " " with "_" on the output side for the code doing > multi-value printing. I think on the write side accepting "Not_Charging" > in addition to "Not Charging" is probably fine as long as we document > that? Right, the problem is how do we implement this since we rely on __sysfs_match_string(), first try __sysfs_match_string() then on failure, if input buf contains a space, copy the input buf, replace space with _ in input buf and try again ? That would be more generic then my hardcoded suggestion above, so I'll take a shot at going that route for v2, when I can make some time to work on v2. >>> Do you know if there is prior examples for this in the kernel by >>> chance? >> >> I'm not aware of any examples where a sysfs attr show() uses the show >> all available values with the active one surrounding by [ ] where >> one of the values has a space in it. It is quite easy to avoid the >> values having spaces if this format is used from the start. > > Then let's please avoid that :) > > P.S.: Last message before my 3 week vacation on this patchset. > Thomas Weißschuh said, that he might have some ideas and will sync > with you next week. Ok, enjoy your vacation. Regards, Hans
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index 16b3c5880cd8..ac42784eab11 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -253,7 +253,10 @@ static ssize_t power_supply_show_enum_with_available( count += sysfs_emit_at(buf, count, "[%s] ", labels[i]); match = true; } else if (available) { - count += sysfs_emit_at(buf, count, "%s ", labels[i]); + if (strchr(labels[i], ' ')) + count += sysfs_emit_at(buf, count, "\"%s\" ", labels[i]); + else + count += sysfs_emit_at(buf, count, "%s ", labels[i]); } }
Some enum style power-supply properties have text-values / labels for some of the enum values containing a space, e.g. "Long Life" for POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. Make power_supply_show_enum_with_available() surround these label with "" when the label is not for the active enum value to make it clear that this is a single label and not 2 different labels for 2 different enum values. After this the output for a battery which support "Long Life" will be e.g.: Fast [Standard] "Long Life" or: Fast Standard [Long Life] Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/power_supply_sysfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)