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+rene...@mailbox.org>
> ---
> Cc: Joe Hershberger <joe.hershber...@ni.com>
> Cc: Nobuhiro Iwamatsu <iwama...@nigauri.org>
> Cc: Paul Barker <paul.barker...@bp.renesas.com>
> Cc: Ramon Fried <rfried....@gmail.com>
> Cc: Tom Rini <tr...@konsulko.com>
> Cc: u-boot@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
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
