linux-kernel - Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <e3bdfa16584d7ec832414dcb854ee4d2582543b3.camel@riseup.net>
Date:   Fri, 23 Jul 2021 20:42:32 +0100
From:   Filipe Laíns <lains@...eup.net>
To:     Hamza Mahfooz <someguy@...ective-light.com>,
        linux-kernel@...r.kernel.org
Cc:     Bastien Nocera <hadess@...ess.net>, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org
Subject: Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY
 property for newer devices

On Fri, 2021-07-23 at 14:57 -0400, Hamza Mahfooz wrote:
> For devices that only support the BATTERY_VOLTAGE (0x1001) feature, UPower
> requires the additional information provided by this patch, to set them up.
> 
> Signed-off-by: Hamza Mahfooz <someguy@...ective-light.com>
> ---
> 
> v2: use ARRAY_SIZE() and set voltages[]'s size to 100
> 
> v3: add a check to ensure that exactly 100 elements are in voltages[]
> ---
>  drivers/hid/hid-logitech-hidpp.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 61635e629469..4921823144de 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1331,6 +1331,33 @@ static int hidpp20_battery_get_battery_voltage(struct
> hidpp_device *hidpp,
>         return 0;
>  }
>  
> +static int hidpp20_map_battery_capacity(int voltage)
> +{
> +       static const int voltages[] = {
> +               4186, 4156, 4143, 4133, 4122, 4113, 4103, 4094, 4086, 4075,
> +               4067, 4059, 4051, 4043, 4035, 4027, 4019, 4011, 4003, 3997,
> +               3989, 3983, 3976, 3969, 3961, 3955, 3949, 3942, 3935, 3929,
> +               3922, 3916, 3909, 3902, 3896, 3890, 3883, 3877, 3870, 3865,
> +               3859, 3853, 3848, 3842, 3837, 3833, 3828, 3824, 3819, 3815,
> +               3811, 3808, 3804, 3800, 3797, 3793, 3790, 3787, 3784, 3781,
> +               3778, 3775, 3772, 3770, 3767, 3764, 3762, 3759, 3757, 3754,
> +               3751, 3748, 3744, 3741, 3737, 3734, 3730, 3726, 3724, 3720,
> +               3717, 3714, 3710, 3706, 3702, 3697, 3693, 3688, 3683, 3677,
> +               3671, 3666, 3662, 3658, 3654, 3646, 3633, 3612, 3579, 3537
> +       };

Hi Hamza,

I think it's important to note that this table depends on the battery technology
(type). While most devices use this map, not all do. I think it's reasonable to
just assume this is the map as we don't really have the capability to test all
products and the products that do not use this map are very few.

That said, I think we should definitely have a comment here nothing that, and
possible have some bounds checks for the reported voltage value hinting that
there may be bug.

Cheers,
Filipe Laíns

> +
> +       int i;
> +
> +       BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
> +
> +       for (i = 0; i < ARRAY_SIZE(voltages); i++) {
> +               if (voltage >= voltages[i])
> +                       return ARRAY_SIZE(voltages) - i;
> +       }
> +
> +       return 0;
> +}
> +
>  static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
>  {
>         u8 feature_type;
> @@ -1354,6 +1381,7 @@ static int hidpp20_query_battery_voltage_info(struct
> hidpp_device *hidpp)
>  
>         hidpp->battery.status = status;
>         hidpp->battery.voltage = voltage;
> +       hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
>         hidpp->battery.level = level;
>         hidpp->battery.charge_type = charge_type;
>         hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
> @@ -1378,6 +1406,7 @@ static int hidpp20_battery_voltage_event(struct
> hidpp_device *hidpp,
>  
>         if (voltage != hidpp->battery.voltage || status != hidpp-
> >battery.status) {
>                 hidpp->battery.voltage = voltage;
> +               hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
>                 hidpp->battery.status = status;
>                 hidpp->battery.level = level;
>                 hidpp->battery.charge_type = charge_type;
> @@ -3717,7 +3746,8 @@ static int hidpp_initialize_battery(struct hidpp_device
> *hidpp)
>         num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
>  
>         if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
> -           hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE)
> +           hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
> +           hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
>                 battery_props[num_battery_props++] =
>                                 POWER_SUPPLY_PROP_CAPACITY;
>  

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