[PATCH] usb: typec: tipd: cd321x: replay the plug on resume

[PATCH] usb: typec: tipd: cd321x: replay the plug on resume

[Jacob Procee]

On Apple Silicon the Type-C PHY and the dwc3 controller behind a CD321x
port lose their state during s2idle while the plug stays connected, and
the CD321x does not raise an interrupt on resume. xhci notices the lost
state and re-initializes the host controller:

  xhci-hcd xhci-hcd.0.auto: xHC error in resume, USBSTS 0x401, Reinit
  usb usb1: root hub lost power or was reset
  usb 1-1: USB disconnect, device number 2

but the PHY is only ever programmed from typec_mux_set(), so a device
left plugged in (here a FIDO2 security key) never comes back until it is
unplugged and plugged in again.

Re-initializing only dwc3 after resume does not work: tearing dwc3 down
also powers off the PHY, and nothing turns it back on, so xhci then
times out ("Host halt failed, -110", "can't setup: -110").

Instead, make cd321x replay the plug on resume when a partner was
connected: mark the plug as changed and queue the update work, which
then goes through the same sequence as a real replug (role none, mux
safe, mux back to the partner's mode, role back on). Queue it on
system_freezable_wq so it runs only after all devices have resumed.

Tested on a 14" MacBook Pro (M1 Pro, j314s) with a backport of this
change to the Asahi 7.3.3 kernel: a USB-C security key left plugged in
re-enumerates right after "PM: suspend exit", where it previously stayed
gone until replugged. Unplugging the device while the system is asleep
also works: after resume the role goes to none, nothing is brought back
up, and replugging it later enumerates normally. This mainline version
was build-tested only (W=1, checkpatch --strict clean).

Fixes: 82432bbfb9e8 ("usb: typec: tipd: Handle mode transitions for CD321x")
Assisted-by: LLM
Signed-off-by: Jacob Procee <sn125164@gmail.com>
---
checkpatch flags the Assisted-by tag (non-standard signature, 'LLM' is
not an email address); the format follows
Documentation/process/coding-assistants.rst.

 drivers/usb/typec/tipd/core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 02539378ffed..2ec380124532 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -164,6 +164,7 @@ struct tipd_data {
 	bool (*read_power_status)(struct tps6598x *tps);
 	int (*reset)(struct tps6598x *tps);
 	int (*connect)(struct tps6598x *tps, u32 status);
+	void (*resume)(struct tps6598x *tps);
 };
 
 struct tps6598x {
@@ -911,6 +912,21 @@ static int cd321x_connect(struct tps6598x *tps, u32 status)
 	return 0;
 }
 
+static void cd321x_resume(struct tps6598x *tps)
+{
+	struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
+
+	guard(mutex)(&tps->lock);
+
+	if (IS_ERR_OR_NULL(tps->partner))
+		return;
+
+	cd321x->update_status.status_changed |= TPS_STATUS_PLUG_PRESENT;
+	cancel_delayed_work(&cd321x->update_work);
+	queue_delayed_work(system_freezable_wq, &cd321x->update_work,
+			   msecs_to_jiffies(CD321X_DEBOUNCE_DELAY_MS));
+}
+
 static irqreturn_t cd321x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
@@ -2014,6 +2030,9 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
 		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
 				   msecs_to_jiffies(POLL_INTERVAL));
 
+	if (tps->data->resume)
+		tps->data->resume(tps);
+
 	return 0;
 }
 
@@ -2040,6 +2059,7 @@ static const struct tipd_data cd321x_data = {
 	.reset = cd321x_reset,
 	.switch_power_state = cd321x_switch_power_state,
 	.connect = cd321x_connect,
+	.resume = cd321x_resume,
 };
 
 static const struct tipd_data tps6598x_data = {
-- 
2.56.0

Re: [PATCH] usb: typec: tipd: cd321x: replay the plug on resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: tipd: inconsistent workqueues for update_work in
  cd321x_resume()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] usb: typec: tps6598x: I2C read before reset deassertion in
  tps6598x_resume() [1]
--

commit 3f767c8b869e9b28074b970a42e30b2e13b93a10
Author: Jacob Procee <sn125164@gmail.com>

usb: typec: tipd: cd321x: replay the plug on resume

This commit adds a resume callback for CD321x that marks the plug status as
changed and queues update_work on system_freezable_wq when a partner is
connected.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 02539378ffeda..2ec3801245329 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[ ... ]

> @@ -911,6 +912,21 @@ static int cd321x_connect(struct tps6598x *tps, u32 status)
>  	return 0;
>  }
>  
> +static void cd321x_resume(struct tps6598x *tps)
> +{
> +	struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> +
> +	guard(mutex)(&tps->lock);
> +
> +	if (IS_ERR_OR_NULL(tps->partner))
> +		return;
> +
> +	cd321x->update_status.status_changed |= TPS_STATUS_PLUG_PRESENT;
> +	cancel_delayed_work(&cd321x->update_work);
> +	queue_delayed_work(system_freezable_wq, &cd321x->update_work,
> +			   msecs_to_jiffies(CD321X_DEBOUNCE_DELAY_MS));
> +}
> +
>  static irqreturn_t cd321x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;

[Severity: Medium]
Can cd321x->update_work still run before all devices have resumed because
cd321x_connect() queues it on system_percpu_wq via schedule_delayed_work()?

CPU0                              CPU1 (system_percpu_wq)
tps6598x_resume()
  cd321x_resume()
    guard(mutex)(&tps->lock)
    status_changed |= ...
    cancel_delayed_work(...)
                                  cd321x_update_work()
                                    guard(mutex)(&tps->lock)
    queue_delayed_work(
      system_freezable_wq, ...)
  /* unlocks tps->lock */
                                    st = cd321x->update_status;
                                    status_changed = 0;  <-- runs before thaw

Because system_percpu_wq is not frozen during suspend and tps6598x_suspend()
does not cancel cd321x->update_work, an in-flight worker from
cd321x_connect() can already be waiting on tps->lock while cd321x_resume()
runs.  cancel_delayed_work() then returns false, and once cd321x_resume()
releases tps->lock, the system_percpu_wq worker consumes
TPS_STATUS_PLUG_PRESENT, clears status_changed, and runs the replug sequence
before devices have finished resuming, leaving the later system_freezable_wq
run with status_changed == 0.

In addition, device IRQs are already enabled when cd321x_resume() runs, so a
subsequent cd321x_interrupt() -> cd321x_connect() will cancel the work on
system_freezable_wq and re-queue it on the unfrozen system_percpu_wq.
Queueing the same work_struct on two different workqueues also breaks
workqueue non-reentrancy in __queue_work() across CPUs.

Should cd321x_connect() queue cd321x->update_work on system_freezable_wq as
well?

> @@ -2014,6 +2030,9 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>  				   msecs_to_jiffies(POLL_INTERVAL));
>  
> +	if (tps->data->resume)
> +		tps->data->resume(tps);
> +
>  	return 0;
>  }

[Severity: High]
This problem wasn't introduced by this patch, but can tps6598x_resume()
leave the controller permanently held in hardware reset [1]?

When tps->reset is configured and tps->wakeup is false, tps6598x_suspend()
asserts the reset GPIO, but tps6598x_resume() performs an I2C read via
tps6598x_check_mode() before deasserting tps->reset:

drivers/usb/typec/tipd/core.c:tps6598x_resume() {
	...
	ret = tps6598x_check_mode(tps);
	if (ret < 0)
		return ret;
	...
	if (tps->wakeup) {
		disable_irq_wake(client->irq);
		enable_irq(client->irq);
	} else if (tps->reset) {
		gpiod_set_value_cansleep(tps->reset, 0);
		msleep(TPS_SETUP_MS);
	}
	...
}

Because the controller is still held in reset, tps6598x_check_mode() fails
over I2C and returns early before gpiod_set_value_cansleep(tps->reset, 0)
can release the reset line, leaving the device disabled.

[1] https://sashiko.dev/#/bug/linux-4e731139-ebc0-453a-8f82-785e6a0e3cdc

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112113005.16418-1-sn125164@gmail.com?part=1