[PATCH v11 01/29] net: recv(): return -EAGAIN instead of 0 when no cleanup is expected

[Jerome Forissier]

On 10/9/24 03:51, Simon Glass wrote:
> On Thu, 3 Oct 2024 at 09:23, Jerome Forissier
> <jerome.forissier at linaro.org> wrote:
>>
>> Note: patch posted separately [0].
>>
>> [0] http://patchwork.ozlabs.org/project/uboot/patch/20240927142038.879037-1-jerome.forissier@linaro.org/
>>
>> Some drivers do not behave properly when free_pkt() is called with a
>> length of zero. It is an issue I observed when developing the lwIP
>> series [1] (see "QEMU CI tests for r2dplus_i82557c, r2dplus_rtl8139"
>> in the change log) and which I fixed incorrectly by not calling
>> free_pkt() when recv() returns 0. That turned out to be wrong for two
>> reasons:
>>
>> 1. The DM documentation [2] clearly requires it:
>>
>>   "The **recv** function polls for availability of a new packet. [...]
>>    If there is an error [...], return 0 if you require the packet to
>>    be cleaned up normally, or a negative error code otherwise (cleanup
>>    not necessary or already done).
>>
>>    If **free_pkt** is defined, U-Boot will call it after a received
>>    packet has been processed [...]. free_pkt() will be called after
>>    recv(), for the same packet [...]"
>>
>> 2. The imx8mp_evk platform will fail with OOM errors if free_pkt() is
>>    not called after recv() returns 0:
>>
>>    u-boot=> tftp 192.168.0.16:50M
>>    Using ethernet at 30be0000 device
>>    TFTP from server 192.168.0.16; our IP address is 192.168.0.48
>>    Filename '50M'.
>>    Load address: 0x40480000
>>    Loading: #######################fecmxc_recv: error allocating packetp
>>    fecmxc_recv: error allocating packetp
>>    fecmxc_recv: error allocating packetp
>>    ...
>>
>> Therefore, make recv() return -EINVAL instead of 0 when no packet is
>> available and the driver doesn't expect free_pkt() to be called
>> subsequently.
> 
> Do you mean -EAGAIN ? Otherwise, it seems like this comment relates to
> a different patch.

Yes, good catch. I will fix the description and resend patch [1] as v2.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20240927142038.879037-1-jerome.forissier@linaro.org/

> 
>>
>> [1] https://lists.denx.de/pipermail/u-boot/2024-August/562861.html
>> [2] doc/develop/driver-model/ethernet.rst
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier at linaro.org>
>> ---
>>  drivers/net/eepro100.c | 2 +-
>>  drivers/net/rtl8139.c  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>

Thanks,
-- 
Jerome

> 
>> diff --git a/drivers/net/eepro100.c b/drivers/net/eepro100.c
>> index d18a8d577ca..f64dbb7d6a1 100644
>> --- a/drivers/net/eepro100.c
>> +++ b/drivers/net/eepro100.c
>> @@ -678,7 +678,7 @@ static int eepro100_recv_common(struct eepro100_priv *priv, uchar **packetp)
>>         status = le16_to_cpu(desc->status);
>>
>>         if (!(status & RFD_STATUS_C))
>> -               return 0;
>> +               return -EAGAIN;
>>
>>         /* Valid frame status. */
>>         if (status & RFD_STATUS_OK) {
>> diff --git a/drivers/net/rtl8139.c b/drivers/net/rtl8139.c
>> index 2e0afad089f..5f4b1e2d3a0 100644
>> --- a/drivers/net/rtl8139.c
>> +++ b/drivers/net/rtl8139.c
>> @@ -433,7 +433,7 @@ static int rtl8139_recv_common(struct rtl8139_priv *priv, unsigned char *rxdata,
>>         int length = 0;
>>
>>         if (inb(priv->ioaddr + RTL_REG_CHIPCMD) & RTL_REG_CHIPCMD_RXBUFEMPTY)
>> -               return 0;
>> +               return -EAGAIN;
>>
>>         priv->rxstatus = inw(priv->ioaddr + RTL_REG_INTRSTATUS);
>>         /* See below for the rest of the interrupt acknowledges.  */
>> --
>> 2.40.1
>>