rework on .flush() callback
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
Hi Arend,
Quoted 8 lines
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 callieee80211_wake_queues(). Due to issues still persisting we went a bit deeper in mac80211.
Quoted 4 lines
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...
Quoted 4 lines
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
Quoted 10 lines · Johannes Berg
Quoted 4 lines
The problem here is that in brcmsmac the flush does the
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 12 lines · Arend van Spriel
Quoted 10 lines · Johannes Berg
Quoted 4 lines
The problem here is that in brcmsmac the flush does the
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
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