[PATCH] b43: use rx desc underrun interrupt

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

Hi,

long user of b43 driver under openwrt. Not sure if it has been posted
previously but using slow and old hardware (as routers running openwrt
most of the time is), the CPU is sometimes slower than the NIC. So
recently I, and several others, has had problem of wifi going down.
See ticket 7552 at openwrt for more info
(https://dev.openwrt.org/ticket/7552)

I have tracked down the problem to the nic getting into a rx
descriptor underrun. Since the driver don't listen to that (or take
care of it in any other way), the Wifi seemingly just dies. The CPU is
waiting for data from the nic and the nic is waiting for a
confirmation that it can keep sending.

I created a patch for handling of that interrupt. I have tested it by
reducing the number of rx slots to 16 and sending a couple of 100GB of
data. Previously Wifi went down within a minute when maxing out the
uplink (so rx for AP), now it has been working flawless for a 1.5week.

BR,
Thommy Jakobsson

Index: drivers/net/wireless/b43/dma.c
===================================================================
--- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.c
2013-04-09 17:33:54.325322046 +0200
+++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.c
2013-04-09 17:34:43.473565843 +0200
@@ -1688,6 +1688,21 @@
         b43_poison_rx_buffer(ring, skb);
         sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
+void b43_dma_rx_discard(struct b43_dmaring *ring)
+{
+   B43_WARN_ON(ring->tx);
+
+    /* Device has filled all buffers, drop all packets in buffers
+     * and let TCP decrease speed.
+     * Set index to one desc after the last one (which is marked)
+     * so the device will see all slots as free again
+     */
+    /*
+     *TODO: How to increase rx_drop in mac80211
+     */
+   b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
+                                     sizeof(struct b43_dmadesc32));
+}

 void b43_dma_rx(struct b43_dmaring *ring)
 {

Index: drivers/net/wireless/b43/dma.h
===================================================================
--- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.h
2013-04-09 17:34:09.561397686 +0200
+++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.h
2013-04-09 17:34:43.461565780 +0200
@@ -10,7 +10,8 @@
 #define B43_DMAIRQ_FATALMASK    ((1 << 10) | (1 << 11) | (1 << 12) \
                                          | (1 << 14) | (1 << 15))
 #define B43_DMAIRQ_NONFATALMASK (1 << 13)
-#define B43_DMAIRQ_RX_DONE              (1 << 16)
+#define B43_DMAIRQ_RX_DONE      (1 << 16)
+#define B43_DMAIRQ_RDESC_UFLOW  (1 << 13)

 /*** 32-bit DMA Engine. ***/

@@ -295,6 +296,8 @@
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
                              const struct b43_txstatus *status);

+void b43_dma_rx_discard(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);

 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,

Index: drivers/net/wireless/b43/main.c
===================================================================
--- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/main.c
2013-04-09 17:33:54.325322046 +0200
+++ compat-wireless-2012-09-07/drivers/net/wireless/b43/main.c
2013-04-09 18:08:01.011471215 +0200
@@ -1894,14 +1894,6 @@
                         b43_controller_restart(dev, "DMA error");
                         return;
                 }
-                if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-                        b43err(dev->wl, "DMA error: "
-                               "0x%08X, 0x%08X, 0x%08X, "
-                               "0x%08X, 0x%08X, 0x%08X\n",
-                               dma_reason[0], dma_reason[1],
-                               dma_reason[2], dma_reason[3],
-                               dma_reason[4], dma_reason[5]);
-                }
         }

         if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1920,6 +1912,14 @@
                 handle_irq_noise(dev);

         /* Check the DMA reason registers for received data. */
+        if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+           //only print 256 time to not flood log
+           if(!(dev->stats.rxdesc_underruns++&0xFF)){
+                        b43warn(dev->wl, "Rx descriptor underrun
(high cpu load?), throwing packets\n");
+           }
+           b43_dma_rx_discard(dev->dma.rx_ring);
+
+        }
         if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
                 if (b43_using_pio_transfers(dev))
                         b43_pio_rx(dev->pio.rx_queue);
@@ -1977,7 +1977,7 @@
                 return IRQ_NONE;

         dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-            & 0x0001DC00;
+            & 0x0001FC00;
         dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
             & 0x0000DC00;
         dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3081,7 +3081,7 @@
                 b43_write32(dev, 0x018C, 0x02000000);
         }
         b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
         b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
         b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
         b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);

===================================================================
--- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/b43.h
2013-04-09 17:04:51.552680190 +0200
+++ compat-wireless-2012-09-07/drivers/net/wireless/b43/b43.h
2013-04-09 17:46:08.472962612 +0200
@@ -671,6 +671,7 @@

 struct b43_stats {
         u8 link_noise;
+        u32 rxdesc_underruns;
 };

 struct b43_key {

[PATCH] b43: use rx desc underrun interrupt

[Jonas Gorski]

Hi,

On Mon, Apr 15, 2013 at 10:02 AM, Thommy Jakobsson <thommyj@gmail.com> wrote:
> Hi,
>
> long user of b43 driver under openwrt. Not sure if it has been posted
> previously but using slow and old hardware (as routers running openwrt
> most of the time is), the CPU is sometimes slower than the NIC. So
> recently I, and several others, has had problem of wifi going down.
> See ticket 7552 at openwrt for more info
> (https://dev.openwrt.org/ticket/7552)
>
> I have tracked down the problem to the nic getting into a rx
> descriptor underrun. Since the driver don't listen to that (or take
> care of it in any other way), the Wifi seemingly just dies. The CPU is
> waiting for data from the nic and the nic is waiting for a
> confirmation that it can keep sending.
>
> I created a patch for handling of that interrupt. I have tested it by
> reducing the number of rx slots to 16 and sending a couple of 100GB of
> data. Previously Wifi went down within a minute when maxing out the
> uplink (so rx for AP), now it has been working flawless for a 1.5week.

Nice, very good to hear. Looks like you found the real issue the rx
descriptor count increase was just papering over. Some comments
regarding your patch.

>
> BR,
> Thommy Jakobsson
>

You are missing a "Signed-off-by" - without it the patch can never be
accepted. Also your patch is has broken tabs (they were converted to
spaces), as well as is line-broken, so it doesn't apply anymore. You
can't use gmail's web interface for sending patches.

> Index: drivers/net/wireless/b43/dma.c
> ===================================================================
> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.c
> 2013-04-09 17:33:54.325322046 +0200
> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.c

You should make your patch against the newest git tree, so usually
wireless-testing or Rafa?'s b43-next.

> 2013-04-09 17:34:43.473565843 +0200
> @@ -1688,6 +1688,21 @@
>          b43_poison_rx_buffer(ring, skb);
>          sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
>  }
> +void b43_dma_rx_discard(struct b43_dmaring *ring)
> +{
> +   B43_WARN_ON(ring->tx);
> +
> +    /* Device has filled all buffers, drop all packets in buffers
> +     * and let TCP decrease speed.
> +     * Set index to one desc after the last one (which is marked)
> +     * so the device will see all slots as free again
> +     */
> +    /*
> +     *TODO: How to increase rx_drop in mac80211
> +     */
> +   b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
> +                                     sizeof(struct b43_dmadesc32));
> +}
>
>  void b43_dma_rx(struct b43_dmaring *ring)
>  {
>
> Index: drivers/net/wireless/b43/dma.h
> ===================================================================
> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.h
> 2013-04-09 17:34:09.561397686 +0200
> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.h
> 2013-04-09 17:34:43.461565780 +0200
> @@ -10,7 +10,8 @@
>  #define B43_DMAIRQ_FATALMASK    ((1 << 10) | (1 << 11) | (1 << 12) \
>                                           | (1 << 14) | (1 << 15))
>  #define B43_DMAIRQ_NONFATALMASK (1 << 13)
> -#define B43_DMAIRQ_RX_DONE              (1 << 16)
> +#define B43_DMAIRQ_RX_DONE      (1 << 16)

Unnecessary whitespace change.

> +#define B43_DMAIRQ_RDESC_UFLOW  (1 << 13)

Why not just rename B43_DMAIRQ_NONFATALMASK ?

>
>  /*** 32-bit DMA Engine. ***/
>
> @@ -295,6 +296,8 @@
>  void b43_dma_handle_txstatus(struct b43_wldev *dev,
>                               const struct b43_txstatus *status);
>
> +void b43_dma_rx_discard(struct b43_dmaring *ring);
> +
>  void b43_dma_rx(struct b43_dmaring *ring);
>
>  void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
>
> Index: drivers/net/wireless/b43/main.c
> ===================================================================
> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/main.c
> 2013-04-09 17:33:54.325322046 +0200
> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/main.c
> 2013-04-09 18:08:01.011471215 +0200
> @@ -1894,14 +1894,6 @@
>                          b43_controller_restart(dev, "DMA error");
>                          return;
>                  }
> -                if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
> -                        b43err(dev->wl, "DMA error: "
> -                               "0x%08X, 0x%08X, 0x%08X, "
> -                               "0x%08X, 0x%08X, 0x%08X\n",
> -                               dma_reason[0], dma_reason[1],
> -                               dma_reason[2], dma_reason[3],
> -                               dma_reason[4], dma_reason[5]);
> -                }

You should say why you remove this one, and you should remove the
B43_DMAIRQ_NONFATALMASK from the if (unlikely()) enclosing it.

>          }
>
>          if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
> @@ -1920,6 +1912,14 @@
>                  handle_irq_noise(dev);
>
>          /* Check the DMA reason registers for received data. */
> +        if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
> +           //only print 256 time to not flood log
> +           if(!(dev->stats.rxdesc_underruns++&0xFF)){
> +                        b43warn(dev->wl, "Rx descriptor underrun
> (high cpu load?), throwing packets\n");

Apart from the style problems, maybe doing a
	if (printk_ratelimit())
		b43warn(...);

which will ensure that it won't flood the log, but still won't stop
reporting them after the 256th time.


> +           }
> +           b43_dma_rx_discard(dev->dma.rx_ring);
> +
> +        }
>          if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
>                  if (b43_using_pio_transfers(dev))
>                          b43_pio_rx(dev->pio.rx_queue);
> @@ -1977,7 +1977,7 @@
>                  return IRQ_NONE;
>
>          dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
> -            & 0x0001DC00;
> +            & 0x0001FC00;
>          dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
>              & 0x0000DC00;
>          dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
> @@ -3081,7 +3081,7 @@
>                  b43_write32(dev, 0x018C, 0x02000000);
>          }
>          b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
> -        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
> +        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
>          b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
>          b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
>          b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
>
> ===================================================================
> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/b43.h
> 2013-04-09 17:04:51.552680190 +0200
> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/b43.h
> 2013-04-09 17:46:08.472962612 +0200
> @@ -671,6 +671,7 @@
>
>  struct b43_stats {
>          u8 link_noise;
> +        u32 rxdesc_underruns;
>  };
>
>  struct b43_key {


Jonas

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Fri, 19 Apr 2013 16:32:00 +0200
Jonas Gorski <jonas.gorski@gmail.com> wrote:

> Apart from the style problems, maybe doing a
> 	if (printk_ratelimit())
> 		b43warn(...);
> 
> which will ensure that it won't flood the log, but still won't stop
> reporting them after the 256th time.

All b43 log helpers are ratelimited already. So no need for that.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130419/a57f356e/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Jonas Gorski]

On Fri, Apr 19, 2013 at 5:55 PM, Michael B?sch <m@bues.ch> wrote:
> On Fri, 19 Apr 2013 16:32:00 +0200
> Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
>> Apart from the style problems, maybe doing a
>>       if (printk_ratelimit())
>>               b43warn(...);
>>
>> which will ensure that it won't flood the log, but still won't stop
>> reporting them after the 256th time.
>
> All b43 log helpers are ratelimited already. So no need for that.

Even better :-)

Jonas

[PATCH] b43: use rx desc underrun interrupt

[Larry Finger]

On 04/19/2013 09:32 AM, Jonas Gorski wrote:
> Hi,
>
> On Mon, Apr 15, 2013 at 10:02 AM, Thommy Jakobsson <thommyj@gmail.com> wrote:
>> Hi,
>>
>> long user of b43 driver under openwrt. Not sure if it has been posted
>> previously but using slow and old hardware (as routers running openwrt
>> most of the time is), the CPU is sometimes slower than the NIC. So
>> recently I, and several others, has had problem of wifi going down.
>> See ticket 7552 at openwrt for more info
>> (https://dev.openwrt.org/ticket/7552)
>>
>> I have tracked down the problem to the nic getting into a rx
>> descriptor underrun. Since the driver don't listen to that (or take
>> care of it in any other way), the Wifi seemingly just dies. The CPU is
>> waiting for data from the nic and the nic is waiting for a
>> confirmation that it can keep sending.
>>
>> I created a patch for handling of that interrupt. I have tested it by
>> reducing the number of rx slots to 16 and sending a couple of 100GB of
>> data. Previously Wifi went down within a minute when maxing out the
>> uplink (so rx for AP), now it has been working flawless for a 1.5week.
>
> Nice, very good to hear. Looks like you found the real issue the rx
> descriptor count increase was just papering over. Some comments
> regarding your patch.
>
>>
>> BR,
>> Thommy Jakobsson
>>
>
> You are missing a "Signed-off-by" - without it the patch can never be
> accepted. Also your patch is has broken tabs (they were converted to
> spaces), as well as is line-broken, so it doesn't apply anymore. You
> can't use gmail's web interface for sending patches.
>
>> Index: drivers/net/wireless/b43/dma.c
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.c
>> 2013-04-09 17:33:54.325322046 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.c
>
> You should make your patch against the newest git tree, so usually
> wireless-testing or Rafa?'s b43-next.
>
>> 2013-04-09 17:34:43.473565843 +0200
>> @@ -1688,6 +1688,21 @@
>>           b43_poison_rx_buffer(ring, skb);
>>           sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
>>   }
>> +void b43_dma_rx_discard(struct b43_dmaring *ring)
>> +{
>> +   B43_WARN_ON(ring->tx);
>> +
>> +    /* Device has filled all buffers, drop all packets in buffers
>> +     * and let TCP decrease speed.
>> +     * Set index to one desc after the last one (which is marked)
>> +     * so the device will see all slots as free again
>> +     */
>> +    /*
>> +     *TODO: How to increase rx_drop in mac80211
>> +     */
>> +   b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
>> +                                     sizeof(struct b43_dmadesc32));
>> +}
>>
>>   void b43_dma_rx(struct b43_dmaring *ring)
>>   {
>>
>> Index: drivers/net/wireless/b43/dma.h
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.h
>> 2013-04-09 17:34:09.561397686 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.h
>> 2013-04-09 17:34:43.461565780 +0200
>> @@ -10,7 +10,8 @@
>>   #define B43_DMAIRQ_FATALMASK    ((1 << 10) | (1 << 11) | (1 << 12) \
>>                                            | (1 << 14) | (1 << 15))
>>   #define B43_DMAIRQ_NONFATALMASK (1 << 13)
>> -#define B43_DMAIRQ_RX_DONE              (1 << 16)
>> +#define B43_DMAIRQ_RX_DONE      (1 << 16)
>
> Unnecessary whitespace change.
>
>> +#define B43_DMAIRQ_RDESC_UFLOW  (1 << 13)
>
> Why not just rename B43_DMAIRQ_NONFATALMASK ?
>
>>
>>   /*** 32-bit DMA Engine. ***/
>>
>> @@ -295,6 +296,8 @@
>>   void b43_dma_handle_txstatus(struct b43_wldev *dev,
>>                                const struct b43_txstatus *status);
>>
>> +void b43_dma_rx_discard(struct b43_dmaring *ring);
>> +
>>   void b43_dma_rx(struct b43_dmaring *ring);
>>
>>   void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
>>
>> Index: drivers/net/wireless/b43/main.c
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/main.c
>> 2013-04-09 17:33:54.325322046 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/main.c
>> 2013-04-09 18:08:01.011471215 +0200
>> @@ -1894,14 +1894,6 @@
>>                           b43_controller_restart(dev, "DMA error");
>>                           return;
>>                   }
>> -                if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
>> -                        b43err(dev->wl, "DMA error: "
>> -                               "0x%08X, 0x%08X, 0x%08X, "
>> -                               "0x%08X, 0x%08X, 0x%08X\n",
>> -                               dma_reason[0], dma_reason[1],
>> -                               dma_reason[2], dma_reason[3],
>> -                               dma_reason[4], dma_reason[5]);
>> -                }
>
> You should say why you remove this one, and you should remove the
> B43_DMAIRQ_NONFATALMASK from the if (unlikely()) enclosing it.
>
>>           }
>>
>>           if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
>> @@ -1920,6 +1912,14 @@
>>                   handle_irq_noise(dev);
>>
>>           /* Check the DMA reason registers for received data. */
>> +        if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
>> +           //only print 256 time to not flood log
>> +           if(!(dev->stats.rxdesc_underruns++&0xFF)){
>> +                        b43warn(dev->wl, "Rx descriptor underrun
>> (high cpu load?), throwing packets\n");
>
> Apart from the style problems, maybe doing a
> 	if (printk_ratelimit())
> 		b43warn(...);
>
> which will ensure that it won't flood the log, but still won't stop
> reporting them after the 256th time.
>
>
>> +           }
>> +           b43_dma_rx_discard(dev->dma.rx_ring);
>> +
>> +        }
>>           if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
>>                   if (b43_using_pio_transfers(dev))
>>                           b43_pio_rx(dev->pio.rx_queue);
>> @@ -1977,7 +1977,7 @@
>>                   return IRQ_NONE;
>>
>>           dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
>> -            & 0x0001DC00;
>> +            & 0x0001FC00;
>>           dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
>>               & 0x0000DC00;
>>           dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
>> @@ -3081,7 +3081,7 @@
>>                   b43_write32(dev, 0x018C, 0x02000000);
>>           }
>>           b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
>> -        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
>> +        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
>>           b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
>>           b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
>>           b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
>>
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/b43.h
>> 2013-04-09 17:04:51.552680190 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/b43.h
>> 2013-04-09 17:46:08.472962612 +0200
>> @@ -671,6 +671,7 @@
>>
>>   struct b43_stats {
>>           u8 link_noise;
>> +        u32 rxdesc_underruns;
>>   };
>>
>>   struct b43_key {

Chris,

With this patch, can the number of RX descriptors be reduced back to 64 on your 
system? I realize it will be be hard to apply with the white space errors and 
line wraps, but perhaps we will get a clean version soon.

Larry

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Fri, 19 Apr 2013 13:17:03 -0500
Larry Finger <Larry.Finger@lwfinger.net> wrote:

> With this patch, can the number of RX descriptors be reduced back to 64 on your 
> system? I realize it will be be hard to apply with the white space errors and 
> line wraps, but perhaps we will get a clean version soon.

It would be good to lower the number of RX descriptors as low as possible.
Maybe even lower than 64. We should do more tests on that.
That certainly could improve performance on low-end devices.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130419/fe2ab06e/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

As I mentioned in the original post I lowered it to 16 on my system
without any problem (and I'm still running like that). I think that
was perhaps a bit too low though, didn't do any real measurements but
it felt like it cut off every other burst coming in. But I guess that
also depends on the target system.

//Thommy

On Fri, Apr 19, 2013 at 8:57 PM, Michael B?sch <m@bues.ch> wrote:
> On Fri, 19 Apr 2013 13:17:03 -0500
> Larry Finger <Larry.Finger@lwfinger.net> wrote:
>
>> With this patch, can the number of RX descriptors be reduced back to 64 on your
>> system? I realize it will be be hard to apply with the white space errors and
>> line wraps, but perhaps we will get a clean version soon.
>
> It would be good to lower the number of RX descriptors as low as possible.
> Maybe even lower than 64. We should do more tests on that.
> That certainly could improve performance on low-end devices.
>
> --
> Michael

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Fri, 19 Apr 2013 22:37:28 +0200
Thommy Jakobsson <thommyj@gmail.com> wrote:

> As I mentioned in the original post I lowered it to 16 on my system
> without any problem (and I'm still running like that). I think that
> was perhaps a bit too low though, didn't do any real measurements but
> it felt like it cut off every other burst coming in. But I guess that
> also depends on the target system.

Yes 16 sounds a little low, but 32 could be a good idea to test.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130419/18347906/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Larry Finger]

On 04/19/2013 03:46 PM, Michael B?sch wrote:
> On Fri, 19 Apr 2013 22:37:28 +0200
> Thommy Jakobsson <thommyj@gmail.com> wrote:
>
>> As I mentioned in the original post I lowered it to 16 on my system
>> without any problem (and I'm still running like that). I think that
>> was perhaps a bit too low though, didn't do any real measurements but
>> it felt like it cut off every other burst coming in. But I guess that
>> also depends on the target system.
>
> Yes 16 sounds a little low, but 32 could be a good idea to test.

I agree.

Larry

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

Hi Jonas,

thanks for input. As you see I'm not used to send in patches, but I'll
give it a new shot as soon as I succeed to download and patch the
latest git tree. Otherwise see my comments inline

//Thommy

On Fri, Apr 19, 2013 at 4:32 PM, Jonas Gorski <jonas.gorski@gmail.com> wrote:
> Hi,
>
> On Mon, Apr 15, 2013 at 10:02 AM, Thommy Jakobsson <thommyj@gmail.com> wrote:
>> Hi,
>>
>> long user of b43 driver under openwrt. Not sure if it has been posted
>> previously but using slow and old hardware (as routers running openwrt
>> most of the time is), the CPU is sometimes slower than the NIC. So
>> recently I, and several others, has had problem of wifi going down.
>> See ticket 7552 at openwrt for more info
>> (https://dev.openwrt.org/ticket/7552)
>>
>> I have tracked down the problem to the nic getting into a rx
>> descriptor underrun. Since the driver don't listen to that (or take
>> care of it in any other way), the Wifi seemingly just dies. The CPU is
>> waiting for data from the nic and the nic is waiting for a
>> confirmation that it can keep sending.
>>
>> I created a patch for handling of that interrupt. I have tested it by
>> reducing the number of rx slots to 16 and sending a couple of 100GB of
>> data. Previously Wifi went down within a minute when maxing out the
>> uplink (so rx for AP), now it has been working flawless for a 1.5week.
>
> Nice, very good to hear. Looks like you found the real issue the rx
> descriptor count increase was just papering over. Some comments
> regarding your patch.
>
>>
>> BR,
>> Thommy Jakobsson
>>
>
> You are missing a "Signed-off-by" - without it the patch can never be
> accepted. Also your patch is has broken tabs (they were converted to
> spaces), as well as is line-broken, so it doesn't apply anymore. You
> can't use gmail's web interface for sending patches.
>
>> Index: drivers/net/wireless/b43/dma.c
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.c
>> 2013-04-09 17:33:54.325322046 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.c
>
> You should make your patch against the newest git tree, so usually
> wireless-testing or Rafa?'s b43-next.
>
>> 2013-04-09 17:34:43.473565843 +0200
>> @@ -1688,6 +1688,21 @@
>>          b43_poison_rx_buffer(ring, skb);
>>          sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
>>  }
>> +void b43_dma_rx_discard(struct b43_dmaring *ring)
>> +{
>> +   B43_WARN_ON(ring->tx);
>> +
>> +    /* Device has filled all buffers, drop all packets in buffers
>> +     * and let TCP decrease speed.
>> +     * Set index to one desc after the last one (which is marked)
>> +     * so the device will see all slots as free again
>> +     */
>> +    /*
>> +     *TODO: How to increase rx_drop in mac80211
>> +     */
>> +   b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
>> +                                     sizeof(struct b43_dmadesc32));
>> +}
>>
>>  void b43_dma_rx(struct b43_dmaring *ring)
>>  {
>>
>> Index: drivers/net/wireless/b43/dma.h
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.h
>> 2013-04-09 17:34:09.561397686 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.h
>> 2013-04-09 17:34:43.461565780 +0200
>> @@ -10,7 +10,8 @@
>>  #define B43_DMAIRQ_FATALMASK    ((1 << 10) | (1 << 11) | (1 << 12) \
>>                                           | (1 << 14) | (1 << 15))
>>  #define B43_DMAIRQ_NONFATALMASK (1 << 13)
>> -#define B43_DMAIRQ_RX_DONE              (1 << 16)
>> +#define B43_DMAIRQ_RX_DONE      (1 << 16)
>
> Unnecessary whitespace change.
>
>> +#define B43_DMAIRQ_RDESC_UFLOW  (1 << 13)
>
> Why not just rename B43_DMAIRQ_NONFATALMASK ?
>
>>
>>  /*** 32-bit DMA Engine. ***/
>>
>> @@ -295,6 +296,8 @@
>>  void b43_dma_handle_txstatus(struct b43_wldev *dev,
>>                               const struct b43_txstatus *status);
>>
>> +void b43_dma_rx_discard(struct b43_dmaring *ring);
>> +
>>  void b43_dma_rx(struct b43_dmaring *ring);
>>
>>  void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
>>
>> Index: drivers/net/wireless/b43/main.c
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/main.c
>> 2013-04-09 17:33:54.325322046 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/main.c
>> 2013-04-09 18:08:01.011471215 +0200
>> @@ -1894,14 +1894,6 @@
>>                          b43_controller_restart(dev, "DMA error");
>>                          return;
>>                  }
>> -                if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
>> -                        b43err(dev->wl, "DMA error: "
>> -                               "0x%08X, 0x%08X, 0x%08X, "
>> -                               "0x%08X, 0x%08X, 0x%08X\n",
>> -                               dma_reason[0], dma_reason[1],
>> -                               dma_reason[2], dma_reason[3],
>> -                               dma_reason[4], dma_reason[5]);
>> -                }
>
> You should say why you remove this one, and you should remove the
> B43_DMAIRQ_NONFATALMASK from the if (unlikely()) enclosing it.

[thommy]: I removed it because I didn't want to have a printout each
time some non fatal happened. That would mean a printout every time a
rx descriptor underrun would happen. Also the printout says "DMA
error", which sounds very serious when in fact it isn't (or not in my opinion
anyway).


>
>>          }
>>
>>          if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
>> @@ -1920,6 +1912,14 @@
>>                  handle_irq_noise(dev);
>>
>>          /* Check the DMA reason registers for received data. */
>> +        if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
>> +           //only print 256 time to not flood log
>> +           if(!(dev->stats.rxdesc_underruns++&0xFF)){
>> +                        b43warn(dev->wl, "Rx descriptor underrun
>> (high cpu load?), throwing packets\n");
>
> Apart from the style problems, maybe doing a
>         if (printk_ratelimit())
>                 b43warn(...);
>
> which will ensure that it won't flood the log, but still won't stop
> reporting them after the 256th time.
[thommy]: My comment in the code is wrong. The meaning was to print it
every 256th time it happens, not to only to print the 256 first times.
As it seems from other messages there are better way of rate limiting
that I was unaware of.


>
>
>> +           }
>> +           b43_dma_rx_discard(dev->dma.rx_ring);
>> +
>> +        }
>>          if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
>>                  if (b43_using_pio_transfers(dev))
>>                          b43_pio_rx(dev->pio.rx_queue);
>> @@ -1977,7 +1977,7 @@
>>                  return IRQ_NONE;
>>
>>          dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
>> -            & 0x0001DC00;
>> +            & 0x0001FC00;
>>          dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
>>              & 0x0000DC00;
>>          dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
>> @@ -3081,7 +3081,7 @@
>>                  b43_write32(dev, 0x018C, 0x02000000);
>>          }
>>          b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
>> -        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
>> +        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
>>          b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
>>          b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
>>          b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
>>
>> ===================================================================
>> --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/b43.h
>> 2013-04-09 17:04:51.552680190 +0200
>> +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/b43.h
>> 2013-04-09 17:46:08.472962612 +0200
>> @@ -671,6 +671,7 @@
>>
>>  struct b43_stats {
>>          u8 link_noise;
>> +        u32 rxdesc_underruns;
>>  };
>>
>>  struct b43_key {
>
>
> Jonas

[PATCH] b43: use rx desc underrun interrupt

[Thommy]

Hi,

new try sending in the patch (with pine instead of gmail webclient this 
time). Patch created on latest wireless-testing 

BR,
Thommy Jakobsson

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/net/wireless/b43/b43.h  |    1 +
 drivers/net/wireless/b43/dma.c  |   16 +++++++++++++++
 drivers/net/wireless/b43/dma.h  |    4 +++-
 drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
 4 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 7f3d461..fdcd00f 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -680,6 +680,7 @@ struct b43_noise_calculation {
 
 struct b43_stats {
 	u8 link_noise;
+	u32 rxdesc_underruns;
 };
 
 struct b43_key {
diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 1221469..2ae00dd 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -1733,6 +1733,22 @@ drop_recycle_buffer:
 	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
 
+void b43_dma_rx_discard(struct b43_dmaring *ring)
+{
+	B43_WARN_ON(ring->tx);
+
+	/* Device has filled all buffers, drop all packets in buffers
+	* and let TCP decrease speed.
+	* Set index to one desc after the last one
+	* so the device will see all slots as free again
+	*/
+	/*
+	*TODO: How to increase rx_drop in mac80211?
+	*/
+	b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
+			sizeof(struct b43_dmadesc32));
+}
+
 void b43_dma_rx(struct b43_dmaring *ring)
 {
 	const struct b43_dma_ops *ops = ring->ops;
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 9fdd198..fed8163 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -9,7 +9,7 @@
 /* DMA-Interrupt reasons. */
 #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
 					 | (1 << 14) | (1 << 15))
-#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
+#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
 #define B43_DMAIRQ_RX_DONE		(1 << 16)
 
 /*** 32-bit DMA Engine. ***/
@@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
 			     const struct b43_txstatus *status);
 
+void b43_dma_rx_discard(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);
 
 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index d377f77..277fe75 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1902,30 +1902,19 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		}
 	}
 
-	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
-					  B43_DMAIRQ_NONFATALMASK))) {
-		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
-			b43err(dev->wl, "Fatal DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-			b43err(dev->wl, "This device does not support DMA "
+	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
+		b43err(dev->wl, "Fatal DMA error: "
+		       "0x%08X, 0x%08X, 0x%08X, "
+		       "0x%08X, 0x%08X, 0x%08X\n",
+		       dma_reason[0], dma_reason[1],
+		       dma_reason[2], dma_reason[3],
+		       dma_reason[4], dma_reason[5]);
+		b43err(dev->wl, "This device does not support DMA "
 			       "on your system. It will now be switched to PIO.\n");
-			/* Fall back to PIO transfers if we get fatal DMA errors! */
-			dev->use_pio = true;
-			b43_controller_restart(dev, "DMA error");
-			return;
-		}
-		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-			b43err(dev->wl, "DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-		}
+		/* Fall back to PIO transfers if we get fatal DMA errors! */
+		dev->use_pio = true;
+		b43_controller_restart(dev, "DMA error");
+		return;
 	}
 
 	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1944,6 +1933,10 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		handle_irq_noise(dev);
 
 	/* Check the DMA reason registers for received data. */
+	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+		b43warn(dev->wl, "RX descriptor underrun, dropping packets\n");
+		b43_dma_rx_discard(dev->dma.rx_ring);
+	}
 	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
 		if (b43_using_pio_transfers(dev))
 			b43_pio_rx(dev->pio.rx_queue);
@@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
 		return IRQ_NONE;
 
 	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-	    & 0x0001DC00;
+	    & 0x0001FC00;
 	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
 	    & 0x0000DC00;
 	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
 		b43_write32(dev, 0x018C, 0x02000000);
 	}
 	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
 	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
-- 
1.7.9.5


On Fri, 19 Apr 2013, Jonas Gorski wrote:

> Hi,
> 
> On Mon, Apr 15, 2013 at 10:02 AM, Thommy Jakobsson <thommyj@gmail.com> wrote:
> > Hi,
> >
> > long user of b43 driver under openwrt. Not sure if it has been posted
> > previously but using slow and old hardware (as routers running openwrt
> > most of the time is), the CPU is sometimes slower than the NIC. So
> > recently I, and several others, has had problem of wifi going down.
> > See ticket 7552 at openwrt for more info
> > (https://dev.openwrt.org/ticket/7552)
> >
> > I have tracked down the problem to the nic getting into a rx
> > descriptor underrun. Since the driver don't listen to that (or take
> > care of it in any other way), the Wifi seemingly just dies. The CPU is
> > waiting for data from the nic and the nic is waiting for a
> > confirmation that it can keep sending.
> >
> > I created a patch for handling of that interrupt. I have tested it by
> > reducing the number of rx slots to 16 and sending a couple of 100GB of
> > data. Previously Wifi went down within a minute when maxing out the
> > uplink (so rx for AP), now it has been working flawless for a 1.5week.
> 
> Nice, very good to hear. Looks like you found the real issue the rx
> descriptor count increase was just papering over. Some comments
> regarding your patch.
> 
> >
> > BR,
> > Thommy Jakobsson
> >
> 
> You are missing a "Signed-off-by" - without it the patch can never be
> accepted. Also your patch is has broken tabs (they were converted to
> spaces), as well as is line-broken, so it doesn't apply anymore. You
> can't use gmail's web interface for sending patches.
> 
> > Index: drivers/net/wireless/b43/dma.c
> > ===================================================================
> > --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.c
> > 2013-04-09 17:33:54.325322046 +0200
> > +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.c
> 
> You should make your patch against the newest git tree, so usually
> wireless-testing or Rafa?'s b43-next.
> 
> > 2013-04-09 17:34:43.473565843 +0200
> > @@ -1688,6 +1688,21 @@
> >          b43_poison_rx_buffer(ring, skb);
> >          sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
> >  }
> > +void b43_dma_rx_discard(struct b43_dmaring *ring)
> > +{
> > +   B43_WARN_ON(ring->tx);
> > +
> > +    /* Device has filled all buffers, drop all packets in buffers
> > +     * and let TCP decrease speed.
> > +     * Set index to one desc after the last one (which is marked)
> > +     * so the device will see all slots as free again
> > +     */
> > +    /*
> > +     *TODO: How to increase rx_drop in mac80211
> > +     */
> > +   b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
> > +                                     sizeof(struct b43_dmadesc32));
> > +}
> >
> >  void b43_dma_rx(struct b43_dmaring *ring)
> >  {
> >
> > Index: drivers/net/wireless/b43/dma.h
> > ===================================================================
> > --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/dma.h
> > 2013-04-09 17:34:09.561397686 +0200
> > +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/dma.h
> > 2013-04-09 17:34:43.461565780 +0200
> > @@ -10,7 +10,8 @@
> >  #define B43_DMAIRQ_FATALMASK    ((1 << 10) | (1 << 11) | (1 << 12) \
> >                                           | (1 << 14) | (1 << 15))
> >  #define B43_DMAIRQ_NONFATALMASK (1 << 13)
> > -#define B43_DMAIRQ_RX_DONE              (1 << 16)
> > +#define B43_DMAIRQ_RX_DONE      (1 << 16)
> 
> Unnecessary whitespace change.
> 
> > +#define B43_DMAIRQ_RDESC_UFLOW  (1 << 13)
> 
> Why not just rename B43_DMAIRQ_NONFATALMASK ?
> 
> >
> >  /*** 32-bit DMA Engine. ***/
> >
> > @@ -295,6 +296,8 @@
> >  void b43_dma_handle_txstatus(struct b43_wldev *dev,
> >                               const struct b43_txstatus *status);
> >
> > +void b43_dma_rx_discard(struct b43_dmaring *ring);
> > +
> >  void b43_dma_rx(struct b43_dmaring *ring);
> >
> >  void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
> >
> > Index: drivers/net/wireless/b43/main.c
> > ===================================================================
> > --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/main.c
> > 2013-04-09 17:33:54.325322046 +0200
> > +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/main.c
> > 2013-04-09 18:08:01.011471215 +0200
> > @@ -1894,14 +1894,6 @@
> >                          b43_controller_restart(dev, "DMA error");
> >                          return;
> >                  }
> > -                if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
> > -                        b43err(dev->wl, "DMA error: "
> > -                               "0x%08X, 0x%08X, 0x%08X, "
> > -                               "0x%08X, 0x%08X, 0x%08X\n",
> > -                               dma_reason[0], dma_reason[1],
> > -                               dma_reason[2], dma_reason[3],
> > -                               dma_reason[4], dma_reason[5]);
> > -                }
> 
> You should say why you remove this one, and you should remove the
> B43_DMAIRQ_NONFATALMASK from the if (unlikely()) enclosing it.
> 
> >          }
> >
> >          if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
> > @@ -1920,6 +1912,14 @@
> >                  handle_irq_noise(dev);
> >
> >          /* Check the DMA reason registers for received data. */
> > +        if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
> > +           //only print 256 time to not flood log
> > +           if(!(dev->stats.rxdesc_underruns++&0xFF)){
> > +                        b43warn(dev->wl, "Rx descriptor underrun
> > (high cpu load?), throwing packets\n");
> 
> Apart from the style problems, maybe doing a
> 	if (printk_ratelimit())
> 		b43warn(...);
> 
> which will ensure that it won't flood the log, but still won't stop
> reporting them after the 256th time.
> 
> 
> > +           }
> > +           b43_dma_rx_discard(dev->dma.rx_ring);
> > +
> > +        }
> >          if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
> >                  if (b43_using_pio_transfers(dev))
> >                          b43_pio_rx(dev->pio.rx_queue);
> > @@ -1977,7 +1977,7 @@
> >                  return IRQ_NONE;
> >
> >          dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
> > -            & 0x0001DC00;
> > +            & 0x0001FC00;
> >          dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
> >              & 0x0000DC00;
> >          dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
> > @@ -3081,7 +3081,7 @@
> >                  b43_write32(dev, 0x018C, 0x02000000);
> >          }
> >          b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
> > -        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
> > +        b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
> >          b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
> >          b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
> >          b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
> >
> > ===================================================================
> > --- compat-wireless-2012-09-07.orig/drivers/net/wireless/b43/b43.h
> > 2013-04-09 17:04:51.552680190 +0200
> > +++ compat-wireless-2012-09-07/drivers/net/wireless/b43/b43.h
> > 2013-04-09 17:46:08.472962612 +0200
> > @@ -671,6 +671,7 @@
> >
> >  struct b43_stats {
> >          u8 link_noise;
> > +        u32 rxdesc_underruns;
> >  };
> >
> >  struct b43_key {
> 
> 
> Jonas
> 

[PATCH] b43: use rx desc underrun interrupt

[Larry Finger]

On 04/20/2013 09:14 AM, Thommy wrote:
> Hi,
>
> new try sending in the patch (with pine instead of gmail webclient this
> time). Patch created on latest wireless-testing
>
> BR,
> Thommy Jakobsson
>
> Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
> ---
>   drivers/net/wireless/b43/b43.h  |    1 +
>   drivers/net/wireless/b43/dma.c  |   16 +++++++++++++++
>   drivers/net/wireless/b43/dma.h  |    4 +++-
>   drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
>   4 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index 7f3d461..fdcd00f 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -680,6 +680,7 @@ struct b43_noise_calculation {
>
>   struct b43_stats {
>   	u8 link_noise;
> +	u32 rxdesc_underruns;
>   };
>
>   struct b43_key {
> diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
> index 1221469..2ae00dd 100644
> --- a/drivers/net/wireless/b43/dma.c
> +++ b/drivers/net/wireless/b43/dma.c
> @@ -1733,6 +1733,22 @@ drop_recycle_buffer:
>   	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
>   }
>
> +void b43_dma_rx_discard(struct b43_dmaring *ring)
> +{
> +	B43_WARN_ON(ring->tx);
> +
> +	/* Device has filled all buffers, drop all packets in buffers
> +	* and let TCP decrease speed.
> +	* Set index to one desc after the last one
> +	* so the device will see all slots as free again
> +	*/
> +	/*
> +	*TODO: How to increase rx_drop in mac80211?
> +	*/
> +	b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
> +			sizeof(struct b43_dmadesc32));
> +}
> +
>   void b43_dma_rx(struct b43_dmaring *ring)
>   {
>   	const struct b43_dma_ops *ops = ring->ops;
> diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
> index 9fdd198..fed8163 100644
> --- a/drivers/net/wireless/b43/dma.h
> +++ b/drivers/net/wireless/b43/dma.h
> @@ -9,7 +9,7 @@
>   /* DMA-Interrupt reasons. */
>   #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
>   					 | (1 << 14) | (1 << 15))
> -#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
> +#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
>   #define B43_DMAIRQ_RX_DONE		(1 << 16)
>
>   /*** 32-bit DMA Engine. ***/
> @@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
>   void b43_dma_handle_txstatus(struct b43_wldev *dev,
>   			     const struct b43_txstatus *status);
>
> +void b43_dma_rx_discard(struct b43_dmaring *ring);
> +
>   void b43_dma_rx(struct b43_dmaring *ring);
>
>   void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index d377f77..277fe75 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -1902,30 +1902,19 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
>   		}
>   	}
>
> -	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
> -					  B43_DMAIRQ_NONFATALMASK))) {
> -		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
> -			b43err(dev->wl, "Fatal DMA error: "
> -			       "0x%08X, 0x%08X, 0x%08X, "
> -			       "0x%08X, 0x%08X, 0x%08X\n",
> -			       dma_reason[0], dma_reason[1],
> -			       dma_reason[2], dma_reason[3],
> -			       dma_reason[4], dma_reason[5]);
> -			b43err(dev->wl, "This device does not support DMA "
> +	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
> +		b43err(dev->wl, "Fatal DMA error: "
> +		       "0x%08X, 0x%08X, 0x%08X, "
> +		       "0x%08X, 0x%08X, 0x%08X\n",
> +		       dma_reason[0], dma_reason[1],
> +		       dma_reason[2], dma_reason[3],
> +		       dma_reason[4], dma_reason[5]);
> +		b43err(dev->wl, "This device does not support DMA "
>   			       "on your system. It will now be switched to PIO.\n");
> -			/* Fall back to PIO transfers if we get fatal DMA errors! */
> -			dev->use_pio = true;
> -			b43_controller_restart(dev, "DMA error");
> -			return;
> -		}
> -		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
> -			b43err(dev->wl, "DMA error: "
> -			       "0x%08X, 0x%08X, 0x%08X, "
> -			       "0x%08X, 0x%08X, 0x%08X\n",
> -			       dma_reason[0], dma_reason[1],
> -			       dma_reason[2], dma_reason[3],
> -			       dma_reason[4], dma_reason[5]);
> -		}
> +		/* Fall back to PIO transfers if we get fatal DMA errors! */
> +		dev->use_pio = true;
> +		b43_controller_restart(dev, "DMA error");
> +		return;
>   	}
>
>   	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
> @@ -1944,6 +1933,10 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
>   		handle_irq_noise(dev);
>
>   	/* Check the DMA reason registers for received data. */
> +	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
> +		b43warn(dev->wl, "RX descriptor underrun, dropping packets\n");
> +		b43_dma_rx_discard(dev->dma.rx_ring);
> +	}
>   	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
>   		if (b43_using_pio_transfers(dev))
>   			b43_pio_rx(dev->pio.rx_queue);
> @@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
>   		return IRQ_NONE;
>
>   	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
> -	    & 0x0001DC00;
> +	    & 0x0001FC00;
>   	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
>   	    & 0x0000DC00;
>   	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
> @@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
>   		b43_write32(dev, 0x018C, 0x02000000);
>   	}
>   	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
> -	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
> +	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
>   	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
>   	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
>   	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);

Thommy,

This version did apply cleanly; however, there are still a couple of things that 
need fixing.

With scripts/checkpatch.pl, there is a warning that a quoted string is split 
over two lines. Although testing for that condition is a recent addition to the 
script, it is probably best to honor that requirement with new code. To do so, 
the lines starting at 1907 in main.c should be

                 b43err(dev->wl,
                        "Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X, 
0x%08X, 0x%08X\n",

My mailer will likely wrap the string, but ignore that. Yes, that line is longer 
than 80 characters, but that is OK for quoted strings. The idea is that if they 
are not split, it is easier to grep for them. In this case, that is not an 
issue, but I prefer to make all new code be free of errors, warnings, and checks.

Your subject and commit message are not appropriate. Have you read 
Documentation/SubmittingPatches? The info you have at the start would not be 
appropriate for the permanent kernel record. If you want to add such informal 
remarks, include a line with --- after the formal commit message, and place 
informal remarks below that line. Everything is available at patch submission 
time, but the scripts strip it off before it is included in the git 
repositories. The b43-dev list is OK for discussions of the patch, but the final 
patch needs to be sent to John Linville <linville@tuxdriver.com>, with Cc: to 
b43-dev, and the linux-wireless mailing list <linux-wireless@vger.kernel.org>.

When the patch is appropriate for backporting to earlier, stable kernels, and 
this one certainly fits that criterion, follow the "Signed-off-by" line with one 
that says "Cc: Stable <stable@vger.kernel.org>". That way, the patch will 
automatically be included in earlier versions as soon as it is incorporated in 
the mainline version (Linus's tree).

Lastly, I have a question about the patch. Is it necessary to log every 
underrun? Perhaps you might want to place the b43_warn statement inside an "if 
(B43_DEBUG)" conditional. That way the condition will be logged for people 
really interested in debugging the driver, but the general user of a distro 
kernel will never see the message.

Thanks for finding this issue.

Larry

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Larry Finger wrote:

> Thommy,
> 
> This version did apply cleanly; however, there are still a couple of things
> that need fixing.
> 
> With scripts/checkpatch.pl, there is a warning that a quoted string is split
> over two lines. Although testing for that condition is a recent addition to
> the script, it is probably best to honor that requirement with new code. To do
> so, the lines starting at 1907 in main.c should be
> 
>                 b43err(dev->wl,
>                        "Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X,
> 0x%08X, 0x%08X\n",
> 
> My mailer will likely wrap the string, but ignore that. Yes, that line is
> longer than 80 characters, but that is OK for quoted strings. The idea is that
> if they are not split, it is easier to grep for them. In this case, that is
> not an issue, but I prefer to make all new code be free of errors, warnings,
> and checks.
> 
> Your subject and commit message are not appropriate. Have you read
> Documentation/SubmittingPatches? The info you have at the start would not be
> appropriate for the permanent kernel record. If you want to add such informal
> remarks, include a line with --- after the formal commit message, and place
> informal remarks below that line. Everything is available at patch submission
> time, but the scripts strip it off before it is included in the git
> repositories. The b43-dev list is OK for discussions of the patch, but the
> final patch needs to be sent to John Linville <linville@tuxdriver.com>, with
> Cc: to b43-dev, and the linux-wireless mailing list
> <linux-wireless@vger.kernel.org>.
> 
> When the patch is appropriate for backporting to earlier, stable kernels, and
> this one certainly fits that criterion, follow the "Signed-off-by" line with
> one that says "Cc: Stable <stable@vger.kernel.org>". That way, the patch will
> automatically be included in earlier versions as soon as it is incorporated in
> the mainline version (Linus's tree).
> 
> Lastly, I have a question about the patch. Is it necessary to log every
> underrun? Perhaps you might want to place the b43_warn statement inside an "if
> (B43_DEBUG)" conditional. That way the condition will be logged for people
> really interested in debugging the driver, but the general user of a distro
> kernel will never see the message.
> 
> Thanks for finding this issue.
> 
> Larry
> 
>

New try, think I have fixed everything but have probably missed something. 
If not Piotras or someone else find a better way I planned to send this in 
to John.

Subject:[PATCH] B43: Handle DMA RX descriptor underrun
From: Thommy Jakobsson <thommyj@gmail.com>

Add handling of rx descriptor underflow. This fixes a fault that could
happen on slow machines, where data is received faster than the CPU can
handle. In such a case the device will use up all rx descriptors and
refuse to send any more data before confirming that it is ok. This
patch enables necessary interrupt to discover such a situation and will
handle them by dropping everything in the ring buffer.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
Cc: Stable <stable@vger.kernel.org>
---
 drivers/net/wireless/b43/b43.h  |    1 +
 drivers/net/wireless/b43/dma.c  |   15 ++++++++++++++
 drivers/net/wireless/b43/dma.h  |    4 +++-
 drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
 4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 7f3d461..fdcd00f 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -680,6 +680,7 @@ struct b43_noise_calculation {
 
 struct b43_stats {
 	u8 link_noise;
+	u32 rxdesc_underruns;
 };
 
 struct b43_key {
diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 1221469..df929b3 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -1733,6 +1733,21 @@ drop_recycle_buffer:
 	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
 
+void b43_dma_rx_discard(struct b43_dmaring *ring)
+{
+	B43_WARN_ON(ring->tx);
+
+	/* Device has filled all buffers, drop all packets in buffers
+	* and let TCP decrease speed.
+	* Set index to one desc after the last one
+	* so the device will see all slots as free again
+	*/
+	/*
+	*TODO: How to increase rx_drop in mac80211?
+	*/
+	ring->ops->set_current_rxslot(ring, ring->nr_slots);
+}
+
 void b43_dma_rx(struct b43_dmaring *ring)
 {
 	const struct b43_dma_ops *ops = ring->ops;
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 9fdd198..fed8163 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -9,7 +9,7 @@
 /* DMA-Interrupt reasons. */
 #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
 					 | (1 << 14) | (1 << 15))
-#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
+#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
 #define B43_DMAIRQ_RX_DONE		(1 << 16)
 
 /*** 32-bit DMA Engine. ***/
@@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
 			     const struct b43_txstatus *status);
 
+void b43_dma_rx_discard(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);
 
 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index d377f77..5662f18 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1902,30 +1902,18 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		}
 	}
 
-	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
-					  B43_DMAIRQ_NONFATALMASK))) {
-		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
-			b43err(dev->wl, "Fatal DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-			b43err(dev->wl, "This device does not support DMA "
+	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
+		b43err(dev->wl,
+			"Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X\n",
+			dma_reason[0], dma_reason[1],
+			dma_reason[2], dma_reason[3],
+			dma_reason[4], dma_reason[5]);
+		b43err(dev->wl, "This device does not support DMA "
 			       "on your system. It will now be switched to PIO.\n");
-			/* Fall back to PIO transfers if we get fatal DMA errors! */
-			dev->use_pio = true;
-			b43_controller_restart(dev, "DMA error");
-			return;
-		}
-		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-			b43err(dev->wl, "DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-		}
+		/* Fall back to PIO transfers if we get fatal DMA errors! */
+		dev->use_pio = true;
+		b43_controller_restart(dev, "DMA error");
+		return;
 	}
 
 	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1944,6 +1932,11 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		handle_irq_noise(dev);
 
 	/* Check the DMA reason registers for received data. */
+	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+		if (B43_DEBUG)
+			b43warn(dev->wl, "RX descriptor underrun\n");
+		b43_dma_rx_discard(dev->dma.rx_ring);
+	}
 	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
 		if (b43_using_pio_transfers(dev))
 			b43_pio_rx(dev->pio.rx_queue);
@@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
 		return IRQ_NONE;
 
 	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-	    & 0x0001DC00;
+	    & 0x0001FC00;
 	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
 	    & 0x0000DC00;
 	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
 		b43_write32(dev, 0x018C, 0x02000000);
 	}
 	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
 	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
-- 
1.7.9.5

 

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sat, 20 Apr 2013 16:14:09 +0200 (CEST)
Thommy <thommyj@gmail.com> wrote:

> +void b43_dma_rx_discard(struct b43_dmaring *ring)
> +{
> +	B43_WARN_ON(ring->tx);
> +
> +	/* Device has filled all buffers, drop all packets in buffers
> +	* and let TCP decrease speed.
> +	* Set index to one desc after the last one
> +	* so the device will see all slots as free again
> +	*/
> +	/*
> +	*TODO: How to increase rx_drop in mac80211?
> +	*/
> +	b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
> +			sizeof(struct b43_dmadesc32));
> +}

You need to check whether this is a 32bit or 64bit DMA engine and
write to B43_DMA32_RXINDEX or B43_DMA64_RXINDEX.
You could simply use the ring->ops->set_current_rxslot() dmaop for that.

And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
Not 100% sure, though, since it is years since I worked on that code.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130420/8697d8d4/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Michael B?sch wrote:

> On Sat, 20 Apr 2013 16:14:09 +0200 (CEST)
> Thommy <thommyj@gmail.com> wrote:
> 
> > +void b43_dma_rx_discard(struct b43_dmaring *ring)
> > +{
> > +	B43_WARN_ON(ring->tx);
> > +
> > +	/* Device has filled all buffers, drop all packets in buffers
> > +	* and let TCP decrease speed.
> > +	* Set index to one desc after the last one
> > +	* so the device will see all slots as free again
> > +	*/
> > +	/*
> > +	*TODO: How to increase rx_drop in mac80211?
> > +	*/
> > +	b43_dma_write(ring, B43_DMA32_RXINDEX, ring->nr_slots *
> > +			sizeof(struct b43_dmadesc32));
> > +}
> 
> You need to check whether this is a 32bit or 64bit DMA engine and
> write to B43_DMA32_RXINDEX or B43_DMA64_RXINDEX.
> You could simply use the ring->ops->set_current_rxslot() dmaop for that.
>
Good point, I'll fix that

> And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
> Not 100% sure, though, since it is years since I worked on that code.
> 
Is that really needed? They should already be the same since the device 
has thrown the underflow interrupt, or do I miss some race condition?

//thommy 

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sat, 20 Apr 2013 21:47:10 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> > And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
> > Not 100% sure, though, since it is years since I worked on that code.
> > 
> Is that really needed? They should already be the same since the device 
> has thrown the underflow interrupt

Hm not sure. But should be easy to verify.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130420/333eb764/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Michael B?sch wrote:

> On Sat, 20 Apr 2013 21:47:10 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
> 
> > > And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
> > > Not 100% sure, though, since it is years since I worked on that code.
> > > 
> > Is that really needed? They should already be the same since the device 
> > has thrown the underflow interrupt
> 
> Hm not sure. But should be easy to verify.
I did check it in the beginning when I was testing the patch, just printed 
the indexes in the kernel log. But of course if there is a race condition
it could be hard to catch. Have used the patch for several weeks 
and several 100Gb now, so it does seem to work. 

The reason for getting this interrupt is that the current descriptor 
pointer (RXDPTR) has the same value as the descriptor index (RXINDEX). The 
descriptor index is set at the end of b43_dma_rx with 
ops->set_current_rxslot. At the next line, and with the same value, the 
ring->current slot is set. So in my opinion I don't need to set it. But I 
could have missed something.

BR,
Thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sat, 20 Apr 2013 22:16:26 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> 
> 
> On Sat, 20 Apr 2013, Michael B?sch wrote:
> 
> > On Sat, 20 Apr 2013 21:47:10 +0200 (CEST)
> > Thommy Jakobsson <thommyj@gmail.com> wrote:
> > 
> > > > And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
> > > > Not 100% sure, though, since it is years since I worked on that code.
> > > > 
> > > Is that really needed? They should already be the same since the device 
> > > has thrown the underflow interrupt
> > 
> > Hm not sure. But should be easy to verify.
> I did check it in the beginning when I was testing the patch, just printed 
> the indexes in the kernel log. But of course if there is a race condition
> it could be hard to catch.

Ok. I don't think there should be a race.
Thanks for checking.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130420/cefb6f3a/attachment-0001.sig>

[PATCH] b43: use rx desc underrun interrupt

[Piotras]

(resending with some corrections after original email bounced)

Hi all,

I tested original version of this patch and it happen to fix the issue
on Linksys WRT54GL (based on Broadcom 5352 SoC). However I think that
the bug is still present.

After closer look it seems that DMA buffers may be used incorrectly in
b43_dma_rx. I don't have documentation for Broadcom WLAN chipsets, but
DMA engine used by for BCM440X apparently has similar design. Looking
at "RECEIVE DESCRIPTOR TABLE POINTER REGISTER"
(page 35 of http://www.broadcom.com/collateral/pg/440X-PG02-R.pdf),
B43_DMA32_RXINDEX should be left pointing after last valid descriptor
as initialized in dmacontroller_setup. Notice that the last valid
descriptor already has B43_DMA32_DCTL_DTABLEEND set, so DMA should
operate in a round-robin fashion.

B43_DMA32_RXINDEX is reprogrammed from b43_dma_rx, by calling
ops->set_current_rxslot(ring, slot). The patch from Thommy simply
reprogrammes the register back with the same value as initialized in
dmacontroller_setup.

I suspect that with set_current_rxslot call removed, the patch from
Thommy will not be necessary anymore. I'm currently testing such fix
and will report back in a few days.


Regards,

Piotr

On Sat, Apr 20, 2013 at 9:16 PM, Thommy Jakobsson <thommyj@gmail.com> wrote:
>
>
> On Sat, 20 Apr 2013, Michael B?sch wrote:
>
>> On Sat, 20 Apr 2013 21:47:10 +0200 (CEST)
>> Thommy Jakobsson <thommyj@gmail.com> wrote:
>>
>> > > And I think you need to set ring->current_slot to the value of ring->ops->get_current_slot().
>> > > Not 100% sure, though, since it is years since I worked on that code.
>> > >
>> > Is that really needed? They should already be the same since the device
>> > has thrown the underflow interrupt
>>
>> Hm not sure. But should be easy to verify.
> I did check it in the beginning when I was testing the patch, just printed
> the indexes in the kernel log. But of course if there is a race condition
> it could be hard to catch. Have used the patch for several weeks
> and several 100Gb now, so it does seem to work.
>
> The reason for getting this interrupt is that the current descriptor
> pointer (RXDPTR) has the same value as the descriptor index (RXINDEX). The
> descriptor index is set at the end of b43_dma_rx with
> ops->set_current_rxslot. At the next line, and with the same value, the
> ring->current slot is set. So in my opinion I don't need to set it. But I
> could have missed something.
>
> BR,
> Thommy
> _______________________________________________
> b43-dev mailing list
> b43-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/b43-dev
>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Piotras wrote:

> Hi all,
> 
> I tested original version of this patch and it happen to fix the issue
> on Linksys WRT54GL (based on Broadcom 5352 SoC). However I think that
> the original cause is still present.
> 
> After closer look it seems that DMA buffers may be used incorrectly in
> b43_dma_rx. I don't have documentation for Broadcom WLAN chipsets, but
> DMA engine used by for BCM440X apparently has similar design. Looking
> at "RECEIVE DESCRIPTOR TABLE POINTER REGISTER"
> (page 35 of http://www.broadcom.com/collateral/pg/440X-PG02-R.pdf),
> B43_DMA32_RXINDEX should be left pointing after last valid descriptor
> as initialized in dmacontroller_setup (the referred descriptor already
> has B43_DMA32_DCTL_DTABLEEND set).
> 
> B43_DMA32_RXINDEX is reprogrammed from b43_dma_rx, by calling
> ops->set_current_rxslot(ring, slot). The patch from Thommy simply
> reprogramms the register back to the value initialized used in
> dmacontroller_setup.
> 
> I suspect that with set_current_rxslot call removed, the patch from
> Thommy will not be necessary anymore. I'm currently testing such fix
> and will report back in a few days.
> 
> 
> Regards,
> 
> Piotr
> 
Hi,

yes I also saw that datasheet and funny enough I also reacted on the same 
thing. But if you keep the rxindex pointing after the last descriptor all 
the time, how can you know if you had an underflow? Of course you could 
argue that it doesn't matter, the device will just keep on sending data 
since the current descriptor index will never reach the descriptor index. 
But that also means that you could start reading a packet at the same time 
as the device is overwriting it with an other, right? I didn't read it to 
carefully but the datasheet doesn't prohibit a change of the rxpointer, 
does it?

//Thommy 

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Thommy Jakobsson wrote:

> 
> 
> On Sat, 20 Apr 2013, Piotras wrote:
> 
> > Hi all,
> > 
> > I tested original version of this patch and it happen to fix the issue
> > on Linksys WRT54GL (based on Broadcom 5352 SoC). However I think that
> > the original cause is still present.
> > 
> > After closer look it seems that DMA buffers may be used incorrectly in
> > b43_dma_rx. I don't have documentation for Broadcom WLAN chipsets, but
> > DMA engine used by for BCM440X apparently has similar design. Looking
> > at "RECEIVE DESCRIPTOR TABLE POINTER REGISTER"
> > (page 35 of http://www.broadcom.com/collateral/pg/440X-PG02-R.pdf),
> > B43_DMA32_RXINDEX should be left pointing after last valid descriptor
> > as initialized in dmacontroller_setup (the referred descriptor already
> > has B43_DMA32_DCTL_DTABLEEND set).
> > 
> > B43_DMA32_RXINDEX is reprogrammed from b43_dma_rx, by calling
> > ops->set_current_rxslot(ring, slot). The patch from Thommy simply
> > reprogramms the register back to the value initialized used in
> > dmacontroller_setup.
> > 
> > I suspect that with set_current_rxslot call removed, the patch from
> > Thommy will not be necessary anymore. I'm currently testing such fix
> > and will report back in a few days.
> > 
> > 
> > Regards,
> > 
> > Piotr
> > 
> Hi,
> 
> yes I also saw that datasheet and funny enough I also reacted on the same 
> thing. But if you keep the rxindex pointing after the last descriptor all 
> the time, how can you know if you had an underflow? Of course you could 
> argue that it doesn't matter, the device will just keep on sending data 
> since the current descriptor index will never reach the descriptor index. 
> But that also means that you could start reading a packet at the same time 
> as the device is overwriting it with an other, right? I didn't read it to 
> carefully but the datasheet doesn't prohibit a change of the rxpointer, 
> does it?
> 
> //Thommy 
> 
Sorry I mean that the datasheet doesn't prohibit a change of the rx 
descriptor index, does it? Where the rx descriptor index in the reversed 
broadcom documentation and code would be translated to the LastDscr
in the BCM440X datasheet (even has the same offset).

//Thommy

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sat, 20 Apr 2013, Thommy Jakobsson wrote:

> 
> 
> On Sat, 20 Apr 2013, Thommy Jakobsson wrote:
> 
> > 
> > 
> > On Sat, 20 Apr 2013, Piotras wrote:
> > 
> > > Hi all,
> > > 
> > > I tested original version of this patch and it happen to fix the issue
> > > on Linksys WRT54GL (based on Broadcom 5352 SoC). However I think that
> > > the original cause is still present.
> > > 
> > > After closer look it seems that DMA buffers may be used incorrectly in
> > > b43_dma_rx. I don't have documentation for Broadcom WLAN chipsets, but
> > > DMA engine used by for BCM440X apparently has similar design. Looking
> > > at "RECEIVE DESCRIPTOR TABLE POINTER REGISTER"
> > > (page 35 of http://www.broadcom.com/collateral/pg/440X-PG02-R.pdf),
> > > B43_DMA32_RXINDEX should be left pointing after last valid descriptor
> > > as initialized in dmacontroller_setup (the referred descriptor already
> > > has B43_DMA32_DCTL_DTABLEEND set).
> > > 
> > > B43_DMA32_RXINDEX is reprogrammed from b43_dma_rx, by calling
> > > ops->set_current_rxslot(ring, slot). The patch from Thommy simply
> > > reprogramms the register back to the value initialized used in
> > > dmacontroller_setup.
> > > 
> > > I suspect that with set_current_rxslot call removed, the patch from
> > > Thommy will not be necessary anymore. I'm currently testing such fix
> > > and will report back in a few days.
> > > 
> > > 
> > > Regards,
> > > 
> > > Piotr
> > > 
> > Hi,
> > 
> > yes I also saw that datasheet and funny enough I also reacted on the same 
> > thing. But if you keep the rxindex pointing after the last descriptor all 
> > the time, how can you know if you had an underflow? Of course you could 
> > argue that it doesn't matter, the device will just keep on sending data 
> > since the current descriptor index will never reach the descriptor index. 
> > But that also means that you could start reading a packet at the same time 
> > as the device is overwriting it with an other, right? I didn't read it to 
> > carefully but the datasheet doesn't prohibit a change of the rxpointer, 
> > does it?
> > 
> > //Thommy 
> > 
> Sorry I mean that the datasheet doesn't prohibit a change of the rx 
> descriptor index, does it? Where the rx descriptor index in the reversed 
> broadcom documentation and code would be translated to the LastDscr
> in the BCM440X datasheet (even has the same offset).
> 
> //Thommy
> 
Looking a bit more carefully in the datasheet I find

"Software makes the new descriptors available for use by updating the 
LastDscr field of the XmtPtr register (see ?Receive Descriptor Table 
Pointer Register (RcvPtr, offset 0x218)? on page 35) to point to the entry 
after the new last valid descriptor." on page 40

Isn't that exactly what we do in b43_dma_rx? The initial rx descriptor 
index is just to make the device to start. Theoretically you could get 
into a fault situation if the device succeds in using up all descriptors 
between b43_dma_rx_discard (added by my patch) and the call to b43_dma_rx. 
Because then you could start reading the same packet as the device is 
writing to.

//Thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sat, 20 Apr 2013 23:23:54 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> Isn't that exactly what we do in b43_dma_rx? The initial rx descriptor 
> index is just to make the device to start. Theoretically you could get 
> into a fault situation if the device succeds in using up all descriptors 
> between b43_dma_rx_discard (added by my patch) and the call to b43_dma_rx. 
> Because then you could start reading the same packet as the device is 
> writing to.

Yes this is true.
And thus I'm currently unsure why we need this patch at all.
_Why_ does the DMA stall as soon as the ring is filled up?

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/201e5823/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sun, 21 Apr 2013, Michael B?sch wrote:

> On Sat, 20 Apr 2013 23:23:54 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
> 
> > Isn't that exactly what we do in b43_dma_rx? The initial rx descriptor 
> > index is just to make the device to start. Theoretically you could get 
> > into a fault situation if the device succeds in using up all descriptors 
> > between b43_dma_rx_discard (added by my patch) and the call to b43_dma_rx. 
> > Because then you could start reading the same packet as the device is 
> > writing to.
> 
> Yes this is true.
> And thus I'm currently unsure why we need this patch at all.
> _Why_ does the DMA stall as soon as the ring is filled up?


The device stops sending when it hits the rx index. Thats what rx index 
is, the address after the last slot the device is allowed to use. In the 
driver it is used as the last slot handled, so if the device would 
continue it will overwrite slots that hasn't been read. So you need to 
handle that in someway. One way is using my patch, another is to never let 
it hit rx index (as Piotras is working on).

This is how I see the problem:
1. the driver get an interrupt that there is data in the ring, the 
interrupt thread is scheduled
2. the interrupt thread (in b43_dma_rx) checks how many slots that the 
device has filled
3. it then loops through them and sends the packets in the ring to the 
kernel (dma_rx). It also allocates new skb_buf:s
4. after all slots has been handled the rx index is updated
5. In the meantime the device continues to pump data in to the ring
6. If the cpu is slow, and we don't have that many slots, there is a 
chance that the device have time to loop through all and hitting rx index. 
If we don't update the rx index in such a situation the device will never 
continue. Since it has hit the end of available descriptors, it doesn't 
have much of a choice, where would it write next?

Normally the device never hit the rx index, and in such case it will just 
use the next descriptor in the table. If it finds a descriptor with END 
set, it will jump back to the beginning. Forming the ring

//Thomy 

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 10:22:51 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> The device stops sending when it hits the rx index. Thats what rx index 
> is, the address after the last slot the device is allowed to use. In the 
> driver it is used as the last slot handled, so if the device would 
> continue it will overwrite slots that hasn't been read. So you need to 
> handle that in someway. One way is using my patch, another is to never let 
> it hit rx index (as Piotras is working on).

Yeah I get that. But why do we need to handle that by putting the stop index
right beyond the ring? That requires us to get another RX interrupt before the
ring fills up again. Why can't we set it so that the ring is fully writable again,
but not set it beyond the ring boundary? I guess that is decrementing it by one,
perhaps, honoring 0-index-wrapover.

> Normally the device never hit the rx index, and in such case it will just 
> use the next descriptor in the table. If it finds a descriptor with END 
> set, it will jump back to the beginning. Forming the ring

I know how that basic stuff works, because I wrote most of the code ;D

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/59279114/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sun, 21 Apr 2013, Michael B?sch wrote:

> On Sun, 21 Apr 2013 10:22:51 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
> 
> > The device stops sending when it hits the rx index. Thats what rx index 
> > is, the address after the last slot the device is allowed to use. In the 
> > driver it is used as the last slot handled, so if the device would 
> > continue it will overwrite slots that hasn't been read. So you need to 
> > handle that in someway. One way is using my patch, another is to never let 
> > it hit rx index (as Piotras is working on).
> 
> Yeah I get that. But why do we need to handle that by putting the stop index
> right beyond the ring? That requires us to get another RX interrupt before the
> ring fills up again. Why can't we set it so that the ring is fully writable again,
> but not set it beyond the ring boundary? I guess that is decrementing it by one,
> perhaps, honoring 0-index-wrapover.
>
All times that I had extra debugging on I always got a normal rx interrupt 
at the same time as the underflow interrupt. So the b43_dma_rx is called 
just after the b43_rx_discard, and the rx index is updated again to 
something better. Writing the rx index to index-1 was my initial plan, the 
only reason why I chose beyond the ring boundary was because thats how it 
is initialised. I never tried index-1, so I don't know if it works though. 
It did try to write the same value back again, but of course that didn't 
work.

Why is it set to to beyond the ring when it is initilised?

> > Normally the device never hit the rx index, and in such case it will just 
> > use the next descriptor in the table. If it finds a descriptor with END 
> > set, it will jump back to the beginning. Forming the ring
> 
> I know how that basic stuff works, because I wrote most of the code ;D
and yet you didn't fix this problem... =) Just kidding, to be honest I 
didn't understand what in my patch your question concerned. So I pretty 
much wrote down all I know about how it works (or how I think it works).

Thanks,
Thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> Why is it set to to beyond the ring when it is initilised?

That's what broadcom does in their implementation. I guess that's the only reason.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/a25c76e8/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sun, 21 Apr 2013, Michael B?sch wrote:

> On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
> 
> > Why is it set to to beyond the ring when it is initilised?
> 
> That's what broadcom does in their implementation. I guess that's the only reason.
> 
Check, I'll give index-1 a go. It should be quite obvious if it works or 
not.

//thommy

[PATCH] b43: use rx desc underrun interrupt

[Piotras]

On Sun, Apr 21, 2013 at 1:12 PM, Thommy Jakobsson <thommyj@gmail.com> wrote:
>
>
> On Sun, 21 Apr 2013, Michael B?sch wrote:
>
>> On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
>> Thommy Jakobsson <thommyj@gmail.com> wrote:
>>
>> > Why is it set to to beyond the ring when it is initilised?
>>
>> That's what broadcom does in their implementation. I guess that's the only reason.
>>
> Check, I'll give index-1 a go. It should be quite obvious if it works or
> not.

Thommy,

I had the same thought, but notice that you also need to update
ring->current_slot in b43_dma_rx_discard.

When ring->current_slot update is missing (as in your original
patch), you will introduce the same problem I hit by removing
ops->set_current_rxslot(ring, slot) from b43_dma_rx (disabling
RX underflow detection). Basically we break assumptions about
ownership of descriptors (which descriptors are exclusively used
by host and which can be written by DMA processor). So next time
b43_dma_rx is called it may process descriptors that are updated
by DMA processor at the same time (race condition).

I guess the following could be used to mark all descriptors for DMA
use from RX underflow interrupt or whenever we decide that number of
free descriptors is too low (not tested):

  slot = prev_slot(ring, ops->get_current_rxslot(ring));
  wmb();
  ops->set_current_rxslot(ring, slot);
  ring->current_slot = slot;

The above code simply puts back all the already filled descriptors
back for DMA processor to be overwritten with new data.

I'm still concerned about FIFO overruns. When descriptor underflow
condition occurs while CPU is busy with b43_dma_rx, we still wait for
b43_dma_rx to finish processing all received packets before any
descriptors become available for DMA to use. Depending on FIFO size
and CPU load we may still hit fatal receive FIFO underflow error.
This should cause driver to reset (possible explanation of why
"discards" and "slot_usage" files disappeared when Richlv tested your
patch in https://dev.openwrt.org/ticket/7552#comment:94).

I suspect that driver reset after FIFO underflow interrupt triggers
is not handled correctly, but not sure why. Possibly this is not
expected by hostapd, so it needs restart?

Not sure if FIFO overruns are really an issue and how to prevent them.
Maybe RX underflow should be handled in top half and reprogram device
to suspend receives (is this possible)? Or maybe we could clean and
set B43_DMA32_RXENABLE bit in B43_DMA32_RXCTL to reset DMA channel
without resetting the driver?


Ideas?

Piotr


>
> //thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 15:46:03 +0100
Piotras <piotras@gmail.com> wrote:

> On Sun, Apr 21, 2013 at 1:12 PM, Thommy Jakobsson <thommyj@gmail.com> wrote:
> >
> >
> > On Sun, 21 Apr 2013, Michael B?sch wrote:
> >
> >> On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
> >> Thommy Jakobsson <thommyj@gmail.com> wrote:
> >>
> >> > Why is it set to to beyond the ring when it is initilised?
> >>
> >> That's what broadcom does in their implementation. I guess that's the only reason.
> >>
> > Check, I'll give index-1 a go. It should be quite obvious if it works or
> > not.
> 
> Thommy,
> 
> I had the same thought, but notice that you also need to update
> ring->current_slot in b43_dma_rx_discard.

I don't think so.
We don't modify the descriptor pointer, but the stop index.

> I guess the following could be used to mark all descriptors for DMA
> use from RX underflow interrupt or whenever we decide that number of
> free descriptors is too low (not tested):
> 
>   slot = prev_slot(ring, ops->get_current_rxslot(ring));
>   wmb();
>   ops->set_current_rxslot(ring, slot);

get_current_rxslot() reads a different register than set_current_rxslot().

get_current_rxslot() returns the descriptor pointer that the device is pointing to.
set_current_rxslot() sets the _stop_ index.
Yes, at least the last one is misnamed.

> Not sure if FIFO overruns are really an issue and how to prevent them.
> Maybe RX underflow should be handled in top half and reprogram device
> to suspend receives (is this possible)?

What is "top half"?

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/4c5e94bf/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Piotras]

On Sun, Apr 21, 2013 at 3:59 PM, Michael B?sch <m@bues.ch> wrote:
> On Sun, 21 Apr 2013 15:46:03 +0100
> Piotras <piotras@gmail.com> wrote:
>
>> On Sun, Apr 21, 2013 at 1:12 PM, Thommy Jakobsson <thommyj@gmail.com> wrote:
>> >
>> >
>> > On Sun, 21 Apr 2013, Michael B?sch wrote:
>> >
>> >> On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
>> >> Thommy Jakobsson <thommyj@gmail.com> wrote:
>> >>
>> >> > Why is it set to to beyond the ring when it is initilised?
>> >>
>> >> That's what broadcom does in their implementation. I guess that's the only reason.
>> >>
>> > Check, I'll give index-1 a go. It should be quite obvious if it works or
>> > not.
>>
>> Thommy,
>>
>> I had the same thought, but notice that you also need to update
>> ring->current_slot in b43_dma_rx_discard.
>
> I don't think so.
> We don't modify the descriptor pointer, but the stop index.

Yes.

My code was incorrect as I should ensure that ring->current_slot is
equal ops->get_current_rxslot(ring). As we are in the RX underflow,
B43_DMA32_RXSTATUS, B43_DMA32_RXINDEX and ring->current_slot should
already point to the same descriptor, so only set_current_rxslot is
needed.


>
>> I guess the following could be used to mark all descriptors for DMA
>> use from RX underflow interrupt or whenever we decide that number of
>> free descriptors is too low (not tested):
>>
>>   slot = prev_slot(ring, ops->get_current_rxslot(ring));
>>   wmb();
>>   ops->set_current_rxslot(ring, slot);
>
> get_current_rxslot() reads a different register than set_current_rxslot().
>
> get_current_rxslot() returns the descriptor pointer that the device is pointing to.
> set_current_rxslot() sets the _stop_ index.
> Yes, at least the last one is misnamed.
>
>> Not sure if FIFO overruns are really an issue and how to prevent them.
>> Maybe RX underflow should be handled in top half and reprogram device
>> to suspend receives (is this possible)?
>
> What is "top half"?

Top half interrupt handler (b43_do_interrupt).


>
> --
> Michael

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 16:24:18 +0100
Piotras <piotras@gmail.com> wrote:

> >> Not sure if FIFO overruns are really an issue and how to prevent them.
> >> Maybe RX underflow should be handled in top half and reprogram device
> >> to suspend receives (is this possible)?
> >
> > What is "top half"?
> 
> Top half interrupt handler (b43_do_interrupt).

No way.
Interrupts are properly blocked before leaving the top-half handler and
re-enabled before leaving the bottom-half handler already.
Adding stuff to the top-half handler would only introduce serious locking
and latency issues and it would fix nothing.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/36056889/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

> I had the same thought, but notice that you also need to update
> ring->current_slot in b43_dma_rx_discard.
> 
> When ring->current_slot update is missing (as in your original
> patch), you will introduce the same problem I hit by removing
> ops->set_current_rxslot(ring, slot) from b43_dma_rx (disabling
> RX underflow detection). Basically we break assumptions about
> ownership of descriptors (which descriptors are exclusively used
> by host and which can be written by DMA processor). So next time
> b43_dma_rx is called it may process descriptors that are updated
> by DMA processor at the same time (race condition).
Why would I update ring->current_slot? That should point where we are in 
the ring from the drivers perspective. Since that i always updated 
together with the rx index (set_current_rxslot) it will always be the same 
as the current index in hw (get_current_rxslot) in the b43_rx_discard. 
That is also where it should be.

It is true that I'll introduce a possibility for ownership problem with 
the original patch, but that will only last until the next time b43_dma_rx 
is called. Which is just after the b43_dma_rx_discard has 
exited since you always will get a rx interrupt at the same time as the 
underflow interrupt. This is also the case at each startup since rx index 
is initilised to the same thing as I used.

I have tested the index-1 that me and michael talked about, from the 
initial test is working just fine. Im running with only 8 rxslots to try 
to find any corner cases, so I see a lot of traffic drops though. This 
would also fix the ownership problem.

BTW, it is confusing that set and get current_rxslot doesn't updated and 
read the same register. 

About the FIFO errors I haven't looked at those at all. I did get some 
other errors when I pumped data at link speed into my router, but that was 
for TX.

//Thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 17:11:23 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> BTW, it is confusing that set and get current_rxslot doesn't updated and 
> read the same register. 

Yes the set_current_rxslot() op should be renamed to set_rx_stop_slot() or something like that.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/ad24cd10/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sun, 21 Apr 2013, Michael B?sch wrote:

> Yeah I get that. But why do we need to handle that by putting the stop index
> right beyond the ring? That requires us to get another RX interrupt before the
> ring fills up again. Why can't we set it so that the ring is fully writable again,
> but not set it beyond the ring boundary? I guess that is decrementing it by one,
> perhaps, honoring 0-index-wrapover.
> 
Hi again,
below you can find the patch that I'm currently trying out. The 
change is according to the discussion above. Instead of setting the rx 
index as the driver does during startup, it decrements it by one.

It has been running at ~1300KB/s in RX and 100KB/s in TX for the last 
couple of hours without problems. Approximately there have been one 
underflow per second. CPU load is constantly over 80%, pretty much just the 
interrupt thread that is using the CPU.

I can see that at the entry of b43_dma_discard all three indexes, 
ring->current_slot(ring), get_current_rxslot(ring) and
b43_dma_read(ring,B43_DMA32_RXINDEX) are always the same. Of course the rx 
index is in bytes. So this is as expected. 
B
I also has to back my statement that I always see a normal rx interrupt at 
the same time as the underflow interrupt. I have seen a couple of cases 
where that didn't happen. So this seems like a better way of doing 
things than setting rx index to max slots when having underflows. This 
way device and driver collisions are better avoided. Good idea Michael.

//Thommy


Subject:[PATCH] B43: Handle DMA RX descriptor underrun
From: Thommy Jakobsson <thommyj@gmail.com>

Add handling of rx descriptor underflow. This fixes a fault that could
happen on slow machines, where data is received faster than the CPU can
handle. In such a case the device will use up all rx descriptors and
refuse to send any more data before confirming that it is ok. This
patch enables necessary interrupt to discover such a situation and will
handle them by dropping everything in the ring buffer.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/net/wireless/b43/b43.h  |    1 +
 drivers/net/wireless/b43/dma.c  |   19 +++++++++++++++++
 drivers/net/wireless/b43/dma.h  |    4 +++-
 drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
 4 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 7f3d461..fdcd00f 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -680,6 +680,7 @@ struct b43_noise_calculation {
 
 struct b43_stats {
 	u8 link_noise;
+	u32 rxdesc_underruns;
 };
 
 struct b43_key {
diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 1221469..139305e 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -1733,6 +1733,25 @@ drop_recycle_buffer:
 	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
 
+void b43_dma_rx_discard(struct b43_dmaring *ring)
+{
+	int current_slot, previous_slot;
+
+	B43_WARN_ON(ring->tx);
+
+	/* Device has filled all buffers, drop all packets in buffers
+	* and let TCP decrease speed.
+	* Set index to one desc after the last one
+	* so the device will see all slots as free again
+	*/
+	/*
+	*TODO: How to increase rx_drop in mac80211?
+	*/
+	current_slot = ring->ops->get_current_rxslot(ring);
+	previous_slot = prev_slot(ring, current_slot);
+	ring->ops->set_current_rxslot(ring, previous_slot);
+}
+
 void b43_dma_rx(struct b43_dmaring *ring)
 {
 	const struct b43_dma_ops *ops = ring->ops;
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 9fdd198..fed8163 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -9,7 +9,7 @@
 /* DMA-Interrupt reasons. */
 #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
 					 | (1 << 14) | (1 << 15))
-#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
+#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
 #define B43_DMAIRQ_RX_DONE		(1 << 16)
 
 /*** 32-bit DMA Engine. ***/
@@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
 			     const struct b43_txstatus *status);
 
+void b43_dma_rx_discard(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);
 
 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index d377f77..5662f18 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1902,30 +1902,18 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		}
 	}
 
-	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
-					  B43_DMAIRQ_NONFATALMASK))) {
-		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
-			b43err(dev->wl, "Fatal DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-			b43err(dev->wl, "This device does not support DMA "
+	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
+		b43err(dev->wl,
+			"Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X\n",
+			dma_reason[0], dma_reason[1],
+			dma_reason[2], dma_reason[3],
+			dma_reason[4], dma_reason[5]);
+		b43err(dev->wl, "This device does not support DMA "
 			       "on your system. It will now be switched to PIO.\n");
-			/* Fall back to PIO transfers if we get fatal DMA errors! */
-			dev->use_pio = true;
-			b43_controller_restart(dev, "DMA error");
-			return;
-		}
-		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-			b43err(dev->wl, "DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-		}
+		/* Fall back to PIO transfers if we get fatal DMA errors! */
+		dev->use_pio = true;
+		b43_controller_restart(dev, "DMA error");
+		return;
 	}
 
 	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1944,6 +1932,11 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		handle_irq_noise(dev);
 
 	/* Check the DMA reason registers for received data. */
+	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+		if (B43_DEBUG)
+			b43warn(dev->wl, "RX descriptor underrun\n");
+		b43_dma_rx_discard(dev->dma.rx_ring);
+	}
 	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
 		if (b43_using_pio_transfers(dev))
 			b43_pio_rx(dev->pio.rx_queue);
@@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
 		return IRQ_NONE;
 
 	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-	    & 0x0001DC00;
+	    & 0x0001FC00;
 	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
 	    & 0x0000DC00;
 	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
 		b43_write32(dev, 0x018C, 0x02000000);
 	}
 	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
 	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
-- 
1.7.9.5

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 20:07:39 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> +void b43_dma_rx_discard(struct b43_dmaring *ring)
> +{
> +	int current_slot, previous_slot;
> +
> +	B43_WARN_ON(ring->tx);
> +
> +	/* Device has filled all buffers, drop all packets in buffers
> +	* and let TCP decrease speed.
> +	* Set index to one desc after the last one
> +	* so the device will see all slots as free again
> +	*/
> +	/*
> +	*TODO: How to increase rx_drop in mac80211?
> +	*/
> +	current_slot = ring->ops->get_current_rxslot(ring);
> +	previous_slot = prev_slot(ring, current_slot);
> +	ring->ops->set_current_rxslot(ring, previous_slot);

Hmmm. While this does work (because the register and ring->current_slot contain the same
value at this point), I'd prefer if you write ring->current_slot - 1
to the stop-index-register.

Also, the comment needs to be updated.

PS: I'll send a patch that renames set_current_rxslot() later, because it's really confusing.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/30d76e76/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

New response since my phone answered with html

On Sun, 21 Apr 2013, Michael B?sch wrote:

> Hmmm. While this does work (because the register and ring->current_slot contain the same
> value at this point), I'd prefer if you write ring->current_slot - 1
> to the stop-index-register.
The reason for the interrupt is because the current index in the device is
the same as the last index. That's why I read the value from the device. But
I fix it, don't have a strong opinion about it.

> Also, the comment needs to be updated
Crap, missed that.

/thommy

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 20:39:49 +0200
Thommy Jakobsson <thommyj@gmail.com> wrote:

> On Apr 21, 2013 8:26 PM, "Michael B?sch" <m@bues.ch> wrote:
> >
> > On Sun, 21 Apr 2013 20:07:39 +0200 (CEST)
> > Thommy Jakobsson <thommyj@gmail.com> wrote:
> >
> > > +void b43_dma_rx_discard(struct b43_dmaring *ring)
> > > +{
> > > +     int current_slot, previous_slot;
> > > +
> > > +     B43_WARN_ON(ring->tx);
> > > +
> > > +     /* Device has filled all buffers, drop all packets in buffers
> > > +     * and let TCP decrease speed.
> > > +     * Set index to one desc after the last one
> > > +     * so the device will see all slots as free again
> > > +     */
> > > +     /*
> > > +     *TODO: How to increase rx_drop in mac80211?
> > > +     */
> > > +     current_slot = ring->ops->get_current_rxslot(ring);
> > > +     previous_slot = prev_slot(ring, current_slot);
> > > +     ring->ops->set_current_rxslot(ring, previous_slot);
> >
> > Hmmm. While this does work (because the register and ring->current_slot
> contain the same
> > value at this point), I'd prefer if you write ring->current_slot - 1
> > to the stop-index-register.
> The reason for the interrupt is because the current index in the device is
> the same as the last index. That's why I read the value from the device.
> But I fix it, don't have a strong opinion about it.

Hm, ok. Well, sounds plausible. So you may keep it as-is.

But you should probably rename the function to b43_dma_handle_rx_overflow(),
because the discard does not work properly, if the function is called without
an overflow in progress.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/a18e0cde/attachment-0001.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

> Hm, ok. Well, sounds plausible. So you may keep it as-is.
> 
> But you should probably rename the function to b43_dma_handle_rx_overflow(),
> because the discard does not work properly, if the function is called without
> an overflow in progress.
Check, better?

//Thommy

Subject:[PATCH] B43: Handle DMA RX descriptor underrun
From: Thommy Jakobsson <thommyj@gmail.com>

Add handling of rx descriptor underflow. This fixes a fault that could
happen on slow machines, where data is received faster than the CPU can
handle. In such a case the device will use up all rx descriptors and
refuse to send any more data before confirming that it is ok. This
patch enables necessary interrupt to discover such a situation and will
handle them by dropping everything in the ring buffer.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
---
 drivers/net/wireless/b43/b43.h  |    1 +
 drivers/net/wireless/b43/dma.c  |   19 +++++++++++++++++
 drivers/net/wireless/b43/dma.h  |    4 +++-
 drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
 4 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 7f3d461..fdcd00f 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -680,6 +680,7 @@ struct b43_noise_calculation {
 
 struct b43_stats {
 	u8 link_noise;
+	u32 rxdesc_underruns;
 };
 
 struct b43_key {
diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 1221469..84af9e1 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -1733,6 +1733,25 @@ drop_recycle_buffer:
 	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
 
+void b43_dma_handle_rx_overflow(struct b43_dmaring *ring)
+{
+	int current_slot, previous_slot;
+
+	B43_WARN_ON(ring->tx);
+
+	/* Device has filled all buffers, drop all packets and let TCP
+	 * decrease speed.
+	 * Decrement RX index by one will let the device to see all slots
+	 * as free again
+	 */
+	/*
+	*TODO: How to increase rx_drop in mac80211?
+	*/
+	current_slot = ring->ops->get_current_rxslot(ring);
+	previous_slot = prev_slot(ring, current_slot);
+	ring->ops->set_current_rxslot(ring, previous_slot);
+}
+
 void b43_dma_rx(struct b43_dmaring *ring)
 {
 	const struct b43_dma_ops *ops = ring->ops;
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 9fdd198..df8c8cd 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -9,7 +9,7 @@
 /* DMA-Interrupt reasons. */
 #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
 					 | (1 << 14) | (1 << 15))
-#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
+#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
 #define B43_DMAIRQ_RX_DONE		(1 << 16)
 
 /*** 32-bit DMA Engine. ***/
@@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
 			     const struct b43_txstatus *status);
 
+void b43_dma_handle_rx_overflow(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);
 
 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index d377f77..618bb03 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1902,30 +1902,18 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		}
 	}
 
-	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
-					  B43_DMAIRQ_NONFATALMASK))) {
-		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
-			b43err(dev->wl, "Fatal DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-			b43err(dev->wl, "This device does not support DMA "
+	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
+		b43err(dev->wl,
+			"Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X\n",
+			dma_reason[0], dma_reason[1],
+			dma_reason[2], dma_reason[3],
+			dma_reason[4], dma_reason[5]);
+		b43err(dev->wl, "This device does not support DMA "
 			       "on your system. It will now be switched to PIO.\n");
-			/* Fall back to PIO transfers if we get fatal DMA errors! */
-			dev->use_pio = true;
-			b43_controller_restart(dev, "DMA error");
-			return;
-		}
-		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-			b43err(dev->wl, "DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-		}
+		/* Fall back to PIO transfers if we get fatal DMA errors! */
+		dev->use_pio = true;
+		b43_controller_restart(dev, "DMA error");
+		return;
 	}
 
 	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1944,6 +1932,11 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		handle_irq_noise(dev);
 
 	/* Check the DMA reason registers for received data. */
+	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+		if (B43_DEBUG)
+			b43warn(dev->wl, "RX descriptor underrun\n");
+		b43_dma_handle_rx_overflow(dev->dma.rx_ring);
+	}
 	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
 		if (b43_using_pio_transfers(dev))
 			b43_pio_rx(dev->pio.rx_queue);
@@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
 		return IRQ_NONE;
 
 	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-	    & 0x0001DC00;
+	    & 0x0001FC00;
 	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
 	    & 0x0000DC00;
 	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
 		b43_write32(dev, 0x018C, 0x02000000);
 	}
 	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
 	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
-- 
1.7.9.5

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 22:13:43 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

>  struct b43_stats {
>  	u8 link_noise;
> +	u32 rxdesc_underruns;

Why do we add that? It's not incremented and used anywhere.

The rest looks good.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/8eb7d860/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Thommy Jakobsson]

On Sun, 21 Apr 2013, Michael B?sch wrote:

> >  struct b43_stats {
> >  	u8 link_noise;
> > +	u32 rxdesc_underruns;
> 
> Why do we add that? It's not incremented and used anywhere.
> 
Sorry, seems like I dont do that of a good joob revewing my own patches. 
It slipped in from one of my debug builts. Should I send in this to John 
or do you think we have to test it further? It has been running during the 
entire night with high load on my device. So that seems to work, but that 
is only on one device of course.
//Thommy


Subject:[PATCH] B43: Handle DMA RX descriptor underrun
From: Thommy Jakobsson <thommyj@gmail.com>

Add handling of rx descriptor underflow. This fixes a fault that could
happen on slow machines, where data is received faster than the CPU can
handle. In such a case the device will use up all rx descriptors and
refuse to send any more data before confirming that it is ok. This
patch enables necessary interrupt to discover such a situation and will
handle them by dropping everything in the ring buffer.

Signed-off-by: Thommy Jakobsson <thommyj@gmail.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/net/wireless/b43/dma.c  |   19 +++++++++++++++++
 drivers/net/wireless/b43/dma.h  |    4 +++-
 drivers/net/wireless/b43/main.c |   43 ++++++++++++++++-----------------------
 3 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 1221469..ee3d640 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -1733,6 +1733,25 @@ drop_recycle_buffer:
 	sync_descbuffer_for_device(ring, dmaaddr, ring->rx_buffersize);
 }
 
+void b43_dma_handle_rx_overflow(struct b43_dmaring *ring)
+{
+	int current_slot, previous_slot;
+
+	B43_WARN_ON(ring->tx);
+
+	/* Device has filled all buffers, drop all packets and let TCP
+	 * decrease speed.
+	 * Decrement RX index by one will let the device to see all slots
+	 * as free again
+	 */
+	/*
+	*TODO: How to increase rx_drop in mac80211?
+	*/
+	current_slot = ring->ops->get_current_rxslot(ring);
+	previous_slot = prev_slot(ring, current_slot);
+	ring->ops->set_current_rxslot(ring, previous_slot);
+}
+
 void b43_dma_rx(struct b43_dmaring *ring)
 {
 	const struct b43_dma_ops *ops = ring->ops;
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 9fdd198..df8c8cd 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -9,7 +9,7 @@
 /* DMA-Interrupt reasons. */
 #define B43_DMAIRQ_FATALMASK	((1 << 10) | (1 << 11) | (1 << 12) \
 					 | (1 << 14) | (1 << 15))
-#define B43_DMAIRQ_NONFATALMASK	(1 << 13)
+#define B43_DMAIRQ_RDESC_UFLOW		(1 << 13)
 #define B43_DMAIRQ_RX_DONE		(1 << 16)
 
 /*** 32-bit DMA Engine. ***/
@@ -295,6 +295,8 @@ int b43_dma_tx(struct b43_wldev *dev,
 void b43_dma_handle_txstatus(struct b43_wldev *dev,
 			     const struct b43_txstatus *status);
 
+void b43_dma_handle_rx_overflow(struct b43_dmaring *ring);
+
 void b43_dma_rx(struct b43_dmaring *ring);
 
 void b43_dma_direct_fifo_rx(struct b43_wldev *dev,
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index d377f77..6dd07e2 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1902,30 +1902,18 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		}
 	}
 
-	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK |
-					  B43_DMAIRQ_NONFATALMASK))) {
-		if (merged_dma_reason & B43_DMAIRQ_FATALMASK) {
-			b43err(dev->wl, "Fatal DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-			b43err(dev->wl, "This device does not support DMA "
+	if (unlikely(merged_dma_reason & (B43_DMAIRQ_FATALMASK))) {
+		b43err(dev->wl,
+			"Fatal DMA error: 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X, 0x%08X\n",
+			dma_reason[0], dma_reason[1],
+			dma_reason[2], dma_reason[3],
+			dma_reason[4], dma_reason[5]);
+		b43err(dev->wl, "This device does not support DMA "
 			       "on your system. It will now be switched to PIO.\n");
-			/* Fall back to PIO transfers if we get fatal DMA errors! */
-			dev->use_pio = true;
-			b43_controller_restart(dev, "DMA error");
-			return;
-		}
-		if (merged_dma_reason & B43_DMAIRQ_NONFATALMASK) {
-			b43err(dev->wl, "DMA error: "
-			       "0x%08X, 0x%08X, 0x%08X, "
-			       "0x%08X, 0x%08X, 0x%08X\n",
-			       dma_reason[0], dma_reason[1],
-			       dma_reason[2], dma_reason[3],
-			       dma_reason[4], dma_reason[5]);
-		}
+		/* Fall back to PIO transfers if we get fatal DMA errors! */
+		dev->use_pio = true;
+		b43_controller_restart(dev, "DMA error");
+		return;
 	}
 
 	if (unlikely(reason & B43_IRQ_UCODE_DEBUG))
@@ -1944,6 +1932,11 @@ static void b43_do_interrupt_thread(struct b43_wldev *dev)
 		handle_irq_noise(dev);
 
 	/* Check the DMA reason registers for received data. */
+	if (dma_reason[0] & B43_DMAIRQ_RDESC_UFLOW) {
+		if (B43_DEBUG)
+			b43warn(dev->wl, "RX descriptor underrun\n");
+		b43_dma_handle_rx_overflow(dev->dma.rx_ring);
+	}
 	if (dma_reason[0] & B43_DMAIRQ_RX_DONE) {
 		if (b43_using_pio_transfers(dev))
 			b43_pio_rx(dev->pio.rx_queue);
@@ -2001,7 +1994,7 @@ static irqreturn_t b43_do_interrupt(struct b43_wldev *dev)
 		return IRQ_NONE;
 
 	dev->dma_reason[0] = b43_read32(dev, B43_MMIO_DMA0_REASON)
-	    & 0x0001DC00;
+	    & 0x0001FC00;
 	dev->dma_reason[1] = b43_read32(dev, B43_MMIO_DMA1_REASON)
 	    & 0x0000DC00;
 	dev->dma_reason[2] = b43_read32(dev, B43_MMIO_DMA2_REASON)
@@ -3130,7 +3123,7 @@ static int b43_chip_init(struct b43_wldev *dev)
 		b43_write32(dev, 0x018C, 0x02000000);
 	}
 	b43_write32(dev, B43_MMIO_GEN_IRQ_REASON, 0x00004000);
-	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001DC00);
+	b43_write32(dev, B43_MMIO_DMA0_IRQ_MASK, 0x0001FC00);
 	b43_write32(dev, B43_MMIO_DMA1_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA2_IRQ_MASK, 0x0000DC00);
 	b43_write32(dev, B43_MMIO_DMA3_IRQ_MASK, 0x0001DC00);
-- 
1.7.9.5

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Mon, 22 Apr 2013 10:11:41 +0200 (CEST)
Thommy Jakobsson <thommyj@gmail.com> wrote:

> On Sun, 21 Apr 2013, Michael B?sch wrote:
> 
> > >  struct b43_stats {
> > >  	u8 link_noise;
> > > +	u32 rxdesc_underruns;
> > 
> > Why do we add that? It's not incremented and used anywhere.
> > 
> Sorry, seems like I dont do that of a good joob revewing my own patches. 
> It slipped in from one of my debug builts. Should I send in this to John 
> or do you think we have to test it further? It has been running during the 
> entire night with high load on my device. So that seems to work, but that 
> is only on one device of course.

Please remove rxdesc_underruns and send the patch (properly signed-off)
to John Linville. Cc the linux-wireless mailinglist, b43-dev mailinglist
and probably everybody involved in the review discussion.

You can also add a
Reviewed-by: Michael Buesch <m@bues.ch>

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130422/48db6429/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Mon, 22 Apr 2013 12:00:47 +0200
Michael B?sch <m@bues.ch> wrote:

> On Mon, 22 Apr 2013 10:11:41 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
> 
> > On Sun, 21 Apr 2013, Michael B?sch wrote:
> > 
> > > >  struct b43_stats {
> > > >  	u8 link_noise;
> > > > +	u32 rxdesc_underruns;
> > > 
> > > Why do we add that? It's not incremented and used anywhere.
> > > 
> > Sorry, seems like I dont do that of a good joob revewing my own patches. 
> > It slipped in from one of my debug builts. Should I send in this to John 
> > or do you think we have to test it further? It has been running during the 
> > entire night with high load on my device. So that seems to work, but that 
> > is only on one device of course.
> 
> Please remove rxdesc_underruns and send the patch (properly signed-off)
> to John Linville. Cc the linux-wireless mailinglist, b43-dev mailinglist
> and probably everybody involved in the review discussion.

whoops I forgot. Please also add
Cc: stable at vger.kernel.org
to the signed-off list


-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130423/e1c123eb/attachment.sig>

[PATCH] b43: use rx desc underrun interrupt

[Piotras]

Michael,

Based on BCM440X Programmer's Reference Guide that may or may not
apply to DMA processor used in WLAN chipsets, B43_DMA32_RXSTATUS may
be updated by hardware to point to the same descriptor as
B43_DMA32_RXINDEX.

For DMA processor this means that no more free descriptors are
available. However b43_dma_rx assumes no data needs to be processed
(see how ring->current_slot is updated and used in loop condition on
next interrupt). This could explain stalls.

I don't expect that changing loop condition is sufficient, as without
dropping some packets we will not prevent FIFO overruns.


Regards,

Piotr

On Sun, Apr 21, 2013 at 7:38 AM, Michael B?sch <m@bues.ch> wrote:
> On Sat, 20 Apr 2013 23:23:54 +0200 (CEST)
> Thommy Jakobsson <thommyj@gmail.com> wrote:
>
>> Isn't that exactly what we do in b43_dma_rx? The initial rx descriptor
>> index is just to make the device to start. Theoretically you could get
>> into a fault situation if the device succeds in using up all descriptors
>> between b43_dma_rx_discard (added by my patch) and the call to b43_dma_rx.
>> Because then you could start reading the same packet as the device is
>> writing to.
>
> Yes this is true.
> And thus I'm currently unsure why we need this patch at all.
> _Why_ does the DMA stall as soon as the ring is filled up?
>
> --
> Michael

[PATCH] b43: use rx desc underrun interrupt

[Michael Büsch]

On Sun, 21 Apr 2013 15:27:49 +0100
Piotras <piotras@gmail.com> wrote:

> Based on BCM440X Programmer's Reference Guide that may or may not
> apply to DMA processor used in WLAN chipsets, B43_DMA32_RXSTATUS may
> be updated by hardware to point to the same descriptor as
> B43_DMA32_RXINDEX.
> 
> For DMA processor this means that no more free descriptors are
> available. However b43_dma_rx assumes no data needs to be processed
> (see how ring->current_slot is updated and used in loop condition on
> next interrupt). This could explain stalls.

Yes this is why it stalls. But how to fix this without additional counting?
I'd rather prefer wiping the whole ring in that case.

-- 
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20130421/9341395e/attachment.sig>