Re: [PATCH 6/7] hid-asus: Allow adding custom hotkey handler logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling platform_profile_cycle() from an atomic context causes a 'scheduling while atomic' bug.
- [High] Missing Kconfig dependency on ACPI_PLATFORM_PROFILE leads to a linker error.
- [Medium] Improper HID event return value causes spurious error logs and incorrect event propagation.
- [Low] Dead code due to unreachable break statement.
--

commit f4dcdddfbfb9d5acd09f6a961fcad0539b0cd8c7
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>

hid-asus: Allow adding custom hotkey handler logic

This commit modifies the Asus HID driver to handle a custom vendor hotkey
for cycling the platform profile when a specific usage code is received.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index fe52fd63d61c6..37e3aec612b6d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -29,6 +29,7 @@
>  #include <linux/input/mt.h>
>  #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
>  #include <linux/power_supply.h>
> +#include <linux/platform_profile.h>

Does the HID_ASUS Kconfig option need to depend on or select
ACPI_PLATFORM_PROFILE? If the kernel is built with
CONFIG_ACPI_PLATFORM_PROFILE=n, won't this result in a linker error since
platform_profile_cycle() is used unconditionally and there are no empty dummy
stubs provided in the header?

[ ... ]

> @@ -362,8 +363,21 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>  	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>  	    (usage->hid & HID_USAGE) != 0x00 &&
>  	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> -		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> -			 usage->hid & HID_USAGE);
> +
> +		/*
> +		 * Some reports do not map directly to standard keys, and need special
> +		 * handling.
> +		 */
> +		switch (usage->hid & HID_USAGE) {
> +			case 0x9d:
> +				if (!value)
> +					break;
> +				return platform_profile_cycle();

Is it safe to call platform_profile_cycle() from here?

The asus_event() callback is invoked by the HID core in an atomic softirq
context when processing USB URB completions. However,
platform_profile_cycle() acquires profile_lock, which is a mutex. Will this
result in a scheduling while atomic panic? Should this action be deferred to
a workqueue, similar to how fn_lock_sync_work is handled?

Also, if platform_profile_cycle() returns a negative error code (like -EINVAL
when no profiles are registered), returning it directly here will cause
hid_process_event() to spam the kernel log with error messages on every key
press. Conversely, if it returns 0 on success, won't the HID core incorrectly
continue processing this event instead of treating it as consumed? Does this
need to handle the return value and return 1 to halt further event
processing?

> +				break;

This isn't a bug, but this break statement is unreachable since the previous
line always returns.

> +			default:
> +				hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> +					 usage->hid & HID_USAGE);
> +		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=6