[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