Re: [PATCH v4] HID: pulsar: add driver for Pulsar gaming mice

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Battery state cache invalidation fails during the first 60 seconds of system uptime.
- [Medium] Power supply sysfs name collision when multiple devices are connected to different USB buses.
- [Low] The driver fails to compile because it uses the MAX() macro for an array bound at file scope.
--

--- Patch [1]: [PATCH v4] HID: pulsar: add driver for Pulsar gaming mice ---
commit 2186fd2a13ddd34f0c6ef2388857c37ac08cf8aa
Author: Nikolas Koesling <[email protected]>

HID: pulsar: add driver for Pulsar gaming mice

Add a HID driver for Pulsar wireless gaming mice (X2 V2, X2H, X2A,
Xlite V3). The driver exposes battery level, voltage, and charging
status through the power supply framework. It supports wired, 1kHz,
and 4kHz wireless dongle connections.

> diff --git a/drivers/hid/hid-pulsar.c b/drivers/hid/hid-pulsar.c
> new file mode 100644
> index 0000000000000..84df69a7fe443
> --- /dev/null
> +++ b/drivers/hid/hid-pulsar.c
> @@ -0,0 +1,758 @@
[ ... ]
> +struct pulsar_battery {
> +	struct power_supply *ps;
> +	struct power_supply_desc desc;
> +	char name[48];
> +	char model[MAX(32, sizeof((struct hid_device){}).name)];

[Severity: Low]
Will this cause a compilation error? The MAX() macro in the kernel is
implemented using statement expressions, which are strictly forbidden in
integer constant expressions in C. Using MAX() to define an array bound at
file scope usually results in a "statement expression not allowed at file
scope" compiler error.

[ ... ]
> +static void power_uevent_work_handler(struct work_struct *work)
> +{
> +	struct pulsar_data *drvdata;
> +	int ret;
> +
> +	drvdata = container_of(work, struct pulsar_data, power_uevent_work);
> +
> +	if (atomic_read(&drvdata->stopping))
> +		return;
> +
> +	down_write(&drvdata->lock_bat);
> +	drvdata->battery.last_read = 0;
> +	drvdata->battery.last_status = 0;
> +	up_write(&drvdata->lock_bat);

[Severity: Medium]
Does this reliably invalidate the cache shortly after system boot?

In read_power(), cache validity is checked using ktime_get_ns() (time since
boot) compared against MAX_BATTERY_AGE_NS (60s):

read_power() {
        now = ktime_get_ns();
        ...
        need_power = battery->available &&
            (now - battery->last_read >= MAX_BATTERY_AGE_NS);
}

If the system uptime is under 60 seconds, now - 0 will be less than
60 seconds. Could this cause the check to evaluate to false and silently skip
reading power updates for the first minute of uptime?

[ ... ]
> +static int pulsar_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
[ ... ]
> +	INIT_DELAYED_WORK(&drvdata->init_work, pulsar_init_work);
> +	drvdata->init_retries = INIT_RETRIES;
> +
> +	snprintf(drvdata->battery.name, sizeof(drvdata->battery.name),
> +		 "pulsar_%s_battery", usbdev->devpath);
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);

[Severity: Medium]
Could this cause a sysfs name collision if identical devices are plugged into
multiple USB host controllers?

The usbdev->devpath property is only unique within a single USB bus tree. If
multiple mice are connected to the same relative port numbers on different
buses (e.g., both yield devpath "1" or "1.2"), this will generate identical
names like "pulsar_1_battery".

Would this cause devm_power_supply_register() to fail for the second device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1