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

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

New issues:
- [High] The `ti,tps66993` compatible string was inadvertently removed from the OF match table during the I2C split.
- [High] The hardware variant mapping for `TIPD_VARIANT_TPS66993` incorrectly points to `tipd_tps6598x_data` instead of `tipd_tps66993_data`.
- [Medium] The supposedly bus-agnostic core still contains direct I2C struct downcasts, risking kernel crashes on non-I2C buses.
- [Low] Direct pointer-to-integer cast causes build warnings/errors on 64-bit systems.

Pre-existing issues:
- [Critical] Missing cancellation of delayed works in the probe error path leads to use-after-free.
- [High] Suspend and resume functions unconditionally disable/enable IRQ 0 when in polling mode, potentially causing system instability.
--

--- Patch [2]: [PATCH v5 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 691ce94a8de82880f19343c1fa6f0daf5755c9d5
Author: Alyssa Milburn <[email protected]>

usb: typec: tipd: Factor out i2c specifics

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

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

[Severity: Medium]
Will this bus-agnostic abstraction fail on non-I2C buses due to remaining I2C
struct downcasts?

For example, tps25750_write_firmware() in core.c still uses to_i2c_client():

	struct i2c_client *client = to_i2c_client(tps->dev);
	...
	slave_addr = client->addr;
	timeout = client->adapter->timeout;
	...
	client->addr = bpms_addr;

Could this cause memory corruption if a non-I2C variant calls
tps25750_apply_patch() -> tps25750_start_patch_burst_mode() ->
tps25750_write_firmware() in the future?

> 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

[ ... ]

> @@ -1887,14 +1760,14 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (tps->irq) {
> -		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +		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);
>  		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>  				   msecs_to_jiffies(POLL_INTERVAL));
>  	}
>  
>  	if (ret)
>  		goto err_disconnect;

[Severity: Critical]
This isn't a bug introduced by this patch, but could this error path leave
delayed works pending and cause a use-after-free?

If tipd_init() fails after queueing tps->wq_poll in polling mode, or after
connect() schedules cd321x->update_work, it jumps to err_disconnect. Since
cancel_delayed_work_sync() is missing in the cleanup, devres will free
the tps structure while the system workqueue might still execute the pending
work and dereference the freed memory.

[ ... ]

> @@ -1955,29 +1826,26 @@ static void tps6598x_remove(struct i2c_client *client)
> +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 is a pre-existing issue, but does this unconditionally disable and enable
IRQ 0 when in polling mode?

If a device tree specifies a wakeup-source without providing an interrupt,
tps->irq remains 0 while tps->wakeup is set to true. During suspend and resume,
tipd_suspend() and tipd_resume() call disable_irq(0) and enable_irq_wake(0).
Manipulating IRQ 0 (typically the system timer on x86) can cause lockups or
panics.

[ ... ]

> @@ -2085,33 +1950,14 @@ static const struct tipd_data tps25750_data = {
> +const struct tipd_data *tipd_hw_variants[] = {
> +	[TIPD_VARIANT_CD321X] = &tipd_cd321x_data,
> +	[TIPD_VARIANT_TPS6598X] = &tipd_tps6598x_data,
> +	[TIPD_VARIANT_TPS66993] = &tipd_tps6598x_data,
> +	[TIPD_VARIANT_TPS25750] = &tipd_tps25750_data,
>  };

[Severity: High]
Is it intentional that TIPD_VARIANT_TPS66993 points to tipd_tps6598x_data
instead of tipd_tps66993_data?

Since tipd_tps66993_data is defined with the correct
tps66993_read_power_status() callback and IRQ masks, assigning
tipd_tps6598x_data here will cause TPS66993 devices to use the wrong
callbacks and experience hardware state parsing failures.

> diff --git a/drivers/usb/typec/tipd/i2c.c b/drivers/usb/typec/tipd/i2c.c
> new file mode 100644
> index 0000000000000..f852cdc327f5d
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/i2c.c
> @@ -0,0 +1,106 @@

[ ... ]

> +static int tps6598x_probe_i2c(struct i2c_client *client)
> +{
> +	enum tipd_hw_variant variant;
> +	const struct tipd_data *data;
> +	struct tps6598x *tps;
> +	int ret;
> +
> +	variant = (enum tipd_hw_variant)i2c_get_match_data(client);

[Severity: Low]
Will this pointer-to-int cast trigger a -Wpointer-to-int-cast build warning on
64-bit architectures?

[ ... ]

> +static const struct of_device_id tps6598x_of_match[] = {
> +	{ .compatible = "ti,tps6598x", (void *)TIPD_VARIANT_TPS6598X},
> +	{ .compatible = "apple,cd321x",  (void *)TIPD_VARIANT_CD321X},
> +	{ .compatible = "ti,tps25750",  (void *)TIPD_VARIANT_TPS25750},
> +	{}
> +};

[Severity: High]
Did we accidentally drop the ti,tps66993 entry here during the refactoring?

Without this compatible string in the I2C driver, it seems we will no longer
bind to TPS66993 devices during boot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2