| Message ID | 20241111-power-supply-extensions-v4-1-7240144daa8e@weissschuh.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: extension API | expand |
Am 11.11.24 um 22:40 schrieb Thomas Weißschuh: > Currently an uevent contains the same string representation of a > property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this > is specially formatted to indicate all possible values. > This doesn't make sense for uevents and complicates parsing. > Instead only include the currently active value in uevents. > > As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR > this change is not a problem. > Soon the property will actually be used so fix the formatting before > that happens. AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, so we have to make sure that no userspace application uses this uevent information. Other than that: Reviewed-by: Armin Wolf <W_Armin@gmx.de> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available( > return count; > } > > -static ssize_t power_supply_show_property(struct device *dev, > - struct device_attribute *attr, > - char *buf) { > +static ssize_t power_supply_format_property(struct device *dev, > + bool uevent, > + struct device_attribute *attr, > + char *buf) > +{ > ssize_t ret; > struct power_supply *psy = dev_get_drvdata(dev); > const struct power_supply_attr *ps_attr = to_ps_attr(attr); > @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev, > psy->desc->usb_types, value.intval, buf); > break; > case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > + if (uevent) /* no possible values in uevents */ > + goto default_format; > ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, > value.intval, buf); > break; > @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev, > ret = sysfs_emit(buf, "%s\n", value.strval); > break; > default: > +default_format: > if (ps_attr->text_values_len > 0 && > value.intval < ps_attr->text_values_len && value.intval >= 0) { > ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); > @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev, > return ret; > } > > +static ssize_t power_supply_show_property(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return power_supply_format_property(dev, false, attr, buf); > +} > + > static ssize_t power_supply_store_property(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t count) { > @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env > pwr_attr = &power_supply_attrs[prop]; > dev_attr = &pwr_attr->dev_attr; > > - ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf); > + ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf); > if (ret == -ENODEV || ret == -ENODATA) { > /* > * When a battery is absent, we expect -ENODEV. Don't abort; >
Nov 24, 2024 18:47:39 Armin Wolf <W_Armin@gmx.de>: > Am 11.11.24 um 22:40 schrieb Thomas Weißschuh: > >> Currently an uevent contains the same string representation of a >> property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this >> is specially formatted to indicate all possible values. >> This doesn't make sense for uevents and complicates parsing. >> Instead only include the currently active value in uevents. >> >> As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR >> this change is not a problem. >> Soon the property will actually be used so fix the formatting before >> that happens. > > AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, > so we have to make sure that no userspace application uses this uevent information. It only does so after this series. Currently it uses the ad-hoc sysfs attributes which don't show up in uevent. (I'm the maintainer) > > Other than that: > > Reviewed-by: Armin Wolf <W_Armin@gmx.de> Thanks! > >> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> >> --- >> drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >> index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644 >> --- a/drivers/power/supply/power_supply_sysfs.c >> +++ b/drivers/power/supply/power_supply_sysfs.c >> @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available( >> return count; >> } >> >> -static ssize_t power_supply_show_property(struct device *dev, >> - struct device_attribute *attr, >> - char *buf) { >> +static ssize_t power_supply_format_property(struct device *dev, >> + bool uevent, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> ssize_t ret; >> struct power_supply *psy = dev_get_drvdata(dev); >> const struct power_supply_attr *ps_attr = to_ps_attr(attr); >> @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev, >> psy->desc->usb_types, value.intval, buf); >> break; >> case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: >> + if (uevent) /* no possible values in uevents */ >> + goto default_format; >> ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, >> value.intval, buf); >> break; >> @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev, >> ret = sysfs_emit(buf, "%s\n", value.strval); >> break; >> default: >> +default_format: >> if (ps_attr->text_values_len > 0 && >> value.intval < ps_attr->text_values_len && value.intval >= 0) { >> ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); >> @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev, >> return ret; >> } >> >> +static ssize_t power_supply_show_property(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + return power_supply_format_property(dev, false, attr, buf); >> +} >> + >> static ssize_t power_supply_store_property(struct device *dev, >> struct device_attribute *attr, >> const char *buf, size_t count) { >> @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env >> pwr_attr = &power_supply_attrs[prop]; >> dev_attr = &pwr_attr->dev_attr; >> >> - ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf); >> + ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf); >> if (ret == -ENODEV || ret == -ENODATA) { >> /* >> * When a battery is absent, we expect -ENODEV. Don't abort; >>
Am 24.11.24 um 18:55 schrieb Thomas Weißschuh: > Nov 24, 2024 18:47:39 Armin Wolf <W_Armin@gmx.de>: > >> Am 11.11.24 um 22:40 schrieb Thomas Weißschuh: >> >>> Currently an uevent contains the same string representation of a >>> property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this >>> is specially formatted to indicate all possible values. >>> This doesn't make sense for uevents and complicates parsing. >>> Instead only include the currently active value in uevents. >>> >>> As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR >>> this change is not a problem. >>> Soon the property will actually be used so fix the formatting before >>> that happens. >> AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, >> so we have to make sure that no userspace application uses this uevent information. > It only does so after this series. > Currently it uses the ad-hoc sysfs attributes which don't show up in uevent. > (I'm the maintainer) I see, in this case you can ignore my comment. Thank, Armin Wolf >> Other than that: >> >> Reviewed-by: Armin Wolf <W_Armin@gmx.de> > Thanks! > >>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> >>> --- >>> drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++---- >>> 1 file changed, 16 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >>> index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644 >>> --- a/drivers/power/supply/power_supply_sysfs.c >>> +++ b/drivers/power/supply/power_supply_sysfs.c >>> @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available( >>> return count; >>> } >>> >>> -static ssize_t power_supply_show_property(struct device *dev, >>> - struct device_attribute *attr, >>> - char *buf) { >>> +static ssize_t power_supply_format_property(struct device *dev, >>> + bool uevent, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> ssize_t ret; >>> struct power_supply *psy = dev_get_drvdata(dev); >>> const struct power_supply_attr *ps_attr = to_ps_attr(attr); >>> @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev, >>> psy->desc->usb_types, value.intval, buf); >>> break; >>> case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: >>> + if (uevent) /* no possible values in uevents */ >>> + goto default_format; >>> ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, >>> value.intval, buf); >>> break; >>> @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev, >>> ret = sysfs_emit(buf, "%s\n", value.strval); >>> break; >>> default: >>> +default_format: >>> if (ps_attr->text_values_len > 0 && >>> value.intval < ps_attr->text_values_len && value.intval >= 0) { >>> ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); >>> @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev, >>> return ret; >>> } >>> >>> +static ssize_t power_supply_show_property(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + return power_supply_format_property(dev, false, attr, buf); >>> +} >>> + >>> static ssize_t power_supply_store_property(struct device *dev, >>> struct device_attribute *attr, >>> const char *buf, size_t count) { >>> @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env >>> pwr_attr = &power_supply_attrs[prop]; >>> dev_attr = &pwr_attr->dev_attr; >>> >>> - ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf); >>> + ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf); >>> if (ret == -ENODEV || ret == -ENODATA) { >>> /* >>> * When a battery is absent, we expect -ENODEV. Don't abort; >>>
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available( return count; } -static ssize_t power_supply_show_property(struct device *dev, - struct device_attribute *attr, - char *buf) { +static ssize_t power_supply_format_property(struct device *dev, + bool uevent, + struct device_attribute *attr, + char *buf) +{ ssize_t ret; struct power_supply *psy = dev_get_drvdata(dev); const struct power_supply_attr *ps_attr = to_ps_attr(attr); @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev, psy->desc->usb_types, value.intval, buf); break; case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: + if (uevent) /* no possible values in uevents */ + goto default_format; ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, value.intval, buf); break; @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev, ret = sysfs_emit(buf, "%s\n", value.strval); break; default: +default_format: if (ps_attr->text_values_len > 0 && value.intval < ps_attr->text_values_len && value.intval >= 0) { ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev, return ret; } +static ssize_t power_supply_show_property(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return power_supply_format_property(dev, false, attr, buf); +} + static ssize_t power_supply_store_property(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env pwr_attr = &power_supply_attrs[prop]; dev_attr = &pwr_attr->dev_attr; - ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf); + ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf); if (ret == -ENODEV || ret == -ENODATA) { /* * When a battery is absent, we expect -ENODEV. Don't abort;
Currently an uevent contains the same string representation of a property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this is specially formatted to indicate all possible values. This doesn't make sense for uevents and complicates parsing. Instead only include the currently active value in uevents. As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this change is not a problem. Soon the property will actually be used so fix the formatting before that happens. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)