[PATCH] bcmgenet: fix DMA buffer management

[PATCH] bcmgenet: fix DMA buffer management

[etienne.duble at gmail.com]

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).
---
 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;
 	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);
 
-- 
2.17.1

[PATCH] bcmgenet: fix DMA buffer management

[Jason Wessel]

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

[PATCH] bcmgenet: fix DMA buffer management

[Jason Wessel]

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. 

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

[PATCH] bcmgenet: fix DMA buffer management

[Jason Wessel]

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

[PATCH] bcmgenet: fix DMA buffer management

[Etienne Dublé]

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. 
Cheers 
Etienne 



De: "Jason Wessel" <jason.wessel@windriver.com> 
?: "Etienne DUBLE" <etienne.duble@gmail.com>, "joe hershberger" <joe.hershberger@ni.com> 
Cc: u-boot at lists.denx.de, "ETIENNE DUBLE" <etienne.duble@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@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); 
>> 
>> 

[PATCH] bcmgenet: fix DMA buffer management

[Petr Tesarik]

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 at windriver.com/
- https://patchwork.ozlabs.org/project/uboot/patch/20200717133200.136257-2-jason.wessel at 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@suse.com>

Cheers,
Petr T

> Cheers 
> Etienne 
> 
> 
> 
> De: "Jason Wessel" <jason.wessel@windriver.com> 
> ?: "Etienne DUBLE" <etienne.duble@gmail.com>, "joe hershberger" <joe.hershberger@ni.com> 
> Cc: u-boot at lists.denx.de, "ETIENNE DUBLE" <etienne.duble@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@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>