[PATCH] net: ravb: Fix RX error handling

[Paul Barker]

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.

Thanks,

-- 
Paul Barker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x27F4B3459F002257.asc
Type: application/pgp-keys
Size: 3520 bytes
Desc: OpenPGP public key
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250415/df526f73/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250415/df526f73/attachment.sig>