Re: [PATCH 3/3] thunderbolt: Add quirk to reset host interface for AMD USB4 routers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] thunderbolt: NULL pointer dereference in nhi_reset_work() due to late dev_set_drvdata()
--

commit 9823327103fd3f6d1af1a55c73b76fed9264dad0
Author: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

thunderbolt: Add quirk to reset host interface for AMD USB4 routers

This commit adds a quirk for AMD USB4 routers to reset the host interface
before reprogramming a reused DMA HopID, preventing TX ring hangs. The reset
operation waits for DMA rings to stop while leaving other tunnels intact.

> diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
> index 4408229d8ac7b..b26f5b39e923b 100644
> --- a/drivers/thunderbolt/pci.c
> +++ b/drivers/thunderbolt/pci.c
[ ... ]
> @@ -63,6 +63,27 @@ static void nhi_pci_check_quirks(struct tb_nhi_pci *nhi_pci)
>  			nhi->quirks |= QUIRK_E2E;
>  			break;
>  		}
> +	} else if (pdev->vendor == PCI_VENDOR_ID_AMD) {
> +		switch (pdev->device) {
[ ... ]
> +		case PCI_DEVICE_ID_AMD_1AH_M70H_NHI0:
> +		case PCI_DEVICE_ID_AMD_1AH_M70H_NHI1:
> +			/*
> +			 * These hosts may hang the Tx ring if its HopID is
> +			 * programmed again without a host interface reset
> +			 * in between.
> +			 */
> +			nhi->quirks |= QUIRK_RESET_DMA_ON_REUSE;

[Severity: High]
Does adding this quirk expose a race condition during probe that leads to a
NULL pointer dereference in nhi_reset_work()?

When QUIRK_RESET_DMA_ON_REUSE is set, it seems nhi_reset_work() can be queued
when a DMA ring stops. However, looking at the initialization sequence in
nhi_probe(), drvdata is set very late:

drivers/thunderbolt/nhi.c:nhi_probe() {
	...
	res = tb_domain_add(tb, host_reset);
	if (res) {
		...
	}
	dev_set_drvdata(dev, tb);
	...
}

During tb_domain_add(), event processing is started and the lock is dropped
before returning:

drivers/thunderbolt/domain.c:tb_domain_add() {
	...
	/* This starts event processing */
	mutex_unlock(&tb->lock);

	device_init_wakeup(&tb->dev, true);
	...
}

Because tb_domain_add() allows asynchronous events (like DMA ring start/stop
operations) to execute before returning to nhi_probe(), nhi_reset_work() can
be scheduled while drvdata is still NULL. If this happens, it leads to a crash
when trying to acquire the lock:

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

	/* The connection manager must be blocked over the reset */
	guard(mutex)(&tb->lock);
	...
}

Can we ensure drvdata is set before enabling event processing, or otherwise
prevent the reset work from running before tb_domain_add() finishes?

> +			break;
> +		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790854235.git.Basavaraj.Natikar@amd.com?part=3