Re: [PATCH v6 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] A delayed work item is queued but never cancelled on the probe error path in polling mode, leading to a Use-After-Free.
- [High] Suspend and resume callbacks operate blindly on IRQ 0 if the `wakeup-source` property is present but no valid IRQ is configured.
- [High] If hardware checks fail during `tipd_resume()`, early return leaves the system IRQ permanently disabled.
--

commit 74c8f85f00d632452859f6773ca966726049ba5e
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
> index 522f56742aa9b..fb7bb68731bc5 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1887,14 +1760,14 @@ static int tps6598x_probe(struct i2c_client *client)
>  			goto err_unregister_port;
>  		ret = tps->data->connect(tps, status);
>  		if (ret)
> -			dev_err(&client->dev, "failed to register partner\n");
> +			dev_err(tps->dev, "failed to register partner\n");
>  	}
>  
> -	if (client->irq) {
> -		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +	if (tps->irq) {
> +		ret = devm_request_threaded_irq(tps->dev, tps->irq, NULL,
>  						tps->data->irq_handler,
>  						IRQF_SHARED | IRQF_ONESHOT,
> -						dev_name(&client->dev), tps);
> +						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);
> @@ -1905,13 +1778,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_disconnect;

[Severity: High]
This is a pre-existing issue, but can this error path in tipd_init() lead
to a use-after-free?

If tps->data->connect() fails in polling mode (where tps->irq == 0), ret
will hold the error code. The polling work is then queued in the else
branch.

Because ret is non-zero, the "if (ret)" check triggers a jump to
err_disconnect. The error handling path misses calling
cancel_delayed_work_sync(&tps->wq_poll). When the devres framework frees
the tps structure upon probe failure, the scheduled delayed work might
execute and access freed memory.

[ ... ]
> -static int __maybe_unused tps6598x_suspend(struct device *dev)
> +int tipd_suspend(struct tps6598x *tps)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
> -
>  	if (tps->wakeup) {
> -		disable_irq(client->irq);
> -		enable_irq_wake(client->irq);
> +		disable_irq(tps->irq);
> +		enable_irq_wake(tps->irq);
>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 1);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does tipd_suspend() blindly
operate on IRQ 0?

During probe, tps->wakeup can be set to true if the wakeup-source property
is found in the device tree, even if no valid IRQ is configured
(tps->irq == 0).

If this happens, tipd_suspend() will pass IRQ 0 to disable_irq() and
enable_irq_wake(), which could crash the system or disable critical system
timers (such as the PIT on x86).

[ ... ]
> -static int __maybe_unused tps6598x_resume(struct device *dev)
> +int tipd_resume(struct tps6598x *tps)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
>  	int ret;
>  
>  	ret = tps6598x_check_mode(tps);
>  	if (ret < 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but could this early return in tipd_resume()
leave the system IRQ permanently disabled?

If tps6598x_check_mode(tps) fails on resume, the function returns early
before balancing the disable_irq() call from suspend with enable_irq().

This creates an unbalanced IRQ depth, keeping the IRQ line disabled, which
will break any other devices sharing the same IRQ.

[ ... ]
> @@ -1991,25 +1859,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);
>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 0);
>  		msleep(TPS_SETUP_MS);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but similar to the suspend path,
will tipd_resume() pass IRQ 0 to disable_irq_wake() and enable_irq() if
no IRQ is configured but the wakeup-source property is present?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-tipd-ace3-v6-0-e919f39a93a2@chaosmail.tech?part=2