[PATCH] net: ravb: Fix RX error handling

[Marek Vasut]

On 4/15/25 12:02 PM, Paul Barker wrote:
> On 13/04/2025 14:11, Marek Vasut wrote:
>> Correctly handle RX errors in ravb_recv() by returning 0 instead
>> of -EAGAIN on RX error.
>>
>> In case the RAVB driver detects an RX error in ravb_recv(), it must
>> not return the -EAGAIN, but instead must return 0. Both error codes
>> are handled in eth-uclass.c eth_rx() and -EAGAIN is rewritten to 0
>> at the end of eth_rx(), but negative return code from the .recv()
>> callback does not trigger .free_pkt() callback, which would clean
>> up and re-enqueue the descriptor which holds the currently received
>> corrupted packet. The .free_pkt() must be called for this descriptor,
>> otherwise the follow up received data become corrupted too, even if
>> those packets are correctly received. Returning 0 from the .recv()
>> callback assures the corrupted packet is not processed by the network
>> stack, but is skipped instead.
>>
>> For TFTP loading, an RX error produces the timeout "T" output and
>> resumes the TFTP loading operation shortly afterward, without any
>> data corruption.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas at mailbox.org>
>> ---
>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>> Cc: Nobuhiro Iwamatsu <iwamatsu at nigauri.org>
>> Cc: Paul Barker <paul.barker.ct at bp.renesas.com>
>> Cc: Ramon Fried <rfried.dev at gmail.com>
>> Cc: Tom Rini <trini at konsulko.com>
>> Cc: u-boot at lists.denx.de
>> ---
>>   drivers/net/ravb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
>> index 539fd37ee59..da3e9dbeacd 100644
>> --- a/drivers/net/ravb.c
>> +++ b/drivers/net/ravb.c
>> @@ -192,7 +192,7 @@ static int ravb_recv(struct udevice *dev, int flags, uchar **packetp)
>>   	/* Check for errors */
>>   	if (desc->data.ctrl & RAVB_RX_DESC_MSC_RX_ERR_MASK) {
>>   		desc->data.ctrl &= ~RAVB_RX_DESC_MSC_MASK;
>> -		return -EAGAIN;
>> +		return 0;
> 
> Hi Marek,
> 
> At this point in the function we haven't stored anything to *packetp.
> 
> Looking at the relevant bit of eth_rx():
> 
>          for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
>                  ret = eth_get_ops(current)->recv(current, flags, &packet);
>                  flags = 0;
>                  if (ret > 0)
>                          net_process_received_packet(packet, ret);
>                  if (ret >= 0 && eth_get_ops(current)->free_pkt)
>                          eth_get_ops(current)->free_pkt(current, packet, ret);
>                  if (ret <= 0)
>                          break;
>          }
> 
> So if ->recv() returns zero without storing to *packetp, we will call
> ->free_pkt() with a packet argument that is either uninitialized or
> points to the packet from the last iteration through the loop.
Fixed in V2, thanks !