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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect error check in `claw_buttons_store()` prevents hardware updates and violates sysfs contract.
- [High] Unsynchronized request tracking and short timeouts cause cross-talk between sysfs reads and corrupt device state.
- [Medium] Writing an empty string silently fails to clear M-key mappings.
--

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

HID: hid-msi: Add M-key mapping attributes

This commit adds sysfs attributes to remap the M-keys (M1 and M2) on MSI
devices in macro mode. It introduces functionality to parse user input,
translate button names to codes, and synchronize these configurations
with the hardware.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 1bf1d6150f4fd..bc0169c992ae8 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u8 data[] = { 0x01, (drvdata->bmap_addr[mkey_idx] >> 8) & 0xff,
> +		      drvdata->bmap_addr[mkey_idx] & 0xff, 0x07,
> +		      0x04, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff };
> +	char **raw_keys __free(argv) = NULL;
> +	size_t len = ARRAY_SIZE(data);
> +	int ret, key_count, i;
> +
> +	raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> +	if (!raw_keys)
> +		return -ENOMEM;
> +
> +	if (key_count > CLAW_KEYS_MAX)
> +		return -EINVAL;
> +
> +	if (key_count == 0)
> +		return 0;

If a user attempts to clear the button mappings by writing an empty string,
key_count will be 0. 

Doesn't this early return prevent sending the pre-initialized data array 
(which is filled with 0xff to denote unmapped keys) to the hardware, 
effectively leaving the previous mappings active?

> +
> +	for (i = 0; i < key_count; i++) {
> +		ret = button_mapping_name_to_code(raw_keys[i]);
> +		if (ret)
> +			return ret;

Does this correctly handle valid button codes?

button_mapping_name_to_code() returns a positive integer for valid codes 
(e.g., 0x01 to 0xce). Since any positive value evaluates to true, won't
this immediately abort the loop, bypass the hardware write, and return a
positive integer up to the VFS layer? 

Should this be checking if (ret < 0) instead?

> +
> +		data[6 + i] = ret;
> +	}
> +
> +	scoped_guard(mutex, &drvdata->rom_mutex) {
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    data, len, 8);
> +		if (ret)
> +			return ret;
> +
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> +	}
> +
> +	return ret;
> +}
> +
> +static int claw_buttons_show(struct device *dev, char *buf, enum claw_key_index m_key)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u8 data[] = { 0x01, (drvdata->bmap_addr[m_key] >> 8) & 0xff,
> +		      drvdata->bmap_addr[m_key] & 0xff, 0x07 };
> +	size_t len = ARRAY_SIZE(data);
> +	int i, ret, count = 0;
> +	const char *name;
> +	u8 *codes;
> +
> +	codes = (m_key == CLAW_KEY_M1) ? drvdata->m1_codes : drvdata->m2_codes;
> +
> +	guard(mutex)(&drvdata->profile_mutex);
> +	drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING : CLAW_M2_PENDING;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);

Is 8ms enough time for the device to respond reliably? On a system with 
HZ=100, 8ms is less than 1 jiffy.

If this times out, profile_pending is reset to CLAW_NO_PENDING below and 
the mutex is released when the function returns. If a delayed ACK from this 
request arrives while a subsequent sysfs read for the other M-key is 
pending, could claw_profile_event() mistakenly write the stale data into 
the new request's buffer and prematurely complete it?

> +	if (ret) {
> +		drvdata->profile_pending = CLAW_NO_PENDING;
> +		return ret;
> +	}

[ ... ]

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