Re: [PATCH 3/5] HID: UNCTRL: add periodic battery reporting using a workqueue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hid: unctrl: initial battery query reply dropped during probe
- [Medium] pm: battery polling work wakes CPU every 5s during s2idle
--

commit 96156a773d135475c7c9f3eca5f3c8ad662fe691
Author: Bishwa Bhusan Saha <contact@bishwasaha.com>

HID: UNCTRL: add periodic battery reporting using a workqueue

This patch adds a delayed workqueue to check the battery status every
5 seconds for the UNCTRL HID device.

> diff --git a/drivers/hid/hid-unctrl-m1.c b/drivers/hid/hid-unctrl-m1.c
> index 59259dee7e429..1e73cad1e3f91 100644
> --- a/drivers/hid/hid-unctrl-m1.c
> +++ b/drivers/hid/hid-unctrl-m1.c

[ ... ]

> @@ -132,6 +137,17 @@ static void unctrl_fetch_battery(struct hid_device *hdev)
>  	kfree(write_buf);
>  }
>  
> +static void unctrl_battery_timer_tick(struct work_struct *work)
> +{
> +	struct unctrl_dev *dev =
> +		container_of(work, struct unctrl_dev, battery_work.work);
> +
> +	unctrl_fetch_battery(dev->hdev);
> +
> +	schedule_delayed_work(&dev->battery_work,
> +			      msecs_to_jiffies(UNCTRL_BATTERY_TIMEOUT_MS));
> +}

[Severity: Medium]
Will this scheduled work continuously wake up the CPU during system suspend?

Since schedule_delayed_work() implicitly queues to the non-freezable system_wq
and the driver does not implement suspend/resume callbacks to cancel this work,
it appears this timer will keep firing every 5 seconds.

If the system enters s2idle suspend, could this prevent deep sleep and cause
unnecessary power drain?

[ ... ]

> @@ -155,6 +171,8 @@ static int unctrl_battery_register(struct unctrl_dev *dev)
>  		 dev->battery_desc.name);
>  
>  	unctrl_fetch_battery(dev->hdev);
> +	schedule_delayed_work(&dev->battery_work,
> +			      msecs_to_jiffies(UNCTRL_BATTERY_TIMEOUT_MS));
>  
>  	return 0;
>  }

[Severity: Medium]
Could the initial battery query reply be dropped by the HID core?

This function is called during unctrl_probe(), and it sends a battery query
via unctrl_fetch_battery(), but hid_device_io_start() hasn't been called to
unblock the input path.

If the device replies quickly before unctrl_probe() finishes and the
driver_input_lock is released, wouldn't the HID core silently drop the incoming
report with -EBUSY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-unctrl-m1-battery-v1-0-4a50203b7177@bishwasaha.com?part=3