linux-kernel - Re: [PATCH] power: supply: core: Ignore -EIO for uevent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220916222521.hxkgml5pslc6ln5v@mercury.elektranox.org>
Date:   Sat, 17 Sep 2022 00:25:21 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Brian Norris <briannorris@...omium.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH] power: supply: core: Ignore -EIO for uevent

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@...omium.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)