Re: [PATCH v4 1/4] HID: hid-msi: Add MSI Claw configuration driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] ABBA deadlock between `cfg_mutex` and sysfs kernfs active references during device removal.
- [High] The `mode_mutex` in `struct claw_drvdata` is never initialized, leading to kernel crashes upon contention.
- [High] Defensive error handling in `claw_remove()` introduces a double execution of `hid_hw_stop()`.
- [High] Unconditional device disconnect and state loss upon system resume.
- [Medium] Sysfs attributes are permanently lost if the system suspends shortly after probe.
- [Medium] Fragile endpoint detection logic checks only the first endpoint in the interface.
- [Medium] Late ACKs from timed-out commands spuriously complete new commands.
- [Medium] Fragile 8ms timeout can cause spurious failures on systems with HZ=100.
--

commit 6fb3a7c70c0068c58170fcddbffdda870a23fe93
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add MSI Claw configuration driver

This commit adds a configuration HID driver for the MSI Claw series of
handheld PC's. It provides an interface to change the gamepad mode,
M-key behavior, and reset the device.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int get_endpoint_address(struct hid_device *hdev)
> +{
> +	struct usb_host_endpoint *ep;
> +	struct usb_interface *intf;
> +
> +	intf = to_usb_interface(hdev->dev.parent);
> +	ep = intf->cur_altsetting->endpoint;

Does this correctly handle configurations where the target IN endpoint is not
at index 0? Since USB endpoint ordering is not strictly mandated, should this
iterate over desc.bNumEndpoints instead of blindly selecting the first one?

[ ... ]
> +static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> +			  u8 *data, int size)
> +{
[ ... ]
> +	switch (cmd_rep->cmd) {
> +	case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> +		ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> +		break;
> +	case CLAW_COMMAND_TYPE_ACK:
> +		break;
> +	default:
> +		dev_dbg(&drvdata->hdev->dev, "Unknown command: %x\n", cmd_rep->cmd);
> +		return 0;
> +	}
> +
> +	complete(&drvdata->send_cmd_complete);

Could late ACKs from previously timed-out commands spuriously trigger this
completion? If a prior command times out and releases the mutex, and a new
command is issued, a delayed ACK might cause the new wait to return
prematurely with stale data.

[ ... ]
> +static ssize_t gamepad_mode_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret, i;
> +
> +	guard(mutex)(&drvdata->mode_mutex);
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 8);

Could this 8ms timeout cause spurious failures on systems configured with
HZ=100? In that configuration, msecs_to_jiffies(8) evaluates to 1 jiffy,
which can expire in a fraction of a millisecond depending on when the system
timer tick occurs.

[ ... ]
> +static void cfg_resume_fn(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> +	struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> +	u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
> +	int ret;
> +
> +	ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> +				    ARRAY_SIZE(data), 0);

The commit message notes that sending the SWITCH_MODE command causes a USB
disconnect in the device. Does sending this unconditionally on system resume
force a physical USB disconnect and re-enumeration every time the host wakes
from sleep?

[ ... ]
> +static int claw_probe(struct hid_device *hdev, u8 ep)
> +{
> +	struct claw_drvdata *drvdata;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->gamepad_mode = CLAW_GAMEPAD_MODE_XINPUT;
> +	drvdata->hdev = hdev;
> +	drvdata->ep = ep;
> +
> +	mutex_init(&drvdata->cfg_mutex);

Is there a missing mutex_init() for drvdata->mode_mutex here? Since the
structure is zero-allocated via devm_kzalloc, locking an uninitialized mutex
later in gamepad_mode_store() can trigger lockdep warnings or crashes.

[ ... ]
> +static void claw_remove(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata) {
> +		hid_hw_stop(hdev);
> +		return;
> +	}

If drvdata is NULL, this function calls hid_hw_stop(hdev). Will this cause a
double-free or use-after-free since the caller msi_remove() also calls
hid_hw_stop(hdev) unconditionally after this returns?

> +
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +
> +	guard(mutex)(&drvdata->cfg_mutex);
> +	device_remove_group(&hdev->dev, &claw_gamepad_attr_group);

Can taking cfg_mutex here cause a deadlock during device removal?

device_remove_group() drains active sysfs operations and waits for them to
complete. However, callbacks like gamepad_mode_store() take cfg_mutex
internally. If a user is reading or writing an attribute concurrently, remove
will block waiting for sysfs, and sysfs will block waiting for cfg_mutex.

[ ... ]
> +static int claw_suspend(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata)
> +		return -ENODEV;
> +
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);

If the system suspends before the 500ms cfg_setup timer expires, this cancels
the setup work. Since claw_resume() only schedules cfg_resume, will the
device silently fail to ever expose its sysfs attributes for the rest of its
lifecycle?

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