RE: [PATCH net v5 2/4] net: ethernet: oa_tc6: Improve the error recovery

[Selvamani Rajagopal]

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Thursday, August 6, 2026 3:43 AM
> Author: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>
> 
> 
> 
> 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.