RE: [PATCH net v3 2/3] net: ethernet: oa_tc6: Improvement in buffer overflow handling

[Selvamani Rajagopal]

> -----Original Message-----
> From: Simon Horman <horms@xxxxxxxxxx>
> Sent: Friday, July 10, 2026 7:20 AM
> To: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>
> Cc: Simon Horman <horms@xxxxxxxxxx>; parthiban.veerasooran@xxxxxxxxxxxxx;

> 
> [High]
> Now that oa_tc6_prcs_rx_frame_end() returns int (and can return -EAGAIN
> via oa_tc6_update_rx_skb() when tail + size > end), should the
> "previous rx frame end + next rx frame start" branch also observe the
> return value? That branch is not shown in the diff, but in the
> resulting file it reads:


No need to check the return value. My comment below gives the reason.

> 
> If oa_tc6_prcs_rx_frame_end() here returns -EAGAIN, oa_tc6_look_for_new_frame()
> has already set tc6->rx_buf_overflow = true and cleared rx_skb. Execution
> then falls through to oa_tc6_prcs_rx_frame_start(), which allocates a
> fresh rx_skb and copies the start bytes, but tc6->rx_buf_overflow stays
> true because the guard

Yes. This is an issue. I will clear the rx_buf_overflow to false as the condition
for clearing is to see a start_valid bit (a.k.a new frame). And we have a new frame 
to process at this stage.


> 
> [Medium]
> Is it intentional that a real -ENOMEM from oa_tc6_prcs_rx_chunk_payload()
> (originating in oa_tc6_allocate_rx_skb() -> netdev_alloc_skb_ip_align())
> is now folded into the retry path and converted to -EAGAIN at the end
> of this function?


Yes. It is intentional. Code changes attempt to recover from either ENOMEM or 
EAGAIN. Recovery actions on both errors, are same. I will add some more comment 
to make it clear.


> 
> Before this patch, -ENOMEM propagated up through oa_tc6_try_spi_transfer()
> and produced a netdev_err("Device error: %d", ret) plus
> oa_tc6_free_ongoing_skbs() cleanup. With this change, the outer while
> loop treats -EAGAIN as non-fatal, so sustained memory exhaustion is
> reported only as increments to stats.rx_dropped from
> oa_tc6_allocate_rx_skb().
> 
> The commit message describes the change as being about buffer overflow
> errors; is the change in visibility for allocation failures intended?


Maybe I will change the commit message as "improvement in error recovery"


> > }
> 
> [High]
> Can this unconditional oa_tc6_look_for_new_frame() discard a frame that
> was successfully started later in the same buffer?
> 
> guard in oa_tc6_prcs_rx_chunk_payload() until yet another start_valid
> arrives. Would it be safer to only call oa_tc6_look_for_new_frame()
> here if the last iteration was still in an errored state, e.g. by
> tracking the outcome of the most recent chunk rather than an aggregate
> retry flag?

I don't think there is no "best method" to solve this issue. But I am open to 
suggestions. In my view, we may have many data chunks with buffer overflow 
set, not just one or two. It is hard to tell whether every chunk with overflow error 
has good (or bad) data. 

Code treats the data chunk as good if there is no "bad header" bit set, and "data valid"
bit is set, which I think is fair.

Here are the choices I see

1) Original code: Discarded all the received chunks once we see overflow error.
2) Current change: Let the loop recover as many frames as possible from
    the data chunks received. At the end of the loop, we trash what is left. Yes. It is 
    possible that we may trash a last one that may be a good one.
3) What Sashiko says: Ignore chunk errors in the middle or beginning of the loop.
    Just worry about the recent chunk with error. 

I don't think we can accurately tell whether 2 is better or 3. It may entirely depend on
how many chunks are set with overflow error it at any given time. 

I can change the Implementation if someone thinks #3 is better.

> 
> [ ... ]