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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [2]: [PATCH net v5 2/4] net: ethernet: oa_tc6: Improve the error recovery ---
commit f8f47e53b81fcb2cf25c3e00de04be99437717cf
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Improve the error recovery

This patch adds a tailroom check before skb_put() in the oa_tc6 receive
path, turns the rx skb update helpers into functions returning an error
code, and makes oa_tc6_process_spi_data_rx_buf() continue over
recoverable (-EAGAIN/-ENOMEM) errors instead of bailing out on the first
one.  A new helper oa_tc6_look_for_new_frame() bundles setting
rx_buf_overflow with dropping the in-progress rx skb.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 5b24cce4f9b56..421f72e6f71e3 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -710,6 +710,12 @@ static void oa_tc6_free_pending_skbs(struct oa_tc6 *tc6)
>  	oa_tc6_cleanup_waiting_tx_skb(tc6);
>  }
>  
> +static void oa_tc6_look_for_new_frame(struct oa_tc6 *tc6)
> +{
> +	tc6->rx_buf_overflow = true;
> +	oa_tc6_cleanup_ongoing_rx_skb(tc6);
> +}
> +

[Severity: Low]
[Finding: main-1-2]
[Sources: opus-5]
Should the rx_buf_overflow field keep that name after this change?

The flag is now also set for conditions that are not MAC-PHY receive
buffer overflows: exhausted skb tailroom and rx_skb == NULL in
oa_tc6_update_rx_skb(), plus the blanket per-buffer retry at the end of
oa_tc6_process_spi_data_rx_buf(), which also covers -ENOMEM from
netdev_alloc_skb_ip_align().

Would a rename (something like rx_resync or skip_until_frame_start), or a
second flag, keep the two conditions distinguishable when debugging?

[ ... ]

> @@ -835,13 +844,35 @@ static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
>  	tc6->rx_skb = NULL;
>  }
>  
> -static void oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u8 length)
> +/* On oversubscribed traffic condition, particularly with overwhelming rx
> + * buffer overflow errors, there could be data chunk loss. If tail + length
> + * goes beyond end pointer, that is an indication that the data chunk with
> + * end_valid bit is lost. Time to look for a data chunk with start_valid bit.
> + *
> + * If rx_skb is NULL, it is time to start looking for data chunk with
> + * start_bit.
> + */
> +static int oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u8 length)
>  {
> +	if (!tc6->rx_skb ||
> +	    (tc6->rx_skb->tail + length) > tc6->rx_skb->end) {
> +		oa_tc6_look_for_new_frame(tc6);
> +		return -EAGAIN;
> +	}
> +
>  	memcpy(skb_put(tc6->rx_skb, length), payload, length);
> +	return 0;
>  }

[Severity: Low]
[Finding: main-1-1]
[Sources: opus-5]
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.

[ ... ]

> @@ -946,6 +980,15 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
>  		if (tc6->rx_skb) {
>  			size = end_byte_offset + 1;
>  			oa_tc6_prcs_rx_frame_end(tc6, data, size);
> +
> +			/* Return value from oa_tc6_prcs_rx_frame_end is not
> +			 * checked. If it returned an error, it is to make
> +			 * the code to look for new frame. At this stage,
> +			 * code below is going to process a new frame. So,
> +			 * error condition is set to false, in case it is
> +			 * set before proceeding.
> +			 */
> +			tc6->rx_buf_overflow = false;
>  		}
>  		size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
>  		return oa_tc6_prcs_rx_frame_start(tc6,

[ ... ]

> @@ -972,8 +1013,9 @@ static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
>  static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  {
>  	u16 no_of_rx_chunks = length / OA_TC6_CHUNK_SIZE;
> +	bool retry = false;
> +	int ret = 0;
>  	u32 footer;
> -	int ret;
>  
>  	/* All the rx chunks in the receive SPI data buffer are examined here */
>  	for (int i = 0; i < no_of_rx_chunks; i++) {
> @@ -982,8 +1024,11 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  						    OA_TC6_CHUNK_PAYLOAD_SIZE);
>  
>  		ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			if (ret != -EAGAIN)
> +				return ret;
> +			retry = true;
> +		}

[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?

Every remaining footer in the same buffer that has
OA_TC6_DATA_FOOTER_EXTENDED_STS set re-enters
oa_tc6_process_extended_status(), which does a blocking
oa_tc6_read_register(OA_TC6_REG_STATUS0) plus an
oa_tc6_write_register() write-back, each taking spi_ctrl_lock, even
though the first invocation already latched and cleared the error.

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.

Does that delay the next data transfer that drains the MAC-PHY receive
buffer, and could it aggravate the overflow condition being recovered
from?  The commit message does not mention this side effect.

>  
>  		/* If there is a data valid chunks then process it for the
>  		 * information needed to determine the validity and the location
> @@ -995,12 +1040,35 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  
>  			ret = oa_tc6_prcs_rx_chunk_payload(tc6, payload,
>  							   footer);
> -			if (ret)
> -				return ret;
> +			if (ret) {
> +				if (ret != -ENOMEM && ret != -EAGAIN)
> +					return ret;
> +				retry = true;
> +			}
>  		}
>  	}
>  
> -	return 0;
> +	/* Not bailing out on recoverable error codes, -EAGAIN and
> +	 * -ENOMEM. If subsequent loop iterations, if any, succeeds,
> +	 * error code would be overwritten. retry flag helps to
> +	 * make the caller to continue and retry. Since recovery
> +	 * action for -ENOMEM and -EAGAIN are same, we are returning
> +	 * one of the error codes, that is -EAGAIN.
> +	 *
> +	 * Successful recovery depends on how small the frames are,
> +	 * how many chunks, among the received chunks triggered the
> +	 * error, whether data is intact even with error conditions.
> +	 * As a result, there is no single, best method to recover
> +	 * most data when error conditions hit. We do our best by
> +	 * processing all the chunks with good "footer header" and
> +	 * "data valid" bit set.
> +	 */
> +	if (retry) {
> +		ret = -EAGAIN;
> +		oa_tc6_look_for_new_frame(tc6);
> +	}
> +
> +	return ret;
>  }

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
Can this drop a frame that was already successfully re-synchronized
inside the same buffer?

The retry flag is sticky, so oa_tc6_look_for_new_frame() runs at loop exit
even when a later chunk recovered.  Sequence:

  chunk i:  extended status set, STATUS0 reports rx buffer overflow
            oa_tc6_process_extended_status()
               oa_tc6_look_for_new_frame()   /* rx_skb freed,
                                                rx_buf_overflow = true */
               return -EAGAIN
            loop sets retry = true and keeps going

  chunk j:  start_valid, so oa_tc6_prcs_rx_chunk_payload() does
               if (start_valid && tc6->rx_buf_overflow)
                       tc6->rx_buf_overflow = false;
            oa_tc6_prcs_rx_frame_start() -> oa_tc6_allocate_rx_skb()
            allocates a fresh rx_skb, later "ongoing" chunks append fine

  loop exit: retry is still true
               ret = -EAGAIN;
               oa_tc6_look_for_new_frame(tc6);   /* frees the good
                                                    rx_skb, rx_dropped++,
                                                    rx_buf_overflow = true */

On the next SPI transfer the continuation and end chunks of that frame are
then discarded by:

	if (tc6->rx_buf_overflow)
		return 0;

until another start_valid chunk arrives.  rx_chunks_available is 5 bits
(max 31) while a 1514-byte frame needs roughly 24 chunks of 64 payload
bytes, so a frame still in progress at buffer end is a normal occurrence.

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.

[ ... ]

> @@ -1162,10 +1230,12 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
>  		}
>  
>  		ret = oa_tc6_process_spi_data_rx_buf(tc6, spi_len);
> -		if (ret) {
> -			if (ret == -EAGAIN)
> -				continue;
>  
> +		/* Not continuing with the next iteration to give
> +		 * waiting_tx_skb a chance to get drained, if
> +		 * needed.
> +		 */
> +		if (ret && ret != -EAGAIN) {
>  			oa_tc6_free_ongoing_skbs(tc6);
>  			netdev_err(tc6->netdev, "Device error: %d\n", ret);
>  			return ret;

[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.

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.