Re: [PATCH v11 02/18] HID: asus: add gamepad configuration
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-26 00:10:47
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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