Re: [PATCH v2 4/7] thunderbolt: add platform NHI driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] thunderbolt: broken system PM when sharing tb_nhi_pm_ops on
  platform_driver
- [Medium] thunderbolt: missing cleanup in nhi_platform_init_irq() and
  tb_nhi_probe()
--

commit 185e80648753aea8ed254f3effc097c624617d73
Author: Chunfeng Yun <chunfeng.yun@mediatek.com>

thunderbolt: add platform NHI driver

This commit adds a platform driver for Thunderbolt/USB4 Native Host Interface
(NHI) controllers integrated as memory-mapped platform devices via Device Tree
or ACPI, including support for an optional dedicated wakeup interrupt.

> diff --git a/drivers/thunderbolt/platform.c b/drivers/thunderbolt/platform.c
> new file mode 100644
> index 0000000000000..3562b6c7ea7db
> --- /dev/null
> +++ b/drivers/thunderbolt/platform.c
> @@ -0,0 +1,172 @@
[ ... ]
> +static int nhi_platform_init(struct tb_nhi *nhi)
> +{
> +	/* avoid device link fail for pcie tunnel */
> +	pm_runtime_set_active(nhi->dev);
> +	pm_runtime_get_noresume(nhi->dev);
> +	pm_runtime_enable(nhi->dev);
> +
> +	device_set_wakeup_capable(nhi->dev, true);
> +	device_enable_async_suspend(nhi->dev);
> +
> +	return 0;
> +}
> +
> +static void nhi_platform_clear_wake_irq(void *dev)
> +{
> +	dev_pm_clear_wake_irq(dev);
> +}
> +
> +static int nhi_platform_init_irq(struct tb_nhi *nhi)
> +{
> +	struct device *dev = nhi->dev;
> +	int ret;
> +
> +	ret = devm_request_irq(dev, nhi->irq, tb_nhi_isr, IRQF_NO_SUSPEND,
> +			       dev_name(dev), nhi);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "request irq failed\n");
> +
> +	if (nhi->wake_irq) {
> +		ret = dev_pm_set_dedicated_wake_irq(dev, nhi->wake_irq);
> +		if (ret) {
> +			return dev_err_probe(dev, ret, "set wake irq %d failed\n", nhi->wake_irq);
> +		}
> +
> +		dev_info(dev, "wake irq %d\n", nhi->wake_irq);
> +
> +		ret = devm_add_action_or_reset(dev, nhi_platform_clear_wake_irq, dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static void nhi_platform_exit(struct tb_nhi *nhi)
> +{
> +	devm_free_irq(nhi->dev, nhi->irq, nhi);
> +	flush_work(&nhi->interrupt_work);
> +
> +	pm_runtime_put_noidle(nhi->dev);
> +	pm_runtime_disable(nhi->dev);
> +	pm_runtime_set_suspended(nhi->dev);
> +}

[Severity: Medium]
Does a probe failure before tb_domain_add() bypass nhi_platform_exit(),
leaving nhi->interrupt_work unflushed and runtime PM enabled on the unbound
device?

In tb_nhi_probe(), tb_nhi_shutdown() is only called if tb_domain_add() fails:

drivers/thunderbolt/nhi.c:tb_nhi_probe() {
	...
	res = nhi->ops->init_interrupts(nhi);
	if (res)
		return dev_err_probe(dev, res, "cannot enable interrupts, aborting\n");

	spin_lock_init(&nhi->lock);

	res = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
	if (res)
		return dev_err_probe(dev, res, "failed to set DMA mask\n");

	if (nhi->ops->init) {
		res = nhi->ops->init(nhi);
		if (res)
			return dev_err_probe(dev, res, "NHI specific init failed\n");
	}

	init_completion(&nhi->domain_released);

	tb = nhi_select_cm(nhi);
	if (!tb)
		return dev_err_probe(dev, -ENODEV,
			"failed to determine connection manager, aborting\n");
	...
}

If dev_pm_set_dedicated_wake_irq() fails in nhi_platform_init_irq(), or if
dma_set_mask_and_coherent() or nhi_select_cm() fails in tb_nhi_probe(),
devres frees nhi->irq, unmaps nhi->iobase, and frees nhi without ever
flushing nhi->interrupt_work, which can cause a use-after-free if
tb_nhi_isr() scheduled the work item.  In addition, if nhi_select_cm() fails
after nhi_platform_init() has run, runtime PM stays enabled with an elevated
usage count, so pm_runtime_reinit() in device_unbind_cleanup() bails out and
a subsequent re-probe warns on unbalanced pm_runtime_enable().

[ ... ]

> +static struct platform_driver nhi_platform_driver = {
> +	.probe = nhi_platform_probe,
> +	.remove = nhi_platform_remove,
> +	.shutdown = nhi_platform_shutdown,
> +	.driver = {
> +		.name = "thunderbolt",
> +		.of_match_table = of_match_ptr(nhi_platform_of_match),
> +		.acpi_match_table = ACPI_PTR(nhi_platform_acpi_match),
> +		.pm = &tb_nhi_pm_ops,
> +	},
> +};

[Severity: High]
Can system suspend and resume run while the platform NHI is runtime-suspended
when using tb_nhi_pm_ops directly on a platform_driver?

On PCI, pci_pm_suspend() runtime-resumes the device before calling the
driver's .suspend and .suspend_noirq callbacks.  Neither the platform bus,
genpd, nor nhi_suspend() does that, and device_wakeup_enable() prevents
direct_complete.  If the controller has autosuspended before system suspend:

1. nhi_suspend_noirq() -> tb_domain_suspend_noirq() runs while the device is
   runtime-suspended (with clocks or power domains gated) and calls
   tb_ctl_stop() on rings already stopped by nhi_runtime_suspend(), triggering
   dev_WARN() in tb_ring_stop().
2. On resume, nhi_resume_noirq() -> tb_domain_resume_noirq() calls
   tb_ctl_start(), touching MMIO while runtime status is still RPM_SUSPENDED
   and queuing ctl->rx_packets[] onto ctl->rx->in_flight.
3. In the complete phase, nhi_complete() sees pm_runtime_suspended(dev) is
    still true and calls pm_runtime_resume(dev) -> nhi_runtime_resume() ->
   tb_ctl_start() a second time:

drivers/thunderbolt/nhi.c:nhi_complete() {
	struct tb *tb = dev_get_drvdata(dev);

	/*
	 * If we were runtime suspended when system suspend started,
	 * schedule runtime resume now. It should bring the domain back
	 * to functional state.
	 */
	if (pm_runtime_suspended(dev))
		pm_runtime_resume(dev);
	else
		tb_domain_complete(tb);
}

The second tb_ctl_start() warns that the rings are already started and calls
tb_ctl_rx_submit() -> __tb_ring_enqueue() -> list_add_tail() on
ctl->rx_packets[] frames that are already linked in ctl->rx->in_flight,
corrupting the ring list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110114925.722014-2-chunfeng.yun@mediatek.com?part=4