Re: [PATCH v5 04/13] HID: asus: add gamepad configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State initialization and restoration logic errors corrupt user settings on re-probe.
- [Medium] Sysfs attributes are exposed to userspace before the device data is fully linked, causing spurious -ENODEV errors.
--

commit 396e8f687c336ca0b0f9ddcf54fbc96925c6f82f
Author: Denis Benato <denis.benato@linux.dev>

    HID: asus: add gamepad configuration
    
    Add the base configuration structures for the gamepad configuration,
    detect capabilities and initialize the device in a known state.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 129cec7e27ec4..4714ebb4a5435 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -610,6 +662,272 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
[ ... ]
> +	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
> +		ret = sysfs_create_group(&hdev->dev.kobj,
> +					 &ally_attr_groups[sysfs_i]);
> +		if (ret < 0) {
> +			hid_err(hdev, "Failed to create sysfs group '%s': %d\n",
> +				ally_attr_groups[sysfs_i].name ?: "", ret);
> +			goto ally_config_create_sysfs_err;
> +		}
> +	}

[Severity: Medium]
Will this expose sysfs attributes to userspace before the device data is fully
linked?

Since ally_drvdata.config is not assigned until later in hid_asus_ally_probe(),
if a userspace tool like udev reads an attribute in this window,
ally_get_config() will return NULL, causing a spurious -ENODEV error.

> +
> +	/* So far the only hardware this is supported is the Ally 1 */
> +	if (cfg->xbox_controller_support) {
> +		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
> +		if (ret < 0)
> +			hid_warn(hdev, "Failed to set default Xbox controller mode: %d\n",
> +				ret);
> +	}
> +
> +	cfg->initialized = true;

[Severity: High]
Does this code unconditionally overwrite the user's previously saved preference
back to true on every probe?

Because this block is outside the `if (!cfg->initialized)` check above, it
seems it will force the hardware and software state to true whenever the device
suspends/resumes or re-enumerates, destroying any saved settings.

[ ... ]
> @@ -882,6 +1201,20 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  	if (ret < 0)
>  		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
>  
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return 0;

[Severity: High]
Will this silently fail to restore cached user settings to the hardware during
probe?

Because hid_asus_ally_init() is called before ally_drvdata.config is published
in hid_asus_ally_probe(), ally_get_config() will return NULL here, causing it
to bail out early.

> +
> +	/* The MCU may have just been reset: restore the cached state. */
> +	guard(mutex)(&cfg->config_mutex);
> +
> +	if (cfg->xbox_controller_enabled) {
> +		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
> +		if (ret < 0)
> +			hid_warn(hdev, "Failed to restore Xbox controller mode: %d\n",
> +				 ret);
> +	}
> +
>  	return 0;
>  }

[Severity: High]
If the hardware reset to true but the user had disabled the Xbox controller
(cfg->xbox_controller_enabled == false), does this incorrectly fail to restore
the state to false?

It looks like this code only restores the state if it was true, and hardcodes
the value passed to ally_set_xbox_controller() to true instead of passing
cfg->xbox_controller_enabled.

[ ... ]
> @@ -1023,6 +1360,14 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  			return ERR_PTR(ret);
>  		}
>  
> +		ally_cfg = ally_config_create(hdev, &ally_drvdata);
> +		if (IS_ERR(ally_cfg)) {
> +			hid_err(hdev, "Failed to create Ally cfg: %ld\n",
> +				PTR_ERR(ally_cfg));
> +			ally_put_udev_if_orphaned();
> +			return ERR_PTR(PTR_ERR(ally_cfg));
> +		}
> +
>  		ret = hid_asus_ally_init(hdev, &ally_drvdata);
>  		if (ret < 0) {
>  			ally_put_udev_if_orphaned();
> @@ -1030,6 +1375,7 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  		}
>  
>  		spin_lock_irqsave(&ally_data_lock, flags);
> +		ally_drvdata.config = ally_cfg;

[Severity: High]
This publishes the configuration pointer to the shared state after
hid_asus_ally_init() has already run, which hides it from the initialization
function above.

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