[PATCH] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[PATCH] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[Aditya Garg]

From: Aditya Garg <gargaditya08@proton.me>

This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.

Signed-off-by: Aditya Garg <gargaditya08@proton.me>
---
 drivers/hid/hid-appletb-kbd.c | 60 +++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75..4f900338b 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_layers;
+module_param_named(double_press_switch_time,
+		   appletb_tb_double_press_switch_layers, int, 0644);
+MODULE_PARM_DESC(double_press_switch_time, "Time in ms within which if fn key is double "
+					   "pressed will switch layers");
+
 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,42 @@ 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_layers) {
+				unsigned long now = jiffies;
+
+				if (time_before(now, kbd->last_fn_press +
+					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
+
+					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.53.0

Re: [PATCH] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[sashiko-bot]

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

New issues:
- [High] Zero-initialized timestamp causes single keypresses to be erroneously treated as double-presses during the first 5 minutes of system uptime.
- [Medium] Dynamic, lockless modification of a read-only global module parameter from a per-device event handler.
- [Low] The module parameter `double_press_switch_time` is defined as a signed integer but used as an unsigned value, allowing negative values to cause abnormally large timeouts.

Pre-existing issues:
- [High] The input event handler calls a function that can sleep, leading to a schedule-while-atomic bug because input handlers execute with spinlocks held and interrupts disabled.
--

commit ff52ac5f6303e0ff3ca2a68218cce8a8beb04bf2
Author: Aditya Garg <gargaditya08@proton.me>

HID: appletb-kbd: add option to switch default layer on double pressing fn key

This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key within a configurable
time interval.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..4f900338b5871 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_layers;
> +module_param_named(double_press_switch_time,
> +		   appletb_tb_double_press_switch_layers, int, 0644);

[Severity: Low]
If this parameter is set to a negative value by root at runtime, will it
be implicitly cast to a massive unsigned value when passed to
msecs_to_jiffies() below?

This could result in an extremely large timeout (e.g. 49 days) where every
sequential press acts as a double-press. Should this parameter be typed as
a uint or have bounds checking applied?

[ ... ]
> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
[ ... ]
> +			if (appletb_tb_double_press_switch_layers) {
> +				unsigned long now = jiffies;
> +
> +				if (time_before(now, kbd->last_fn_press +
> +					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {

[Severity: High]
Does this logic correctly handle the first 5 minutes of system uptime?

Because the kernel initializes jiffies to a large negative value (typically
-300 * HZ to detect wraparound bugs), and kbd->last_fn_press is initialized
and reset to 0, won't time_before() evaluate to unconditionally true during
early boot?

This might cause every single keypress to be falsely treated as a
double-press until the system has been running long enough for jiffies to
become positive.

> +
> +					appletb_tb_def_mode =
> +						appletb_switch_mode(
> +							appletb_tb_def_mode);

[Severity: Medium]
Is it safe to modify appletb_tb_def_mode here?

This variable is exposed as a module parameter with 0444 (read-only)
permissions. Modifying it programmatically alters the default state for all
current and future connected instances of the device based on a single
device's event.

Should this default mode be managed as a per-device state inside
struct appletb_kbd instead?

> +
> +					appletb_kbd_set_mode(kbd,
> +						appletb_tb_def_mode);

[Severity: High]
This is a pre-existing issue, but does calling appletb_kbd_set_mode() here
cause a scheduling while atomic bug?

This handler executes with dev->event_lock held and interrupts disabled.
However, appletb_kbd_set_mode() calls hid_hw_power(..., PM_HINT_FULLON),
which for USB HID devices invokes usbhid_power() and
usb_autopm_get_interface().

Since those functions lock a mutex and can sleep while waiting on power
management operations, calling them from an atomic context might cause a
kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521132447.19241-1-gargaditya08@proton.me?part=1

[PATCH v2] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[Aditya Garg]

From: Aditya Garg <gargaditya08@proton.me>

This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter whose
value depicts the time in milliseconds within which the fn key must be
pressed again to switch the default layer. If set to 0, it simply
disables this behaviour.

Signed-off-by: Aditya Garg <gargaditya08@proton.me>
---

v2: added a check to ensure negative fn switch times are ignored.

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

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..34bfc595949d 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_layers;
+module_param_named(double_press_switch_time,
+		   appletb_tb_double_press_switch_layers, int, 0644);
+MODULE_PARM_DESC(double_press_switch_time, "Time in ms within which if fn key is double "
+					   "pressed will switch layers");
+
 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,42 @@ 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_layers > 0) {
+				unsigned long now = jiffies;
+
+				if (time_before(now, kbd->last_fn_press +
+					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {
+
+					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);
 		}
 	}

Range-diff against v1:
1:  5c9d8c8ba8c0 ! 1:  4f6a818b98b9 HID: appletb-kbd: add option to switch default layer on double pressing fn key
    @@ drivers/hid/hid-appletb-kbd.c: static void appletb_kbd_inp_event(struct input_ha
     -			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_layers) {
    ++			if (appletb_tb_double_press_switch_layers > 0) {
     +				unsigned long now = jiffies;
     +
     +				if (time_before(now, kbd->last_fn_press +
-- 
2.53.0

Re: [PATCH v2] HID: appletb-kbd: add option to switch default layer on double pressing fn key

[sashiko-bot]

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

New issues:
- [Medium] Dynamic mutation of a read-only global module parameter from a per-device atomic event handler without synchronization.
- [Medium] Incorrect double-press detection during early system uptime on 32-bit systems due to jiffies wrap-around with a 0-initialized timestamp.
- [Low] Intervening non-Fn key presses do not reset the double-press detection timer.

Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power` and `hid_hw_request`) from an atomic context causes a kernel panic.
- [High] A race condition during `probe()` allows incoming HID events to trigger a NULL pointer dereference.
--

commit 9461d5707b93702971195b71e23ddea06392c720
Author: Aditya Garg <gargaditya08@proton.me>

HID: appletb-kbd: add option to switch default layer on double pressing fn key

This patch enables a user to switch the default layer from media to fn
keys and vice-versa upon double pressing the fn key. This behaviour can
be configured using the double_press_switch_time module parameter.

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

[ ... ]

> @@ -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;
>  }
>  

[Severity: High]
This is a pre-existing issue, but does a race condition during probe() allow
incoming HID events to trigger a NULL pointer dereference?

If hid_hw_start() and hid_hw_open() are called before hid_set_drvdata() is
executed, an incoming keyboard event might invoke appletb_kbd_hid_event()
before probe() finishes. This would call hid_get_drvdata(hdev) returning
NULL, which is then passed to reset_inactivity_timer() and unconditionally
dereferenced.

> +static u8 appletb_switch_mode(u8 mode)
> +{

[ ... ]

> @@ -250,15 +269,42 @@ 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)) {
> +

[Severity: Low]
Is it possible that intervening non-Fn key presses should reset the
double-press detection timer?

If a user presses Fn, types another key, and then presses Fn again within
the delay timeout, the driver might erroneously interpret this as a
double-press and switch the layer.

>  		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_layers > 0) {
> +				unsigned long now = jiffies;
> +
> +				if (time_before(now, kbd->last_fn_press +
> +					msecs_to_jiffies(appletb_tb_double_press_switch_layers))) {

[Severity: Medium]
Could this double-press detection trigger incorrectly during early system
uptime on 32-bit systems?

Since last_fn_press is zero-initialized and jiffies starts at a large
negative value, time_before() might evaluate to true for the first 5
minutes of uptime.

This would cause any single press of the Fn key to erroneously trigger a
double-press layer switch.

> +
> +					appletb_tb_def_mode =
> +						appletb_switch_mode(
> +							appletb_tb_def_mode);

[Severity: Medium]
Is it safe to dynamically mutate the appletb_tb_def_mode module parameter
from a per-device atomic event handler without synchronization?

Since appletb_tb_def_mode is a global variable exported as read-only,
modifying it dynamically could violate its sysfs contract and introduce a
data race if multiple keyboards are used concurrently.

This would also improperly alter the default mode for all current and future
connected devices.

> +
> +					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
this atomic context cause a kernel panic?

The appletb_kbd_inp_event handler is invoked by the input subsystem with
the dev->event_lock spinlock held and interrupts disabled.

appletb_kbd_set_mode() calls hid_hw_power() and hid_hw_request(). For USB
HID devices, hid_hw_power() may sleep and will warn in atomic context,
potentially leading to a scheduling while atomic panic.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526154954.29002-1-gargaditya08@proton.me?part=1