Hi Seth, all, > [points 1-5, go to the previous email to read them] All of these issues, btw, are, I think, shortcomings of the way powersave and scanning is currently implemented in mac80211. First, I think it is instructive to compare it to HW scan/powersave: mac80211 can transmit/receive quite normally, process everything pretty much just as it normally would, etc. Queues might be stopped by the driver while it's scanning or become full quicker while, but fundamentally that's not really different from regular operation. Now let's go back to the software case: the real cause of all of these problems is that mac80211 does the scan/PS TX via the regular TX mechanism. This is *wrong*, it's not a regular TX that should be subjected to stopped queues etc., it's *special*. For this reason we can't properly stop the queues, and thus the saga of all these bugs begins, with flushing, etc. I actually think that our (your) time would be much better spent implementing a new powersave/scan/offchannel module that, in a way, sits between the driver and mac80211. Using that module, drivers could implement the HW scan and HW offchannel callbacks, and powersave. Obviously, all of this could be implemented directly in mac80211, with if()s etc. However, I think splitting it out would actually create a cleaner and easier to understand solution -- I'm hoping you might agree by the end of this email :-) So let's see what such a module would look like. I'm going to call it "swpso" for "software Powersave/Scan/Offchannel" for now. Obviously, it has to provide helper functions to implement * hw_scan * cancel_hw_scan * remain_on_channel * cancel_remain_on_channel If we allow this module to have a single pointer in "struct ieee80211_hw" (which we can, it should be logically part of mac80211) then we can even have the module export functions that can directly be assigned, so let's say it does that: int ieee80211_swpso_scan(...) // prototype matching hw_scan // etc. Then drivers can just do .hw_scan = ieee80211_swpso_scan, .remain_on_channel = ieee80211_swpso_roc, etc. The module also needs to provide a function that the driver can call in the TX path, for dynamic powersave. This would adjust the timers in the TX path etc. Additionally, the module might require queue control wrappers, so drivers would call ieee80211_swpso_stop_queue() and ieee80211_swpso_wake_queue() instead of the normal versions, obviously it would forward there, but only after some own internal checking. So how would this work? Let's consider remain-on-channel, instead of scan, since scan is really just a bunch of remain-on-channels one after another. On receiving ROC, it would first call mac80211 to stop all queues (except the, possibly entirely virtual, offchannel queue!), and internally mark the queues as SWPSO-stopped. It also has to continually maintain driver-stopped, hence the wrappers I talked about earlier. If the queues are SWPSO-stopped, and the driver wakes it, this wake doesn't propagate to mac80211. This really works much like __ieee80211_wake_queue() and it could actually be combined into that function since SWPSO would be a part of mac80211. Then drivers also wouldn't need the wrappers here, that seems sensible. Then, it would flush all queues in the driver, to make sure that nothing is there. (1) After that, it would call ->tx() to send out any nulldata frame(s). Since the queues are empty, this must succeed (2). If the flush caused the driver to wake a queue because it was stopped, this won't propagate to mac80211, where it would cause all kinds of issues. Now switch channel, obviously. This is ROC, so SWPSO has done its job. Any packets that now get transmitted on the offchannel queue by mac80211 are transmitted normally, on the normal queue, by the driver (it needs a bit of extra logic for that). If it was scanning, it would now also transmit the probe request. Upon finishing with the channel, it just has to switch the channel back, send nulldata frames and re-enable the queues. Ok and now that I've typed it all up, I think it points to how we can do it much more easily without a separate module and without really changing the drivers. Do a few things: * introduce IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL * stop all queues, using the new reason, before going offchannel * then flush * then *directly* tell the driver to transmit the nulldata packets, checking only if the driver has the queue stopped, in which case we have IEEE80211_QUEUE_STOP_REASON_DRIVER set; if it does, fail going off channel * similarly, *directly* tell the driver to transmit probe requests and offchannel packets from cfg80211, if it fails (stop reason driver) then do nothing (probe request) or return a failure to userspace (offchannel TX) In fact the last two bullets should probably be implemented by passing some sort of "offchan_tx_ok" to ieee80211_tx_skb() and friends, and checking queue_stop_reasons more carefully in ieee80211_tx_frags(). I'll address powersave another day, but it also needs major rework. I also think that the framework should somehow better integrate with multi-channel, although I'm not sure anyone really wants to implement that in software. It should be possible with some HW though, if it has enough queues etc. johannes (1) Actually ... this whole queue stopping business could be improved even further, if the driver has a hardware queue it can dedicate to the transmissions we need here. Then, the driver could stop the *hardware* queues, possibly leaving frames sitting on them. However, for ROC/scan this really isn't interesting. It becomes interesting only for SW multi-channel. It also cannot be implemented with all drivers, certainly not with USB devices I think. (2) I think there's a single driver that only accepts one packet at a time, but that's not a big deal, since it also only support a single virtual interface -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
