gmane.linux.drivers.bcm54xx.devel

Re: linux-next: Tree for May 6 (b43/bcma)

Larry Finger — 2013-05-07T03:06:23

Does any Kconfig wizard know the magic incantation to accomplish the following:

1. Ensure that if SSB and BMCA are both selected, then both are either "m" or "y"

2. Ensure that BRCMSMAC is not "y" if BMCA is "m".

3. Ensure that B43 is not "y" if SSB and/or BCMA is "m".

Thanks,

Larry

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-23T06:22:35

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


whoops I forgot. Please also add
Cc: stable-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
to the signed-off list

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-22T10:00:47

On Mon, 22 Apr 2013 10:11:41 +0200 (CEST)
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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< at >bues.ch>

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-22T08:11:41


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

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-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>

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-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>
Cc: stable <stabl

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T21:53:13

On Sun, 21 Apr 2013 22:13:43 +0200 (CEST)
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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

The rest looks good.

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T20:13:43

Check, better?

//Thommy

Subject:[PATCH] B43: Handle DMA RX descriptor underrun
From: Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>

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-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>
---
 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..fdc

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T18:44:39

On Sun, 21 Apr 2013 20:39:49 +0200
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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.

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T18:44:24

New response since my phone answered with html

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

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.

Crap, missed that.

/thommy

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T18:26:01

On Sun, 21 Apr 2013 20:07:39 +0200 (CEST)
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> 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.

Also, the comment needs to be updated.

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

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T18:07:39


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

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 whe

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T16:35:14

On Sun, 21 Apr 2013 16:24:18 +0100
Piotras <piotras-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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.

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T16:31:30

On Sun, 21 Apr 2013 17:11:23 +0200 (CEST)
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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

Re: [PATCH] b43: use rx desc underrun interrupt

Piotras — 2013-04-21T15:24:18

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.



Top half interrupt handler (b43_do_interrupt).

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T15:11:23


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 curr

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T14:59:51

On Sun, 21 Apr 2013 15:46:03 +0100
Piotras <piotras-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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


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.


What is "top half"?

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T14:51:42

On Sun, 21 Apr 2013 15:27:49 +0100
Piotras <piotras-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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.

Re: [PATCH] b43: use rx desc underrun interrupt

Piotras — 2013-04-21T14:46:03

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

Re: [PATCH] b43: use rx desc underrun interrupt

Piotras — 2013-04-21T14:27:49

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< at >bues.ch> wrote:

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T12:12:05


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

Check, I'll give index-1 a go. It should be quite obvious if it works or 
not.

//thommy_______________________________________________
b43-dev mailing list
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r< at >public.gmane.org
http://lists.infradead.org/mailman/listinfo/b43-dev

Re: [PATCH] b43: use rx desc underrun interrupt

Michael Büsch — 2013-04-21T09:21:18

On Sun, 21 Apr 2013 11:01:11 +0200 (CEST)
Thommy Jakobsson <thommyj-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


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

Re: [PATCH] b43: use rx desc underrun interrupt

Thommy Jakobsson — 2013-04-21T09:01:11


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

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?

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_______________________________________________
b43-dev mailing list
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r< at >public.gmane.org
http://lists.infradead.org/mai