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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent sysfs read and write operations interleave commands and corrupt hardware state due to a block-scoped guard() bug on cfg_mutex.
- [High] State machine corruption due to failure to defensively clear profile_pending on unexpected generic ACKs.
--

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

HID: hid-msi: Add Rumble Intensity Attributes

This commit adds intensity adjustment for the left and right rumble motors.
The code was co-developed with AI assistance for data gathering and initial
framing of the sysfs show and store functions.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index a628b77bfb7b5..cffd6ed253ec9 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -815,6 +831,126 @@ 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)
> +{

[ ... ]

> +	guard(mutex)(&drvdata->rom_mutex);
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +				    data, ARRAY_SIZE(data), 8);

[ ... ]

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

[ ... ]

> +	guard(mutex)(&drvdata->profile_mutex);
> +	drvdata->profile_pending = CLAW_RUMBLE_LEFT_PENDING;
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data,
> +				    ARRAY_SIZE(data), 8);
> +	if (ret) {
> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +		return ret;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_left);
> +}

Since rumble_intensity_left_store uses rom_mutex and
rumble_intensity_left_show uses profile_mutex, these functions can be
called concurrently.

Does claw_hw_output_report serialize these hardware requests?

Looking at claw_hw_output_report, it attempts to protect the transaction
with a block-scoped guard:

static int claw_hw_output_report(...)
{
        ...
        if (timeout) {
                guard(mutex)(&drvdata->cfg_mutex);
                reinit_completion(&drvdata->send_cmd_complete);
        }
        
        // drvdata->cfg_mutex is released here
        
        ...
        ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
        ...
        if (timeout) {
                ret = wait_for_completion_interruptible_timeout(
                                        &drvdata->send_cmd_complete, ...);
        ...
}

Because the guard is scoped only to the if statement, the lock is released
before the USB transmission and wait occur.

Will concurrent sysfs operations issue uncoordinated USB commands and wait
on the exact same send_cmd_complete object, resulting in desynchronized
responses?

If a concurrent store operation triggers a generic ACK that satisfies the
wait in the show thread, ret will be 0, but claw_profile_event will not
have executed.

In this success path, does drvdata->profile_pending remain stuck at
CLAW_RUMBLE_LEFT_PENDING instead of being defensively cleared?

If an unrelated READ_PROFILE_ACK arrives later, could claw_profile_event
incorrectly demultiplex it based on the stuck state, copying arbitrary
payload bytes into drvdata->rumble_intensity_left?

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