Re: [PATCH v11 02/18] HID: asus: add gamepad configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: asus: premature cfg_hdev role release in hid_asus_ally_remove()
--

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

HID: asus: add gamepad configuration

This commit adds the base configuration structures and sysfs attribute
groups for ROG Ally gamepad configuration, detects hardware capabilities,
and initializes the controller into a known state.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 9e6796bbbdcbd..6d1007433daa8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1162,8 +1531,25 @@ static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *
>  		ally->keyboard_input = NULL;
>  		ally->keyboard_hdev = NULL;
>  	}
> +
> +	owns_cfg = ally->cfg_hdev == hdev;
> +	if (owns_cfg) {
> +		cfg = ally->config;
> +		ally->cfg_hdev = NULL;
> +		ally->config = NULL;
> +	}
> +
>  	spin_unlock_irqrestore(&ally_data_lock, flags);
>  
> +	/*
> +	 * The config teardown removes sysfs groups and takes sleeping locks:
> +	 * it must not run under ally_data_lock. The config pointer has been
> +	 * unpublished above, so no new sysfs callback can find it while the
> +	 * groups are being removed.
> +	 */
> +	if (owns_cfg)
> +		ally_config_remove(hdev, cfg);

[Severity: Medium]
In hid_asus_ally_remove(), should ally->cfg_hdev be cleared only after
ally_config_remove(hdev, cfg) completes, rather than before it?

Clearing ally->cfg_hdev before ally_config_remove() releases the
HID_ALLY_INTF_CFG_IN role while sysfs_remove_group() is still sleeping (for
example, waiting in kernfs_drain() for an in-flight xbox_controller_store()
callback). During that window:

1. A concurrent hid_asus_ally_remove() on the last sibling interface sees
   ally_drvdata.cfg_hdev == NULL and ally_drvdata.cfg_probe_in_progress ==
   false in ally_put_udev_if_orphaned(), dropping ally_drvdata.udev early.

2. A concurrent hid_asus_ally_probe() for HID_ALLY_INTF_CFG_IN (from a
   re-enumerated device or duplicate endpoint) is no longer blocked by the
   role check:

	if ((ep == HID_ALLY_INTF_CFG_IN &&
	     (ally_drvdata.cfg_hdev || ally_drvdata.cfg_probe_in_progress)) ||

   and can enter ally_config_create() concurrently with ally_config_remove().

That allows ally_config_create() to mutate the singleton ally_config
(including cfg->initialized and the capability flags) and re-publish
ally->config = cfg while the old hdev's sysfs groups are still draining,
which also lets sysfs callbacks on the unbinding hdev see a non-NULL
ally_get_config() again.

By comparison, the hid_asus_ally_init() error path in hid_asus_ally_probe()
clears ally_drvdata.config = NULL under ally_data_lock, calls
ally_config_remove(), and only afterwards releases the role via
ally_probe_release_role(ep) and ally_put_udev_if_orphaned().

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