Re: [PATCH v2 05/12] HID: asus: add joysticks inner and outer range configuration

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `intf_mutex` lock in `ally_set_joystick_thresholds` allows concurrent HID requests to interleave and corrupt device state.
- [Medium] Joystick threshold sysfs 'show' functions do not check for hardware support, returning a dummy default value even when the feature is unsupported.
--

--- Patch [5]: [PATCH v2 05/12] HID: asus: add joysticks inner and outer range configuration ---
commit 2e6189717a427b3132c324a798d81ea6224e0246
Author: Denis Benato <[email protected]>

HID: asus: add joysticks inner and outer range configuration

This commit adds sysfs attributes to configure the inner and outer ranges
for the ROG Ally joysticks, allowing userspace to modify sensitivity.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6b005afe01a24..d028697f61bfd 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> +static int ally_set_joystick_thresholds(struct hid_device *hdev, struct ally_config *cfg,
> +					u8 left_it, u8 left_ot, u8 right_it, u8 right_ot)
> +{
> +	u8 payload[] = { left_it, left_ot, right_it, right_ot };
> +	int ret;
> +
> +	if (!cfg->xbox_controller_support)
> +		return -ENODEV;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_JOYSTICK_DEADZONE, payload, sizeof(payload));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);

[Severity: High]
Does this need to hold ally->intf_mutex when sending the report?

Other configuration functions like ally_set_xbox_controller() and
ally_set_gamepad_mode() use ally_gamepad_send_packet(), which safely
acquires the lock before calling ally_dev_set_report().

If called concurrently with a USB reset (where ally_gamepad_check_ready()
holds intf_mutex but not config_mutex), could this interleave between the
set and get report phases and corrupt the device state?

[ ... ]
> +static ssize_t left_joystick_inner_threshold_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +
> +	if (!ally)
> +		return -ENODEV;
> +
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return -ENODEV;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		return sysfs_emit(buf, "%u\n", cfg->left_deadzone);
> +}

[Severity: Medium]
Should this check cfg->xbox_controller_support before emitting the value?

The corresponding store paths return -ENODEV via
ally_set_joystick_thresholds() if the hardware doesn't support the feature,
but the show paths seem to return the default initialized values regardless
of hardware support.

This appears to apply to all the joystick threshold show attributes added in
this patch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5