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

[sashiko-bot@kernel.org]

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