Re: [PATCH v5 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] Hardware state machine desynchronization due to abandoned interruptible waits and lack of request tracking.
- [Medium] Data race on shared state variables `rumble_intensity_left`, `rumble_intensity_right`, and `profile_pending`.
--

commit 9659fef77f1187c96b3944136a0f1b514f35ccfc
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
to the MSI claw driver. It implements sysfs show and store attributes and
handles the hardware profile events for the rumble intensity states.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 4443f53b54cb1..6658d7f2b2ddc 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -403,6 +413,12 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
>  			drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
>  		}
>  
> +		break;
> +	case CLAW_RUMBLE_LEFT_PENDING:
> +		drvdata->rumble_intensity_left = cmd_rep->data[4];
> +		break;
> +	case CLAW_RUMBLE_RIGHT_PENDING:
> +		drvdata->rumble_intensity_right = cmd_rep->data[4];
>  		break;
>  	default:

Is there a data race on rumble_intensity_left and profile_pending here?

The store functions update rumble_intensity_left while holding rom_mutex,
and the show functions set profile_pending while holding profile_mutex.
However, it appears claw_profile_event() can be triggered concurrently by
in-flight hardware responses in the HID event handler, writing to
rumble_intensity_left and clearing profile_pending without holding either
mutex.

[ ... ]
> @@ -822,6 +838,126 @@ static ssize_t button_mapping_options_show(struct device *dev,
>  }
[ ... ]
> +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;
> +
> +	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);

Can this sequence lead to desynchronization of the hardware state machine?

Since claw_hw_output_report() uses an interruptible wait with a short
timeout (8ms), if the wait times out or is interrupted by a signal, the
hardware will likely continue processing the command.

If a subsequent sysfs operation (like rumble_intensity_right_show) then
sets profile_pending to CLAW_RUMBLE_RIGHT_PENDING, could a delayed ACK
from the previously aborted left rumble request arrive?

Because claw_profile_event() relies solely on the global profile_pending
state without validating payload addresses or transaction IDs, it seems
this delayed ACK would mistakenly apply the left rumble response to the
right rumble variable.

Additionally, could a delayed generic ACK from a timed-out SYNC_TO_ROM
prematurely complete the wait here, causing the show() operation to return
immediately with stale data while leaving profile_pending dangling?

> +	if (ret) {
> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +		return ret;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_left);
> +}

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