Re: [PATCH v13 03/18] HID: asus: add gamepad configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] HID: asus: unlocked feature report transfers in hid_asus_ally_reset_resume()
- [Low] HID: asus: ally_detect_capabilities() mixes guard(mutex) with goto
- [Low] HID: asus: open-coded sysfs group helpers in ally_prefs_setup()
--

commit 23ba9584ee0ff2df9f8412928af4abbe35ed412d
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add gamepad configuration

This commit adds gamepad capability detection and preference storage for
ASUS ROG Ally devices, exposes the xbox_controller sysfs attribute, and
waits for the gamepad MCU to report ready during reset_resume.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 2a3e8c399f0aa..8277d7108bb97 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -762,6 +776,281 @@ static int ally_get_endpoint_address(struct hid_device *hdev)
>  	return ep->desc.bEndpointAddress;
>  }

[ ... ]

> +static int ally_detect_capabilities(struct hid_device *hdev)
> +{
> +	int ret;
> +
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	ally_prefs.is_ally_x = (hdev->product == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X);
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_XBOX_SUPPORT);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.xbox_controller_support = ret;
> +
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_USER_CAL_SUPPORT);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.user_cal_support = ret;
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_TURBO_SUPPORT);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.turbo_support = ret;
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_RESP_CURVE_SUPPORT);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.resp_curve_support = ret;
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_DIR_TO_BTN_SUPPORT);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.dir_to_btn_support = ret;
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_GYRO_TO_JOYSTICK);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.gyro_support = ret;
> +
> +	ret = ally_check_capability(hdev, CMD_CHECK_ANTI_DEADZONE);
> +	if (ret < 0)
> +		goto ally_detect_capabilities_err;
> +	ally_prefs.anti_deadzone_support = ret;
> +
> +	return 0;
> +ally_detect_capabilities_err:
> +	hid_err(hdev, "Failed to detect capabilities: %d\n", ret);
> +	return ret;
> +}

[Severity: Low]
This isn't a bug, but ally_detect_capabilities() in drivers/hid/hid-asus.c
mixes guard(mutex)(&ally_prefs.lock) with goto ally_detect_capabilities_err,
contrary to the cleanup.h convention of not mixing scope-based cleanup
helpers with goto error paths in the same function.

Since ally_check_capability() and the caller hid_asus_ally_probe() already
log hid_err() on failure, and ally_detect_capabilities_err performs no
resource cleanup, could each failed check in ally_detect_capabilities()
simply return ret directly?

[ ... ]

> +static int ally_prefs_setup(struct hid_device *hdev, struct ally_intf *intf)
> +{
> +	int ret, i;
> +
> +	ret = ally_detect_capabilities(hdev);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.initialized) {
> +			ally_prefs.gamepad_mode = 0x01;
> +			ally_prefs.xbox_controller_enabled = true;
> +			ally_prefs.initialized = true;
> +		}
> +
> +		if (intf->sysfs_live)
> +			return 0;
> +	}
> +
> +	/*
> +	 * Create and remove sysfs groups without ally_prefs.lock. Attribute
> +	 * methods take that lock while holding a kernfs active reference;
> +	 * removing a group drains those references, so locking here would
> +	 * invert the order and deadlock.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++) {
> +		ret = sysfs_create_group(&hdev->dev.kobj, &ally_attr_groups[i]);
> +		if (ret) {
> +			hid_err(hdev, "Failed to create Ally sysfs group '%s': %d\n",
> +				ally_attr_groups[i].name ?: "", ret);
> +			while (i--)
> +				sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
> +			return ret;
> +		}
> +	}
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		intf->sysfs_live = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ally_sysfs_remove(struct hid_device *hdev, struct ally_intf *intf)
> +{
> +	int i;
> +
> +	if (!intf)
> +		return;
> +
> +	if (ally_get_endpoint_address(hdev) != HID_ALLY_INTF_CFG_IN)
> +		return;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!intf->sysfs_live)
> +			return;
> +
> +		intf->sysfs_live = false;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
> +		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
> +}

[Severity: Low]
This isn't a bug, but ally_prefs_setup() and ally_sysfs_remove() in
drivers/hid/hid-asus.c open-code the iteration and error rollback over
ally_attr_groups using sysfs_create_group() and sysfs_remove_group().

Could ally_attr_groups be defined with ATTRIBUTE_GROUPS() (or a
NULL-terminated struct attribute_group pointer array) so ally_prefs_setup()
and ally_sysfs_remove() can use sysfs_create_groups() and
sysfs_remove_groups() instead?

[ ... ]

> @@ -944,6 +1233,14 @@ static int hid_asus_ally_init(struct hid_device *hdev)
>  	if (ret < 0)
>  		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
>  
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	if (ally_prefs.initialized && ally_prefs.xbox_controller_support) {
> +		ret = ally_set_xbox_controller(hdev, ally_prefs.xbox_controller_enabled);
> +		if (ret < 0)
> +			hid_warn(hdev, "Failed to apply Xbox controller mode: %d\n", ret);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1038,6 +1335,12 @@ static struct ally_intf *hid_asus_ally_probe(struct hid_device *hdev)
>  			return ERR_PTR(ret);
>  		}
>  
> +		ret = ally_prefs_setup(hdev, intf);
> +		if (ret < 0) {
> +			hid_err(hdev, "Failed to create Ally cfg: %d\n", ret);
> +			return ERR_PTR(ret);
> +		}
> +
>  		hid_asus_ally_init(hdev);
>  		break;
>  	case HID_ALLY_X_INTF_IN:
> @@ -1058,7 +1361,7 @@ static struct ally_intf *hid_asus_ally_probe(struct hid_device *hdev)
>  
>  static int hid_asus_ally_reset_resume(struct hid_device *hdev, struct ally_intf *intf)
>  {
> -	int ep;
> +	int ep, ret;
>  
>  	/*
>  	 * The extensions failed to probe and the device is operating as a
> @@ -1071,6 +1374,12 @@ static int hid_asus_ally_reset_resume(struct hid_device *hdev, struct ally_intf
>  	if (ep != HID_ALLY_INTF_CFG_IN)
>  		return 0;
>  
> +	ret = ally_gamepad_check_ready(hdev);
> +	if (ret < 0) {
> +		hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
> +		return ret;
> +	}
> +
>  	return hid_asus_ally_init(hdev);
>  }

[Severity: Medium]
Can a concurrent write to xbox_controller_store() race with the unlocked
feature report transfers in hid_asus_ally_reset_resume() and
hid_asus_ally_init()?

Once ally_prefs_setup() exposes the sysfs group on hdev->dev.kobj,
xbox_controller_store() remains live across suspend and reset_resume (as
well as during the rest of hid_asus_ally_probe()):

    xbox_controller_store()
      guard(mutex)(&ally_prefs.lock)
      ally_set_xbox_controller()
        ally_gamepad_send_packet()
          ally_dev_set_report() /* feature report 0x5A */

However, hid_asus_ally_reset_resume() runs ally_gamepad_check_ready()
without holding ally_prefs.lock, where each attempt performs a two-step
command and response sequence over USB control transfers:

    hid_asus_ally_reset_resume()
      ally_gamepad_check_ready()
        ally_gamepad_send_receive_packet()
          ally_dev_set_report() /* feature report 0x5A */
          ally_dev_get_report() /* feature report 0x0D */

Similarly, hid_asus_ally_init() sends ally_force_feedback_off via
ally_gamepad_send_packet() before acquiring ally_prefs.lock.

If userspace writes to xbox_controller during hid_asus_ally_reset_resume()
or hid_asus_ally_init(), can CMD_SET_XBOX_CONTROLLER be sent before the MCU
is ready or interleave between ally_dev_set_report() and
ally_dev_get_report() in ally_gamepad_check_ready(), causing the readiness
check or configuration command to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104013935.512713-1-denis.benato@linux.dev?part=3