[PATCH] thunderbolt: stream: Make read return framing error to the userspace

[PATCH] thunderbolt: stream: Make read return framing error to the userspace

[Mika Westerberg]

If any of the frames are corrupted during the receive we currently log a
warning and pass them to the userspace without it knowing they were
corrupted. Make the driver behave better and return an error (-EIO) to
the userspace in that case.

For the CLOSE packet, payload does not matter so we handle it the usual
way.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/stream.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/thunderbolt/stream.c b/drivers/thunderbolt/stream.c
index a49b0b62518d..bdcca78f3449 100644
--- a/drivers/thunderbolt/stream.c
+++ b/drivers/thunderbolt/stream.c
@@ -375,6 +375,7 @@ tbstream_dev_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 {
 	struct tbstream_frame *sf = container_of(frame, typeof(*sf), frame);
 	struct tbstream_dev *sdev = sf->sdev;
+	__poll_t mask;
 
 	if (canceled)
 		return;
@@ -382,19 +383,17 @@ tbstream_dev_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	sf->completed = true;
 	sdev->rx_ring.prod++;
 
-	if (sf->frame.flags & RING_DESC_CRC_ERROR) {
-		pr_warn("RX CRC error\n");
-	} else if (sf->frame.flags & RING_DESC_BUFFER_OVERRUN) {
-		pr_warn("RX buffer overrun\n");
-	} else {
-		__poll_t mask = EPOLLIN | EPOLLRDNORM;
-
-		if (sf->frame.eof == TBSTREAM_CLOSE) {
-			WRITE_ONCE(sdev->close_received, true);
-			mask |= EPOLLHUP;
-		}
-		wake_up_interruptible_poll(&sdev->wait, mask);
+	mask = EPOLLIN | EPOLLRDNORM;
+	/*
+	 * The CLOSE packet does not have a payload so the flags do not
+	 * matter. The read_iter() deals with the flags.
+	 */
+	if (sf->frame.eof == TBSTREAM_CLOSE) {
+		WRITE_ONCE(sdev->close_received, true);
+		mask |= EPOLLHUP;
 	}
+
+	wake_up_interruptible_poll(&sdev->wait, mask);
 }
 
 static struct tbstream_frame *
@@ -875,6 +874,19 @@ tbstream_dev_fops_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 				WRITE_ONCE(sdev->closed, true);
 			}
 			break;
+		} else if (sf->frame.flags &
+			   (RING_DESC_CRC_ERROR | RING_DESC_BUFFER_OVERRUN)) {
+			/*
+			 * If something was already read return that now
+			 * and next read will report the error.
+			 */
+			if (!nbytes) {
+				pr_warn("corrupted frame received, flags %#x\n",
+					sf->frame.flags);
+				tbstream_dev_consume_rx(sdev);
+				ret = -EIO;
+			}
+			break;
 		}
 
 		sf_size = tb_ring_frame_size(&sf->frame);
-- 
2.50.1

Re: [PATCH] thunderbolt: stream: Make read return framing error to the userspace

[Ivy Lopez]

On Mon, Sep 21, 2026 at 11:19 AM UTC, Mika Westerberg wrote:
> +                     if (!nbytes) {
> +                             pr_warn("corrupted frame received, flags %#x\n",
> +                                     sf->frame.flags);
> +                             tbstream_dev_consume_rx(sdev);
> +                             ret = -EIO;
> +                     }
> +                     break;

One question: EPOLLIN is set unconditionally in the rx callback
regardless of frame flags, and tbstream_dev_fops_poll() only checks
tbstream_ring_available() (pure ring depth), never frame flags.
EPOLLERR is used elsewhere in this file but only for device-level
invalidity, not per-frame corruption. So userspace still has no way
to learn a corrupted frame is queued via poll() before calling
read() and getting -EIO. I'm just wondering, is this intentional,
or worth a follow-up?

Also, fwiw, this doesn't apply cleanly against current usb-next for
me, git apply fails on the rx_callback hunk. Looks like it's against
a tree predating the busy-polling changes.

Not a blocker either way.

Reviewed-by: Ivy Lopez <skunkolee@gmail.com>

Re: [PATCH] thunderbolt: stream: Make read return framing error to the userspace

[Mika Westerberg]

Hi,

On Mon, Sep 21, 2026 at 06:17:42PM -0600, Ivy Lopez wrote:
> On Mon, Sep 21, 2026 at 11:19 AM UTC, Mika Westerberg wrote:
> > +                     if (!nbytes) {
> > +                             pr_warn("corrupted frame received, flags %#x\n",
> > +                                     sf->frame.flags);
> > +                             tbstream_dev_consume_rx(sdev);
> > +                             ret = -EIO;
> > +                     }
> > +                     break;
> 
> One question: EPOLLIN is set unconditionally in the rx callback
> regardless of frame flags, and tbstream_dev_fops_poll() only checks
> tbstream_ring_available() (pure ring depth), never frame flags.
> EPOLLERR is used elsewhere in this file but only for device-level
> invalidity, not per-frame corruption. So userspace still has no way
> to learn a corrupted frame is queued via poll() before calling
> read() and getting -EIO. I'm just wondering, is this intentional,
> or worth a follow-up?

It's intentional. EPOLLERR is used to indicate that this file is
"finished". A single framing error would allow the userspace still to
continue.

> Also, fwiw, this doesn't apply cleanly against current usb-next for
> me, git apply fails on the rx_callback hunk. Looks like it's against
> a tree predating the busy-polling changes.

It'a against thunderbolt.git/next [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git/log/?h=next

> Not a blocker either way.
> 
> Reviewed-by: Ivy Lopez <skunkolee@gmail.com>

Thanks!

Re: [PATCH] thunderbolt: stream: Make read return framing error to the userspace

[Mika Westerberg]

On Mon, Sep 21, 2026 at 01:19:21PM +0200, Mika Westerberg wrote:
> If any of the frames are corrupted during the receive we currently log a
> warning and pass them to the userspace without it knowing they were
> corrupted. Make the driver behave better and return an error (-EIO) to
> the userspace in that case.
> 
> For the CLOSE packet, payload does not matter so we handle it the usual
> way.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied to thunderbolt.git/next.