Re: [PATCH v4 07/13] HID: asus: add triggers inner and outer range configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The configuration pointer ally_drvdata.config is never initialized, causing all sysfs configurations to silently fail.
- [Medium] Sysfs attributes are registered before their backing driver state is initialized.
--

commit 80d5926aadeb16828c5f704e5f821fff45eb12b6
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add triggers inner and outer range configuration

This patch allows userspace to configure the inner and outer ranges for
the trigger buttons on ROG Ally devices by exposing new sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 7702427bde451..6688615f644a1 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1318,6 +1318,350 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
[ ... ]
> +static ssize_t left_trigger_range_lower_limit_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;

[Severity: High]
This is a pre-existing issue, but does ally_drvdata.config ever get
initialized?

In hid_asus_ally_probe(), the pointer returned by ally_config_create() is
stored in a local variable ally_cfg, but it is never assigned to the global
state struct:

drivers/hid/hid-asus.c:hid_asus_ally_probe() {
    ...
        spin_lock_irqsave(&ally_data_lock, flags);
        ally_drvdata.cfg_hdev = hdev;
        spin_unlock_irqrestore(&ally_data_lock, flags);
        break;
    ...
}

As a result, ally_get_config() permanently returns NULL, causing every sysfs
callback, including this new one, to immediately abort with -ENODEV.

[ ... ]
> @@ -1429,6 +1801,10 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  	cfg->left_outer_threshold = 90;
>  	cfg->right_deadzone = 10;
>  	cfg->right_outer_threshold = 90;
> +	cfg->left_trigger_min = 0;
> +	cfg->left_trigger_max = 100;
> +	cfg->right_trigger_min = 0;
> +	cfg->right_trigger_max = 100;

[Severity: Medium]
This is a pre-existing issue, but do these driver states get initialized too
late?

The sysfs nodes for the trigger ranges become active and accessible to
userspace earlier in ally_config_create() when devm_device_add_group() is
called:

drivers/hid/hid-asus.c:ally_config_create() {
    ...
    ret = devm_device_add_group(&hdev->dev,
                                ally_cal_attr_groups[sysfs_i]);
    ...
}

This creates a window where udev or another userspace process can access the
newly created sysfs nodes immediately upon registration and read uninitialized
defaults before the configuration is initialized here.

>  	cfg->vibration_intensity_left = 100;
>  	cfg->vibration_intensity_right = 100;
>

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