[PATCH] bcmgenet: fix DMA buffer management
Jason Wessel
jason.wessel at windriver.com
Fri Jul 17 04:45:12 CEST 2020
On 7/16/20 11:02 AM, Jason Wessel wrote:
>
>
> 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.
>
diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c
index a4facfd63f..1b7e7ba2bf 100644
--- a/drivers/net/bcmgenet.c
+++ b/drivers/net/bcmgenet.c
@@ -405,6 +405,7 @@ static void rx_ring_init(struct bcmgenet_eth_priv *priv)
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;
+ priv->rx_index &= 0xFF;
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);
@@ -424,6 +425,7 @@ static void tx_ring_init(struct bcmgenet_eth_priv *priv)
/* 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);
+ priv->tx_index &= 0xFF;
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,
After some testing it turns the lower 8 bits of the indexes will always match up with the DMA buffer index.
If you also apply the patch above the ethernet becomes reliable.
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);
>>>
>>>
More information about the U-Boot
mailing list