Re: [PATCH v5] 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] Double-press detection falsely triggers on every single press during the first ~5 minutes of system uptime due to jiffies wrap and zero initialization.
- [Medium] Runtime mutation of a global, read-only module parameter from within a device-specific event handler without synchronization.

Pre-existing issues:
- [High] The input event handler calls a function that can sleep (`hid_hw_power`), violating the atomic context requirements of the input subsystem.
--

commit 8744bab7e31d541dd6e18fb45fd65fb8da3adc97
Author: Aditya Garg <aditya.garg@linux.dev>

HID: appletb-kbd: support layer switching on Fn double press

This commit adds an optional module parameter double_press_switch_time
to switch the default touch bar layer persistently upon a double-press
of the Fn key. It introduces new logic in the input event handler to
detect double presses.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 5cc27066f6026..02b938daeb2b3 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))) {

[Severity: High]
Does this time_before check falsely trigger on every single press during the
first ~5 minutes of system uptime?

On Linux, jiffies is initialized to a large negative number (e.g., -300 * HZ)
to catch wrapping bugs early. Because kbd->last_fn_press is 0 initially,
the check mathematically evaluates as:

(long)now - (long)timeout < 0

Since now is negative and timeout is positive, this evaluates to true on the
very first press, causing the double-press feature logic to completely fail
and convert single presses into double-press actions.

> +					appletb_tb_def_mode =
> +						appletb_switch_mode(appletb_tb_def_mode);

[Severity: Medium]
Does mutating the global read-only module parameter appletb_tb_def_mode
inside a device-specific event handler break per-device isolation?

Since this parameter is defined with 0444 permissions and intended to
configure default initialization, changing it dynamically bleeds the runtime
state of one device instance into the global configuration, applying it to
any future device probes or suspend/resume cycles.

> +
> +					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 callback cause a kernel panic?

The input core invokes event callbacks with the device's event_lock spinlock
held and interrupts disabled. Inside appletb_kbd_set_mode():

appletb_kbd_set_mode()
    hid_hw_power(..., PM_HINT_FULLON)
        usb_autopm_get_interface()
            pm_runtime_get_sync()

This translates into synchronous PM runtime resume calls which will sleep,
leading to a scheduling-while-atomic oops. Although this existed prior to
this patch for the fn_toggle feature, this patch adds new execution paths
that trigger the identical crash.

> +
> +					kbd->saved_mode = appletb_tb_def_mode;
> +					kbd->last_fn_press = 0;

[Severity: High]
Does resetting the tracker to 0 instead of a valid timestamp re-arm
the jiffies wrapping bug indefinitely?

Setting this back to 0 guarantees that subsequent single presses will
also falsely trigger as double-presses until jiffies eventually wraps
to a positive value.

> +
> +					return;
> +				}
> +
> +				kbd->last_fn_press = now;
> +			}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911200019.17028-1-aditya.garg@linux.dev?part=1