[PATCH net 0/2] net: bcmgenet: TX reclaim and DMA fixes

[PATCH net 0/2] net: bcmgenet: TX reclaim and DMA fixes

[Florian Fainelli]

Hi David,

This patch set contains one fix for an accounting problem while reclaiming
transmitted buffers having fragments, and the second fix is to make sure
that the DMA shutdown is properly controlled.

Florian Fainelli (2):
  net: bcmgenet: fix TX reclaim accounting for fragments
  net: bcmgenet: call bcmgenet_dma_teardown in bcmgenet_fini_dma

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH net 1/2] net: bcmgenet: fix TX reclaim accounting for fragments

[Florian Fainelli]

The GENET driver supports SKB fragments, and succeeds in transmitting
them properly, but when reclaiming these transmitted fragments, we will
only update the count of free buffer descriptors by 1, even for SKBs
with fragments. This leads to the networking stack thinking it has more
room than the hardware has when pushing new SKBs, and backing off
consequently because we return NETDEV_TX_BUSY.

Fix this by accounting for the SKB nr_frags plus one (itself) and update
ring->free_bds accordingly with that value for each iteration loop in
__bcmgenet_tx_reclaim().

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cdef86a03862..11a96437862d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -875,6 +875,7 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
 	int last_tx_cn, last_c_index, num_tx_bds;
 	struct enet_cb *tx_cb_ptr;
 	struct netdev_queue *txq;
+	unsigned int bds_compl;
 	unsigned int c_index;
 
 	/* Compute how many buffers are transmitted since last xmit call */
@@ -899,7 +900,9 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
 	/* Reclaim transmitted buffers */
 	while (last_tx_cn-- > 0) {
 		tx_cb_ptr = ring->cbs + last_c_index;
+		bds_compl = 0;
 		if (tx_cb_ptr->skb) {
+			bds_compl = skb_shinfo(tx_cb_ptr->skb)->nr_frags + 1;
 			dev->stats.tx_bytes += tx_cb_ptr->skb->len;
 			dma_unmap_single(&dev->dev,
 					 dma_unmap_addr(tx_cb_ptr, dma_addr),
@@ -916,7 +919,7 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
 			dma_unmap_addr_set(tx_cb_ptr, dma_addr, 0);
 		}
 		dev->stats.tx_packets++;
-		ring->free_bds += 1;
+		ring->free_bds += bds_compl;
 
 		last_c_index++;
 		last_c_index &= (num_tx_bds - 1);
-- 
1.9.1

[PATCH net 2/2] net: bcmgenet: call bcmgenet_dma_teardown in bcmgenet_fini_dma

[Florian Fainelli]

We should not be manipulaging the DMA_CTRL registers directly by writing
0 to them to disable DMA. This is an operation that needs to be timed to
make sure the DMA engines have been properly stopped since their state
machine stops on a packet boundary, not immediately.

Make sure that tha bcmgenet_fini_dma() calls bcmgenet_dma_teardown() to
ensure a proper DMA engine state.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 11a96437862d..5d9eac37993d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1749,8 +1749,7 @@ static void bcmgenet_fini_dma(struct bcmgenet_priv *priv)
 	int i;
 
 	/* disable DMA */
-	bcmgenet_rdma_writel(priv, 0, DMA_CTRL);
-	bcmgenet_tdma_writel(priv, 0, DMA_CTRL);
+	bcmgenet_dma_teardown(priv);
 
 	for (i = 0; i < priv->num_tx_bds; i++) {
 		if (priv->tx_cbs[i].skb != NULL) {
-- 
1.9.1

Re: [PATCH net 0/2] net: bcmgenet: TX reclaim and DMA fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 18 Sep 2014 17:48:15 -0700

> This patch set contains one fix for an accounting problem while reclaiming
> transmitted buffers having fragments, and the second fix is to make sure
> that the DMA shutdown is properly controlled.

Florian I am seriesly irritated, are you even build testing these
changes?

bcmgenet_dma_teardown() is a static function declared far after the
new call you are adding in patch #2, so now we get:

drivers/net/ethernet/broadcom/genet/bcmgenet.c: In function ‘bcmgenet_fini_dma’:
drivers/net/ethernet/broadcom/genet/bcmgenet.c:1752:2: error: implicit declaration of function ‘bcmgenet_dma_teardown’ [-Werror=implicit-function-declaration]
drivers/net/ethernet/broadcom/genet/bcmgenet.c: At top level:
drivers/net/ethernet/broadcom/genet/bcmgenet.c:2111:12: error: static declaration of ‘bcmgenet_dma_teardown’ follows non-static declaration
drivers/net/ethernet/broadcom/genet/bcmgenet.c:1752:2: note: previous implicit declaration of ‘bcmgenet_dma_teardown’ was here

I want to make it clear to you that whatever time you think you're
saving by skipping even the most basic compilation test, goes directly
to _ME_ and you are therefore having a negative impact on every single
developer who is also waiting for me to review and integrate their
networking changes.

Please do not do this any more.

Thanks.

Re: [PATCH net 0/2] net: bcmgenet: TX reclaim and DMA fixes

[Florian Fainelli]

On 09/22/2014 11:46 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Thu, 18 Sep 2014 17:48:15 -0700
> 
>> This patch set contains one fix for an accounting problem while reclaiming
>> transmitted buffers having fragments, and the second fix is to make sure
>> that the DMA shutdown is properly controlled.
> 
> Florian I am seriesly irritated, are you even build testing these
> changes?
> 
> bcmgenet_dma_teardown() is a static function declared far after the
> new call you are adding in patch #2, so now we get:
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c: In function ‘bcmgenet_fini_dma’:
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1752:2: error: implicit declaration of function ‘bcmgenet_dma_teardown’ [-Werror=implicit-function-declaration]
> drivers/net/ethernet/broadcom/genet/bcmgenet.c: At top level:
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:2111:12: error: static declaration of ‘bcmgenet_dma_teardown’ follows non-static declaration
> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1752:2: note: previous implicit declaration of ‘bcmgenet_dma_teardown’ was here
> 
> I want to make it clear to you that whatever time you think you're
> saving by skipping even the most basic compilation test, goes directly
> to _ME_ and you are therefore having a negative impact on every single
> developer who is also waiting for me to review and integrate their
> networking changes.
> 
> Please do not do this any more.

Very sorry about that, v2 is on your way, I will be more careful.
--
Florian