[PATCH] b43: use rx desc underrun interrupt
Thommy Jakobsson
thommyj at gmail.com
Fri Apr 19 16:30:56 EDT 2013
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 at gmail.com> wrote:
> Hi,
>
> On Mon, Apr 15, 2013 at 10:02 AM, Thommy Jakobsson <thommyj at 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
More information about the b43-dev
mailing list