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

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

[Andre Eikmeyer]

Hello everyone,

Runtime suspend of the Touch Bar display stalls because its HID suspend
callback requests a runtime PM resume while already running as part of the
same USB PM transition. The nested request waits for the transition that
invoked the callback to finish.

The matching resume callback has the same problem. It can stall after the
virtual host controller has successfully resumed the port.

The mode update is therefore split into an active-device helper and a
runtime-PM-managed wrapper. Both PM callbacks call the active-device
helper and no longer acquire a nested runtime PM reference.

We maintain the apple-bce/t2bce virtual host controller used on these
systems. Although that module is not yet upstream, its runtime PM path has
already been adjusted so the complete suspend and resume cycle works with
this change.

Thanks for your time and work.

Signed-off-by: Andre Eikmeyer <dev@deq.rocks>
---
 drivers/hid/hid-appletb-kbd.c | 37 ++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-appletb-kbd.c b/drivers/hid/hid-appletb-kbd.c
index 462010a75899..cbaf7587feea 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[] = {
 	{ KE_END, 0 }
 };
 
-static int appletb_kbd_set_mode(struct appletb_kbd *kbd, u8 mode)
+static int appletb_kbd_set_mode_active(struct appletb_kbd *kbd, u8 mode)
 {
 	struct hid_report *report = kbd->mode_field->report;
 	struct hid_device *hdev = report->device;
 	int ret;
 
-	ret = hid_hw_power(hdev, PM_HINT_FULLON);
-	if (ret) {
-		hid_err(hdev, "Device didn't resume (%pe)\n", ERR_PTR(ret));
-		return ret;
-	}
-
 	ret = hid_set_field(kbd->mode_field, 0, mode);
 	if (ret) {
 		hid_err(hdev, "Failed to set mode field to %u (%pe)\n", mode, ERR_PTR(ret));
-		goto power_normal;
+		return ret;
 	}
 
 	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 
 	kbd->current_mode = mode;
+	return 0;
+}
+
+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);
+	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);
 }
 
 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);
 }
 
 static const struct hid_device_id appletb_kbd_hid_ids[] = {

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

[sashiko-bot]

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 <dev@deq.rocks>

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/20260810140352.35866-1-dev@deq.rocks?part=1