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

[PATCH v2 RESEND] 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 RESEND] 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:
- [Medium] The driver dynamically mutates a read-only global module parameter without synchronization.
- [Medium] Zero-initialization of `kbd->last_fn_press` paired with jiffies wrap logic causes spurious double-press detections during the first 5 minutes of system uptime.

Pre-existing issues:
- [High] Calling sleeping functions (`hid_hw_power`, `pm_runtime_get_sync`) from the atomic input event handler causes 'BUG: scheduling while atomic'.
- [High] Probe enables hardware events before setting driver data, causing a NULL pointer dereference.
--

commit e33a32d065a3f43246ae8063aed73e4573a2176b
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 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.

> 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

[ ... ]

> @@ -250,15 +269,42 @@ static void appletb_kbd_inp_event(struct input_handle *handle, unsigned int type
>  			      unsigned int code, int value)
>  {
>  	struct appletb_kbd *kbd = handle->private;
>  
>  	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))) {

[Severity: Medium]
Since the system jiffies timer initializes to a negative value at boot, does
evaluating time_before() with a zero-initialized kbd->last_fn_press cause
spurious double-press detections during the first 5 minutes of system uptime?

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

[Severity: Medium]
Since appletb_tb_def_mode is a global module parameter with read-only
permissions, does dynamically reassigning it here violate its read-only
contract and globally modify the default layer for all device instances
instead of applying the state locally to the specific kbd instance?

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

[Severity: High]
This isn't a bug introduced by this patch, but does appletb_kbd_set_mode()
unconditionally call hid_hw_power(), which can sleep via pm_runtime_get_sync()?

Since the input core invokes appletb_kbd_inp_event() from atomic context with
the dev->event_lock spinlock held and local IRQs disabled, can this trigger a
scheduling while atomic bug?

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

[Severity: Medium]
Does explicitly resetting last_fn_press back to 0 here cause the spurious
evaluations to continue for subsequent presses within the first 5 minutes of
uptime?

> +
> +					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);
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does appletb_kbd_probe() enable hardware
events before setting the driver data?

drivers/hid/hid-appletb-kbd.c:appletb_kbd_probe() {
    ...
    ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
    ...
    hid_set_drvdata(hdev, kbd);
    ...
}

Once the hardware is opened, it can immediately begin delivering HID reports
which invoke appletb_kbd_hid_event(). Could appletb_kbd_hid_event() fetch an
uninitialized NULL pointer from hid_get_drvdata(hdev) and pass it to
reset_inactivity_timer(), causing a NULL pointer dereference?

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