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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] State machine desynchronization and data corruption due to lockless `profile_pending` and lack of request sequence verification.
- [Medium] Undocumented and broken modification to button mapping functionality.
- [Medium] Data race on `rumble_intensity_left` and `rumble_intensity_right` variables due to inconsistent locking.
--

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

HID: hid-msi: Add Rumble Intensity Attributes

This commit adds sysfs attributes to adjust the left and right rumble
intensity for MSI claw devices.

> Adds intensity adjustment for the left and right rumble motors.

The commit message states this patch only adds intensity adjustment for the
left and right rumble motors, but it introduces undocumented changes to the
button mapping functionality.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
> @@ -234,6 +236,7 @@ static const struct {
>  	{ 0xcc, "BTN_EXTRA" },
>  	{ 0xcd, "REL_WHEEL_UP" },
>  	{ 0xce, "REL_WHEEL_DOWN" },
> +	{ 0xff, "DISABLED" },
>  };

[ ... ]

> @@ -678,6 +695,9 @@ static const char *button_mapping_code_to_name(u8 code)
>  {
>  	int i;
>  
> +	if (code == 0xff)
> +		return NULL;
> +

Does this logic prevent the newly added "DISABLED" mapping from ever being
returned? By explicitly returning NULL here, will callers print "(null)" to
userspace instead of the intended "DISABLED" string?

[ ... ]

> @@ -400,6 +411,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;

Are these assignments exposed to a data race?

This function updates the rumble intensity variables asynchronously without
holding any locks. However, the variables are read and written elsewhere
using entirely different locks:

rumble_intensity_left_store():
    guard(mutex)(&drvdata->rom_mutex);
    ...
    drvdata->rumble_intensity_left = val;

rumble_intensity_left_show():
    guard(mutex)(&drvdata->profile_mutex);
    ...
    return sysfs_emit(buf, "%u\n", drvdata->rumble_intensity_left);

Additionally, is there a risk of cross-talk if a delayed hardware ACK arrives
after a timeout?

If rumble_intensity_left_show() times out while waiting for
claw_hw_output_report(), it resets profile_pending to CLAW_NO_PENDING and
drops profile_mutex. The hardware command is not aborted.

If rumble_intensity_right_show() is subsequently called, it sets
profile_pending to CLAW_RUMBLE_RIGHT_PENDING.

If the delayed left motor ACK arrives at this exact moment, it appears
claw_profile_event() will observe CLAW_RUMBLE_RIGHT_PENDING.

Since this function does not verify the response address for rumble
requests, could it incorrectly assign the incoming left motor data to
drvdata->rumble_intensity_right?

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