Re: [PATCH v2 15/22] thunderbolt: Add ring_configure to tb_nhi_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Combining the ring enable and E2E flow control writes into a single register write overrides a documented 2-step hardware initialization sequence, risking breakage on standard Intel Thunderbolt controllers.
--

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

    thunderbolt: Add ring_configure to tb_nhi_ops
    
    The Apple NHI also uses a separate register range for the receive ring
    PDF masks. Add an optional tb_nhi_ops hook for it which falls back to
    the standard USB4 NHI method when it is not set.
    
    For the standard method also just write E2E flow control at the same
    time the rest of the configuration is written.

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 4ef86e7c02851..2d622b365c4f9 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -244,6 +244,24 @@ static void ring_iowrite32options(struct tb_ring *ring, u32 value, u32 offset)
>  	iowrite32(value, nhi_ring_options_base(ring) + offset);
>  }
>  
> +static void ring_configure(struct tb_ring *ring, u32 flags, u32 e2e_flags)
> +{
> +	if (ring->is_tx)
> +		ring_iowrite32options(ring, 0, 4);
> +	else
> +		ring_iowrite32options(ring, ring->sof_mask << 16 | ring->eof_mask, 4);
> +
> +	ring_iowrite32options(ring, flags | e2e_flags, 0);
> +}

[Severity: Medium]
Does combining the ring valid bit and E2E configuration into this single
write in ring_configure() cause a regression by overriding the required
hardware initialization sequence?

By writing flags and e2e_flags at the same time, this could cause standard
Intel controllers to silently ignore the E2E configuration if they expect
the valid bit to be set first. Any time an RX ring with RING_FLAG_E2E is
started via tb_ring_start() on a standard Intel controller, this path is
triggered.

[ ... ]

> @@ -722,30 +741,13 @@ void tb_ring_start(struct tb_ring *ring)
>  		flags = RING_FLAG_ENABLE | RING_FLAG_RAW;
>  	}
>  
> -	ring_iowrite64desc(ring, ring->descriptors_dma, 0);
> -	if (ring->is_tx) {
> -		ring_iowrite32desc(ring, ring->size, 12);
> -		ring_iowrite32options(ring, 0, 4);
> -		ring_iowrite32options(ring, flags, 0);
> -	} else {
> -		u32 sof_eof_mask = ring->sof_mask << 16 | ring->eof_mask;
> -
> -		ring_iowrite32desc(ring, (frame_size << 16) | ring->size, 12);
> -		ring_iowrite32options(ring, sof_eof_mask, 4);
> -		ring_iowrite32options(ring, flags, 0);
> -	}
> -
> -	/*
> -	 * Now that the ring valid bit is set we can configure E2E if
> -	 * enabled for the ring.
> -	 */

[Severity: Medium]
Could replacing this explicit two-stage initialization sequence in
tb_ring_start() break E2E flow control on existing hardware?

The removed comment implies the hardware specifically requires the valid bit
to be set before it will accept E2E flow control configuration. If this is
done in a single step via nhi_ring_configure(), it might lead to RX ring
overflows and packet loss in subsystems that rely on it, such as Thunderbolt
networking.

>  	if (ring->flags & RING_FLAG_E2E) {
>  		if (!ring->is_tx) {
>  			u32 hop;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-b4-apple-soc-tbt-v2-0-1f80085f93fb@kernel.org?part=15