| Message ID | f980100c26995da542aff85bf89788e19aceb7b6.1727968902.git.jerome.forissier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce the lwIP network stack | expand |
On Thu, 3 Oct 2024 at 09:23, Jerome Forissier <jerome.forissier@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@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. > > [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@linaro.org> > --- > drivers/net/eepro100.c | 2 +- > drivers/net/rtl8139.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> > 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 >
On 10/9/24 03:51, Simon Glass wrote: > On Thu, 3 Oct 2024 at 09:23, Jerome Forissier > <jerome.forissier@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@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@linaro.org> >> --- >> drivers/net/eepro100.c | 2 +- >> drivers/net/rtl8139.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks,
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. */
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@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. [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@linaro.org> --- drivers/net/eepro100.c | 2 +- drivers/net/rtl8139.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)