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

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

[Aditya Garg]

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 <aditya.garg@linux.dev>
Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
Changes in v2:
- Added a check to ensure negative fn switch times are ignored.

Changes in v3:
- Renamed the backing variable to match the module parameter.
- Fixed the reported code style issues.

Changes in v4:
- Added missing "break;" in case statements.
- Change author's email to aditya.garg@linux.dev

Changes in v5:
- Remove the break; statements added by mistake in v4.

 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 5cc27066f..02b938dae 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 v5] 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] 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