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

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

[Andre Eikmeyer]

From: Aditya Garg <gargaditya08@proton.me>

Holding Fn temporarily switches the Touch Bar between media controls and
function keys. Users who want to keep the alternate layer currently need
to change the mode through sysfs.

An optional double_press_switch_time module parameter makes a double press
persistently switch the default layer. Its value specifies the double-press
interval in milliseconds, while zero leaves the behavior disabled.

Signed-off-by: Aditya Garg <gargaditya08@proton.me>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
Changes in v3:
- Renamed the backing variable to match the module parameter.
- Fixed the reported code style issues.

 drivers/hid/hid-appletb-kbd.c | 54 ++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..b7bbb81a8d47 100644
--- a/drivers/hid/hid-appletb-kbd.c
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -56,6 +56,12 @@ static int appletb_tb_idle_timeout = 15;
 module_param_named(idle_timeout, appletb_tb_idle_timeout, int, 0644);
 MODULE_PARM_DESC(idle_timeout, "Idle timeout in sec");
 
+static int appletb_tb_double_press_switch_time;
+module_param_named(double_press_switch_time,
+		   appletb_tb_double_press_switch_time, int, 0644);
+MODULE_PARM_DESC(double_press_switch_time,
+		 "Fn double-press interval in ms (0 disables layer switching)");
+
 struct appletb_kbd {
 	struct hid_field *mode_field;
 	struct input_handler inp_handler;
@@ -68,6 +74,7 @@ struct appletb_kbd {
 	bool has_turned_off;
 	u8 saved_mode;
 	u8 current_mode;
+	unsigned long last_fn_press;
 };
 
 static const struct key_entry appletb_kbd_keymap[] = {
@@ -243,6 +250,18 @@ static int appletb_kbd_hid_event(struct hid_device *hdev, struct hid_field *fiel
 	return kbd->current_mode == APPLETB_KBD_MODE_OFF;
 }
 
+static u8 appletb_switch_mode(u8 mode)
+{
+	switch (mode) {
+	case APPLETB_KBD_MODE_SPCL:
+		return APPLETB_KBD_MODE_FN;
+	case APPLETB_KBD_MODE_FN:
+		return APPLETB_KBD_MODE_SPCL;
+	default:
+		return mode;
+	}
+}
+
 static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type,
 			      unsigned int code, int value)
 {
@@ -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);
+
+					appletb_kbd_set_mode(kbd, appletb_tb_def_mode);
+
+					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);
 		}
 	}
-- 
2.55.0

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

[sashiko-bot]

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