AnsweredAssumed Answered

BUG in drivers/net/bfin_mac.c

Question asked by Toast on Jun 19, 2013
Latest reply on Jul 4, 2013 by Aaronwu

Hi!

 

I suppose I have found a bug in distro 2009R1-RC6s bfin_mac.c driver, Kernel is 2.6.28-10-AD-2009R1. I looked at newer kernels, it seems noone has corrected or recognized it yet. Our board is a custom BF537.

 

We have found the problem with a module which calls "dev->hard_start_xmit(skb_buff)" or "dev_queue_xmit()" out of a tasklet a few dozen times every 20ms:

 

...

while(cnt--)

   if (dev->hard_start_xmit(cs, dev))

        printk("fast packet %i failed\r\n",);

 

No error happens, but once in a while a packet on the net is lost, ~1 per 1000, significantly distributed like paket #0/1 no loss, packet #3 most losses, packet #4/5/6... decreasing losses.

 

The rx/tx descriptor queue is configurated as rx: 90, tx:40 and therefore located in (not cached) SD-RAM.

 

I found that if the macs tx-dma is already running and a new skb_buff is placed in the descriptor-chain, it may be "overtaken" by dma before the DMAEN is set (and purged into SD-RAM) in the descriptors dma-config. This leads to a orphaned descriptor which will be just freed by adjust_tx_list, so this packet is lost.

Additionly DMA may have finished after writing the descriptor and before testing DMA_RUN, so this packet would be sent twice then (even less often).

 

My fixes:

 

--- linux-2.6.x/drivers/net/bfin_mac.c  2013-06-19 11:33:56.000000000 +0200

+++ linux-2.6.x/drivers/net/bfin_mac.c  2013-06-19 11:36:30.000000000 +0200

@@ -604,11 +604,19 @@

        if (current_tx_ptr->next->next == tx_list_head) {

                while (tx_list_head->status.status_word == 0) {

                        udelay(10);

-                       if (tx_list_head->status.status_word != 0

-                           || !(bfin_read_DMA2_IRQ_STATUS() & DMA_RUN)) {

+                       if (tx_list_head->status.status_word != 0)

+                       {

+                               goto adjust_head;

+                       }

+                       if(!(bfin_read_DMA2_IRQ_STATUS() & DMA_RUN))

+                       {

+                               printk(KERN_ERR DRV_NAME

+                                                               ": no status on tx list head and dma finished. Packet lost.\n");

+

                                goto adjust_head;

                        }

                        if (timeout_cnt-- < 0) {

                                printk(KERN_ERR DRV_NAME

                                ": wait for adjust tx list head timeout\n");

                                break;

@@ -633,6 +641,7 @@

                               ": no sk_buff in a transmitted frame!\n");

                }

                tx_list_head = tx_list_head->next;

+               SSYNC();

        } while (tx_list_head->status.status_word != 0

                 && current_tx_ptr != tx_list_head);

        return;

@@ -676,10 +685,33 @@

        /* enable this packet's dma */

        current_tx_ptr->desc_a.config |= DMAEN;

 

-       /* tx dma is running, just return */

+       /* ssync again, dma-config must be written before DMA_RUN test.

+        * Currently running DMA may otherwise recognize this header as final

+        * -> lost packet in adjust_tx_list

+        * */

+       SSYNC();        /* MARK */

+

+       /* tx dma is STILL running, so current_tx_ptr will be done for sure, just return */

        if (bfin_read_DMA2_IRQ_STATUS() & DMA_RUN)

                goto out;

 

+       /* DMA has finished

+        * ssync so we can read current_tx_ptr status safely

+        * */

+       SSYNC();

+

+       /* DMA might have been running during SSYNC() at "MARK", so current_tx_ptr might have been done already

+        */

+

+       if(current_tx_ptr->status.status_word != 0)

+       {

+               /*

+                * DMA was running and already did "current_tx_ptr"

+                * -> would lead to duplicate packet, so don't start it again

+                */

+               goto out;

+       }

+                                

Outcomes