| Message ID | 20220824165459.1.I059ae712dd6d324897162ee9f37c22849aa22745@changeid (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: core: Ignore -EIO for uevent | expand |
Hi, On Wed, Aug 24, 2022 at 04:57:20PM -0700, Brian Norris wrote: > For uevents, we enumerate all properties. Some battery implementations > don't implement all standard properties, and may return -EIO for > properties that aren't recognized. This means we never report uevents > for such batteries. > > It's better to ignore these errors and skip the property, as we do with > ENODATA and ENODEV. > > Example battery implementation: Acer Chromebook Tab 10 (a.k.a. Google > Gru-Scarlet) has a virtual "SBS" battery implementation in its Embedded > Controller on top of an otherwise non-SBS battery. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- -EIO means input/output error. If a driver is reporting that for an unimplemented feature it's a bug that should be fixed in the driver. Handling it here means userspace ABI changes for temporary issues. -- Sebastian > drivers/power/supply/power_supply_sysfs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index 4239591e1522..36fce572a213 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -439,10 +439,12 @@ static int add_prop_uevent(struct device *dev, struct kobj_uevent_env *env, > dev_attr = &pwr_attr->dev_attr; > > ret = power_supply_show_property(dev, dev_attr, prop_buf); > - if (ret == -ENODEV || ret == -ENODATA) { > + if (ret == -ENODEV || ret == -ENODATA || ret == -EIO) { > /* > * When a battery is absent, we expect -ENODEV. Don't abort; > - * send the uevent with at least the the PRESENT=0 property > + * send the uevent with at least the PRESENT=0 property. Some > + * batteries also report EIO, even for some standard > + * properties. > */ > return 0; > } > -- > 2.37.2.672.g94769d06f0-goog >
Hi Sebastian, Thanks for the response. On Thu, Aug 25, 2022 at 04:02:43PM +0200, Sebastian Reichel wrote: > > For uevents, we enumerate all properties. Some battery implementations > > don't implement all standard properties, and may return -EIO for > > properties that aren't recognized. This means we never report uevents > > for such batteries. > > > > It's better to ignore these errors and skip the property, as we do with > > ENODATA and ENODEV. > > > > Example battery implementation: Acer Chromebook Tab 10 (a.k.a. Google > > Gru-Scarlet) has a virtual "SBS" battery implementation in its Embedded > > Controller on top of an otherwise non-SBS battery. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > -EIO means input/output error. If a driver is reporting that for an > unimplemented feature it's a bug that should be fixed in the driver. > Handling it here means userspace ABI changes for temporary issues. I suppose I can agree with your last sentence. But the first part is much easier said than done. This is sbs-battery.c, on top of i2c-cros-ec-tunnel.c, talking to an EC (whose firmware is pretty much unchangeable at this point), which implements a subset of commands. The intention is that i2c-cros-ec-tunnel.c will see something like a NAK / "invalid argument" response, and it converts that to ENXIO. Unforunately, for reasons I have yet to figure out, it's very common for retries (|i2c_retry_count|) to eventually yield an unexpected response size, which i2c_smbus_xfer_emulated() treats as EIO; so this layer is seeing EIO. Anyway, I might be able to coax the i2c/sbs-battery driver to return ENXIO instead. Would you consider that to be a better case to handle here? "No such device or address" seems like an appropriate description of a permanent error, and not a temporary IO error. Brian
Hi, On Thu, Aug 25, 2022 at 06:11:09PM -0700, Brian Norris wrote: > Hi Sebastian, > > Thanks for the response. > > On Thu, Aug 25, 2022 at 04:02:43PM +0200, Sebastian Reichel wrote: > > > For uevents, we enumerate all properties. Some battery implementations > > > don't implement all standard properties, and may return -EIO for > > > properties that aren't recognized. This means we never report uevents > > > for such batteries. > > > > > > It's better to ignore these errors and skip the property, as we do with > > > ENODATA and ENODEV. > > > > > > Example battery implementation: Acer Chromebook Tab 10 (a.k.a. Google > > > Gru-Scarlet) has a virtual "SBS" battery implementation in its Embedded > > > Controller on top of an otherwise non-SBS battery. > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > --- > > > > -EIO means input/output error. If a driver is reporting that for an > > unimplemented feature it's a bug that should be fixed in the driver. > > Handling it here means userspace ABI changes for temporary issues. > > I suppose I can agree with your last sentence. > > But the first part is much easier said than done. This is sbs-battery.c, > on top of i2c-cros-ec-tunnel.c, talking to an EC (whose firmware is > pretty much unchangeable at this point), which implements a subset of > commands. > > The intention is that i2c-cros-ec-tunnel.c will see something like a NAK > / "invalid argument" response, and it converts that to ENXIO. > Unforunately, for reasons I have yet to figure out, it's very common for > retries (|i2c_retry_count|) to eventually yield an unexpected response > size, which i2c_smbus_xfer_emulated() treats as EIO; so this layer is > seeing EIO. > > Anyway, I might be able to coax the i2c/sbs-battery driver to return > ENXIO instead. Would you consider that to be a better case to handle > here? "No such device or address" seems like an appropriate description > of a permanent error, and not a temporary IO error. > > Brian The device is obviously not fully implementing the SBS standard, so I think the best is to create a new compatible value. Then the driver can register a different set of properties based on that and you never run into any error at all and userspace knows what to expect. -- Sebastian
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index 4239591e1522..36fce572a213 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -439,10 +439,12 @@ static int add_prop_uevent(struct device *dev, struct kobj_uevent_env *env, dev_attr = &pwr_attr->dev_attr; ret = power_supply_show_property(dev, dev_attr, prop_buf); - if (ret == -ENODEV || ret == -ENODATA) { + if (ret == -ENODEV || ret == -ENODATA || ret == -EIO) { /* * When a battery is absent, we expect -ENODEV. Don't abort; - * send the uevent with at least the the PRESENT=0 property + * send the uevent with at least the PRESENT=0 property. Some + * batteries also report EIO, even for some standard + * properties. */ return 0; }
For uevents, we enumerate all properties. Some battery implementations don't implement all standard properties, and may return -EIO for properties that aren't recognized. This means we never report uevents for such batteries. It's better to ignore these errors and skip the property, as we do with ENODATA and ENODEV. Example battery implementation: Acer Chromebook Tab 10 (a.k.a. Google Gru-Scarlet) has a virtual "SBS" battery implementation in its Embedded Controller on top of an otherwise non-SBS battery. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/power/supply/power_supply_sysfs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)