Re: [PATCH 4/4] HID: hid-msi-claw: Add Rumble Intensity Attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The underlying `mcu_property_out()` function uses an improperly scoped `guard(mutex)`, breaking all driver synchronization.
- [High] Unprotected modification of `drvdata->profile_pending` causes race conditions during concurrent sysfs reads.
- [High] Torn transactions in sysfs store handlers lead to state mismatch between hardware and software.
- [Medium] Failure to reset `drvdata->profile_pending` on `mcu_property_out()` error paths corrupts state machine.
--

commit 2a92587430af9e91f5af5fbf2205fea99b2d1000
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi-claw: Add Rumble Intensity Attributes

This commit adds intensity adjustment for the left and right rumble motors
via new sysfs attributes.

> diff --git a/drivers/hid/hid-msi-claw.c b/drivers/hid/hid-msi-claw.c
> index f4fe74a784c29..6d089f49abdb4 100644
> --- a/drivers/hid/hid-msi-claw.c
> +++ b/drivers/hid/hid-msi-claw.c

[ ... ]

> @@ -795,6 +811,116 @@ static ssize_t button_mapping_options_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(button_mapping_options);
>  
> +static ssize_t rumble_intensity_left_store(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count)
> +{

[ ... ]

> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +			       data, ARRAY_SIZE(data), 8);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	drvdata->rumble_intensity_left = val;
> +
> +	return count;
> +}

Could this sequence introduce a regression if interrupted by concurrent sysfs
stores?

Since there doesn't appear to be a lock held across this sequence, concurrent
writes could interleave their WRITE_PROFILE_DATA and SYNC_TO_ROM commands.
This could cause the hardware to synchronize an unintended state or cause the
local driver state to fall out of sync with the actual hardware value.

Also, regarding the underlying mcu_property_out() function: does it use an
improperly scoped guard(mutex)? If the configuration mutex is acquired
strictly inside a limited scope, the lock would be released before
hid_hw_output_report() and wait_for_completion_interruptible_timeout() are
executed. This could leave all concurrent sysfs calls unprotected, causing a
regression with overlapping USB output reports.

> +static ssize_t rumble_intensity_left_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{

[ ... ]

> +	drvdata->profile_pending = CLAW_RUMBLE_LEFT_PENDING;
> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, ARRAY_SIZE(data), 8);
> +	if (ret)
> +		return ret;

Is it possible for concurrent sysfs reads to cause a regression here?

Since drvdata->profile_pending is modified without any locking, if two sysfs
attributes are read concurrently, one thread's modification could overwrite
the other's state. When the asynchronous USB response later arrives,
claw_profile_event() might rely on this clobbered value to route the incoming
data, which could result in userspace reading incorrect data.

Additionally, if mcu_property_out() returns an error, the function returns
immediately but leaves drvdata->profile_pending set to the pending state.
Could this stale state cause a regression where a subsequent unrelated
asynchronous profile event is incorrectly intercepted by claw_profile_event()?

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