Re: [PATCH v2 3/3] usb: typec: tipd: Add sn201202x support

[sashiko-bot@kernel.org]

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

New issues:
- [High] `sn201202x_suspend` fails to rollback state if the SPMI sleep command fails, permanently breaking the device.
- [Medium] Missing SPMI sleep command on probe error path leaks hardware power state.

Pre-existing issues:
- [High] Pending delayed work is not canceled when `tipd_init()` fails, leading to Use-After-Free.
--

--- Patch [3]: [PATCH v2 3/3] usb: typec: tipd: Add sn201202x support ---
commit 698268fd8b63e1a6de3f13213aca2f84b22da5ef
Author: Sasha Finkelstein <k@chaosmail.tech>
Subject: usb: typec: tipd: Add sn201202x support

Add support for sn201202x (aka ACE3), a tipd variant that uses a very
similar register map, that is exposed over a "logical register"
interface on the SPMI bus.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index aadadfdc6ed12..6577bc2f1e0f9 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1889,6 +1889,26 @@ const struct tipd_data tipd_tps25750_data = {
>  };
>  EXPORT_SYMBOL_GPL(tipd_tps25750_data);
>  
> +const struct tipd_data tipd_sn201202x_data = {
> +	.irq_handler = cd321x_interrupt,
> +	.irq_mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_PLUG_EVENT,
> +	.tps_struct_size = sizeof(struct sn201202x),
> +	.remove = cd321x_remove,
> +	.register_port = cd321x_register_port,
> +	.unregister_port = cd321x_unregister_port,
> +	.trace_data_status = trace_cd321x_data_status,
> +	.trace_power_status = trace_tps6598x_power_status,
> +	.trace_status = trace_tps6598x_status,
> +	.init = cd321x_init,
> +	.read_data_status = cd321x_read_data_status,
> +	.reset = cd321x_reset,
> +	.switch_power_state = cd321x_switch_power_state,
> +	.connect = cd321x_connect,
> +};

[Severity: High]
This is a pre-existing issue, but does this error path leave delayed work
pending, potentially causing a use-after-free?

In tipd_init(), if a plug is present, tps->data->connect(tps, status)
is called. For this struct, that resolves to cd321x_connect(), which
schedules delayed work.

If a subsequent step in tipd_init() fails (like requesting the IRQ):

core.c:tipd_init() {
    ...
    if (tps->irq) {
        ret = devm_request_threaded_irq(...);
    }
    if (ret)
        goto err_disconnect;
    ...

The err_disconnect label does not cancel the delayed work before returning:

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

When devres frees the device structure memory on probe failure, won't the
workqueue execute the callback on the freed memory when the timer expires?

> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c
[ ... ]
> +static int sn201202x_probe(struct spmi_device *device)
> +{
[ ... ]
> +	ret = spmi_command_wakeup(device);
> +	if (ret)
> +		return ret;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +
> +	return tipd_init(tps);
> +}

[Severity: Medium]
Does this leak the hardware power state if tipd_init() fails?

If tipd_init(tps) fails, the error is returned directly. Should this path 
call spmi_command_sleep() to reverse the spmi_command_wakeup() called
earlier in this function?

[ ... ]
> +static int __maybe_unused sn201202x_suspend(struct device *dev)
> +{
> +	struct tps6598x *tps = dev_get_drvdata(dev);
> +	struct sn201202x *sn = tps_to_sn(tps);
> +	int err;
> +
> +	err = tipd_suspend(tps);
> +	if (err)
> +		return err;
> +	reinit_completion(&sn->sleep_completion);
> +	err = spmi_command_sleep(sn->sdev);
> +	if (err)
> +		return err;
> +	if (!wait_for_completion_timeout(&sn->sleep_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +	return 0;
> +}

[Severity: High]
If spmi_command_sleep() fails or times out, does the device state need to be
rolled back?

If this returns an error, the PM core aborts system suspend but does not
invoke the resume callback for the failing device. Should this path call
tipd_resume(tps) to undo the effects of tipd_suspend(tps) before returning,
so the device is not left permanently broken and unresponsive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-tipd-ace3-v2-0-1345957bd635@chaosmail.tech?part=3