Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

[Gregory CLEMENT <gclement00@gmail.com>]

Haavard Skinnemoen a écrit :
> On Thu, 13 Dec 2007 08:51:57 +0100
> Gregory CLEMENT <novexz@gmail.com> wrote:
> 
>> Hi,
>> I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 
>> with iperf.
>>
>> From: Gregory CLEMENT <gclement00@gmail.com>
>> Date: Wed, 12 Dec 2007 18:10:14 +0100
>> Subject: [PATCH] MACB: clear transmit buffers properly on TX Underrun
>>
>> Initially transmit buffer pointers were only reset. But buffer descriptors
>> were possibly still set as ready, and buffer in upper layer was not
>> freed. This caused driver hang under big load.
>> Now reset clean properly the buffer descriptor and freed upper layer.
> 
> Nice. I think we want this for 2.6.24.
> 
> But the patch is a bit mangled, so I don't think it will apply. Please
> have a look in Documentation/email-clients.txt for information on how
> to set up Thunderbird to avoid this.

I read it and apply it. Hope it will be OK as I didn't see any problem in m first mail...

> 
>> Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
>> ---
>>  drivers/net/macb.c |   26 +++++++++++++++++++++++++-
>>  1 files changed, 25 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>> index 047ea7b..2ee1dab 100644
>> --- a/drivers/net/macb.c
>> +++ b/drivers/net/macb.c
>> @@ -307,9 +307,33 @@ static void macb_tx(struct macb *bp)
>>          (unsigned long)status);
>>  
>>      if (status & MACB_BIT(UND)) {
>> +        int i;
>>          printk(KERN_ERR "%s: TX underrun, resetting buffers\n",
>> -               bp->dev->name);
>> +               bp->dev->name);
>> +
>> +        head = bp->tx_head;
>> +
>> +        /* free transmit buffer in upper layer*/
>> +        for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
>> +            struct ring_info *rp = &bp->tx_skb[tail];
>> +            struct sk_buff *skb = rp->skb;
>> +
>> +            BUG_ON(skb == NULL);
>> +
>> +            rmb();
>> +
>> +            dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
>> +                             DMA_TO_DEVICE);
>> +            rp->skb = NULL;
>> +            dev_kfree_skb_irq(skb);
>> +        }
>> +
>> +        /*Mark all the buffer as used to avoid sending a lost buffer*/
>> +        for (i = 0; i < RX_RING_SIZE; i++)
>> +            bp->tx_ring[i].ctrl = MACB_BIT(TX_USED);
> 
> That should be TX_RING_SIZE, shouldn't it?

Yes  I fixed it

> 
> I also think this should be done as part of the previous loop, before
> they are freed. Having free buffers in the ring sounds dangerous, even
> if it's only for a short time.

OK, I moved this loop before the previous loop, as I wanted to be sure that
all buffers are marked.
I did it, but I think we don't really need to do it, as when underrun happen, 
controller stop transmitting, so it won't transmit a free buffer.

> 
>> +
>>          bp->tx_head = bp->tx_tail = 0;
> 
> Now, I wonder if it would be better to just scan the ring descriptors,
> find the one that failed and just move the DMA pointer to the next
> entry. The hardware resets the DMA pointer when an underrun happens, so
> I think your code will work, but we're losing more packets than
> strictly necessary. In any case, it's better than the existing code.

As hardware reset the DMA pointer, we have to rewrite all the buffer descriptor.

For example if descriptor n°78 failed, and there is descriptor 79 and 80 which
are pending. When underrun happen on 78 the hardware reset DMA pointer which
will point on descriptor 0. So we have to copy descriptor 79 on 0 and 80 on 1.
The other option is to change Transmit Buffer Queue Pointer Register (TBQP), to
make it point on descriptor 79, but on the next descriptor having wrapped bit set,
the DMA pointer will wrapped on descriptor 79 and not on 0. So I don't think it
is a good solution.

> Perhaps we need a check in macb_start_xmit() as well to avoid starting
> a transmission when the ring has just been reset.
> 

I joined the patch change along your recommendation, if there is no more error, you
can maybe submit it for 2.6.24 release, and other improvement can be done later.


From: Gregory CLEMENT <gclement00@gmail.com>
Date: Sun, 16 Dec 2007 11:45:03 +0100
Subject: [PATCH]  MACB: clear transmit buffers properly on transmit underrun

Initially transmit buffer pointers were only reset. But buffer descriptors
were possibly still set as ready, and buffer in upper layer was not
freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.

Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
---
 drivers/net/macb.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 047ea7b..e898713 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -307,9 +307,34 @@ static void macb_tx(struct macb *bp)
 		(unsigned long)status);
 
 	if (status & MACB_BIT(UND)) {
+		int i;
 		printk(KERN_ERR "%s: TX underrun, resetting buffers\n",
-		       bp->dev->name);
+			   bp->dev->name);
+
+		head = bp->tx_head;
+
+		/*Mark all the buffer as used to avoid sending a lost buffer*/
+		for (i = 0; i < TX_RING_SIZE; i++)
+			bp->tx_ring[i].ctrl = MACB_BIT(TX_USED);
+
+		/* free transmit buffer in upper layer*/
+		for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
+			struct ring_info *rp = &bp->tx_skb[tail];
+			struct sk_buff *skb = rp->skb;
+
+			BUG_ON(skb == NULL);
+
+			rmb();
+
+			dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
+							 DMA_TO_DEVICE);
+			rp->skb = NULL;
+			dev_kfree_skb_irq(skb);
+		}
+
+
 		bp->tx_head = bp->tx_tail = 0;
+
 	}
 
 	if (!(status & MACB_BIT(COMP)))
-- 
1.5.2.5