linux-kernel - Re: [PATCH v5] hid-logitech-hidpp: read battery voltage from newer devices

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: <cb3e7c8f-b8f9-7a59-d358-8682625e4adf@pedrovanzella.com>
Date:   Fri, 13 Sep 2019 17:45:45 -0400
From:   Pedro Vanzella <pedro@...rovanzella.com>
To:     Filipe Laíns <lains@...hlinux.org>,
        linux-input@...r.kernel.org
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] hid-logitech-hidpp: read battery voltage from newer
 devices

On 9/9/19 8:00 AM, Filipe Laíns wrote:
> On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote:
>> +static int hidpp20_battery_map_status_voltage(u8 data[3], int
>> *voltage)
>> +{
>> +	int status;
>> +
>> +	switch (data[2]) {
>> +	case 0x00: /* discharging */
>> +		status = POWER_SUPPLY_STATUS_DISCHARGING;
>> +		break;
>> +	case 0x10: /* wireless charging */
>> +	case 0x80: /* charging */
>> +		status = POWER_SUPPLY_STATUS_CHARGING;
>> +		break;
>> +	case 0x81: /* fully charged */
>> +		status = POWER_SUPPLY_STATUS_FULL;
>> +		break;
>> +	default:
>> +		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +	}
>> +
>> +	*voltage = get_unaligned_be16(data);
>> +
>> +	return status;
>> +}
> 
> Here's the missing specification:
> 
> +--------+-------------------------------------------------------------------------+
> |  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
> 
> I know this is already on the 5th revision but could you please change
> hidpp20_battery_map_status_voltage() to properly handle the 3rd byte?
> Also, if you could make sure those values are properly exported via
> sysfs it would be great! We will need them for proper upower support.

Sure thing. I'll have a new patch in a few days.

Thanks for figuring that stuff out, by the way.

> 
> Sorry for not saying this in my first review. Since then I have done
> some testing and I discovered that if we want to get accurate upower
> support we will need the values exposed by the 3rd byte to be
> exported properly.
> 
> I am not sure about the endianness of chargeStatus but you can find
> that easily.
> 
> Thank you!
> Filipe Laíns
>