Re: [PATCH v3] HID: hid-msi: Send a uevent when the MCU changes the mode by itself

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: hid-msi: missed uevent and stale state in
  claw_switch_mode() race
--

commit cd52e9bad8fc6caa521f438abf30e16af93b21b9
Author: Ana Veto <green.koala2979@fastmail.com>

HID: hid-msi: Send a uevent when the MCU changes the mode by itself

This commit schedules a work item to emit a KOBJ_CHANGE uevent when an
incoming GAMEPAD_MODE_ACK report changes the stored gamepad mode or M-key
function, and updates the stored state after a SWITCH_MODE output report
unless a report arrived during the transfer.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 23d731f700c90..dcbab90f32060 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -387,17 +389,40 @@ static int get_endpoint_address(struct hid_device *hdev)
>  	return -ENODEV;
>  }
>  
> +static void claw_mode_event_fn(struct work_struct *work)
> +{
> +	struct claw_drvdata *drvdata = container_of(work, struct claw_drvdata, mode_event);
> +
> +	kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
> +}
> +
>  static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
>  				   struct claw_command_report *cmd_rep)
>  {
> +	bool changed;
> +
>  	if (cmd_rep->data[0] >= ARRAY_SIZE(claw_gamepad_mode_text) ||
>  	    !claw_gamepad_mode_text[cmd_rep->data[0]] ||
>  	    cmd_rep->data[1] >= ARRAY_SIZE(claw_mkeys_function_text))
>  		return -EINVAL;
>  
>  	scoped_guard(spinlock_irqsave, &drvdata->mode_lock) {
> +		changed = drvdata->gamepad_mode != cmd_rep->data[0] ||
> +			  drvdata->mkeys_function != cmd_rep->data[1];
>  		drvdata->gamepad_mode = cmd_rep->data[0];
>  		drvdata->mkeys_function = cmd_rep->data[1];
> +		drvdata->mode_reports++;
> +	}
> +
> +	/*
> +	 * The MCU changes the mode or M-key function by itself, e.g. while
> +	 * View+LB is held. Userspace learns of changes only through uevents.
> +	 */
> +	if (changed) {
> +		guard(spinlock_irqsave)(&drvdata->registration_lock);
> +		/* Pairs with smp_store_release from cfg_setup_fn and claw_remove */
> +		if (smp_load_acquire(&drvdata->gp_registered))
> +			schedule_work(&drvdata->mode_event);
>  	}
>  
>  	return 0;
> @@ -656,11 +681,14 @@ static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
>  static int claw_switch_mode(struct hid_device *hdev, enum claw_mode_field field, u8 val)
>  {
>  	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	unsigned int reports;
>  	u8 data[2];
> +	int ret;
>  
>  	guard(mutex)(&drvdata->cfg_mutex);
>  
>  	scoped_guard(spinlock_irqsave, &drvdata->mode_lock) {
> +		reports = drvdata->mode_reports;
>  		switch (field) {
>  		case CLAW_FIELD_GAMEPAD_MODE:
>  			data[0] = val;
> @@ -673,8 +701,23 @@ static int claw_switch_mode(struct hid_device *hdev, enum claw_mode_field field,
>  		}
>  	}
>  
> -	return __claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> -				       ARRAY_SIZE(data), 0);
> +	ret = __claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> +				      ARRAY_SIZE(data), 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The driver's own change is no change to report. A report that came
> +	 * in during the transfer may already show a newer state; keep it.
> +	 */
> +	scoped_guard(spinlock_irqsave, &drvdata->mode_lock) {
> +		if (drvdata->mode_reports == reports) {
> +			drvdata->gamepad_mode = data[0];
> +			drvdata->mkeys_function = data[1];
> +		}
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
Can a GAMEPAD_MODE_ACK report that arrives during claw_switch_mode()
suppress drvdata->mode_event when it carries the same state that was stored
in drvdata before the write?

When a report sets drvdata->mkeys_function to CLAW_MKEY_FUNCTION_DISABLED,
userspace reacts to the uevent by calling mkeys_function_store() ->
claw_switch_mode() to write CLAW_MKEY_FUNCTION_MACRO.  Because
claw_switch_mode() leaves drvdata->mkeys_function at the old value while
__claw_hw_output_report() runs, a second report carrying
CLAW_MKEY_FUNCTION_DISABLED during the transfer sees no change from the
pre-write state:

CPU0                                  CPU1
claw_switch_mode()
  /* mkeys_function is DISABLED */
  reports = drvdata->mode_reports;
  data[1] = CLAW_MKEY_FUNCTION_MACRO;
  __claw_hw_output_report(...)
                                      claw_gamepad_mode_event()
                                        /* cmd_rep->data[1] is DISABLED */
                                        changed = false;
                                        drvdata->mode_reports++;
                                        /* mode_event not scheduled */
  /* mode_reports != reports */
  /* skips updating drvdata */        <-- no uevent emitted

If that report was sent after the MCU processed SWITCH_MODE (for example,
View+LB overriding M-keys back to disabled), the controller stays disabled
with no KOBJ_CHANGE uevent emitted; if the report was already in flight
before the MCU processed SWITCH_MODE, the MCU switches to macro while
drvdata->mkeys_function remains DISABLED, so the next unsolicited report
reverting to disabled will also see changed == false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115032314.7094-1-green.koala2979@fastmail.com?part=1