-----Original Message-----
From: Paolo Abeni <pabeni@redhat.com>
Sent: Thursday, August 6, 2026 3:43 AM
Author: Selvamani Rajagopal [off-list ref]
Would a rename (something like rx_resync or skip_until_frame_start), or a
second flag, keep the two conditions distinguishable when debugging?
Agreed. Changing the flag to "wait_until_start_valid"
[ ... ]
This isn't a bug, but could this use the documented accessor instead of
reaching into the sk_buff layout fields?
if (!tc6->rx_skb || skb_tailroom(tc6->rx_skb) < length)
The arithmetic here does match the overflow test inside skb_put() for both
representations selected by NET_SKBUFF_DATA_USES_OFFSET, but skb->tail and
skb->end are internal fields, and skb_tailroom() in
include/linux/skbuff.h additionally returns 0 for non-linear skbs.
Changed the code not to access fields in skbuff and using skb_tailroom instead.
[ ... ]
[Severity: Medium]
[Finding: main-2-5]
[Sources: opus-5]
Since the loop no longer returns on the first -EAGAIN, how many STATUS0
control transfers does one buffer now cost?
All footers of one buffer are sampled on the wire before any of them is
processed, so a single overflow event tends to leave the extended status
bit set in most footers of that buffer. With OA_TC6_MAX_TX_CHUNKS = 48
that turns one read/write pair into up to 48 pairs of control transfers
in the SPI IRQ thread, before the received frames are handed to the
stack.
While this is true, we can't afford to not to read STATUS0 register while handling
max 48 data chunks, as there is a chance that status bits may be set during this
period. Improving it to not to go for write when status0 register value is 0.
The retry flag is sticky, so oa_tc6_look_for_new_frame() runs at loop exit
even when a later chunk recovered. Sequence:
This also appears to undo the tc6->rx_buf_overflow = false added in the
end-plus-start-in-one-chunk branch of oa_tc6_prcs_rx_chunk_payload().
Is the trailing call needed for state consistency at all? The overflow
path already sets the flag in oa_tc6_process_extended_status(), the
tailroom and NULL paths set it in oa_tc6_update_rx_skb(), and the only
remaining case, -ENOMEM from oa_tc6_allocate_rx_skb(), leaves rx_skb NULL
so the next mid-frame chunk hits the !tc6->rx_skb branch and sets the
flag anyway.
[ ... ]
I agree with all these comments. But as I added in the comment, there is no single logic
that'll work perfectly. When buffer overflow hits, we don't know which data chunks are good or bad,
which among the total received are going to form the full frame. So, we may release one or two frames
which we don't need to. Improvement here is that we try to recover as much as possible compared
to dropping all data chunks once we hit buffer overflow.
Current logic would recover more frames when the frame size is smaller that
would require fewer chunks.
[Severity: Low]
[Finding: main-3-7]
[Sources: opus-5]
Does this comment match the resulting control flow? The while (true) loop
does proceed to the next iteration on -EAGAIN.
Yes. Comment not matching. Because it talks about the code that was removed.
Removing the comment.
What changed is that the continue was removed, so the tail of the loop
body is now reached on the recoverable-error path:
if (!tc6->waiting_tx_skb && netif_queue_stopped(tc6->netdev))
netif_wake_queue(tc6->netdev);
That code wakes the tx queue, which lets the stack queue new skbs, rather
than draining waiting_tx_skb. Could the comment be reworded to describe
the wake-queue check being reached?
--
This is an AI-generated review.