Re: [PATCH v5] HID: hp: Add support for HyperX headset devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] HID: hp: wireless_status not initialized when headset is off in hp_probe()
- [Medium] HID: hp: battery state not polled on connect in hp_raw_event()
--

commit eff0223529a8e019e0454a1c8760eff7536e4ec0
Author: Sofia Schneider <sofia@schn.dev>

HID: hp: Add support for HyperX headset devices

This commit introduces the hid-hp driver for HyperX Cloud III Wireless
headsets, polling the device to report battery capacity, charging state,
and USB wireless connection status.

> diff --git a/drivers/hid/hid-hp.c b/drivers/hid/hid-hp.c
> new file mode 100644
> index 0000000000000..32164b3c8e04a
> --- /dev/null
> +++ b/drivers/hid/hid-hp.c
> @@ -0,0 +1,398 @@

[ ... ]

> +static void hp_headset_battery_work(struct work_struct *work)
> +{
> +	struct hp_headset_device *drvdata =
> +		container_of(work, struct hp_headset_device, battery_work);
> +	struct power_supply_config battery_cfg = { .drv_data = drvdata };
> +	struct power_supply *ps;
> +	unsigned long flags;
> +	bool connected;
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +	connected = drvdata->is_connected;
> +	ps = drvdata->battery;
> +	spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +	hp_headset_set_wireless_status(drvdata->hdev, connected);
> +
> +	if (connected && !ps) {
> +		ps = power_supply_register(&drvdata->hdev->dev,
> +					   &drvdata->battery_desc,
> +					   &battery_cfg);
> +		if (IS_ERR(ps)) {
> +			hid_err(drvdata->hdev, "power_supply_register failed\n");
> +			return;
> +		}
> +
> +		power_supply_powers(ps, &drvdata->hdev->dev);
> +
> +		spin_lock_irqsave(&drvdata->lock, flags);
> +		drvdata->battery = ps;
> +		spin_unlock_irqrestore(&drvdata->lock, flags);
> +	} else if (!connected && ps) {
> +		spin_lock_irqsave(&drvdata->lock, flags);
> +		drvdata->battery = NULL;
> +		spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +		power_supply_unregister(ps);
> +	}
> +}

[ ... ]

> +static void
> +hp_headset_parse_connected_event(struct hp_headset_device *drvdata, u8 *data)
> +{
> +	unsigned long flags;
> +	bool state_changed = false;
> +	bool connected = (data[2] == 1);
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +
> +	if (drvdata->is_connected != connected) {
> +		drvdata->is_connected = connected;
> +		state_changed = true;
> +	}
> +
> +	spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +	if (state_changed)
> +		schedule_work(&drvdata->battery_work);
> +}

[Severity: Medium]
When the headset is turned on after hp_probe() has already run, the device
sends a connection report handled by:

  hp_raw_event()
    -> hp_headset_parse_connected_event()
      -> schedule_work(&drvdata->battery_work)

In that path, hp_headset_battery_work() registers drvdata->battery without
sending HYPERX_CMD_GET_BATTERY or HYPERX_CMD_GET_CHARGING and without
rescheduling drvdata->poll_work. Because hp_probe() initializes
drvdata->battery_level to 100 and drvdata->is_charging to false (and
disconnected battery replies with state1 == 0 && state2 == 0 are ignored in
hp_headset_parse_battery_event()), the newly registered power supply will
report dummy (100% / POWER_SUPPLY_STATUS_FULL) or stale values until the
next 2-minute poll interval expires.

Also, during hp_headset_poll_work(), HYPERX_CMD_GET_CONNECTED is sent
immediately before HYPERX_CMD_GET_BATTERY and HYPERX_CMD_GET_CHARGING. If
those battery and charging replies arrive in hp_raw_event() while
hp_headset_battery_work() is still inside power_supply_register() before
drvdata->battery = ps is assigned, hp_headset_parse_battery_event() and
hp_headset_parse_charging_event() see drvdata->battery as NULL and skip
power_supply_changed().

Should connecting the headset trigger an immediate battery and charging
query (for example via mod_delayed_work() on drvdata->poll_work or after
power_supply_register() completes in hp_headset_battery_work())?

> +
> +static int hp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct hp_headset_device *drvdata;
> +	struct power_supply *ps;
> +	unsigned long flags;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +	drvdata->hdev = hdev;
> +	drvdata->is_connected = false;
> +	drvdata->is_charging = false;
> +	drvdata->battery_level = 100;
> +	spin_lock_init(&drvdata->lock);
> +	hid_set_drvdata(hdev, drvdata);

[ ... ]

> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw_open failed\n");
> +		goto err_stop_hw;
> +	}
> +
> +	schedule_delayed_work(&drvdata->poll_work, 0);
> +
> +	return 0;

[Severity: Medium]
What happens to the USB interface wireless_status sysfs attribute if the
USB dongle is probed while the headset is powered off?

In hp_probe(), drvdata->is_connected is initialized to false, and
hp_headset_set_wireless_status() is only called from
hp_headset_battery_work(). When the initial hp_headset_poll_work() queries
HYPERX_CMD_GET_CONNECTED and the receiver reports data[2] == 0
(connected == false), hp_headset_parse_connected_event() sees:

	if (drvdata->is_connected != connected) {
		drvdata->is_connected = connected;
		state_changed = true;
	}

Since drvdata->is_connected is already false, state_changed remains false
and drvdata->battery_work is never scheduled. As a result,
usb_set_wireless_status(intf, USB_WIRELESS_STATUS_DISCONNECTED) is never
called, leaving intf->wireless_status at USB_WIRELESS_STATUS_NA and keeping
the wireless_status sysfs attribute hidden until the headset is turned on
and then off.

Would it make sense to call hp_headset_set_wireless_status(hdev, false)
during hp_probe()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021183205.13866-1-sofia@schn.dev?part=1