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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] State machine desynchronization and data race on `profile_pending` leads to corrupted rumble intensity reads.
--

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

    HID: hid-msi: Add Rumble Intensity Attributes
    
    Adds intensity adjustment for the left and right rumble motors.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 4d267b896da72..339f256b60dbe 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -882,6 +901,142 @@ static ssize_t button_mapping_options_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(button_mapping_options);

[ ... ]

> +static ssize_t rumble_intensity_left_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	u8 data[4] = { 0x01, (rumble_addr[0] >> 8) & 0xff, rumble_addr[0] & 0xff, 0x01 };
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
> +	u8 val;
> +
> +	if (!drvdata->gamepad_registered)
> +		return -ENODEV;
> +
> +	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), 25);
> +	if (ret) {
> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +		return ret;
> +	}

Can the profile_pending state become desynchronized if a hardware read times
out?

If claw_hw_output_report() times out, it sets
drvdata->profile_pending = CLAW_NO_PENDING and returns.

If a subsequent call to rumble_intensity_right_show() sets
drvdata->profile_pending = CLAW_RUMBLE_RIGHT_PENDING, and then the delayed
hardware ACK from the first left request arrives, claw_profile_event() will
misattribute the response.

It will read the left motor's intensity into drvdata->rumble_intensity_right,
corrupting the state:

	case CLAW_RUMBLE_RIGHT_PENDING:
		scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
			drvdata->rumble_intensity_right = cmd_rep->data[4];

Additionally, claw_profile_event() clears
drvdata->profile_pending = CLAW_NO_PENDING locklessly at the end, which
could race with other process contexts accessing it under profile_mutex.

Should claw_profile_event() validate the memory address in the response payload
to ensure it belongs to the pending request, and synchronize accesses to
profile_pending?

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