| 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
| ||
|
Message-ID: <CAO-hwJLXQSjq0J-emUMCu9Z69FmzzQO3Am0E3Xiw6aWBfiEjnA@mail.gmail.com>
Date: Tue, 28 Jan 2020 16:25:33 +0100
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: Pedro Vanzella <pedro@...rovanzella.com>
Cc: Filipe Laíns <lains@...hlinux.org>,
Jiri Kosina <jikos@...nel.org>,
Henrik Rydberg <rydberg@...math.org>,
"open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: logitech-hidpp: BatteryVoltage: only read
chargeStatus if extPower is active
On Mon, Jan 27, 2020 at 10:12 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> Hi,
>
> On Mon, Jan 20, 2020 at 2:43 PM Pedro Vanzella <pedro@...rovanzella.com> wrote:
> >
> > On 1/11/20 4:24 PM, Filipe Laíns wrote:
> > > In the HID++ 2.0 function getBatteryInfo() from the BatteryVoltage
> > > (0x1001) feature, chargeStatus is only valid if extPower is active.
> > >
> > > Previously we were ignoring extPower, which resulted in wrong values.
> >
> > Nice catch. Sorry for missing that the first time around.
> >
> > >
> > > Example:
> > > With an unplugged mouse
> > >
> > > $ cat /sys/class/power_supply/hidpp_battery_0/status
> > > Charging
> >
> > Tested and it works as expected now.
>
> Thanks for the patch and the tests.
>
> Unfortunately, the merge window is already opened, and I'd rather not
> sneak this one right now. This patch doesn't seem very critical so I
> rather not annoy the other maintainers.
> I'll make sure it gets in the 5.6 final by pushing it into a rc when
> things are calmer for everybody.
>
> So the plan would be:
> - wait for the 'normal' 5.6 HID pull request to be sent
> - apply this one in for-5.6/upstream-fixes
> - sent this branch for either 5.6-rc1 or 5.6-rc2
>
> Cheers,
> Benjamin
>
> >
> > >
> > > This patch makes fixes that, it also renames charge_sts to flags as
Fixed the typo: 's/makes //' and pushed to for-5.6/upstream-fixes
Cheers,
Benjamin
> > > charge_sts can be confused with chargeStatus from the spec.
> > >
> > > Spec:
> > > +--------+-------------------------------------------------------------------------+
> > > | byte | 2 |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > | bit | 0..2 | 3 | 4 | 5 | 6 | 7 |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > | buffer | chargeStatus | fastCharge | slowCharge | critical | (unused) | extPower |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > Table 1 - battery voltage (0x1001), getBatteryInfo() (ASE 0), 3rd byte
> > >
> > > +-------+--------------------------------------+
> > > | value | meaning |
> > > +-------+--------------------------------------+
> > > | 0 | Charging |
> > > +-------+--------------------------------------+
> > > | 1 | End of charge (100% charged) |
> > > +-------+--------------------------------------+
> > > | 2 | Charge stopped (any "normal" reason) |
> > > +-------+--------------------------------------+
> > > | 7 | Hardware error |
> > > +-------+--------------------------------------+
> > > Table 2 - chargeStatus value
> > >
> > > Signed-off-by: Filipe Laíns <lains@...hlinux.org>
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 43 ++++++++++++++++----------------
> > > 1 file changed, 21 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index bb063e7d48df..39a5ee0aaab0 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -1256,36 +1256,35 @@ static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage,
> > > {
> > > int status;
> > >
> > > - long charge_sts = (long)data[2];
> > > + long flags = (long) data[2];
> > >
> > > - *level = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
> > > - switch (data[2] & 0xe0) {
> > > - case 0x00:
> > > - status = POWER_SUPPLY_STATUS_CHARGING;
> > > - break;
> > > - case 0x20:
> > > - status = POWER_SUPPLY_STATUS_FULL;
> > > - *level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> > > - break;
> > > - case 0x40:
> > > + if (flags & 0x80)
> > > + switch (flags & 0x07) {
> > > + case 0:
> > > + status = POWER_SUPPLY_STATUS_CHARGING;
> > > + break;
> > > + case 1:
> > > + status = POWER_SUPPLY_STATUS_FULL;
> > > + *level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> > > + break;
> > > + case 2:
> > > + status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > + break;
> > > + default:
> > > + status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > + break;
> > > + }
> > > + else
> > > status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > - break;
> > > - case 0xe0:
> > > - status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > - break;
> > > - default:
> > > - status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > - }
> > >
> > > *charge_type = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> > > - if (test_bit(3, &charge_sts)) {
> > > + if (test_bit(3, &flags)) {
> > > *charge_type = POWER_SUPPLY_CHARGE_TYPE_FAST;
> > > }
> > > - if (test_bit(4, &charge_sts)) {
> > > + if (test_bit(4, &flags)) {
> > > *charge_type = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> > > }
> > > -
> > > - if (test_bit(5, &charge_sts)) {
> > > + if (test_bit(5, &flags)) {
> > > *level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> > > }
> > >
> > >
> >
> > Tested-by: Pedro Vanzella <pedro@...rovanzella.com>
> > Reviewed-by: Pedro Vanzella <pedro@...rovanzella.com>
> >
Powered by blists - more mailing lists