Re: [PATCH 08/19] thunderbolt: Allocate ring HopID before requesting the ring interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The teardown order in tb_ring_free() was not updated to match the new reversed initialization order in tb_ring_alloc(), introducing a race condition where a hop ID is freed before its associated IRQ is released.
--

commit ed19fe4c2978aaf176d934b42ef25a517be4cd98
Author: Sven Peter <sven@kernel.org>

thunderbolt: Allocate ring HopID before requesting the ring interrupt

This commit changes the setup order in tb_ring_alloc() to allocate the
HopID before requesting the ring interrupt. This is necessary because
the Apple NHI picks the interrupt based on the HopID.

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 5809809f64d42..6a97dcd57deb2 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c

[ ... ]

> @@ -581,19 +591,18 @@ static struct tb_ring *tb_ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
>  	if (!ring->descriptors)
>  		goto err_free_ring;
>  
> +	if (nhi_alloc_hop(nhi, ring))
> +		goto err_free_descs;
> +
>  	if (!(flags & RING_FLAG_NO_INTERRUPT) && nhi->ops->request_ring_irq) {
>  		if (nhi->ops->request_ring_irq(ring, flags & RING_FLAG_NO_SUSPEND))
> -			goto err_free_descs;
> +			goto err_free_hop;
>  	}
>  
> -	if (nhi_alloc_hop(nhi, ring))
> -		goto err_release_msix;

[Severity: High]
Since the allocation order in tb_ring_alloc() is reversed here so the hop ID
is allocated before the IRQ, does the teardown order in tb_ring_free() also
need to be updated to match?

Looking at tb_ring_free(), it clears the hop ID from the nhi arrays and drops
the lock before releasing the IRQ:

drivers/thunderbolt/nhi.c:tb_ring_free() {
    ...
    if (ring->is_tx)
        ring->nhi->tx_rings[ring->hop] = NULL;
    else
        ring->nhi->rx_rings[ring->hop] = NULL;
    ...
    spin_unlock_irq(&ring->nhi->lock);

    if (nhi->ops->release_ring_irq)
        nhi->ops->release_ring_irq(ring);
    ...
}

Could this leave a window during concurrent hotplug, stream/network
initialization, or teardown of multiple devices where another thread calling
tb_ring_alloc() dynamically reallocates this same, newly-available hop ID?

If that happens, especially on Apple NHI where the IRQ is determined directly
by the hop ID, the new thread might attempt to request an IRQ that is still
registered to the old ring. Would this cause devm_request_irq() to fail
with an unexpected -EBUSY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830-b4-apple-soc-tbt-v1-0-44bc9348683c@kernel.org?part=8