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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] HID: asus: ABBA deadlock holding ally_prefs.lock in ally_sysfs_remove()
- [Low] HID: asus: inconsistent threshold defines and callback duplication
--

commit 21c1f6aa8c8abefa0a006966f1d360a594341b9e
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add joysticks inner and outer range configuration

This commit adds sysfs attributes under left_joystick_axis and
right_joystick_axis on ROG Ally devices to configure and restore the inner
and outer thresholds for both joysticks.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index fe2008f9d51d1..50b0e60405958 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -98,6 +98,9 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  
>  #define ASUS_HID_INPUT_KBD_REPORT_ID 0x01
>  
> +#define ASUS_ALLY_OUTER_THRESHOLD_MIN 70
> +#define ASUS_ALLY_OUTER_THRESHOLD_MAX 100
> +

[ ... ]

> @@ -826,7 +834,6 @@ static int ally_detect_capabilities(struct hid_device *hdev)
>  	return 0;
>  }
>  
> -/* Caller holds ally_prefs.lock. The packet is sent on @hdev. */
>  static int ally_set_xbox_controller(struct hid_device *hdev, bool enabled)
>  {
>  	const u8 payload[] = { enabled ?
> @@ -1042,6 +1049,304 @@ static struct attribute *ally_right_vibration_attrs[] = {

[ ... ]

> +static ssize_t left_joystick_inner_threshold_store(struct device *dev,
> +						   struct device_attribute *attr,
> +						   const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 50)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.user_cal_support)
> +			return -EOPNOTSUPP;
> +
> +		ret = ally_set_joystick_thresholds(hdev, value,
> +						   ally_prefs.left_outer_threshold,
> +						   ally_prefs.right_deadzone,
> +						   ally_prefs.right_outer_threshold);
> +		if (ret)
> +			return ret;
> +
> +		ally_prefs.left_deadzone = value;
> +	}
> +
> +	sysfs_notify(&dev->kobj, "left_joystick_axis", "inner_threshold");
> +
> +	return count;
> +}
> +
> +static ssize_t left_joystick_inner_threshold_range_show(struct device *dev,
> +							struct device_attribute *attr,
> +							char *buf)
> +{
> +	return sysfs_emit(buf, "0 50\n");
> +}
> +
> +static ssize_t left_joystick_outer_threshold_show(struct device *dev,
> +						  struct device_attribute *attr,
> +						  char *buf)
> +{
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	if (!ally_prefs.user_cal_support)
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "%u\n", ally_prefs.left_outer_threshold);
> +}
> +
> +static ssize_t left_joystick_outer_threshold_store(struct device *dev,
> +						   struct device_attribute *attr,
> +						   const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value < 70 || value > 100)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.user_cal_support)
> +			return -EOPNOTSUPP;
> +
> +		ret = ally_set_joystick_thresholds(hdev, ally_prefs.left_deadzone,
> +						   value, ally_prefs.right_deadzone,
> +						   ally_prefs.right_outer_threshold);
> +		if (ret)
> +			return ret;
> +
> +		ally_prefs.left_outer_threshold = value;
> +	}
> +
> +	sysfs_notify(&dev->kobj, "left_joystick_axis", "outer_threshold");
> +
> +	return count;
> +}
> +
> +static ssize_t left_joystick_outer_threshold_range_show(struct device *dev,
> +							struct device_attribute *attr,
> +							char *buf)
> +{
> +	return sysfs_emit(buf, "70 100\n");
> +}

[ ... ]

> +static ssize_t right_joystick_inner_threshold_store(struct device *dev,
> +						    struct device_attribute *attr,
> +						    const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 50)
> +		return -EINVAL;

[ ... ]

> +static ssize_t right_joystick_inner_threshold_range_show(struct device *dev,
> +							 struct device_attribute *attr,
> +							 char *buf)
> +{
> +	return sysfs_emit(buf, "0 50\n");
> +}

[ ... ]

> +static ssize_t right_joystick_outer_threshold_store(struct device *dev,
> +						    struct device_attribute *attr,
> +						    const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value < ASUS_ALLY_OUTER_THRESHOLD_MIN || value > ASUS_ALLY_OUTER_THRESHOLD_MAX)
> +		return -EINVAL;

[ ... ]

> +static ssize_t right_joystick_outer_threshold_range_show(struct device *dev,
> +							 struct device_attribute *attr,
> +							 char *buf)
> +{
> +	return sysfs_emit(buf, "%u %u\n", ASUS_ALLY_OUTER_THRESHOLD_MIN,
> +			  ASUS_ALLY_OUTER_THRESHOLD_MAX);
> +}

[Severity: Low]
As reported by Ilpo Järvinen on v11 [1], this isn't a bug, but
ASUS_ALLY_OUTER_THRESHOLD_MIN and ASUS_ALLY_OUTER_THRESHOLD_MAX are only
used in right_joystick_outer_threshold_store() and
right_joystick_outer_threshold_range_show(), while
left_joystick_outer_threshold_store() and
left_joystick_outer_threshold_range_show() still use literal 70 and 100.

Similarly, the inner threshold upper bound of 50 is hardcoded in
left_joystick_inner_threshold_store(),
left_joystick_inner_threshold_range_show(),
right_joystick_inner_threshold_store(), and
right_joystick_inner_threshold_range_show(), and the left and right range
show callbacks are identical.

Could these threshold bounds all use shared defines, share the range show
callbacks between left and right joysticks, and keep the locking comment
above ally_set_xbox_controller()?

[ ... ]

> @@ -1093,6 +1399,27 @@ static int ally_prefs_setup(struct hid_device *hdev)
>  		}
>  	}
>  
> +	/* Skip the calibration groups when the capability is missing. */
> +	if (ally_prefs.user_cal_support) {
> +		int cal_i;
> +
> +		for (cal_i = 0; cal_i < ARRAY_SIZE(ally_cal_attr_groups); cal_i++) {
> +			ret = sysfs_create_group(&hdev->dev.kobj,
> +						 ally_cal_attr_groups[cal_i]);
> +			if (ret) {
> +				hid_err(hdev, "Failed to create Ally sysfs group '%s': %d\n",
> +					ally_cal_attr_groups[cal_i]->name, ret);
> +				while (cal_i--)
> +					sysfs_remove_group(&hdev->dev.kobj,
> +							   ally_cal_attr_groups[cal_i]);
> +				for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
> +					sysfs_remove_group(&hdev->dev.kobj,
> +							   &ally_attr_groups[i]);
> +				return ret;
> +			}
> +		}
> +	}
> +
>  	ally_prefs.sysfs_live = true;
>  	return 0;
>  }
> @@ -1104,15 +1431,21 @@ static void ally_sysfs_remove(struct hid_device *hdev)
>  	if (ally_get_endpoint_address(hdev) != HID_ALLY_INTF_CFG_IN)
>  		return;
>  
> -	scoped_guard(mutex, &ally_prefs.lock) {
> -		if (!ally_prefs.sysfs_live)
> -			return;
> +	guard(mutex)(&ally_prefs.lock);
>  
> -		ally_prefs.sysfs_live = false;
> -	}
> +	if (!ally_prefs.sysfs_live)
> +		return;
> +
> +	ally_prefs.sysfs_live = false;
>  
>  	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
>  		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
> +
> +	if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support ||
> +	    ally_prefs.resp_curve_support) {
> +		for (i = 0; i < ARRAY_SIZE(ally_cal_attr_groups); i++)
> +			sysfs_remove_group(&hdev->dev.kobj, ally_cal_attr_groups[i]);
> +	}
>  }

[Severity: High]
Can changing scoped_guard(mutex, &ally_prefs.lock) to
guard(mutex)(&ally_prefs.lock) in ally_sysfs_remove() (and calling
sysfs_remove_group() under ally_prefs.lock in the error path of
ally_prefs_setup()) cause an ABBA deadlock with concurrent sysfs reads or
writes?

When userspace reads or writes an Ally sysfs attribute such as
left_joystick_inner_threshold_show() or
left_joystick_inner_threshold_store(), kernfs acquires the active reference
on the kernfs_node before invoking the callback, which then locks
ally_prefs.lock:

  kernfs_fop_read_iter() / kernfs_fop_write_iter()
    kernfs_get_active()
      left_joystick_inner_threshold_show()
        mutex_lock(&ally_prefs.lock)

When ally_sysfs_remove() or the error rollback in ally_prefs_setup() runs
concurrently, it holds ally_prefs.lock while calling sysfs_remove_group(),
which waits in kernfs_drain() for all active references on those attributes
to finish:

  ally_sysfs_remove()
    mutex_lock(&ally_prefs.lock)
    sysfs_remove_group()
      __kernfs_remove()
        kernfs_drain()

Should ally_prefs.lock be released before calling sysfs_remove_group()?

[1] https://lore.kernel.org/r/2bf5c5f8-185e-c322-dd76-ec182514c93c@linux.intel.com

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