[PATCH] bcmgenet: fix DMA buffer management
Petr Tesarik
ptesarik at suse.cz
Fri Aug 28 18:32:23 CEST 2020
On Mon, 20 Jul 2020 09:29:31 +0200 (CEST)
etienne.duble at imag.fr (Etienne Dublé) wrote:
> Hi Jason,
> Thanks for testing and for the additional fix.
> In my case (with dhcp followed by 3 TFTP transfers), the board booted fine 20 times in a row with only my fix applied. Previously it was booting once in 10 trials or so.
> The driver resets tx_index in bcmgenet_gmac_eth_send() if it is higher than 256, and the same occurs for rx_index in bcmgenet_gmac_free_pkt(), so I suppose the issue you faced only occurs when the boundary case is reached between two commands?
> Anyway, thanks for the update.
FWIW I also have an update. My Raspberry Pi 4 was consistently failing
to boot from the network. Network packet capture was full of truncated,
duplicate and otherwise abnormal packets. Most interestingly, many
truncated packets had the same length as a packet transmitted exactly
256 frames before.
Matthias has rebuilt U-Boot with these two patches added:
- https://patchwork.ozlabs.org/project/uboot/patch/20200717133200.136257-1-jason.wessel@windriver.com/
- https://patchwork.ozlabs.org/project/uboot/patch/20200717133200.136257-2-jason.wessel@windriver.com/
With these changes, the boot process is rock-solid, and I'm not seeing
any abnormal packets in the capture any longer.
Therefore let me add my
Tested-by: Petr Tesarik <ptesarik at suse.com>
Cheers,
Petr T
> Cheers
> Etienne
>
>
>
> De: "Jason Wessel" <jason.wessel at windriver.com>
> ?: "Etienne DUBLE" <etienne.duble at gmail.com>, "joe hershberger" <joe.hershberger at ni.com>
> Cc: u-boot at lists.denx.de, "ETIENNE DUBLE" <etienne.duble at imag.fr>
> Envoy?: Jeudi 16 Juillet 2020 18:02:11
> Objet: Re: [PATCH] bcmgenet: fix DMA buffer management
>
> On 7/16/20 7:02 AM, Jason Wessel wrote:
> > On 7/9/20 3:11 AM, etienne.duble at gmail.com wrote:
> >> From: Etienne Dubl? <etienne.duble at imag.fr>
> >>
> >> This commit fixes a serious issue occuring when several network
> >> commands are run on a raspberry pi 4 board: for instance a "dhcp"
> >> command and then one or several "tftp" commands. In this case,
> >> packet recv callbacks were called several times on the same packets,
> >> and send function was failing most of the time.
> >>
> >> note: if the boot procedure is made of a single network
> >> command, the issue is not visible.
> >>
> >> The issue is related to management of the packet ring buffers
> >> (producer / consumer) and DMA.
> >> Each time a packet is received, the ethernet device stores it
> >> in the buffer and increments an index called RDMA_PROD_INDEX.
> >> Each time the driver outputs a received packet, it increments
> >> another index called RDMA_CONS_INDEX.
> >>
> >> Between each pair of network commands, as part of the driver
> >> 'start' function, previous code tried to reset both RDMA_CONS_INDEX
> >> and RDMA_PROD_INDEX to 0. But RDMA_PROD_INDEX cannot be written from
> >> driver side, thus its value was actually not updated, and only
> >> RDMA_CONS_INDEX was reset to 0. This was resulting in a major
> >> synchronization issue between the driver and the device. Most
> >> visible bahavior was that the driver seemed to receive again the
> >> packets from the previous commands (e.g. DHCP response packets
> >> "received" again when performing the first TFTP command).
> >>
> >> This fix consists in setting RDMA_CONS_INDEX to the same
> >> value as RDMA_PROD_INDEX, when resetting the driver.
> >>
> >> The same kind of fix was needed on the TX side, and a few variables
> >> had to be reset accordingly (c_index, tx_index, rx_index).
> >
> >
> > While there is some kind of problem with the driver, because I too
> > have observed a problem with multiple requests timing out or failing,
> > this patch makes the problem much worse. I was only able to complete
> > a single tftp request.
> >
> > In my case I am using a static IP address and serverip.
> >
> > Also your patch was missing the sign-off line. Please consider
> > running your patches through scripts/checkpatch.pl.
> >
> > Cheers,
> > Jason.
> >
> >> ---
> >> drivers/net/bcmgenet.c | 15 +++++++--------
> >> 1 file changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c
> >> index 11b6148ab6..a4facfd63f 100644
> >> --- a/drivers/net/bcmgenet.c
> >> +++ b/drivers/net/bcmgenet.c
> >> @@ -378,8 +378,6 @@ static void rx_descs_init(struct bcmgenet_eth_priv *priv)
> >> u32 len_stat, i;
> >> void *desc_base = priv->rx_desc_base;
> >>
> >> - priv->c_index = 0;
> >> -
> >> len_stat = (RX_BUF_LENGTH << DMA_BUFLENGTH_SHIFT) | DMA_OWN;
> >>
> >> for (i = 0; i < RX_DESCS; i++) {
> >> @@ -403,8 +401,10 @@ static void rx_ring_init(struct bcmgenet_eth_priv *priv)
> >> writel(RX_DESCS * DMA_DESC_SIZE / 4 - 1,
> >> priv->mac_reg + RDMA_RING_REG_BASE + DMA_END_ADDR);
> >>
> >> - writel(0x0, priv->mac_reg + RDMA_PROD_INDEX);
> >> - writel(0x0, priv->mac_reg + RDMA_CONS_INDEX);
> >> + /* cannot init RDMA_PROD_INDEX to 0, so align RDMA_CONS_INDEX on it instead */
> >> + priv->c_index = readl(priv->mac_reg + RDMA_PROD_INDEX);
> >> + writel(priv->c_index, priv->mac_reg + RDMA_CONS_INDEX);
> >> + priv->rx_index = priv->c_index;
>
>
> printf("before RX_IDX: 0x%x\n", priv->rx_index);
>
> I added a printf() like above for the RX and TX to see what is going on when
> I try and transfer a kernel Image file the second time.
>
>
> U-Boot> tftp ${loadaddr} bootfs/Image
> before RX_IDX: 0x0
> before TX_IDX: 0x0
> Using ethernet at 7d580000 device
> Filename 'bootfs/Image'.
> Load address: 0x80000
> Loading: ## Warning: gatewayip needed but not set
> ################################################## 16.8 MiB
> 6.1 MiB/s
> done
> Bytes transferred = 17615360 (10cca00 hex)
> U-Boot> tftp ${loadaddr} bootfs/Image
> before RX_IDX: 0xe4
> before TX_IDX: 0x2ee3
> Using ethernet at 7d580000 device
> Filename 'bootfs/Image'.
> Load address: 0x80000
> Loading: ## Warning: gatewayip needed but not set
>
>
>
> The TX_IDX is now 0x2ee3 which is definitely not going to work.
>
> According to the driver file there are only 256 (0xFF) slots,
> which is why it hangs, with your change.
>
> Jason.
>
> >> writel((RX_DESCS << DMA_RING_SIZE_SHIFT) | RX_BUF_LENGTH,
> >> priv->mac_reg + RDMA_RING_REG_BASE + DMA_RING_BUF_SIZE);
> >> writel(DMA_FC_THRESH_VALUE, priv->mac_reg + RDMA_XON_XOFF_THRESH);
> >> @@ -421,8 +421,9 @@ static void tx_ring_init(struct bcmgenet_eth_priv *priv)
> >> writel(0x0, priv->mac_reg + TDMA_WRITE_PTR);
> >> writel(TX_DESCS * DMA_DESC_SIZE / 4 - 1,
> >> priv->mac_reg + TDMA_RING_REG_BASE + DMA_END_ADDR);
> >> - writel(0x0, priv->mac_reg + TDMA_PROD_INDEX);
> >> - writel(0x0, priv->mac_reg + TDMA_CONS_INDEX);
> >> + /* cannot init TDMA_CONS_INDEX to 0, so align TDMA_PROD_INDEX on it instead */
> >> + priv->tx_index = readl(priv->mac_reg + TDMA_CONS_INDEX);
> >> + writel(priv->tx_index, priv->mac_reg + TDMA_PROD_INDEX);
> >> writel(0x1, priv->mac_reg + TDMA_RING_REG_BASE + DMA_MBUF_DONE_THRESH);
> >> writel(0x0, priv->mac_reg + TDMA_FLOW_PERIOD);
> >> writel((TX_DESCS << DMA_RING_SIZE_SHIFT) | RX_BUF_LENGTH,
> >> @@ -469,8 +470,6 @@ static int bcmgenet_gmac_eth_start(struct udevice *dev)
> >>
> >> priv->tx_desc_base = priv->mac_reg + GENET_TX_OFF;
> >> priv->rx_desc_base = priv->mac_reg + GENET_RX_OFF;
> >> - priv->tx_index = 0x0;
> >> - priv->rx_index = 0x0;
> >>
> >> bcmgenet_umac_reset(priv);
> >>
> >>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: Digit��ln�� podpis OpenPGP
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200828/4eb61568/attachment.sig>
More information about the U-Boot
mailing list