Re: [PATCH 12/19] 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:

Pre-existing issues:
- [High] Missing memory barrier (dma_rmb) after reading DMA descriptor completion flag
--

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

thunderbolt: Add ring_configure to tb_nhi_ops

The Apple NHI has to configure and enable each ring with a single write
and 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.

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 3cb69deec63e8..696da52abff6c 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c

[Severity: High]
This isn't a bug introduced by this patch, but this patchset specifically
targets arm64 (Apple Silicon), where weak memory ordering will actively
manifest an issue in the existing code.

In both ring_work() and tb_ring_poll(), the DMA descriptor completion flag
is checked, and if completed, the other descriptor fields (length, eof, sof)
are read immediately without enforcing memory read ordering.

drivers/thunderbolt/nhi.c:ring_work() {
    ...
	while (!ring_empty(ring)) {
		if (!(ring->descriptors[ring->tail].flags
				& RING_DESC_COMPLETED))
			break;
		frame = list_first_entry(&ring->in_flight, typeof(*frame),
					 list);
		list_move_tail(&frame->list, &done);
		if (!ring->is_tx) {
			frame->size = ring->descriptors[ring->tail].length;
			frame->eof = ring->descriptors[ring->tail].eof;
			frame->sof = ring->descriptors[ring->tail].sof;
			frame->flags = ring->descriptors[ring->tail].flags;
		}
    ...
}

drivers/thunderbolt/nhi.c:tb_ring_poll() {
    ...
	if (ring->descriptors[ring->tail].flags & RING_DESC_COMPLETED) {
		frame = list_first_entry(&ring->in_flight, typeof(*frame),
					 list);
		list_del_init(&frame->list);

		if (!ring->is_tx) {
			frame->size = ring->descriptors[ring->tail].length;
			frame->eof = ring->descriptors[ring->tail].eof;
			frame->sof = ring->descriptors[ring->tail].sof;
			frame->flags = ring->descriptors[ring->tail].flags;
		}
    ...
}

Could the CPU speculatively reorder these reads?

If it fetches length before the device DMA transaction writing the complete
descriptor is fully visible, could it result in reading a stale value and
improper frame handling?

Does this code need a memory barrier like dma_rmb() after reading the
completion flag?

[ ... ]

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