Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Johannes Berg wrote: > I'm a bit conflicted about this. If we add mgd_prepare_tx() here, what's > to say that we don't need to also do this for a variety of other frames? > Like sending delBA action frames? I guess the difference would be that > we don't disassoc immediately in those case. Deauth was the main problem with ath9k, I didn't check delba or other mgmt frames. > However, if you consider deauth/flush while you're on a different > context, then mgd_prepare_tx() would potentially not really be > sufficient either. If that forces or waits for a context switch, you > really have no guarantee that the deauth frame went out because it might > be waiting behind other frames, and there's nothing that guarantees > enough time in the context slice to actually send out all of them ... It works with ath9k now because all non-data frames are sent immediately, without being added to the internal SW queue. > In that sense, I think you still have to implement flush. Yes, I think we might have to. :) > But I also think you see it wrongly - you don't have to force a context > switch in flush, flush can essentially be "wait for queue to be empty" > and the context switching etc. happens in the background. flush() implementation in ath9k currently waits for the HW queues and the SW queues assigned to the current channel context to become empty. A context switch can be forced to expedite the process of draining the queues in all contexts and we probably have to send out a new NoA if a GO is active to accommodate the longer absence period when flushing. The fact that, in ath9k, the same flush routine is called from the main context scheduler and the mac80211 callback makes things hairy. :) But, I agree (and Emmanuel was right), flush() in ath9k needs to be reworked. Please drop this patch, I'll carry it internally until flush is fixed. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
On Thu, 2014-09-18 at 11:19 +0530, Sujith Manoharan wrote: > I can see your point and agree that the original intention of the > mgd_prepare_tx() > callback is as described in the documentation. But with multiple active > contexts, flush() becomes a big hammer rather than a simple emptying of > the queues. :-) I'm a bit conflicted about this. If we add mgd_prepare_tx() here, what's to say that we don't need to also do this for a variety of other frames? Like sending delBA action frames? I guess the difference would be that we don't disassoc immediately in those case. However, if you consider deauth/flush while you're on a different context, then mgd_prepare_tx() would potentially not really be sufficient either. If that forces or waits for a context switch, you really have no guarantee that the deauth frame went out because it might be waiting behind other frames, and there's nothing that guarantees enough time in the context slice to actually send out all of them ... In that sense, I think you still have to implement flush. But I also think you see it wrongly - you don't have to force a context switch in flush, flush can essentially be "wait for queue to be empty" and the context switching etc. happens in the background. mgd_prepare_tx() is, the way I see it, really more for the case where you really have no context scheduling *at all* and have to somehow send a frame anyway. In the case we're talking about you have regular context scheduling, and there shouldn't be a need to add a special scheduling on top. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Emmanuel Grumbach wrote: > I still think it does not match the original intent of mgd_prepare_tx(). > The way I see it, mgd_prepare_tx() ensures that the Tx *will* happen > in cases the driver has no specific reason to be on channel, mainly > because the context is not associated yet. Note that after association > mac80211 does not interfere with the context switches. The driver is in > charge of all this - why would it be different in this case? > flush() makes sure that the Tx *has* happened before destroying things. > The context switches from association until destruction are under the driver's > responsibility. > > At least this is the way I see it. > We (Intel) will not break if you add this call, but in my eyes this does not > comply with the design. I guess you can always send a patch to add it and > see what others will say, but Johannes is away right now. > If you choose to do that, please add a reason as a parameter to > mgd_prepare_tx() > so that we can ignore it when it is called before sending a frame that > is flush()'ed. > I still prefer to rely on flush(). And I agree that flush() isn't a > trivial implementation. I can see your point and agree that the original intention of the mgd_prepare_tx() callback is as described in the documentation. But with multiple active contexts, flush() becomes a big hammer rather than a simple emptying of the queues. :-) I think we can extend the intention of the API and use mgd_prepare_tx() for deauth/disassoc too. And this was done in the original patch, anyway. This would greatly reduce driver complexity. I'll send a patch including your suggestion about adding a parameter to mgd_prepare_tx() which iwlwifi can use. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Hi, On Thu, Sep 18, 2014 at 7:13 AM, Sujith Manoharan wrote: > Sujith Manoharan wrote: >> I don't think trying to switch channel contexts in the flush() callback >> is a good idea. We would be changing the meaning of the callback if >> we do that. Moreover flush() is called from many other places too, like the >> PS code. >> >> It seems appropriate to instruct a driver to make the necessary >> preparations before sending a deauth frame. Do you see any problems >> if mgd_prepare_tx() is called before initiating deauth ? > > Calling mgd_prepare_tx() in ieee80211_send_deauth_disassoc(), which > was the case originally, is probably more correct, since there are other > situations where this will be a problem. > > CSA is one example. The driver can receive a beacon, mac80211 begins to > process the CSA and if multiple contexts are active, initiates a disconnect > operation. But, in the meantime, if the driver has switched to a different > context, things will break. > I still think it does not match the original intent of mgd_prepare_tx(). The way I see it, mgd_prepare_tx() ensures that the Tx *will* happen in cases the driver has no specific reason to be on channel, mainly because the context is not associated yet. Note that after association mac80211 does not interfere with the context switches. The driver is in charge of all this - why would it be different in this case? flush() makes sure that the Tx *has* happened before destroying things. The context switches from association until destruction are under the driver's responsibility. At least this is the way I see it. We (Intel) will not break if you add this call, but in my eyes this does not comply with the design. I guess you can always send a patch to add it and see what others will say, but Johannes is away right now. If you choose to do that, please add a reason as a parameter to mgd_prepare_tx() so that we can ignore it when it is called before sending a frame that is flush()'ed. I still prefer to rely on flush(). And I agree that flush() isn't a trivial implementation. Note that the only reason I implemented flush() is because of 11w that forces us to make sure that the deauth is sent, otherwise you'd be kicked by the AP after 3 cycles of association / disassociation. So not sending the deauth from time to time isn't really a big deal in my eyes, but one can argue about that (just like about pretty much everything) :) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Sujith Manoharan wrote: > I don't think trying to switch channel contexts in the flush() callback > is a good idea. We would be changing the meaning of the callback if > we do that. Moreover flush() is called from many other places too, like the > PS code. > > It seems appropriate to instruct a driver to make the necessary > preparations before sending a deauth frame. Do you see any problems > if mgd_prepare_tx() is called before initiating deauth ? Calling mgd_prepare_tx() in ieee80211_send_deauth_disassoc(), which was the case originally, is probably more correct, since there are other situations where this will be a problem. CSA is one example. The driver can receive a beacon, mac80211 begins to process the CSA and if multiple contexts are active, initiates a disconnect operation. But, in the meantime, if the driver has switched to a different context, things will break. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Emmanuel Grumbach wrote: > mac80211 will not wait until the context switch happen specifically, > but if you can wait > in the driver in the flush() callback until the deauth is being sent - > whatever you need to > happen to have that done, context switch, TX queues to move etc... - > then you should be good? > IIRC, mac80211 sends the deauth to the driver and then calls flush() - > only then, it'll remove the > contexts. > > So I guess that we agree - the solution for you I guess is to wait > until you have context switch > in the flush() callback. I don't think trying to switch channel contexts in the flush() callback is a good idea. We would be changing the meaning of the callback if we do that. Moreover flush() is called from many other places too, like the PS code. It seems appropriate to instruct a driver to make the necessary preparations before sending a deauth frame. Do you see any problems if mgd_prepare_tx() is called before initiating deauth ? Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
On Wed, Sep 17, 2014 at 9:17 AM, Sujith Manoharan wrote: > Grumbach, Emmanuel wrote: >> It can wait until a context switch will occur in ath9k - you should switch >> from time to time anyway to avoid starvation, and this is not under the radar >> of mac80211. > > But mac80211 doesn't wait for a switch to happen. It removes the BSS and > station > info immediately after sending the deauth frame to the driver and the driver > doesn't > get a chance to process the pending frames for the other context. I think > this race could be avoided by making sure the driver switches to the > required context before sending deauth. > mac80211 will not wait until the context switch happen specifically, but if you can wait in the driver in the flush() callback until the deauth is being sent - whatever you need to happen to have that done, context switch, TX queues to move etc... - then you should be good? IIRC, mac80211 sends the deauth to the driver and then calls flush() - only then, it'll remove the contexts. So I guess that we agree - the solution for you I guess is to wait until you have context switch in the flush() callback. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Grumbach, Emmanuel wrote: > It can wait until a context switch will occur in ath9k - you should switch > from time to time anyway to avoid starvation, and this is not under the radar > of mac80211. But mac80211 doesn't wait for a switch to happen. It removes the BSS and station info immediately after sending the deauth frame to the driver and the driver doesn't get a chance to process the pending frames for the other context. I think this race could be avoided by making sure the driver switches to the required context before sending deauth. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
> > Grumbach, Emmanuel wrote: > > I see - we also had this problem and because of this, we implemented > > the flush callback that allows to wait until all the queues are empty. > > So mac80211 will send the deauth, flush and only then it'd remove the > > VIF. This should allow the frame to make it to the air. Note that API > > explicitly states that mgd_prepare_tx is before assoc only. > > flush() doesn't force a context switch, no ? No it doesn't. > > For example, consider a P2P-GO in freq 2412 and STA in 5180 and both are > active. > The current context is GO (2412) and at this time if a local deauth for the > STA > comes from userspace, mac80211 sends a deauth frame to the driver and > calls flush(), but the context in the driver is still in channel 2412, so how > will > this work ? > It can wait until a context switch will occur in ath9k - you should switch from time to time anyway to avoid starvation, and this is not under the radar of mac80211. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Grumbach, Emmanuel wrote: > I see - we also had this problem and because of this, we implemented the flush > callback that allows to wait until all the queues are empty. So mac80211 will > send the deauth, flush and only then it'd remove the VIF. This should allow > the frame to make it to the air. Note that API explicitly states that > mgd_prepare_tx is before assoc only. flush() doesn't force a context switch, no ? For example, consider a P2P-GO in freq 2412 and STA in 5180 and both are active. The current context is GO (2412) and at this time if a local deauth for the STA comes from userspace, mac80211 sends a deauth frame to the driver and calls flush(), but the context in the driver is still in channel 2412, so how will this work ? Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
> > Grumbach, Emmanuel wrote: > > Why are mgmt frames different from data frames with respect to that? > > You have a separated queue for mgmt frames that is common to all the > contexts? > > Just like an "offchannel" queue? > > No, ath9k treats them the same, but if the current, active context is > different > from the incoming frame's assigned context, then it is added to a software > queue for deferred transmission. But, before a switch to the next context > can happen, mac80211 proceeds to tear down the BSS/stations etc. and the > VIF/context is also removed. > I see - we also had this problem and because of this, we implemented the flush callback that allows to wait until all the queues are empty. So mac80211 will send the deauth, flush and only then it'd remove the VIF. This should allow the frame to make it to the air. Note that API explicitly states that mgd_prepare_tx is before assoc only. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Grumbach, Emmanuel wrote: > Why are mgmt frames different from data frames with respect to that? > You have a separated queue for mgmt frames that is common to all the contexts? > Just like an "offchannel" queue? No, ath9k treats them the same, but if the current, active context is different from the incoming frame's assigned context, then it is added to a software queue for deferred transmission. But, before a switch to the next context can happen, mac80211 proceeds to tear down the BSS/stations etc. and the VIF/context is also removed. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
> > Sujith Manoharan wrote: > > From: Sujith Manoharan > > > > With drivers that support multi-channel concurrency, make sure that > > the correct context is in use before sending a deauth frame. The > > driver could be in a different context, in which case the frame could > > be dropped. > > The behavior appears to have been changed in: > > commit 8c7d857c4a4a552d8d3e1b2e24e1864ec2989285 > Author: Emmanuel Grumbach > Date: Wed Jul 25 01:42:36 2012 +0300 > > mac80211: don't call mgd_prepare_tx when associated > > > I am not sure how it can be guaranteed that the driver is on the right context > when trying to send a mgmt frame. > ath9k uses mgd_prepare_tx() to switch to the required context, so I think > this call is necessary even if we are associated. > Why are mgmt frames different from data frames with respect to that? You have a separated queue for mgmt frames that is common to all the contexts? Just like an "offchannel" queue? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication
Sujith Manoharan wrote: > From: Sujith Manoharan > > With drivers that support multi-channel concurrency, > make sure that the correct context is in use before > sending a deauth frame. The driver could be in a different > context, in which case the frame could be dropped. The behavior appears to have been changed in: commit 8c7d857c4a4a552d8d3e1b2e24e1864ec2989285 Author: Emmanuel Grumbach Date: Wed Jul 25 01:42:36 2012 +0300 mac80211: don't call mgd_prepare_tx when associated I am not sure how it can be guaranteed that the driver is on the right context when trying to send a mgmt frame. ath9k uses mgd_prepare_tx() to switch to the required context, so I think this call is necessary even if we are associated. Sujith -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
