power: supply: core: Ignore -EIO for uevent

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(-)

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