rework on .flush() callback

rework on .flush() callback

[Arend van Spriel]

Hi Johannes,

The brcmsmac driver still faces a lot of bug reports related to the 
.flush() callback (see [1] from Dave Jones).

Previously I had looked into this and found that during the flush the 
.tx callback still gets new frames. So I put ieee80211_stop_queues() in 
the flush and upon exit call ieee80211_wake_queues(). Due to issues 
still persisting we went a bit deeper in mac80211.

It turns out that mac80211 stops the netif queues for each interface 
(except monitor iftype) and not the internal queues during a scan. So it 
stops the netif queues, calls the flush(), and upon returning to the 
associated channel it wakes up the netif queues.

The problem here is that in brcmsmac the flush does the 
ieee80211_wake_queues() call, because that could also wakeup the netif 
queues. So doing it in the driver seems a bad idea. Any suggestion on 
how to solve this?

off topic question: boy or girl?

Gr. AvS

[1] http://codemonkey.org.uk/2012/11/09/brcmsmac-bugs/

Re: rework on .flush() callback

[Johannes Berg]

Hi Arend,

> The brcmsmac driver still faces a lot of bug reports related to the 
> .flush() callback (see [1] from Dave Jones).
> 
> Previously I had looked into this and found that during the flush the 
> .tx callback still gets new frames. So I put ieee80211_stop_queues() in 
> the flush and upon exit call ieee80211_wake_queues(). Due to issues 
> still persisting we went a bit deeper in mac80211.
> 
> It turns out that mac80211 stops the netif queues for each interface 
> (except monitor iftype) and not the internal queues during a scan. So it 
> stops the netif queues, calls the flush(), and upon returning to the 
> associated channel it wakes up the netif queues.

Hmm, interesting. Yeah this is for scanning, that software scan stuff is
a bit buggy in this area, I agree. This only really works if there's
nothing on the internal queues, otherwise during flush the driver will
wake the queues and get more packets etc...

> The problem here is that in brcmsmac the flush does the 
> ieee80211_wake_queues() call, because that could also wakeup the netif 
> queues. So doing it in the driver seems a bad idea. Any suggestion on 
> how to solve this?

Yeah so .. I actually thought about this at some point, it's tricky. For
the global queues we check what reasons we had for stopping them, but we
don't do that for the netif queues. Maybe we should? I also think flush
should at least have the option to be per queue, so that we could do
something per sdata in mac80211 if the driver uses different HW queues
for different interfaces.

However, I'm not sure I'll have time to work on corner cases with
software scanning since we don't use that. I might work on the flush
thing though, that could be interesting.

johannes

Re: rework on .flush() callback

[Arend van Spriel]

On 11/16/2012 02:34 PM, Johannes Berg wrote:
>> The problem here is that in brcmsmac the flush does the
>> >ieee80211_wake_queues() call, because that could also wakeup the netif
>> >queues. So doing it in the driver seems a bad idea. Any suggestion on
>> >how to solve this?
> Yeah so .. I actually thought about this at some point, it's tricky. For
> the global queues we check what reasons we had for stopping them, but we
> don't do that for the netif queues. Maybe we should? I also think flush
> should at least have the option to be per queue, so that we could do
> something per sdata in mac80211 if the driver uses different HW queues
> for different interfaces.

To me so from driver perspective, it was not clear what 
ieee80211_stop/wake_queues was operating on. Somehow the knowledge that 
the netif queues are already stopped upon calling 
ieee80211_stop_queues() should be retained.

Regarding the flush do you mean flush per queue or flush per vif? Per 
vif could have its perks, I guess.

> However, I'm not sure I'll have time to work on corner cases with
> software scanning since we don't use that. I might work on the flush
> thing though, that could be interesting.

ok. The drv_flush() call is done in several places so not only scanning. 
All with the drop flag set to false. Is that just to be prepared or do 
you foresee an actual use-case?

Gr. AvS

Re: rework on .flush() callback

[Johannes Berg]

On Fri, 2012-11-16 at 18:08 +0100, Arend van Spriel wrote:
> On 11/16/2012 02:34 PM, Johannes Berg wrote:
> >> The problem here is that in brcmsmac the flush does the
> >> >ieee80211_wake_queues() call, because that could also wakeup the netif
> >> >queues. So doing it in the driver seems a bad idea. Any suggestion on
> >> >how to solve this?
> > Yeah so .. I actually thought about this at some point, it's tricky. For
> > the global queues we check what reasons we had for stopping them, but we
> > don't do that for the netif queues. Maybe we should? I also think flush
> > should at least have the option to be per queue, so that we could do
> > something per sdata in mac80211 if the driver uses different HW queues
> > for different interfaces.
> 
> To me so from driver perspective, it was not clear what 
> ieee80211_stop/wake_queues was operating on. Somehow the knowledge that 
> the netif queues are already stopped upon calling 
> ieee80211_stop_queues() should be retained.

Yes. From the driver perspective, those calls operate on the HW queues
(which you can modify now, using the IEEE80211_HW_QUEUE_CONTROL flag and
associated APIs). But we don't retain the netif queue state separately,
which I agree is a bug.

> Regarding the flush do you mean flush per queue or flush per vif? Per 
> vif could have its perks, I guess.

Not sure. With the IEEE80211_HW_QUEUE_CONTROL per queue might make
sense, and would actually make more sense than per vif in most cases.
However, it'd have to be a bitmap so the flush can happen in parallel.

> > However, I'm not sure I'll have time to work on corner cases with
> > software scanning since we don't use that. I might work on the flush
> > thing though, that could be interesting.
> 
> ok. The drv_flush() call is done in several places so not only scanning. 
> All with the drop flag set to false. Is that just to be prepared or do 
> you foresee an actual use-case?

I think at some point I thought we would use it, but if somebody is
going to change the flush callback (e.g. by passing a hw queue bitmap) I
would suggest to remove the drop flag.

johannes