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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The suspend and resume handlers send SPMI sleep and wakeup commands directly to the hardware without acquiring `tps->lock` or the regmap lock, racing with concurrent I/O.
- [Low] The driver calls `devm_request_irq()` with a `NULL` `devname` parameter for the select, sleep, and wake interrupts, breaking interrupt debugging.
- [Low] Missing `depends on POWER_SUPPLY` for `CONFIG_TYPEC_SN201202X` causes a build failure.

Pre-existing issues:
- [High] Delayed works (`cd321x->update_work` and `tps->wq_poll`) are not cancelled on the `tipd_init()` error paths, leading to a Use-After-Free.
--

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

usb: typec: tipd: Add sn201202x support

This commit adds support for the sn201202x (ACE3) controller. It is a tipd
variant that exposes a logical register interface over the SPMI bus.

> diff --git a/drivers/usb/typec/tipd/Kconfig b/drivers/usb/typec/tipd/Kconfig
> --- a/drivers/usb/typec/tipd/Kconfig
> +++ b/drivers/usb/typec/tipd/Kconfig
> @@ -14,3 +14,15 @@ config TYPEC_TPS6598X
>  
>  	  If you choose to build this driver as a dynamically linked module, the
>  	  module will be called tps6598x.ko.
> +
> +config TYPEC_SN201202X
> +	tristate "TI SN201202x USB Power Delivery controller support"
> +	depends on SPMI
> +	select TYPEC_TPS6598X_CORE

[Severity: Low]
Can this lead to a build failure when TYPEC_SN201202X is built into the kernel
and POWER_SUPPLY is built as a module?

TYPEC_TPS6598X_CORE uses power supply APIs, so if this symbol selects
TYPEC_TPS6598X_CORE without also depending on POWER_SUPPLY, it could cause
unresolved symbols during linking.

[ ... ]
> 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)
> +{
[ ... ]
> +	init_completion(&sn->select_completion);
> +	init_completion(&sn->sleep_completion);
> +	init_completion(&sn->wake_completion);
> +
> +	ret = devm_request_irq(&device->dev, irq_select, sn201202x_irq,
> +			       0, NULL, &sn->select_completion);

[Severity: Low]
Is it intentional to pass NULL as the devname parameter for the select, sleep,
and wake interrupts here? Passing NULL causes these interrupts to be displayed
as (null) in /proc/interrupts and tracepoints, which might break interrupt
debugging tools.

[ ... ]
> +	ret = spmi_command_wakeup(device);
> +	if (ret)
> +		return ret;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +
> +	ret = tipd_init(tps);
> +	if (ret)
> +		spmi_command_sleep(device);
> +	return ret;
> +}

[Severity: High]
This is a pre-existing issue, but does tipd_init() fail to cancel the delayed
works on its error paths?

If the driver is in polling mode, tipd_init() queues tps->wq_poll:

	if (tps->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;

If connect() failed earlier and ret is set, or if another error occurs, it
jumps to err_disconnect and returns an error without cancelling the queued
delayed works. When probe fails, devres will free the tps structure,
potentially leaving a use-after-free when the work executes.

[ ... ]
> +static int __maybe_unused sn201202x_resume(struct device *dev)
> +{
> +	struct tps6598x *tps = dev_get_drvdata(dev);
> +	struct sn201202x *sn = tps_to_sn(tps);
> +	int err;
> +
> +	reinit_completion(&sn->wake_completion);
> +	err = spmi_command_wakeup(sn->sdev);

[Severity: High]
Do the suspend and resume handlers race with concurrent regmap I/O?

Since the system workqueue is not freezable, cd321x->update_work can run
concurrently with sn201202x_suspend() and sn201202x_resume(). Sending SPMI
sleep or wakeup commands directly to the hardware without acquiring tps->lock
or the regmap lock could allow these commands to interleave with active
multi-register regmap transactions, potentially corrupting the bus state.

[ ... ]
> +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);

[Severity: High]
Could the SPMI sleep command here also interleave with an active regmap
transaction if the update work executes concurrently?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-tipd-ace3-v4-0-79855b150914@chaosmail.tech?part=3