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

[sashiko-bot@kernel.org]

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

New issues:
- [High] sn201202x_suspend fails to wake up the device if the sleep completion times out, leaving the device in an unresponsive state during error recovery.
- [High] Passing NULL as the devname parameter to devm_request_irq causes a kernel panic during procfs directory creation.
- [Medium] Missing reinit_completion() before spmi_command_wakeup() in probe can cause premature timeout completion due to a spurious or pending interrupt.
- [Medium] The busy-wait check `val == (reg | 0x80)` incorrectly handles register addresses >= 0x80, leading to an infinite polling loop.

Pre-existing issues:
- [High] If wakeup-source is present in DT but irq is missing, tipd_suspend and tipd_resume will erroneously operate on IRQ 0.
- [High] If connect() fails and no IRQ is present, tipd_init leaks the initialized delayed work queue, leading to a Use-After-Free.
--

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

This patch adds support for the sn201202x (ACE3) controller, which is a tipd
variant utilizing the SPMI bus. It provides the SPMI regmap implementation
along with probe and PM routines.

> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> index 0000000000000..73575a854bde1
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c
[ ... ]
> +static int regmap_sn201202x_select_reg(struct spmi_device *sdev, u8 reg)
> +{
[ ... ]
> +	while (--attempts) {
> +		err = spmi_register_read(sdev, 0, &val);
> +		if (err)
> +			return err;
> +		if (val == (reg | 0x80)) {
> +			if (!warned) {
> +				dev_warn(tps->dev,
> +					 "Got interrupt but selection not complete?\n");
> +				warned = true;
> +			}
> +			msleep(20);
> +			continue;
> +		}
> +		if (val == reg)
> +			return 0;
> +		return -EIO;
> +	}
> +
> +	return -EIO;
> +}

[Severity: Medium]
Does this logic work correctly for register addresses >= 0x80? If reg is
0x85, for example, then (reg | 0x80) evaluates to just reg. This would make
the busy condition val == (reg | 0x80) identical to the success condition 
val == reg. Since the busy condition is checked first, it will falsely match,
trigger the warning, and eventually timeout returning -EIO.

[ ... ]
> +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);
> +	if (ret)
> +		return ret;
> +	ret = devm_request_irq(&device->dev, irq_sleep, sn201202x_irq,
> +			       0, NULL, &sn->sleep_completion);
> +	if (ret)
> +		return ret;
> +	ret = devm_request_irq(&device->dev, irq_wake, sn201202x_irq,
> +			       0, NULL, &sn->wake_completion);

[Severity: High]
Can passing NULL as the devname parameter to devm_request_irq() cause a kernel
panic during boot? The IRQ subsystem passes the devname to proc_mkdir() to 
create an entry in /proc/irq/, and calling strlen() on a NULL name will fault.

> +	if (ret)
> +		return ret;
> +
> +	spmi_device_set_drvdata(device, tps);
> +	tps->regmap = devm_regmap_init_sn201202x(device, &tps6598x_regmap_config);
> +	if (IS_ERR(tps->regmap))
> +		return PTR_ERR(tps->regmap);
> +
> +	ret = spmi_command_wakeup(device);
> +	if (ret)
> +		return ret;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;

[Severity: Medium]
Should there be a call to reinit_completion(&sn->wake_completion) right 
before sending the wakeup command? If a pending or spurious interrupt fires 
immediately after the IRQ is registered above, wait_for_completion_timeout()
might consume the stale completion and return immediately without waiting for
the actual hardware wakeup sequence to finish.

[ ... ]
> +static int __maybe_unused sn201202x_suspend(struct device *dev)
> +{
[ ... ]
> +	err = spmi_command_sleep(sn->sdev);
> +	if (err)
> +		goto out_resume;
> +	if (!wait_for_completion_timeout(&sn->sleep_completion, msecs_to_jiffies(100))) {
> +		err = -ETIMEDOUT;
> +		goto out_resume;
> +	}
> +	return 0;
> +
> +out_resume:
> +	tipd_resume(tps);
> +	return err;
> +}

[Severity: High]
Does this error path leave the device in an unresponsive state? If the sleep 
command succeeds but wait_for_completion_timeout() times out (perhaps due to 
a lost interrupt), we jump to out_resume and call tipd_resume(). Since the
device was commanded to sleep and no spmi_command_wakeup() is issued in this
error path, the regmap reads inside tipd_resume() could fail or hang.


I also noticed a couple of pre-existing issues in the core code that might be
worth addressing:

[Severity: High]
This is a pre-existing issue, but does tipd_suspend() correctly handle cases
where the wakeup-source property is present in the device tree but the irq
property is missing?

drivers/usb/typec/tipd/core.c:tipd_suspend() {
    if (tps->wakeup) {
        disable_irq(tps->irq);
        enable_irq_wake(tps->irq);
    } else if (tps->reset) {
...

In tipd_init(), tps->wakeup can be set to true even if tps->irq is 0. If that
happens, calling disable_irq() and enable_irq_wake() here will operate on
IRQ 0, which might disable a core system interrupt like the timer.


[Severity: High]
This isn't a regression introduced by this patch, but could tipd_init() leak 
the delayed work queue if connect() fails?

drivers/usb/typec/tipd/core.c:tipd_init() {
...
    if (status & TPS_STATUS_PLUG_PRESENT) {
        ret = -EINVAL;
        ...
        ret = tps->data->connect(tps, status);
        if (ret)
            dev_err(tps->dev, "failed to register partner\n");
    }

    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 tps->data->connect() fails, ret holds an error code. If the device has no
IRQ, we queue the polling work. Since ret isn't cleared, the subsequent
if (ret) check triggers a jump to err_disconnect, freeing the tps structure
without cancelling the delayed work queue. This could lead to a
use-after-free when the timer fires.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-tipd-ace3-v3-0-8332f2eca4fc@chaosmail.tech?part=3