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

[sashiko-bot@kernel.org]

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