Re: [PATCH v3] HID: appletb-kbd: support layer switching on Fn double press

[sashiko-bot@kernel.org]

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

New issues:
- [High] The double-press logic mutates a read-only global module parameter dynamically from a per-device interrupt-driven event handler, causing a data race and state corruption.
- [Medium] Failure of `appletb_kbd_set_mode()` is ignored in the double-press path, causing software tracking state to desynchronize from actual hardware state.

Pre-existing issues:
- [High] Calling `appletb_kbd_set_mode()` from the atomic input event handler causes a sleep in atomic context (BUG: scheduling while atomic).
--

--- Patch [1]: [PATCH v3] HID: appletb-kbd: support layer switching on Fn double press ---
commit 3f8289667f750dcd9fc87072f493735127cd7f85
Author: Aditya Garg <gargaditya08@proton.me>

HID: appletb-kbd: support layer switching on Fn double press
    
Holding Fn temporarily switches the Touch Bar between media controls and
function keys. An optional double_press_switch_time module parameter makes
a double press persistently switch the default layer.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..b7bbb81a8d473 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c

[ ... ]

> @@ -250,15 +269,36 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
>  
>  	reset_inactivity_timer(kbd);
>  
> -	if (type == EV_KEY && code == KEY_FN && appletb_tb_fn_toggle &&
> -		(kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> -		 kbd->current_mode == APPLETB_KBD_MODE_FN)) {
> +	if (type == EV_KEY && code == KEY_FN &&
> +	    (kbd->current_mode == APPLETB_KBD_MODE_SPCL ||
> +	     kbd->current_mode == APPLETB_KBD_MODE_FN)) {
>  		if (value == 1) {
> -			kbd->saved_mode = kbd->current_mode;
> -			appletb_kbd_set_mode(kbd, kbd->current_mode == APPLETB_KBD_MODE_SPCL
> -						? APPLETB_KBD_MODE_FN : APPLETB_KBD_MODE_SPCL);
> +			if (appletb_tb_double_press_switch_time > 0) {
> +				unsigned long now = jiffies;
> +
> +				if (time_before(now, kbd->last_fn_press +
> +					msecs_to_jiffies(appletb_tb_double_press_switch_time))) {
> +					appletb_tb_def_mode =
> +						appletb_switch_mode(appletb_tb_def_mode);

[Severity: High]
Does this double-press logic mutate a read-only global module parameter
dynamically from a per-device interrupt-driven event handler? Modifying
it changes the initialization default for any subsequently probed devices.

If a user changes the mode via sysfs, a subsequent double-press will blindly
toggle the global variable and overwrite the user's per-device sysfs
configuration.

> +
> +					appletb_kbd_set_mode(kbd, appletb_tb_def_mode);

[Severity: High]
This is a pre-existing issue, but does calling appletb_kbd_set_mode() from
the atomic input event handler cause a sleep in atomic context?

The input core invokes appletb_kbd_inp_event() from input_pass_values()
which holds dev->event_lock (spinlock) and disables local interrupts.

The callback then calls appletb_kbd_set_mode(), which calls
hid_hw_power(hdev, PM_HINT_FULLON). For USB HID devices, this maps to
usbhid_power(), which calls usb_autopm_get_interface(), subsequently
calling pm_runtime_get_sync() which sleeps.

[Severity: Medium]
Is the return value of appletb_kbd_set_mode() ignored here? If the hardware
mode switch fails, the software tracking variables are updated anyway,
causing the software to assume the mode switched successfully.

> +
> +					kbd->saved_mode = appletb_tb_def_mode;
> +					kbd->last_fn_press = 0;
> +
> +					return;
> +				}
> +
> +				kbd->last_fn_press = now;
> +			}
> +			if (appletb_tb_fn_toggle) {
> +				kbd->saved_mode = kbd->current_mode;
> +
> +				appletb_kbd_set_mode(kbd, appletb_switch_mode(kbd->current_mode));
> +			}
>  		} else if (value == 0) {
> -			if (kbd->saved_mode != kbd->current_mode)
> +			if (appletb_tb_fn_toggle &&
> +			    kbd->saved_mode != kbd->current_mode)
>  				appletb_kbd_set_mode(kbd, kbd->saved_mode);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722141221.13844-1-dev@deq.rocks?part=1