Re: [PATCH] HID: apple-touchbar: avoid nested runtime PM in PM callbacks

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

New issues:
- [Medium] Touch Bar display OFF command is queued but not sent during runtime suspend.
- [Medium] The PM callbacks `suspend` and `resume` directly propagate error codes from `hid_set_field`, which can unexpectedly abort system suspend transitions.

Pre-existing issues:
- [High] The input event handler `appletb_kbd_inp_event` triggers a sleep while holding a spinlock with IRQs disabled, leading to a kernel panic.
--

--- Patch [1]: [PATCH] HID: apple-touchbar: avoid nested runtime PM in PM callbacks ---
commit d7e9b031fc57c77cbf461d6ee7d2a03d269e31be
Author: Andre Eikmeyer <[email protected]>

HID: apple-touchbar: avoid nested runtime PM in PM callbacks

This patch splits the mode update into an active-device helper and a
runtime-PM-managed wrapper to avoid nested runtime PM resume requests during
USB PM transitions. Both PM callbacks now call the active-device helper
directly without acquiring a nested runtime PM reference.

> diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
> index 462010a758993..cbaf7587feea4 100644
> --- a/drivers/hid/hid-appletb-kbd.c
> +++ b/drivers/hid/hid-appletb-kbd.c
> @@ -87,29 +87,37 @@ static const struct key_entry appletb_kbd_keymap[] = {

[ ... ]

> +static int appletb_kbd_set_mode(struct appletb_kbd *kbd, u8 mode)
> +{
> +	struct hid_device *hdev = kbd->mode_field->report->device;
> +	int ret;
> +
> +	ret = hid_hw_power(hdev, PM_HINT_FULLON);

[Severity: High]
This isn't a bug introduced by this patch, but does this function sleep in
atomic context?

The input subsystem calls the input handler event callback,
appletb_kbd_inp_event(), from input_pass_values() while holding the
dev->event_lock spinlock with local IRQs disabled.

When this handler receives a KEY_FN event, it calls appletb_kbd_set_mode(),
which invokes hid_hw_power() here. For USB HID devices, this translates to
usb_autopm_get_interface(), which invokes pm_runtime_get_sync().

If the device is currently suspended, pm_runtime_get_sync() will sleep to
synchronously resume it.

Can this cause a scheduling while atomic bug when the spinlock is held?

> +	if (ret) {
> +		hid_err(hdev, "Device didn't resume (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = appletb_kbd_set_mode_active(kbd, mode);
>  
> -power_normal:
>  	hid_hw_power(hdev, PM_HINT_NORMAL);
>  
>  	return ret;
>  }

[ ... ]

> @@ -487,18 +495,19 @@ static int appletb_kbd_suspend(struct hid_device *hdev, pm_message_t msg)
>  	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>  
>  	kbd->saved_mode = kbd->current_mode;
> -	appletb_kbd_set_mode(kbd, APPLETB_KBD_MODE_OFF);
>  
> -	return 0;
> +	/* This callback already runs as part of a USB PM transition. Taking a
> +	 * nested runtime-PM reference would wait for that transition to complete
> +	 * and deadlock the USB PM state machine.
> +	 */
> +	return appletb_kbd_set_mode_active(kbd, APPLETB_KBD_MODE_OFF);

[Severity: Medium]
Does this result in the display OFF command being deferred until resume?

When the device undergoes runtime suspend, usbhid's hid_suspend executes.
For runtime PM transitions, usbhid sets the HID_SUSPENDED flag before
invoking the driver's suspend callback.

When appletb_kbd_suspend() subsequently calls hid_hw_request() via the
active-device helper, usbhid detects the HID_SUSPENDED flag and enqueues
the URB without submitting it.

Will this prevent the OFF command from being sent before sleep, and cause
spurious state flapping when the device wakes up?

[Severity: Medium]
Will returning this result directly abort a system suspend?

If hid_set_field() fails due to a malformed HID descriptor, it returns -1
which evaluates to -EPERM.

Because this callback now propagates that error code directly, could a
failure to update this cosmetic feature unexpectedly abort the entire
system-wide suspend operation?

>  }
>  
>  static int appletb_kbd_resume(struct hid_device *hdev)
>  {
>  	struct appletb_kbd *kbd = hid_get_drvdata(hdev);
>  
> -	appletb_kbd_set_mode(kbd, kbd->saved_mode);
> -
> -	return 0;
> +	return appletb_kbd_set_mode_active(kbd, kbd->saved_mode);

[Severity: Medium]
Similar to the suspend path, could propagating the error code from
appletb_kbd_set_mode_active() cause issues during system resume?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1