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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Redundant I2C read of the power status register for legacy TPS6598x controllers on plug events.

Pre-existing issues:
- [High] Background work items (`wq_poll` and `cd321x->update_work`) are queued during `tps6598x_probe()` but are not cancelled if the probe function subsequently fails, leading to Use-After-Free.
--

--- Patch [3]: [PATCH v3 3/3] usb: typec: tipd: add TPS66993 support ---
commit 4f820239b3f04ea4ac3411ddf0ac69f6736b3b7b
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

usb: typec: tipd: add TPS66993 support

Derive power status from the STATUS register (0x1A) now that TPS66993
deprecates the Power_Status register (0x3F). Add support for the "APP1"
mode string. TPS66993 controller is configured in polling mode and only
type-c flip orientation feature is supported on AMD Versal AI Edge Gen 2
VEK385 Evaluation Kit.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 9097b0d40a71d..522f56742aa9b 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1026,6 +1056,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 +1066,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: Low]
Does this cause a redundant I2C read for legacy controllers?

For legacy TPS6598x controllers, a plug event typically sets both
TPS_REG_INT_POWER_STATUS_UPDATE and TPS_REG_INT_PLUG_EVENT concurrently.

Since tps->data->read_power_status(tps) is already called earlier in this
function when TPS_REG_INT_POWER_STATUS_UPDATE is set, calling it again here
without checking if it was already read results in an unnecessary I2C block
read during physical plug events.

>  		tps6598x_handle_plug_event(tps, status);
> +	}
>  
>  err_unlock:
>  	mutex_unlock(&tps->lock);
[ ... ]
> @@ -1813,6 +1852,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  		goto err_clear_mask;
>  	}
>  
> +	tps->status = status;
> +
>  	/*
>  	 * This fwnode has a "compatible" property, but is never populated as a
>  	 * struct device. Instead we simply parse it to read the properties.

[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free in the
probe error path when handling delayed work items.

If the device uses polling mode (client->irq == 0), the probe function queues
wq_poll:

drivers/usb/typec/tipd/core.c:tps6598x_probe() {
    ...
    if (client->irq) {
        ...
    } 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;
    ...
}

If connect() failed earlier, ret holds a negative error code, and the probe
jumps to the error path:

drivers/usb/typec/tipd/core.c:tps6598x_probe() {
    ...
err_disconnect:
    tps6598x_disconnect(tps, 0);
err_unregister_port:
    tps->data->unregister_port(tps);
    ...
}

The cleanup path fails to cancel the pending delayed work. The tps structure
is then freed by devres when the probe returns the error, causing the delayed
work to dereference freed memory when it executes.

Does this same missing cleanup apply to cd321x->update_work if connect()
succeeds but devm_request_threaded_irq() subsequently fails?

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