Re: [PATCH v2] HID: add driver for Gigabyte Aero vendor-specific brightness keys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional capability mapping forces the registration of all hid_inputs and unpredictably overwrites drvdata->input.
- [Low] Uses 'ret' instead of 'error' or 'err' for error variable.
--

--- Patch [1]: [PATCH v2] HID: add driver for Gigabyte Aero vendor-specific brightness keys ---
commit a88dff22dbe03f72cc6b7e002b5c019c9cd1010f
Author: Uddhav Swami <uddhavswami@gmail.com>

HID: add driver for Gigabyte Aero vendor-specific brightness keys

This patch intercepts vendor-defined HID reports for brightness keys on
Gigabyte Aero laptops and maps them to standard KEY_BRIGHTNESSUP and
KEY_BRIGHTNESSDOWN input events.

> diff --git a/drivers/hid/hid-gigabyte.c b/drivers/hid/hid-gigabyte.c
> new file mode 100644
> index 0000000000000..3f0c9a1b6bc1d
> --- /dev/null
> +++ b/drivers/hid/hid-gigabyte.c

[ ... ]

> +static int gigabyte_aero_input_configured(struct hid_device *hdev,
> +					  struct hid_input *hi)
> +{
> +	struct gigabyte_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct input_dev *input = hi->input;
> +
> +	input_set_capability(input, EV_KEY, KEY_BRIGHTNESSUP);
> +	input_set_capability(input, EV_KEY, KEY_BRIGHTNESSDOWN);
> +
> +	drvdata->input = input;

[Severity: Medium]
Does this code accidentally force the registration of all hid_inputs?

Since capabilities are applied unconditionally to every hid_input application
exposed by the device, it appears this defeats the HID core's
hidinput_has_been_populated() check. This could result in dummy input devices
being registered for collections that have no mapped usages and would
otherwise be cleaned up.

Also, because drvdata->input is unconditionally overwritten on every callback
during device probe, will brightness events be unpredictably routed to
whichever hid_input happened to be parsed last by the HID core?

> +
> +	return 0;
> +}
> +
> +static int gigabyte_aero_probe(struct hid_device *hdev,
> +			       const struct hid_device_id *id)
> +{
> +	struct gigabyte_drvdata *drvdata;
> +	int ret;

[Severity: Low]
This isn't a bug, but could we use a different variable name here?

The input subsystem maintainer preferences suggest using 'error' or 'err' for
variables that hold only negative error codes and 0 for success, rather than
'ret'.

> +
> +	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715041556.112673-1-uddhavswami@gmail.com?part=1