Re: [PATCH v7 2/3] usb: typec: tipd: Factor out i2c specifics

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `tipd_suspend()` and `tipd_resume()` blindly operate on IRQ 0 if the device has a `wakeup-source` property but no valid interrupt line.
- [High] Probe failure in polling mode causes a Use-After-Free because `tps->wq_poll` is queued but never canceled in the error path.
- [High] Device is accessed over I2C before being taken out of reset during resume, causing a persistent failure.
--

commit 483e340a7e2db3c0bcd0902d89a807ddfbd80a8f
Author: Alyssa Milburn <amilburn@zall.org>

usb: typec: tipd: Factor out i2c specifics

Make the core driver more bus-agnostic to prepare for SPMI variants of
the tipd chip.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1898,14 +1771,14 @@ static int tps6598x_probe(struct i2c_client *client)
[ ... ]
>  	if (tps->irq) {
>  		ret = devm_request_threaded_irq(tps->dev, tps->irq, NULL,
>  						tps->data->irq_handler,
>  						IRQF_SHARED | IRQF_ONESHOT,
>  						dev_name(tps->dev), tps);
>  	} else {
>  		dev_dbg(tps->dev, "no IRQ specified, using polling mode\n");
>  		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
> @@ -1916,13 +1789,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_disconnect;

[Severity: High]
This is a pre-existing issue, but does this leak memory or cause a
use-after-free if the probe fails while in polling mode?

If the device is configured in polling mode (no IRQ specified) and the call
to tps->data->connect() fails, ret holds a non-zero error code. The polling
work tps->wq_poll is queued here, and then the code jumps to err_disconnect.

Looking at the err_disconnect path in tipd_init():

err_disconnect:
        tps6598x_disconnect(tps, 0);
err_unregister_port:
        tps->data->unregister_port(tps);
...
        return ret;

The error path returns without canceling the queued work. When devres frees
the tps structure, the delayed work timer can still fire 500ms later, which
would lead to a use-after-free when it accesses the freed memory.

Could we add a cancel_delayed_work_sync() call in the error handling path?

[ ... ]

> -static int __maybe_unused tps6598x_suspend(struct device *dev)
> +int tipd_suspend(struct tps6598x *tps)
>  {
>  	if (tps->wakeup) {
> -		disable_irq(client->irq);
> -		enable_irq_wake(client->irq);
> +		disable_irq(tps->irq);
> +		enable_irq_wake(tps->irq);

[Severity: High]
This isn't a bug introduced by this patch, but could this blindly operate
on IRQ 0 during system suspend?

If a device is configured in polling mode, tps->irq is 0. If that device
also has the wakeup-source property set in its device tree, tipd_suspend()
will call disable_irq(0) and enable_irq_wake(0). On architectures where
IRQ 0 is a system interrupt (such as the timer tick on x86), disabling it
can cause a hang during suspend.

[ ... ]

>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 1);
>  	}
>  
> -	if (!client->irq)
> +	if (!tps->irq)
>  		cancel_delayed_work_sync(&tps->wq_poll);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(tipd_suspend);
>  
> -static int __maybe_unused tps6598x_resume(struct device *dev)
> +int tipd_resume(struct tps6598x *tps)
>  {
>  	int ret;
>  
>  	ret = tps6598x_check_mode(tps);

[Severity: High]
This is a pre-existing issue, but will this I2C read persistently fail
during resume?

In tipd_suspend(), if the device lacks a wakeup source but has a reset GPIO,
it is put into reset. However, during tipd_resume(), the very first operation
is a call to tps6598x_check_mode(tps), which attempts an I2C read.

Since the device is still in reset when this I2C read is attempted, the read
will fail and return an error early, leaving the device permanently in reset
after a system suspend.

[ ... ]

>  	if (ret < 0)
>  		return ret;
>  
> @@ -2002,25 +1870,22 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  	}
>  
>  	if (tps->wakeup) {
> -		disable_irq_wake(client->irq);
> -		enable_irq(client->irq);
> +		disable_irq_wake(tps->irq);
> +		enable_irq(tps->irq);

[Severity: High]
This isn't a bug introduced by this patch, but similar to the suspend path,
will this operate on IRQ 0 if the device has a wakeup-source property but
no valid interrupt line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-tipd-ace3-v7-0-6c6ebe2193f2@chaosmail.tech?part=2