Re: [PATCH v2 3/3] usb: typec: tipd: add TPS66993 support

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] For TPS66993, the driver fails to update the synthesized power status when a PD contract is established after the initial plug event.
- [Medium] Redundant I2C reads and potential dropping of plug events for legacy TPS6598x devices in the shared interrupt handler.
- [Low] Use of undeclared identifiers 'TPS_POWER_STATUS_CONNECTION_MASK' and 'TPS_POWER_STATUS_SOURCESINK_MASK' causes compilation failure.

Pre-existing issues:
- [High] The `tps->wq_poll` delayed work is queued but never canceled if the probe fails, leading to a Use-After-Free.
- [High] The `cd321x->update_work` delayed work is leaked and runs after free if probe fails after a successful connect.
- [High] The probe function exposes the Type-C port to userspace before it fully initializes the connection state, leading to a race condition with sysfs.
--

commit f75143dbec357f0c1c49269aeab93ef676edfcef
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Subject: usb: typec: tipd: add TPS66993 support

This patch adds support for the TPS66993 controller. It derives power status
from the STATUS register instead of the deprecated Power_Status register and
adds support for the APP1 mode string. The controller is configured in
polling mode.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a6cb233a055da..d2394d23a2dc4 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[ ... ]

> @@ -631,6 +633,35 @@ static bool tps6598x_read_power_status(struct tps6598x *tps)
>  	return true;
>  }
>  
> +/*
> + * TPS66993 deprecated Power_Status register (0x3F). BC1.2 is not supported
> + * and the remaining bits are redundant with STATUS register (0x1A).
> + * Synthesize pwr_status from the already-read STATUS register.
> + */
> +static bool tps66993_read_power_status(struct tps6598x *tps)
> +{
> +	u16 pwr_status = 0;
> +
> +	/* Same masks as TPS_POWER_STATUS_CONNECTION() / SOURCESINK() / PWROPMODE() in tps6598x.h */
> +	if (tps->status & TPS_STATUS_PLUG_PRESENT)
> +		pwr_status |= FIELD_PREP(TPS_POWER_STATUS_CONNECTION_MASK, 1);
> +
> +	/* SOURCESINK: 1=sink; STATUS.PortRole 1=source, opposite convention */
> +	if (!TPS_STATUS_TO_TYPEC_PORTROLE(tps->status))
> +		pwr_status |= FIELD_PREP(TPS_POWER_STATUS_SOURCESINK_MASK, 1);

[Severity: Low]
Does this code cause a compilation failure? The macros
TPS_POWER_STATUS_CONNECTION_MASK and TPS_POWER_STATUS_SOURCESINK_MASK appear
to be undeclared in the kernel tree.

[ ... ]

> @@ -1026,6 +1057,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_unlock;
>  
> +	tps->status = status;
> +
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
>  		if (!tps->data->read_power_status(tps))
>  			goto err_unlock;
> @@ -1034,9 +1067,15 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		if (!tps->data->read_data_status(tps))
>  			goto err_unlock;
>  
> -	/* Handle plug insert or removal */
> -	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT)
> +	/*
> +	 * Refresh power status before connect - needed for TPS66993 which
> +	 * synthesizes pwr_status from STATUS and never gets POWER_STATUS_UPDATE.
> +	 */
> +	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT) {
> +		if (!tps->data->read_power_status(tps))
> +			goto err_unlock;

[Severity: Medium]
For legacy TPS6598x devices where both TPS_REG_INT_POWER_STATUS_UPDATE and
TPS_REG_INT_PLUG_EVENT can be set, does this cause redundant I2C reads since
read_power_status is called for both events?

If the second read fails, will it jump to err_unlock and silently drop the
plug event?

[Severity: Medium]
For the TPS66993, since read_power_status is now restricted to plug events in
this handler, does the driver fail to update the synthesized power status if a
PD contract is established after the initial plug event?

> +		tps6598x_handle_plug_event(tps, status);
> +	}
>  
>  err_unlock:

[ ... ]

> @@ -1810,6 +1850,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  		goto err_clear_mask;
>  	}
>  
> +	tps->status = status;
> +

[Severity: High]
This is a pre-existing issue, but does exposing the Type-C port to userspace
via tps->data->register_port() later in this probe function create a race
condition?

Userspace sysfs writes could potentially race with the subsequent lockless
port mutations in read_power_status, read_data_status, and connect later in
this function.

[Severity: High]
This is a pre-existing issue, but if the device is probed in polling mode
and an earlier connect step fails, the delayed work tps->wq_poll is
initialized and queued:

tps6598x_probe() {
	...
	if (client->irq) {
		...
	} else {
		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;
	...
}

When the code jumps to err_disconnect, tps->wq_poll is never canceled. Can
this lead to a use-after-free when the queued work executes on freed memory?

[Severity: High]
This is a pre-existing issue, but does the error path in this probe function
leak the cd321x->update_work delayed work?

If a CD321x device connects successfully during probe but a subsequent setup
step fails, the error teardown jumps to err_disconnect without calling
remove() or explicitly canceling update_work. Could this result in a
use-after-free?

>  	/*
>  	 * This fwnode has a "compatible" property, but is never populated as a
>  	 * struct device. Instead we simply parse it to read the properties.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702190527.1820671-1-radhey.shyam.pandey@amd.com?part=3