[PATCH] bcmgenet: fix DMA buffer management

[Jason Wessel <jason.wessel@windriver.com>]

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@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);
>>>  
>>>