Re: [PATCH 2/4] HID: hid-msi-claw: Add M-key mapping attributes

On May 11, 2026 1:51:07 PM PDT, [email protected] wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [High] Data race on `drvdata->profile_pending` and lack of transaction atomicity during concurrent sysfs accesses.
>--
>
>commit b9bc6d29326e3dd3ed065aca7059668a1b674bc8
>Author: Derek J. Clark <[email protected]>
>
>HID: hid-msi-claw: Add M-key mapping attributes
>
>Adds sysfs attributes to remap the M1 and M2 rear buttons to up to 5 events
>(gamepad, keyboard, or mouse) when in macro mode.
>
>> diff --git a/drivers/hid/hid-msi-claw.c b/drivers/hid/hid-msi-claw.c
>> index 7a3cd940ec49e..60694d075d56f 100644
>> --- a/drivers/hid/hid-msi-claw.c
>> +++ b/drivers/hid/hid-msi-claw.c
>[ ... ]
>> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
>> +{
>[ ... ]
>> +set_buttons:
>> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
>> +	if (ret < 0)
>> +		goto err_free;
>> +
>> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
>
>Is it possible for concurrent writers to interleave these commands?
>
>Looking at mcu_property_out(), it takes the cfg_mutex internally, but there
>is no lock held across both calls here. If a second writer executes its
>WRITE_PROFILE_DATA before the first writer executes SYNC_TO_ROM, could the MCU
>be left in an inconsistent state?
>

No, the sync to rom simply writes the state to permanent memory so it remains in the same configuration after a power cycle. The first call writes it to temporary storage.

>[ ... ]
>> +static int claw_buttons_show(struct device *dev, char *buf, enum claw_key_index m_key)
>> +{
>[ ... ]
>> +	codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
>> +	drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING : CLAW_M2_PENDING;
>> +
>> +	ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);
>
>Can concurrent reads of button_m1 and button_m2 race and corrupt the
>profile_pending state?
>
>If Thread A and Thread B access the sysfs attributes simultaneously,
>profile_pending is updated locklessly here before mcu_property_out() takes
>the cfg_mutex.
>
>Could Thread B overwrite profile_pending to CLAW_M2_PENDING before Thread A's
>command executes? If so, when the hardware replies to Thread A's M1 command,
>claw_profile_event() might route Thread A's data into Thread B's m2_codes
>buffer and clear the state to CLAW_NO_PENDING.
>
>This would cause Thread B's subsequent reply to be dropped and trigger the
>"Got profile event without changes pending" warning.
>
Possible. I'll look at improving this pattern by managing the pending flag with a mutex.
- Derek