[PATCH RFC 0/2] HID: Replace system_wq with system_dfl_wq

[PATCH RFC 0/2] HID: Replace system_wq with system_dfl_wq

[Marco Crivellari]

Hi,

Currently the code uses the per-cpu workqueue system_wq to schedule
delayed works.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Another good reason to have this unbound,
is the "queue_delayed_work()" function, used to enqueue the work item.

~~~ Details about queue_delayed_work ~~~

system_wq is a per-cpu workqueue and it is used as a parameter of
mod_delayed_work(). This function schedule an item that it will later
be enqueued (once the timer will fire). __queue_delayed_work() does the job
receiving as "cpu" WORK_CPU_UNBOUND:

    if (housekeeping_enabled(HK_TYPE_TIMER)) {
    //      [....]
    } else {
            if (likely(cpu == WORK_CPU_UNBOUND))
                    add_timer_global(timer);
            else
                    add_timer_on(timer, cpu);
    }

The timer is global, so can fire everywhere, and the work item will be
enqueued where the timer fired.

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change the
workqueue with system_dfl_wq, so that the used workqueue is now unbound
and can benefit from scheduler task placement.

Thanks!

Marco Crivellari (2):
  HID: hid-oxp: Replace system_wq with system_dfl_wq
  HID: appletb-kdb: Replace system_wq with system_dfl_wq

 drivers/hid/hid-appletb-kbd.c | 6 +++---
 drivers/hid/hid-oxp.c         | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.54.0

[PATCH RFC 1/2] HID: hid-oxp: Replace system_wq with system_dfl_wq

[Marco Crivellari]

The function end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption.

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change the
workqueue with the new unbound version, system_dfl_wq.

Cc: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 drivers/hid/hid-oxp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 20a54f337220..d2ded6b08ce9 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -398,7 +398,7 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
 	 * Re-apply our settings after this has been received.
 	 */
 	if (data[3] == OXP_EFFECT_MONO_TRUE) {
-		mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
+		mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
 		return 0;
 	}
 
@@ -788,7 +788,7 @@ static ssize_t map_button_store(struct device *dev,
 	default:
 		return -EINVAL;
 	}
-	mod_delayed_work(system_wq, &drvdata.oxp_btn_queue, msecs_to_jiffies(50));
+	mod_delayed_work(system_dfl_wq, &drvdata.oxp_btn_queue, msecs_to_jiffies(50));
 	return count;
 }
 
@@ -1349,7 +1349,7 @@ static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
 				   enum led_brightness brightness)
 {
 	led_cdev->brightness = brightness;
-	mod_delayed_work(system_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
+	mod_delayed_work(system_dfl_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
 }
 
 static struct attribute *oxp_rgb_attrs[] = {
@@ -1502,7 +1502,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
 	drvdata.rumble_intensity = 5;
 
 	INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
-	mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
+	mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
 
 	ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
 	if (ret)
-- 
2.54.0

[PATCH RFC 2/2] HID: appletb-kdb: Replace system_wq with system_dfl_wq

[Marco Crivellari]

Currently the code enqueue work items using mod_delayed_work(), using
system_wq, the old per-CPU Workqueue.

The function end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption.

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change the
workqueue with the new unbound version, system_dfl_wq.

Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 drivers/hid/hid-appletb-kbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..5cc27066f602 100644
--- a/drivers/hid/hid-appletb-kbd.c
+++ b/drivers/hid/hid-appletb-kbd.c
@@ -175,7 +175,7 @@ static void appletb_inactivity_work(struct work_struct *work)
 		if (!kbd->has_dimmed) {
 			backlight_device_set_brightness(kbd->backlight_dev, 1);
 			kbd->has_dimmed = true;
-			mod_delayed_work(system_wq, &kbd->inactivity_work,
+			mod_delayed_work(system_dfl_wq, &kbd->inactivity_work,
 					 secs_to_jiffies(appletb_tb_idle_timeout));
 		} else if (!kbd->has_turned_off) {
 			backlight_device_set_brightness(kbd->backlight_dev, 0);
@@ -201,7 +201,7 @@ static void reset_inactivity_timer(struct appletb_kbd *kbd)
 			kbd->has_turned_off = false;
 			schedule_work(&kbd->restore_brightness_work);
 		}
-		mod_delayed_work(system_wq, &kbd->inactivity_work,
+		mod_delayed_work(system_dfl_wq, &kbd->inactivity_work,
 				 secs_to_jiffies(appletb_tb_dim_timeout));
 	}
 }
@@ -423,7 +423,7 @@ static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id
 		INIT_DELAYED_WORK(&kbd->inactivity_work, appletb_inactivity_work);
 		INIT_WORK(&kbd->restore_brightness_work,
 			  appletb_restore_brightness_work);
-		mod_delayed_work(system_wq, &kbd->inactivity_work,
+		mod_delayed_work(system_dfl_wq, &kbd->inactivity_work,
 				 secs_to_jiffies(appletb_tb_dim_timeout));
 	}
 
-- 
2.54.0

Re: [PATCH RFC 2/2] HID: appletb-kdb: Replace system_wq with system_dfl_wq

[sashiko-bot]

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

New issues:
- [Low] Incomplete migration to the unbound workqueue: `schedule_work()` still targets the per-CPU workqueue.

Pre-existing issues:
- [High] Race condition during device probe causes a NULL pointer dereference if a HID event arrives before `hid_set_drvdata()` is called.
- [High] Sleeping in atomic context when calling `hid_hw_power` from the input event handler.
--

--- Patch [2]: [PATCH RFC 2/2] HID: appletb-kdb: Replace system_wq with system_dfl_wq ---
commit b55f729cf441f481f051cec559ad520a64ea4188
Author: Marco Crivellari <marco.crivellari@suse.com>

HID: appletb-kdb: Replace system_wq with system_dfl_wq

This commit changes the driver to use the unbound system_dfl_wq workqueue
instead of the per-CPU system_wq, to optimize task placement and power
consumption.

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

[ ... ]

> @@ -201,7 +201,7 @@ static void reset_inactivity_timer(struct appletb_kbd *kbd)
>  			kbd->has_turned_off = false;
>  			schedule_work(&kbd->restore_brightness_work);
>  		}

[Severity: Low]
Since the goal is to move entirely to the unbound system_dfl_wq, did we miss
updating this schedule_work() call? 

The schedule_work() function implicitly targets the per-CPU system_wq, so
restore_brightness_work won't benefit from the scheduler optimizations
intended by this patch.

Could we use queue_work() with system_dfl_wq here instead?

> -		mod_delayed_work(system_wq, &kbd->inactivity_work,
> +		mod_delayed_work(system_dfl_wq, &kbd->inactivity_work,
>  				 secs_to_jiffies(appletb_tb_dim_timeout));
>  	}
>  }

[ ... ]

> @@ -423,7 +423,7 @@ static int appletb_kbd_probe(struct hid_device *hdev, const struct hid_device_id
>  		INIT_DELAYED_WORK(&kbd->inactivity_work, appletb_inactivity_work);
>  		INIT_WORK(&kbd->restore_brightness_work,
>  			  appletb_restore_brightness_work);
> -		mod_delayed_work(system_wq, &kbd->inactivity_work,
> +		mod_delayed_work(system_dfl_wq, &kbd->inactivity_work,
>  				 secs_to_jiffies(appletb_tb_dim_timeout));
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the surrounding
appletb_kbd_probe() setup, is there a race condition between hid_hw_open()
and driver data initialization?

Earlier in appletb_kbd_probe(), hid_hw_open() is called before
hid_set_drvdata() executes. If a hardware event arrives immediately after
hid_hw_open() starts polling, the hid core calls the event handler:

appletb_kbd_hid_event()
   struct appletb_kbd *kbd = hid_get_drvdata(hdev);

Because hid_set_drvdata() hasn't been called yet, hid_get_drvdata() returns
NULL. When reset_inactivity_timer() later dereferences kbd->backlight_dev,
could this cause a kernel panic?

[Severity: High]
This is another pre-existing issue, but does appletb_kbd_inp_event() sleep
in an atomic context?

When the input subsystem invokes the .event callback, it holds the device's
event_lock spinlock with local interrupts disabled. If the fn key is toggled,
the callback path goes:

appletb_kbd_inp_event()
  appletb_kbd_set_mode()
    hid_hw_power(hdev, PM_HINT_FULLON);

For usb devices, hid_hw_power() resolves to usbhid_power(), which calls
usb_autopm_get_interface(). This uses pm_runtime_get_sync(), which acquires a
mutex and can sleep. 

Could this trigger a scheduling while atomic panic when the user presses
the fn key?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707145327.313292-1-marco.crivellari@suse.com?part=2

Re: [PATCH RFC 1/2] HID: hid-oxp: Replace system_wq with system_dfl_wq

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver uses a single global statically allocated LED class device, causing list corruption and state clobbering if multiple matching devices are connected.
- [High] Inadequate teardown of delayed works causes Use-After-Free vulnerabilities during device removal and probe failures.
- [High] Handling a GEN2 status event unconditionally dereferences `drvdata.led_mc`, which is NULL on hybrid MCU devices.
- [High] The driver blindly accesses HID event data without validating the `size` parameter, leading to out-of-bounds reads.
--

--- Patch [1]: [PATCH RFC 1/2] HID: hid-oxp: Replace system_wq with system_dfl_wq ---
commit 17346571c8ccd09d42318cd808c4c65570f51a89
Author: Marco Crivellari <marco.crivellari@suse.com>

    HID: hid-oxp: Replace system_wq with system_dfl_wq
    
    The function end up calling __queue_delayed_work(), which set a global
    timer that could fire anywhere, enqueuing the work where the timer fired.
    
    Unbound works could benefit from scheduler task placement, to optimize
    performance and power consumption.
    
    Since the workqueue work doesn't rely on per-cpu variables, there is no
    obvious reason that justify the use of a per-cpu workqueue. So change the
    workqueue with the new unbound version, system_dfl_wq.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 20a54f337220d..d2ded6b08ce9e 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -398,7 +398,7 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>  	 * Re-apply our settings after this has been received.
>  	 */
>  	if (data[3] == OXP_EFFECT_MONO_TRUE) {
> -		mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
> +		mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));

[Severity: High]
This isn't a bug introduced by this patch, but does the driver validate the
size parameter before accessing data[0] or data[3] in this function?

It appears that if a device sends a smaller report than expected, we could hit
an out-of-bounds read when checking data[3] or when dumping OXP_PACKET_SIZE
bytes in oxp_hid_raw_event().

[Severity: High]
This is a pre-existing issue, but is it possible for drvdata.led_mc to be NULL
here?

If oxp_cfg_probe() jumps to skip_rgb for hybrid MCU devices, drvdata.led_mc
is left uninitialized (NULL). If the device later sends a GEN2 status event,
this function eventually does:

    led_mc->led_cdev.brightness = rgb_rep->brightness / 4 * ...

Could this cause a NULL pointer dereference on hybrid devices?

>  		return 0;
>  	}
>  
[ ... ]
> @@ -1349,7 +1349,7 @@ static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
>  				   enum led_brightness brightness)
>  {
>  	led_cdev->brightness = brightness;
> -	mod_delayed_work(system_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
> +	mod_delayed_work(system_dfl_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));

[Severity: High]
This is a pre-existing issue, but can this delayed work cause a use-after-free
during device removal?

Because oxp_hid_remove() uses cancel_delayed_work() instead of the synchronous
cancel_delayed_work_sync(), the work might still be running. Also, when devres
unregisters the LED classdev after remove() finishes, the LED core calls this
function, which unconditionally re-arms the workqueue.

Could the workqueue execute after devres has freed memory structures, leading
to a use-after-free?

>  }
>  
[ ... ]
> @@ -1502,7 +1502,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
>  	drvdata.rumble_intensity = 5;
>  
>  	INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
> -	mod_delayed_work(system_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
> +	mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
>  
>  	ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
>  	if (ret)

[Severity: High]
This isn't a bug introduced by this patch, but if devm_device_add_group()
fails here, the probe function returns the error directly.

Does this leave the oxp_mcu_init delayed work scheduled, potentially causing a
use-after-free if the work runs after the device and its devres allocations
have been freed?

[Severity: Critical]
This is a pre-existing issue, but does oxp_cfg_probe() register a single
globally statically allocated led class device?

Earlier in oxp_cfg_probe():

    ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);

Since oxp_cdev_rgb is statically allocated, if multiple matching HID devices
are connected, we will call devm_led_classdev_multicolor_register() multiple
times with the same pointer. Could this corrupt the LED subsystem's internal
linked list and cause a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707145327.313292-1-marco.crivellari@suse.com?part=1