linux-kernel - Re: [PATCH] drivers/hid: Implement a battery status polling mechanism for selected input 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: <pp1pr2rn-7so7-orro-60o7-1o29r7n26q24@xreary.bet>
Date: Tue, 14 Oct 2025 14:26:18 +0200 (CEST)
From: Jiri Kosina <jikos@...nel.org>
To: huangzaiyang <huangzaiyang@...o.com>
cc: bentiss@...nel.org, linux-input@...r.kernel.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/hid: Implement a battery status polling mechanism
 for selected input devices.

On Tue, 24 Jun 2025, huangzaiyang wrote:

> Reading battery capacity and status could fail and end up with timeout
> after 5s for some input devices, for example:8BitDo SN30 Pro+ gamepad.
> Implement a battery status polling mechanism for selected input devices,
> set HID_BATTERY_QUIRK_AVOID_QUERY and HID_BATTERY_QUIRK_POLLING_QUERY
> for 8BitDo SN30 Pro+ gamepad.
> to Avoid UI freezing when reading battery capacity/status.

Thanks for the patch (and sorry for the delay, it fell in between cracks 
unfortunately). Minor nits:

> Signed-off-by: huangzaiyang <huangzaiyang@...o.com>
> ---
>  drivers/hid/hid-input.c | 63 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/hid.h     |  2 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 9d80635a91eb..b113f5c49d03 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -341,6 +341,7 @@ static enum power_supply_property hidinput_battery_props[] = {
>  #define HID_BATTERY_QUIRK_FEATURE      (1 << 1) /* ask for feature report */
>  #define HID_BATTERY_QUIRK_IGNORE       (1 << 2) /* completely ignore the battery */
>  #define HID_BATTERY_QUIRK_AVOID_QUERY  (1 << 3) /* do not query the battery */
> +#define HID_BATTERY_QUIRK_POLLING_QUERY        (1 << 4) /* polling query the battery */
> 
>  static const struct hid_device_id hid_battery_quirks[] = {
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
> @@ -390,6 +391,16 @@ static const struct hid_device_id hid_battery_quirks[] = {
>          * set HID_BATTERY_QUIRK_IGNORE for all Elan I2C-HID devices.
>          */
>         { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, HID_ANY_ID), HID_BATTERY_QUIRK_IGNORE },
> +       /*
> +        * Reading battery capacity and status could fail and end up
> +        * with timeout after 5s for some input devices, for example:
> +        * 8BitDo SN30 Pro+ gamepad.
> +        * set HID_BATTERY_QUIRK_AVOID_QUERY and HID_BATTERY_QUIRK_POLLING_QUERY
> +        * for 8BitDo SN30 Pro+ gamepad.
> +        * to Avoid UI freezing when reading battery capacity/status
> +        */

We are usually not putting such verbose comments here. Describing the 
quirk when defining it is sufficient, and enumerating which devices need 
the quirk in the comment feels superfluous.

> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> +         HID_BATTERY_QUIRK_AVOID_QUERY | HID_BATTERY_QUIRK_POLLING_QUERY },
>         {}
>  };
> 
> @@ -497,6 +508,33 @@ static int hidinput_get_battery_property(struct power_supply *psy,
>         return ret;
>  }
> 
> +/*
> + * hid input device battery polling handler
> + */
> +static void hidinput_battery_polling_handler(struct work_struct *work)
> +{
> +       int value;
> +       struct hid_device *dev = container_of(work, struct hid_device, battery_delayed_work.work);
> +
> +       dev->battery_status = HID_BATTERY_QUERIED;
> +       dev->battery_avoid_query = true;
> +       value = hidinput_query_battery_capacity(dev);
> +       if (value < 0) {
> +               dev->battery_status = HID_BATTERY_UNKNOWN;
> +               hid_err(dev, "cannot get battery capacity from device!\n");
> +       } else {
> +               dev->battery_capacity = value;
> +               dev->battery_avoid_query = false;
> +               dev->battery_status = HID_BATTERY_REPORTED;
> +               hid_err(dev, "get battery capacity from device:%d!\n", value);
> +       }
> +
> +       /*keep polling period same to battery_ratelimit_time*/

This comment doesn't adhere to standard kernel commeting style, please add 
spaces at the beginning and the end.

Could you please resubmit with these small things fixed?

Thanks,

-- 
Jiri Kosina
SUSE Labs