| Message ID | 1390227670-19030-6-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 20 January 2014 15:21, Michal Kazior <michal.kazior@tieto.com> wrote: > The patch improves channel switch related locking > (STA, IBSS, AP, mesh). > > Now read access to sdata->vif.csa_active is > protected by wdev.mtx and local->mtx so holding > either is enough for read access but both are > required for write access. Keep in mind sdata lock > must be taken before local->mtx. Taking them in > reverse order creates a deadlock situation. > > The only exception is ieee80211_beacon_get_tim() > but it's safe to leave it as is and it doesn't > influence mac80211 state in any way. > > The patch adds a few lockdep assertions along for > easier code/locking maintenance. > > This also prepares for multi-interface CSA. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > --- > net/mac80211/cfg.c | 22 +++++++++++++++++++--- > net/mac80211/ibss.c | 18 ++++++++++++++---- > net/mac80211/iface.c | 28 ++++++++++++++++++++++++++-- > net/mac80211/mesh.c | 18 ++++++++++++++++-- > net/mac80211/mlme.c | 20 ++++++++++++++------ > 5 files changed, 89 insertions(+), 17 deletions(-) > [..] > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > index bfb81cb..d898dc9 100644 > --- a/net/mac80211/mlme.c > +++ b/net/mac80211/mlme.c > @@ -1987,10 +1988,9 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata) > u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; > > sdata_lock(sdata); > - if (!ifmgd->associated) { > - sdata_unlock(sdata); > - return; > - } > + mutex_lock(&sdata->local->mtx); > + if (!ifmgd->associated) > + goto out; > > ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, > WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY, Just noticed ths deadlocks STA CSA failpath. This deadlocks with ieee80211_set_disassoc() -> ieee80211_stop_poll() and mutex around ieee80211_vif_release_channel(). I suppose s/ieee80211_stop_poll/__ieee80211_stop/ and removing explicit local->mtx lock around ieee80211_vif_relase_channel() should be enough. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote: > The patch improves channel switch related locking > (STA, IBSS, AP, mesh). > > Now read access to sdata->vif.csa_active is > protected by wdev.mtx and local->mtx so holding > either is enough for read access but both are > required for write access. Keep in mind sdata lock > must be taken before local->mtx. Taking them in > reverse order creates a deadlock situation. > > The only exception is ieee80211_beacon_get_tim() > but it's safe to leave it as is and it doesn't > influence mac80211 state in any way. > > The patch adds a few lockdep assertions along for > easier code/locking maintenance. --- > This also prepares for multi-interface CSA. There's only one or two lines for that preparation as far as I can tell, but I think you should separate it (or maybe make that part of another patch)... or did I miss something? I mean this: > + sdata->csa_chandef = params.chandef; doesn't seem to belong here. It would also be good to explain why you're doing this? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21 January 2014 16:06, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote: >> The patch improves channel switch related locking >> (STA, IBSS, AP, mesh). >> >> Now read access to sdata->vif.csa_active is >> protected by wdev.mtx and local->mtx so holding >> either is enough for read access but both are >> required for write access. Keep in mind sdata lock >> must be taken before local->mtx. Taking them in >> reverse order creates a deadlock situation. >> >> The only exception is ieee80211_beacon_get_tim() >> but it's safe to leave it as is and it doesn't >> influence mac80211 state in any way. >> >> The patch adds a few lockdep assertions along for >> easier code/locking maintenance. > --- >> This also prepares for multi-interface CSA. > > There's only one or two lines for that preparation as far as I can tell, > but I think you should separate it (or maybe make that part of another > patch)... or did I miss something? True. I'll remove the sentence. > I mean this: > >> + sdata->csa_chandef = params.chandef; > > doesn't seem to belong here. I mistakenly left that while rebasing. I'll fix that. I think that's the only thing that shouldn't be in this patch? > It would also be good to explain why you're doing this? You mean the whole locking patch? Basically to be able safely assess CSA state. Taking a bunch of wdev->mtx seems like a bad idea. Due to how the locks are ordered and organized I've came up with using local->mtx as an extra CSA-related variable protection. Using local->mtx makes it possible to iterate over interfaces and check for/update CSA state in an atomic way. Is this a satisfactory explanation? Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 07:51 +0100, Michal Kazior wrote: > > I mean this: > > > >> + sdata->csa_chandef = params.chandef; > > > > doesn't seem to belong here. > > I mistakenly left that while rebasing. I'll fix that. I think that's > the only thing that shouldn't be in this patch? It's the only thing I found, but ... :) > > It would also be good to explain why you're doing this? > > You mean the whole locking patch? Basically to be able safely assess > CSA state. Taking a bunch of wdev->mtx seems like a bad idea. Due to > how the locks are ordered and organized I've came up with using > local->mtx as an extra CSA-related variable protection. Using > local->mtx makes it possible to iterate over interfaces and check > for/update CSA state in an atomic way. > > Is this a satisfactory explanation? Almost - where exactly do you need that? I'm just trying to weigh this patch vs. the other that I didn't like to see if this is needed regardless of the particular approach of how we switch channels later. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 January 2014 09:52, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2014-01-22 at 07:51 +0100, Michal Kazior wrote: > >> > I mean this: >> > >> >> + sdata->csa_chandef = params.chandef; >> > >> > doesn't seem to belong here. >> >> I mistakenly left that while rebasing. I'll fix that. I think that's >> the only thing that shouldn't be in this patch? > > It's the only thing I found, but ... :) > >> > It would also be good to explain why you're doing this? >> >> You mean the whole locking patch? Basically to be able safely assess >> CSA state. Taking a bunch of wdev->mtx seems like a bad idea. Due to >> how the locks are ordered and organized I've came up with using >> local->mtx as an extra CSA-related variable protection. Using >> local->mtx makes it possible to iterate over interfaces and check >> for/update CSA state in an atomic way. >> >> Is this a satisfactory explanation? > > Almost - where exactly do you need that? I'm just trying to weigh this > patch vs. the other that I didn't like to see if this is needed > regardless of the particular approach of how we switch channels later. Hmm.. ieee80211_check_concurrent_iface() checks for vif.csa_active so I assumed it should be protected by a lock too. But now I'm questioning if it should check csa_active at all? It's just an interface type change. You can't change iftype of a running (i.e. connected/associated/beaconing) interface, right? If interface isn't actually running it doesn't use a channel context, so it is fine to change iftype regardless of CSA state. What should actually be forbidden is to bind to a channel context that is a part of a CSA (which I do in the other patch). local->mtx is also used in the later patch to lock out conflicting (current impl. just assumes concurrent = conflicting) CSA requests. But I wonder (assuming we forege csa_active check in ieee80211_check_concurrent_iface) - if we move CSA to be more chanctx centric it might actually make more sense to depend on chanctx_mtx instead of local->mtx, no? Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 10:07 +0100, Michal Kazior wrote: > > Almost - where exactly do you need that? I'm just trying to weigh this > > patch vs. the other that I didn't like to see if this is needed > > regardless of the particular approach of how we switch channels later. > > Hmm.. ieee80211_check_concurrent_iface() checks for vif.csa_active so > I assumed it should be protected by a lock too. But now I'm > questioning if it should check csa_active at all? It's just an > interface type change. You can't change iftype of a running (i.e. > connected/associated/beaconing) interface, right? If interface isn't > actually running it doesn't use a channel context, so it is fine to > change iftype regardless of CSA state. What should actually be > forbidden is to bind to a channel context that is a part of a CSA > (which I do in the other patch). That seems pretty much fine, though I was talking to Luca yesterday and there is actually a case where a CSA chanctx should be usable later, for example if you have this: * managed mode interface receives a channel switch announcements and acts on it * it also sends an event to userspace (this is a TODO but we're on it) * wpa_s decides that a concurrent GO should also switch In that case we'd want them to share the "new" chanctx that it's being switched into, so the rules may need to be a bit different. However, for the non-chanctx case, I completely agree, and we should probably invent a 'fake' or 'pending' chanctx for that like I had discussed in another email yesterday. > local->mtx is also used in the later patch to lock out conflicting > (current impl. just assumes concurrent = conflicting) CSA requests. > But I wonder (assuming we forege csa_active check in > ieee80211_check_concurrent_iface) - if we move CSA to be more chanctx > centric it might actually make more sense to depend on chanctx_mtx > instead of local->mtx, no? Well, not sure. You still would have to check for each interface that it's not already doing a CSA (two CSAs on the same interface seem odd), but that's an early check? And beyond that, why would two CSAs conflict? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 January 2014 10:13, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2014-01-22 at 10:07 +0100, Michal Kazior wrote: > >> > Almost - where exactly do you need that? I'm just trying to weigh this >> > patch vs. the other that I didn't like to see if this is needed >> > regardless of the particular approach of how we switch channels later. >> >> Hmm.. ieee80211_check_concurrent_iface() checks for vif.csa_active so >> I assumed it should be protected by a lock too. But now I'm >> questioning if it should check csa_active at all? It's just an >> interface type change. You can't change iftype of a running (i.e. >> connected/associated/beaconing) interface, right? If interface isn't >> actually running it doesn't use a channel context, so it is fine to >> change iftype regardless of CSA state. What should actually be >> forbidden is to bind to a channel context that is a part of a CSA >> (which I do in the other patch). > > That seems pretty much fine, though I was talking to Luca yesterday and > there is actually a case where a CSA chanctx should be usable later, for > example if you have this: > * managed mode interface receives a channel switch announcements and > acts on it > * it also sends an event to userspace (this is a TODO but we're on it) > * wpa_s decides that a concurrent GO should also switch Hmm. This sounds interesting. But it makes me wonder.. If STA iface receives CSA it starts it right away? It doesn't care if other interfaces are using a given channel context too? It just assumes other intefaces may actively request channel switch and if they do, the CSA will be 'aggregated/merged'? What about cfg80211 intf combinations? If GO sends a channel switch it will fail on num_available_channels unless you implicitly consider the userspace event you mentioned or export CSA state down to cfg80211. At one point I was wondering if channel contexts should be actually exposed in cfg80211. It would probably make it easier to coordinate some CSA scenarios like this in a sane way, don't you think? But then again that's quite big of a change. > In that case we'd want them to share the "new" chanctx that it's being > switched into, so the rules may need to be a bit different. > > However, for the non-chanctx case, I completely agree, and we should > probably invent a 'fake' or 'pending' chanctx for that like I had > discussed in another email yesterday. > >> local->mtx is also used in the later patch to lock out conflicting >> (current impl. just assumes concurrent = conflicting) CSA requests. >> But I wonder (assuming we forege csa_active check in >> ieee80211_check_concurrent_iface) - if we move CSA to be more chanctx >> centric it might actually make more sense to depend on chanctx_mtx >> instead of local->mtx, no? > > Well, not sure. You still would have to check for each interface that > it's not already doing a CSA (two CSAs on the same interface seem odd), > but that's an early check? And beyond that, why would two CSAs conflict? That was just my assumption for a simplified/initial implementation. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 11:13 +0100, Michal Kazior wrote: > > That seems pretty much fine, though I was talking to Luca yesterday and > > there is actually a case where a CSA chanctx should be usable later, for > > example if you have this: > > * managed mode interface receives a channel switch announcements and > > acts on it > > * it also sends an event to userspace (this is a TODO but we're on it) > > * wpa_s decides that a concurrent GO should also switch > > Hmm. This sounds interesting. But it makes me wonder.. > > If STA iface receives CSA it starts it right away? It doesn't care if > other interfaces are using a given channel context too? It just > assumes other intefaces may actively request channel switch and if > they do, the CSA will be 'aggregated/merged'? Well, in Luca's implementation a new channel context has to be available. In the current implementation, it will simply disconnect if using chanctx, or if there's any other interface using the channel (I believe) > What about cfg80211 intf combinations? If GO sends a channel switch it > will fail on num_available_channels unless you implicitly consider the > userspace event you mentioned or export CSA state down to cfg80211. Yeah, that's a good point, need to consider that. > At one point I was wondering if channel contexts should be actually > exposed in cfg80211. It would probably make it easier to coordinate > some CSA scenarios like this in a sane way, don't you think? But then > again that's quite big of a change. Yeah that'd be a bit of a change, but maybe it's worth it? Not sure. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 11:19 +0100, Johannes Berg wrote: > On Wed, 2014-01-22 at 11:13 +0100, Michal Kazior wrote: > > > > That seems pretty much fine, though I was talking to Luca yesterday and > > > there is actually a case where a CSA chanctx should be usable later, for > > > example if you have this: > > > * managed mode interface receives a channel switch announcements and > > > acts on it > > > * it also sends an event to userspace (this is a TODO but we're on it) > > > * wpa_s decides that a concurrent GO should also switch > > > > Hmm. This sounds interesting. But it makes me wonder.. > > > > If STA iface receives CSA it starts it right away? It doesn't care if > > other interfaces are using a given channel context too? It just > > assumes other intefaces may actively request channel switch and if > > they do, the CSA will be 'aggregated/merged'? > > Well, in Luca's implementation a new channel context has to be > available. In the current implementation, it will simply disconnect if > using chanctx, or if there's any other interface using the channel (I > believe) Yes, that's right. Unless the channel context we're going to switch to already exists. Let's say: we have a STA in channel 1 and a GO in channel 11; the STA receives a CSA from its AP to change to channel 11; the STA interface "reserves" the channel 11 context. The context already exists, so we don't need a new one and, since we reserve it, it will remain available, even if the GO goes away from it. I don't think we should try to merge the channel switches. We should just perform them separately, especially because the exact time of the switch will most likely not be the same (since the TBTTs are not in sync). In the STA and GO sharing the context case, the idea is that the STA interface will send a notification to userspace and let the userspace take action. In this case, the userspace will trigger a GO CSA as well. The userspace will decide the exact timing of the GO CSA (it should be as close as possible to the original STA CSA). The userspace will probably tell the GO to switch to the same context as the STA is switching to, so both will end up reserving the same channel context. The non-chanctx case needs to be considered separately. As Johannes already mentioned, I simply ignored non-chanctx in my patch for now. :P > > What about cfg80211 intf combinations? If GO sends a channel switch it > > will fail on num_available_channels unless you implicitly consider the > > userspace event you mentioned or export CSA state down to cfg80211. > > Yeah, that's a good point, need to consider that. Very good point. I hadn't thought about it. When mac80211 decides to change channel to an existing context, it needs to know whether the combination will be valid or not. > > At one point I was wondering if channel contexts should be actually > > exposed in cfg80211. It would probably make it easier to coordinate > > some CSA scenarios like this in a sane way, don't you think? But then > > again that's quite big of a change. > > Yeah that'd be a bit of a change, but maybe it's worth it? Not sure. That's probably going to be necessary. How would mac80211 be able to know if the future combination would be possible or not? I thought about asking cfg80211 if the combination would be okay when reserving a context, but that wouldn't work if we have >= 2 reservations. cfg80211 needs to know about the reservations as well. :( -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > I don't think we should try to merge the channel switches. We should > just perform them separately, especially because the exact time of the > switch will most likely not be the same (since the TBTTs are not in > sync). Do you mean that we shouldn't even have all that new API to switch multiple interfaces simultaneously? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > > > I don't think we should try to merge the channel switches. We should > > just perform them separately, especially because the exact time of the > > switch will most likely not be the same (since the TBTTs are not in > > sync). > > Do you mean that we shouldn't even have all that new API to switch > multiple interfaces simultaneously? Right, I'm not really sure it's necessary. PErhaps with non-chanctx we need something like that, but maybe it would still be better not to do this in the nl80211 API, but sync/merge in cfg80211/mac80211? -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: >> >> > I don't think we should try to merge the channel switches. We should >> > just perform them separately, especially because the exact time of the >> > switch will most likely not be the same (since the TBTTs are not in >> > sync). >> >> Do you mean that we shouldn't even have all that new API to switch >> multiple interfaces simultaneously? > > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we > need something like that, but maybe it would still be better not to do > this in the nl80211 API, but sync/merge in cfg80211/mac80211? I was thinking about it. This should work, mostly, as long as you're able to submit CSA requests fast enough and you don't use count 0 or 1, in which case it becomes racy. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: > On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > >> > >> > I don't think we should try to merge the channel switches. We should > >> > just perform them separately, especially because the exact time of the > >> > switch will most likely not be the same (since the TBTTs are not in > >> > sync). > >> > >> Do you mean that we shouldn't even have all that new API to switch > >> multiple interfaces simultaneously? > > > > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we > > need something like that, but maybe it would still be better not to do > > this in the nl80211 API, but sync/merge in cfg80211/mac80211? > > I was thinking about it. This should work, mostly, as long as you're > able to submit CSA requests fast enough and you don't use count 0 or > 1, in which case it becomes racy. CSA with count 0 or 1 are really tricky in many respects. But still I don't see why it would get racy. The interfaces will switch independently, making their own chanctx reservations and so on... -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 January 2014 13:36, Luca Coelho <luca@coelho.fi> wrote: > On Wed, 2014-01-22 at 11:19 +0100, Johannes Berg wrote: >> On Wed, 2014-01-22 at 11:13 +0100, Michal Kazior wrote: [...] >> > What about cfg80211 intf combinations? If GO sends a channel switch it >> > will fail on num_available_channels unless you implicitly consider the >> > userspace event you mentioned or export CSA state down to cfg80211. >> >> Yeah, that's a good point, need to consider that. > > Very good point. I hadn't thought about it. When mac80211 decides to > change channel to an existing context, it needs to know whether the > combination will be valid or not. > > >> > At one point I was wondering if channel contexts should be actually >> > exposed in cfg80211. It would probably make it easier to coordinate >> > some CSA scenarios like this in a sane way, don't you think? But then >> > again that's quite big of a change. >> >> Yeah that'd be a bit of a change, but maybe it's worth it? Not sure. > > That's probably going to be necessary. How would mac80211 be able to > know if the future combination would be possible or not? I thought about > asking cfg80211 if the combination would be okay when reserving a > context, but that wouldn't work if we have >= 2 reservations. cfg80211 > needs to know about the reservations as well. :( You could probably get away without exposing channel contexts to cfg80211 as long as you at least provide some level of channel switching state back to cfg80211. You could have a 'channel switch state' which says a channel X will become channel Y for A,B,C.. interfaces "soon". mac80211 (or any cfg80211-based driver for that matter) could tell cfg80211 "the switch is complete" (and possibly a status code to indicate failure). This way cfg80211 would be the one to stop/disconnect interfaces that were on channel X but weren't included in a given channel switch to channel Y. I think this should work with >= 2 reservations including error handling (i.e. failing in the middle). What bothers me here is locking. You can't just use wdev->mtx -- you need a more global lock for protecting this sort of thing. Since the reservation would be accessible from userspace (even implicitly via some existing nl80211 commands) and from cfg80211-based (i.e. mac80211) it might be tricky? But I suppose having channel contexts in cfg80211 would involve this predicament as well. Also currently cfg80211 is oblivious to HT40+/HT40- and It doesn't consider HT40+ @ chan6 and HT40- @ chan6 as different channels, right? Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: > On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > >> > >> > I don't think we should try to merge the channel switches. We should > >> > just perform them separately, especially because the exact time of the > >> > switch will most likely not be the same (since the TBTTs are not in > >> > sync). > >> > >> Do you mean that we shouldn't even have all that new API to switch > >> multiple interfaces simultaneously? > > > > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we > > need something like that, but maybe it would still be better not to do > > this in the nl80211 API, but sync/merge in cfg80211/mac80211? > > I was thinking about it. This should work, mostly, as long as you're > able to submit CSA requests fast enough and you don't use count 0 or > 1, in which case it becomes racy. CSA with count 0 or 1 are really tricky in many respects. But still I don't see why it would get racy. The interfaces will switch independently, making their own chanctx reservations and so on... -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: >> >> >> >> > I don't think we should try to merge the channel switches. We should >> >> > just perform them separately, especially because the exact time of the >> >> > switch will most likely not be the same (since the TBTTs are not in >> >> > sync). >> >> >> >> Do you mean that we shouldn't even have all that new API to switch >> >> multiple interfaces simultaneously? >> > >> > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we >> > need something like that, but maybe it would still be better not to do >> > this in the nl80211 API, but sync/merge in cfg80211/mac80211? >> >> I was thinking about it. This should work, mostly, as long as you're >> able to submit CSA requests fast enough and you don't use count 0 or >> 1, in which case it becomes racy. > > CSA with count 0 or 1 are really tricky in many respects. But still I > don't see why it would get racy. The interfaces will switch > independently, making their own chanctx reservations and so on... You can't really switch multiple interfaces independently with a single-channel hardware. You'll either end up with some interfaces disconnected or dragged to a different channel forcefully. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(adding Andrei, since he should be aware of these discussions as well ;) On Thu, 2014-01-23 at 07:41 +0100, Michal Kazior wrote: > On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: > > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: > >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > >> >> > >> >> > I don't think we should try to merge the channel switches. We should > >> >> > just perform them separately, especially because the exact time of the > >> >> > switch will most likely not be the same (since the TBTTs are not in > >> >> > sync). > >> >> > >> >> Do you mean that we shouldn't even have all that new API to switch > >> >> multiple interfaces simultaneously? > >> > > >> > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we > >> > need something like that, but maybe it would still be better not to do > >> > this in the nl80211 API, but sync/merge in cfg80211/mac80211? > >> > >> I was thinking about it. This should work, mostly, as long as you're > >> able to submit CSA requests fast enough and you don't use count 0 or > >> 1, in which case it becomes racy. > > > > CSA with count 0 or 1 are really tricky in many respects. But still I > > don't see why it would get racy. The interfaces will switch > > independently, making their own chanctx reservations and so on... > > You can't really switch multiple interfaces independently with a > single-channel hardware. You'll either end up with some interfaces > disconnected or dragged to a different channel forcefully. Right... I think there's no way around dragging them all forcefully. What about this: when mac80211 sees that one interface is about to switch channel and there are other interfaces in a single-channel device, it automatically starts channel switch on the other interfaces as well (really, there's no way around it). Obviously it needs to inform userspace in this case and then it's up to the userspace to decide what to do (drop the connection, or accept the change). In multi-channel interfaces, we don't switch automatically, but let the userspace decide what to do with the other interfaces. If it doesn't do anything, we end up with each interface on a different channel. Actually this could be generalized too. We don't switch automatically only when we have single-channel interfaces, but when we don't have a free context for the new channel. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I agree with Luca that we need to leave an option to switch multiple interface separately. Suppose the following scenario: BSS_1 and GO_1 on channel X, and BSS_2 and GO_2 on channel Y. Now BSS_1 receives channel switch (count 5 -> channel Z) and the user space decides to move GO_1 (only). Slightly later, when the previous channel switch is still in progress BSS_2 also receives a channel switch, so wpa_s would try to trigger GO_2's CSA, which is impossible because there's already a "merged CSA" in progress. So the best way here is to send a separate CSA command for each interface and let the kernel decide how to perform the switch. The merged API is ok with me, but it should leave the option to switch separately (with little delays). -----Original Message----- From: Luca Coelho [mailto:luca@coelho.fi] Sent: Thursday, January 23, 2014 09:32 To: Michal Kazior Cc: Johannes Berg; linux-wireless; Otcheretianski, Andrei Subject: Re: [PATCH 5/7] mac80211: improve CSA locking (adding Andrei, since he should be aware of these discussions as well ;) On Thu, 2014-01-23 at 07:41 +0100, Michal Kazior wrote: > On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: > > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: > >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > >> >> > >> >> > I don't think we should try to merge the channel switches. We > >> >> > should just perform them separately, especially because the > >> >> > exact time of the switch will most likely not be the same > >> >> > (since the TBTTs are not in sync). > >> >> > >> >> Do you mean that we shouldn't even have all that new API to > >> >> switch multiple interfaces simultaneously? > >> > > >> > Right, I'm not really sure it's necessary. PErhaps with > >> > non-chanctx we need something like that, but maybe it would still > >> > be better not to do this in the nl80211 API, but sync/merge in cfg80211/mac80211? > >> > >> I was thinking about it. This should work, mostly, as long as > >> you're able to submit CSA requests fast enough and you don't use > >> count 0 or 1, in which case it becomes racy. > > > > CSA with count 0 or 1 are really tricky in many respects. But still > > I don't see why it would get racy. The interfaces will switch > > independently, making their own chanctx reservations and so on... > > You can't really switch multiple interfaces independently with a > single-channel hardware. You'll either end up with some interfaces > disconnected or dragged to a different channel forcefully. Right... I think there's no way around dragging them all forcefully. What about this: when mac80211 sees that one interface is about to switch channel and there are other interfaces in a single-channel device, it automatically starts channel switch on the other interfaces as well (really, there's no way around it). Obviously it needs to inform userspace in this case and then it's up to the userspace to decide what to do (drop the connection, or accept the change). In multi-channel interfaces, we don't switch automatically, but let the userspace decide what to do with the other interfaces. If it doesn't do anything, we end up with each interface on a different channel. Actually this could be generalized too. We don't switch automatically only when we have single-channel interfaces, but when we don't have a free context for the new channel. -- Luca. --------------------------------------------------------------------- A member of the Intel Corporation group of companies This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 23 January 2014 08:31, Luca Coelho <luca@coelho.fi> wrote: > (adding Andrei, since he should be aware of these discussions as well ;) > > On Thu, 2014-01-23 at 07:41 +0100, Michal Kazior wrote: >> On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: >> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: >> >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: >> >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: >> >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: >> >> >> >> >> >> > I don't think we should try to merge the channel switches. We should >> >> >> > just perform them separately, especially because the exact time of the >> >> >> > switch will most likely not be the same (since the TBTTs are not in >> >> >> > sync). >> >> >> >> >> >> Do you mean that we shouldn't even have all that new API to switch >> >> >> multiple interfaces simultaneously? >> >> > >> >> > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we >> >> > need something like that, but maybe it would still be better not to do >> >> > this in the nl80211 API, but sync/merge in cfg80211/mac80211? >> >> >> >> I was thinking about it. This should work, mostly, as long as you're >> >> able to submit CSA requests fast enough and you don't use count 0 or >> >> 1, in which case it becomes racy. >> > >> > CSA with count 0 or 1 are really tricky in many respects. But still I >> > don't see why it would get racy. The interfaces will switch >> > independently, making their own chanctx reservations and so on... >> >> You can't really switch multiple interfaces independently with a >> single-channel hardware. You'll either end up with some interfaces >> disconnected or dragged to a different channel forcefully. > > Right... I think there's no way around dragging them all forcefully. > > What about this: when mac80211 sees that one interface is about to > switch channel and there are other interfaces in a single-channel > device, it automatically starts channel switch on the other interfaces > as well (really, there's no way around it). Obviously it needs to > inform userspace in this case and then it's up to the userspace to > decide what to do (drop the connection, or accept the change). This is actually the tricky part. How do you automatically switch an AP interface? You should be able to at least generate CSA IE - true - but what about beacon after CSA? You need to update HT IE and VHT IE at least. Perhaps we could split CSA into two parts somehow and make it more userspace coordinated. The first part of CSA - announcing it, is trivial and can actually be done entirely within mac80211 if I'm not mistaken. What needs userspace input is what happens after the announcement. This would mean that there is a possible time gap between announcement being completed and the actual channel switch & operation resuming. This approach would also help with case of handling CSA to a DFS "usable" channel that needs a CAC first. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote: > On 23 January 2014 08:31, Luca Coelho <luca@coelho.fi> wrote: > > (adding Andrei, since he should be aware of these discussions as well ;) > > > > On Thu, 2014-01-23 at 07:41 +0100, Michal Kazior wrote: > >> On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: > >> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: > >> >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: > >> >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: > >> >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: > >> >> >> > >> >> >> > I don't think we should try to merge the channel switches. We should > >> >> >> > just perform them separately, especially because the exact time of the > >> >> >> > switch will most likely not be the same (since the TBTTs are not in > >> >> >> > sync). > >> >> >> > >> >> >> Do you mean that we shouldn't even have all that new API to switch > >> >> >> multiple interfaces simultaneously? > >> >> > > >> >> > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we > >> >> > need something like that, but maybe it would still be better not to do > >> >> > this in the nl80211 API, but sync/merge in cfg80211/mac80211? > >> >> > >> >> I was thinking about it. This should work, mostly, as long as you're > >> >> able to submit CSA requests fast enough and you don't use count 0 or > >> >> 1, in which case it becomes racy. > >> > > >> > CSA with count 0 or 1 are really tricky in many respects. But still I > >> > don't see why it would get racy. The interfaces will switch > >> > independently, making their own chanctx reservations and so on... > >> > >> You can't really switch multiple interfaces independently with a > >> single-channel hardware. You'll either end up with some interfaces > >> disconnected or dragged to a different channel forcefully. > > > > Right... I think there's no way around dragging them all forcefully. > > > > What about this: when mac80211 sees that one interface is about to > > switch channel and there are other interfaces in a single-channel > > device, it automatically starts channel switch on the other interfaces > > as well (really, there's no way around it). Obviously it needs to > > inform userspace in this case and then it's up to the userspace to > > decide what to do (drop the connection, or accept the change). > > This is actually the tricky part. How do you automatically switch an > AP interface? You should be able to at least generate CSA IE - true - > but what about beacon after CSA? You need to update HT IE and VHT IE > at least. Good point, so instead of doing it automatically, we send a notification that says "switch or die". Then userspace needs to request the switch or drop. > Perhaps we could split CSA into two parts somehow and make it more > userspace coordinated. The first part of CSA - announcing it, is > trivial and can actually be done entirely within mac80211 if I'm not > mistaken. What needs userspace input is what happens after the > announcement. This would mean that there is a possible time gap > between announcement being completed and the actual channel switch & > operation resuming. Yeah, this is similar to what I was thinking about while replying to the other thread. > This approach would also help with case of handling CSA to a DFS > "usable" channel that needs a CAC first. The CAC might take too long. If we have an AP1 and a STA and the STA gets the CSA from its AP2 with a short count, AP1 may not have the time to CAC. In this case, AP1 have two choices: trust that AP2 is doing the right thing and moving to a usable DFS channel or shut itself down. Still, we leave the final decision to the userspace. The only thing mac80211 can know for sure is that all the interfaces in that channel must "switch or die". -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 January 2014 10:50, Luca Coelho <luca@coelho.fi> wrote: > On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote: >> On 23 January 2014 08:31, Luca Coelho <luca@coelho.fi> wrote: >> > (adding Andrei, since he should be aware of these discussions as well ;) >> > >> > On Thu, 2014-01-23 at 07:41 +0100, Michal Kazior wrote: >> >> On 23 January 2014 07:31, Luca Coelho <luca@coelho.fi> wrote: >> >> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote: >> >> >> On 22 January 2014 16:13, Luca Coelho <luca@coelho.fi> wrote: >> >> >> > On Wed, 2014-01-22 at 16:10 +0100, Johannes Berg wrote: >> >> >> >> On Wed, 2014-01-22 at 14:36 +0200, Luca Coelho wrote: >> >> >> >> >> >> >> >> > I don't think we should try to merge the channel switches. We should >> >> >> >> > just perform them separately, especially because the exact time of the >> >> >> >> > switch will most likely not be the same (since the TBTTs are not in >> >> >> >> > sync). >> >> >> >> >> >> >> >> Do you mean that we shouldn't even have all that new API to switch >> >> >> >> multiple interfaces simultaneously? >> >> >> > >> >> >> > Right, I'm not really sure it's necessary. PErhaps with non-chanctx we >> >> >> > need something like that, but maybe it would still be better not to do >> >> >> > this in the nl80211 API, but sync/merge in cfg80211/mac80211? >> >> >> >> >> >> I was thinking about it. This should work, mostly, as long as you're >> >> >> able to submit CSA requests fast enough and you don't use count 0 or >> >> >> 1, in which case it becomes racy. >> >> > >> >> > CSA with count 0 or 1 are really tricky in many respects. But still I >> >> > don't see why it would get racy. The interfaces will switch >> >> > independently, making their own chanctx reservations and so on... >> >> >> >> You can't really switch multiple interfaces independently with a >> >> single-channel hardware. You'll either end up with some interfaces >> >> disconnected or dragged to a different channel forcefully. >> > >> > Right... I think there's no way around dragging them all forcefully. >> > >> > What about this: when mac80211 sees that one interface is about to >> > switch channel and there are other interfaces in a single-channel >> > device, it automatically starts channel switch on the other interfaces >> > as well (really, there's no way around it). Obviously it needs to >> > inform userspace in this case and then it's up to the userspace to >> > decide what to do (drop the connection, or accept the change). >> >> This is actually the tricky part. How do you automatically switch an >> AP interface? You should be able to at least generate CSA IE - true - >> but what about beacon after CSA? You need to update HT IE and VHT IE >> at least. > > Good point, so instead of doing it automatically, we send a notification > that says "switch or die". Then userspace needs to request the switch > or drop. An extreme approach would to actually update AP IEs in mac80211. This way it would be possible to pull any interface into CSA implicitly if you're out of channels. This way single-channel multi-BSS AP could probably switch atomically without races and your multi-channel GO-possibly-follows-STA would still work. Or less intrusively - instead of "drop" just silence beaconing, notify userspace and wait for a channel switch (which would be immediate at this point, it would just take chandef, beacon_after and presp_after from it). It could "drop" if target channel requires CAC. >> Perhaps we could split CSA into two parts somehow and make it more >> userspace coordinated. The first part of CSA - announcing it, is >> trivial and can actually be done entirely within mac80211 if I'm not >> mistaken. What needs userspace input is what happens after the >> announcement. This would mean that there is a possible time gap >> between announcement being completed and the actual channel switch & >> operation resuming. > > Yeah, this is similar to what I was thinking about while replying to the > other thread. > > >> This approach would also help with case of handling CSA to a DFS >> "usable" channel that needs a CAC first. > > The CAC might take too long. If we have an AP1 and a STA and the STA > gets the CSA from its AP2 with a short count, AP1 may not have the time > to CAC. In this case, AP1 have two choices: trust that AP2 is doing the > right thing and moving to a usable DFS channel or shut itself down. That's the point. AP1 doesn't have time to perform CAC in this case, but it should still be possible for AP1 to _just_ beacon CSA as it's only a hint. AP1 could then be stopped to perform CAC, and once it's completed restart the AP1. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 07:35 +0100, Michal Kazior wrote: > Also currently cfg80211 is oblivious to HT40+/HT40- and It doesn't > consider HT40+ @ chan6 and HT40- @ chan6 as different channels, right? Yes, this is a bug I've known about for a long time. I'll buy whoever solves it a beer :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 08:35 +0200, Luca Coelho wrote: > > I was thinking about it. This should work, mostly, as long as you're > > able to submit CSA requests fast enough and you don't use count 0 or > > 1, in which case it becomes racy. > > CSA with count 0 or 1 are really tricky in many respects. But still I > don't see why it would get racy. The interfaces will switch > independently, making their own chanctx reservations and so on... You keep forgetting that Micha? is mostly concerned with the non-chanctx case, in which you can't switch them independently. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote: > This is actually the tricky part. How do you automatically switch an > AP interface? You should be able to at least generate CSA IE - true - > but what about beacon after CSA? You need to update HT IE and VHT IE > at least. > > Perhaps we could split CSA into two parts somehow and make it more > userspace coordinated. The first part of CSA - announcing it, is > trivial and can actually be done entirely within mac80211 if I'm not > mistaken. What needs userspace input is what happens after the > announcement. This would mean that there is a possible time gap > between announcement being completed and the actual channel switch & > operation resuming. I believe that even announcing it isn't necessarily possible without userspace involvement, particularly for things like 11ac's channel switch wrapper IE and/or operating class changes. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 11:33 +0100, Michal Kazior wrote: > An extreme approach would to actually update AP IEs in mac80211. This > way it would be possible to pull any interface into CSA implicitly if > you're out of channels. This way single-channel multi-BSS AP could > probably switch atomically without races and your multi-channel > GO-possibly-follows-STA would still work. I don't really like updating IEs in mac80211 much. Asking userspace shouldn't be *that* much slower. > > The CAC might take too long. If we have an AP1 and a STA and the STA > > gets the CSA from its AP2 with a short count, AP1 may not have the time > > to CAC. In this case, AP1 have two choices: trust that AP2 is doing the > > right thing and moving to a usable DFS channel or shut itself down. > > That's the point. AP1 doesn't have time to perform CAC in this case, > but it should still be possible for AP1 to _just_ beacon CSA as it's > only a hint. AP1 could then be stopped to perform CAC, and once it's > completed restart the AP1. I don't get this side discussion about CAC. There's no time to do CAC in the middle of a CSA *anyway* since you can't be beaconing on the old channel while doing CAC on the new channel, and if you stop beaconing entirely for the time of the CAC then all clients will likely go away... I'm not sure I understand how you can ever do a CSA to a radar channel that would still need CAC? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 January 2014 13:20, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-01-23 at 11:33 +0100, Michal Kazior wrote: > >> An extreme approach would to actually update AP IEs in mac80211. This >> way it would be possible to pull any interface into CSA implicitly if >> you're out of channels. This way single-channel multi-BSS AP could >> probably switch atomically without races and your multi-channel >> GO-possibly-follows-STA would still work. > > I don't really like updating IEs in mac80211 much. Asking userspace > shouldn't be *that* much slower. I'm not really fond the idea either. Though it's not about being slower but about the 'dragging other interfaces along' being possibly automatic when you're out of channels. >> > The CAC might take too long. If we have an AP1 and a STA and the STA >> > gets the CSA from its AP2 with a short count, AP1 may not have the time >> > to CAC. In this case, AP1 have two choices: trust that AP2 is doing the >> > right thing and moving to a usable DFS channel or shut itself down. >> >> That's the point. AP1 doesn't have time to perform CAC in this case, >> but it should still be possible for AP1 to _just_ beacon CSA as it's >> only a hint. AP1 could then be stopped to perform CAC, and once it's >> completed restart the AP1. > > I don't get this side discussion about CAC. There's no time to do CAC in > the middle of a CSA *anyway* since you can't be beaconing on the old > channel while doing CAC on the new channel, and if you stop beaconing > entirely for the time of the CAC then all clients will likely go away... You're right, but.. > I'm not sure I understand how you can ever do a CSA to a radar channel > that would still need CAC? You're supposed to fulfil a uniform channel usage spread which includes "usable" DFS channels. With CSA you're at least able to tell stations to stop Tx and should wait for you (the AP) on a different channel. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 January 2014 13:09, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote: > >> This is actually the tricky part. How do you automatically switch an >> AP interface? You should be able to at least generate CSA IE - true - >> but what about beacon after CSA? You need to update HT IE and VHT IE >> at least. >> >> Perhaps we could split CSA into two parts somehow and make it more >> userspace coordinated. The first part of CSA - announcing it, is >> trivial and can actually be done entirely within mac80211 if I'm not >> mistaken. What needs userspace input is what happens after the >> announcement. This would mean that there is a possible time gap >> between announcement being completed and the actual channel switch & >> operation resuming. > > I believe that even announcing it isn't necessarily possible without > userspace involvement, particularly for things like 11ac's channel > switch wrapper IE and/or operating class changes. Aww, too bad then. But splitting would still be useful (at least in some sense) if you consider CSA to "usable" DFS channels. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 13:38 +0100, Michal Kazior wrote: > On 23 January 2014 13:20, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2014-01-23 at 11:33 +0100, Michal Kazior wrote: > > > >> An extreme approach would to actually update AP IEs in mac80211. This > >> way it would be possible to pull any interface into CSA implicitly if > >> you're out of channels. This way single-channel multi-BSS AP could > >> probably switch atomically without races and your multi-channel > >> GO-possibly-follows-STA would still work. > > > > I don't really like updating IEs in mac80211 much. Asking userspace > > shouldn't be *that* much slower. > > I'm not really fond the idea either. Though it's not about being > slower but about the 'dragging other interfaces along' being possibly > automatic when you're out of channels. Yeah, I agree that updating the IEs in mac80211 is not a good idea. That's why I would prefer to have the notifications to the user space ("your interface must switch channel") and wait for the userspace to request a channel switch (with all the necessary information). > >> > The CAC might take too long. If we have an AP1 and a STA and the STA > >> > gets the CSA from its AP2 with a short count, AP1 may not have the time > >> > to CAC. In this case, AP1 have two choices: trust that AP2 is doing the > >> > right thing and moving to a usable DFS channel or shut itself down. > >> > >> That's the point. AP1 doesn't have time to perform CAC in this case, > >> but it should still be possible for AP1 to _just_ beacon CSA as it's > >> only a hint. AP1 could then be stopped to perform CAC, and once it's > >> completed restart the AP1. > > > > I don't get this side discussion about CAC. There's no time to do CAC in > > the middle of a CSA *anyway* since you can't be beaconing on the old > > channel while doing CAC on the new channel, and if you stop beaconing > > entirely for the time of the CAC then all clients will likely go away... > > You're right, but.. > > > > I'm not sure I understand how you can ever do a CSA to a radar channel > > that would still need CAC? > > You're supposed to fulfil a uniform channel usage spread which > includes "usable" DFS channels. > > With CSA you're at least able to tell stations to stop Tx and should > wait for you (the AP) on a different channel. I think that we should stop transmission in all interfaces that are on a certain channel when that channel receives a CSA with "quiet" mode. I'm not sure this would help your CAC case, but I think it's a wise thing to do to prevent the other interfaces from transmitting. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-01-23 at 13:41 +0100, Michal Kazior wrote: > On 23 January 2014 13:09, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote: > > > >> This is actually the tricky part. How do you automatically switch an > >> AP interface? You should be able to at least generate CSA IE - true - > >> but what about beacon after CSA? You need to update HT IE and VHT IE > >> at least. > >> > >> Perhaps we could split CSA into two parts somehow and make it more > >> userspace coordinated. The first part of CSA - announcing it, is > >> trivial and can actually be done entirely within mac80211 if I'm not > >> mistaken. What needs userspace input is what happens after the > >> announcement. This would mean that there is a possible time gap > >> between announcement being completed and the actual channel switch & > >> operation resuming. > > > > I believe that even announcing it isn't necessarily possible without > > userspace involvement, particularly for things like 11ac's channel > > switch wrapper IE and/or operating class changes. > > Aww, too bad then. > > But splitting would still be useful (at least in some sense) if you > consider CSA to "usable" DFS channels. I don't like the split idea that much. If we send the CS announcement automatically but the userspace never reacts to our indication, we will be stuck in the middle of the channel switch... -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-01-24 at 09:41 +0200, Luca Coelho wrote: > Yeah, I agree that updating the IEs in mac80211 is not a good idea. > That's why I would prefer to have the notifications to the user space > ("your interface must switch channel") and wait for the userspace to > request a channel switch (with all the necessary information). Even in the sentence "your interface must switch channel" you're conflating multiple things though. Consider a managed+AP interface being concurrent, with the managed one receiving CSA from its BSS. 1) maybe it's a radar channel, and the AP detected radar, then we must switch (to make authorities happy); but in this case we should have detected radar as well on the AP we're running... 2) maybe it's a non-radar 5 GHz channel, and the device (like iwlwifi) only permitted beaconing on 5 GHz as long as connected to another AP, then we also must switch (to make the device happy) 3) maybe it's neither, or the device is allowed to use non-radar channels for AP/GO operation, but the AP we're connected to just decided to switch for other implementation-specific reasons, e.g. wanting to use a better channel, *AND* we don't have enough channel contexts to stay, so we "must" switch The same cases exist if, like you suggested, we'd make such a notification when one AP interface starts to switch. I'm still mostly against that though. "[Y]our interface must switch channel" is therefore not very well defined, and I'd hate to see a client interface CSA-started notification interpreted as such; you can see that there's at least one sub-case where other interfaces don't have to switch. It sounds to me like you're still hung up on the whole "ohh, what if there's wpa_s and hostapd running" thing ... I really don't think it's a concern. I'd be rather surprised if this was the only thing that'd break then. The question also is whether we want to handle all the corner cases. Consider this corner case: * non-chanctx driver * running at least one AP/GO * running one station interface connected to BSS1 * BSS1 decides to do a CSA Even today, we'll try to execute the channel switch, even though that completely leaves the AP/GO interface dead in the water and beaconing wrongly. That's something we should probably address. The question is how we should address it. It seems to me that you actually want to handle that case by asking wpa_s to switch the AP/GO interface, but I'm not really sure it's necessary? The question also is how to handle it if wpa_s doesn't respond (e.g. old wpa_s version that has no idea what's going on) - which interface should "lose"? IMHO we don't have to design it to handle all these quirky corner cases. This particular case might be something we want to handle for case 2) above, but who actually cares? The only driver that is looking at case 2 is ours, and that has channel contexts, so we don't really have to handle it with non-chanctx. It's obvious that we need a "CSA started" notification for managed mode interfaces. It's not clear to me that we need it for AP interfaces, since handling multiple AP interfaces with different hostapd instances is stupid to start with, and IMHO this makes the situation more complex. Let's enumerate all the corner cases. Let me know if I missed some. 1. chanctx drivers a) managed mode CSA received with other interfaces present, but no available channel context(s) b) AP CSA requested by userspace with other interfaces present, but no available channel context(s) c) (mesh CSA has similar cases, I believe) d) (IBSS CSA has similar cases for drivers that allow IBSS/other combinations) 2. non-chanctx drivers a) managed mode CSA received with other interfaces present b) AP CSA requested by userspace with other interfaces present c) (mesh) d) (IBSS) (Is that all?) Those cases are (mostly) identical if we treat non-chanctx drivers as having exactly one channel context. I can't really speak for cases c) and d), but would think handling them like the others would make sense. I'd also leave the question of regulatory compliance out of the CSA mechanism entirely. If we discuss the CSA case 2) at the beginning of this email, for example, then we could argue that we should require the CSA or whatever. I say that we should enforce the regulatory stuff after the fact, and stop the AP interface if the managed one is gone for a while/too long from that channel. That can be enforced in cfg80211 fairly easily. With that then, I'd suggest that in case a), the managed mode interface just disconnects, as it does today. There would be a possibility that we could send the "I need to do CSA" event to userspace so it could simultaneously switch the AP to the same channel, but I'm not convinced it's worth it. In case b), I'd say that the CSA should be refused. If needed, wpa_supplicant could have disconnected the managed interfaces (it knows about possible channel combinations), otherwise this will fail and it needs to be prepared for failures anyway (and will likely shut down the GO in that case.) This does, however, imply that we need multi-AP switching like Micha? implemented. I don't really see how else to do it, because if you want to keep this as multiple single-interface switches you (i) can't enforce the switch time and (ii) have to add complex code to handle the case where userspace 'forgets' to switch one of the many interfaces. IMHO the complexity of multi-switch is less than for trying to handle that (ii) case and similar corner cases. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-01-24 at 09:40 +0100, Johannes Berg wrote: > On Fri, 2014-01-24 at 09:41 +0200, Luca Coelho wrote: > > > Yeah, I agree that updating the IEs in mac80211 is not a good idea. > > That's why I would prefer to have the notifications to the user space > > ("your interface must switch channel") and wait for the userspace to > > request a channel switch (with all the necessary information). > > Even in the sentence "your interface must switch channel" you're > conflating multiple things though. Consider a managed+AP interface being > concurrent, with the managed one receiving CSA from its BSS. > > 1) maybe it's a radar channel, and the AP detected radar, then we must > switch > (to make authorities happy); but in this case we should have > detected radar > as well on the AP we're running... Fine, the AP would have detected the radar. Then it gets the "must switch" notification triggered by the managed interface and can sync the count, so both switch at the same time (which must be the case anyway). > 2) maybe it's a non-radar 5 GHz channel, and the device (like iwlwifi) > only > permitted beaconing on 5 GHz as long as connected to another AP, > then we > also must switch (to make the device happy) Check! > 3) maybe it's neither, or the device is allowed to use non-radar > channels for > AP/GO operation, but the AP we're connected to just decided to > switch for > other implementation-specific reasons, e.g. wanting to use a better > channel, > *AND* we don't have enough channel contexts to stay, so we "must" > switch Check! > The same cases exist if, like you suggested, we'd make such a > notification when one AP interface starts to switch. I'm still mostly > against that though. I think we're in a deadlock of I'm pro and you're against. :) > "[Y]our interface must switch channel" is therefore not very well > defined, and I'd hate to see a client interface CSA-started notification > interpreted as such; We don't send the "must switch" notification to the interfaces that *triggered* the channel switch. Either if it was a client-interface-started CSA or if it was a remotely triggered CSA (ie. the AP/GO our managed/client is connected to). We send the notification to the *other* interfaces that are on the same channel. > > you can see that there's at least one sub-case where other > > interfaces don't have to switch. The notification is also just sent if it *must* switch (ie. when there's no extra context available). In this case, even if the "other interfaces don't have to switch", they must switch or die. > It sounds to me like you're still hung up on the whole "ohh, what if > there's wpa_s and hostapd running" thing ... I really don't think it's a > concern. I'd be rather surprised if this was the only thing that'd break > then. Not really. I don't care what is the binary listening to the notifications. I just think it's probably much cleaner to treat the interfaces independently. Let's say hostapd is managing two APs. It decides that AP1 needs to switch channel. At this point it doesn't necessarily know that AP2 must also switch because there are no extra channel contexts available to keep them in separate channels. mac80211 knows and indicates that so that hostapd decides to switch AP2 as well. With the single-CSA-request-for-mutliple-interfaces proposal, it must switch all of them at once even if it *would be* possible to keep them in separate channels, because there's no way of know that beforehand. > The question also is whether we want to handle all the corner cases. > Consider this corner case: > * non-chanctx driver > * running at least one AP/GO > * running one station interface connected to BSS1 > * BSS1 decides to do a CSA > > Even today, we'll try to execute the channel switch, even though that > completely leaves the AP/GO interface dead in the water and beaconing > wrongly. That's something we should probably address. I think today, if we have non-chanctx with more than one vif and the station gets a CSA, we disconnect the station, don't we? > The question is > how we should address it. It seems to me that you actually want to > handle that case by asking wpa_s to switch the AP/GO interface, but I'm > not really sure it's necessary? With non-chanctx or no-more-chanctx-available cases the AP/GO must switch. Or, if hostapd is managing both, it can disconnect the STA, upon receiving the "AP must switch" notification so that the AP doesn't have to change channel (if for whatever reason hostapd thinks the AP has higher priority). > The question also is how to handle it if > wpa_s doesn't respond (e.g. old wpa_s version that has no idea what's > going on) - which interface should "lose"? If it doesn't respond, the AP/GO will get disconnected. That's where the "switch or *die*" comes in. If we're using an old wpa_s, this wouldn't work anyway. With my proposal and a *new* wpa_s, we could do what I suggested above. How would the single-CSA-request-for-multiple-interfaces proposal help in this case? > IMHO we don't have to design it to handle all these quirky corner cases. > This particular case might be something we want to handle for case 2) > above, but who actually cares? The only driver that is looking at case 2 > is ours, and that has channel contexts, so we don't really have to > handle it with non-chanctx. To tell you the truth, I didn't think about this "quirky corner case", but it just happens that my "switch or die notification" proposal would still allow it to be handled. > It's obvious that we need a "CSA started" notification for managed mode > interfaces. It's not clear to me that we need it for AP interfaces, > since handling multiple AP interfaces with different hostapd instances > is stupid to start with, and IMHO this makes the situation more complex. I don't care about the "CSA started" notification for AP interfaces. What I want to see is a "your interface must switch or die" indication when *needed* (ie. with non-chanctx or no-extra-chanctx-available). > Let's enumerate all the corner cases. Let me know if I missed some. > > 1. chanctx drivers > a) managed mode CSA received with other interfaces present, but no > available > channel context(s) > b) AP CSA requested by userspace with other interfaces present, but > no > available channel context(s) > c) (mesh CSA has similar cases, I believe) > d) (IBSS CSA has similar cases for drivers that allow IBSS/other > combinations) > 2. non-chanctx drivers > a) managed mode CSA received with other interfaces present > b) AP CSA requested by userspace with other interfaces present > c) (mesh) > d) (IBSS) > > (Is that all?) I assume that MESH and IBSS are, in the current context, the same as either a) or b), depending on whether our station started or is responding to the channel switch. > Those cases are (mostly) identical if we treat non-chanctx drivers as > having exactly one channel context. Yes. > I can't really speak for cases c) and d), but would think handling them > like the others would make sense. I'd also leave the question of > regulatory compliance out of the CSA mechanism entirely. If we discuss > the CSA case 2) at the beginning of this email, for example, then we > could argue that we should require the CSA or whatever. I say that we > should enforce the regulatory stuff after the fact, and stop the AP > interface if the managed one is gone for a while/too long from that > channel. That can be enforced in cfg80211 fairly easily. Yes, I think we should treat regulatory separately. Regardless of what wpa_s/hostapd decides to do, we can enforce it in cfg80211 when the request is made. > With that then, I'd suggest that in case a), the managed mode interface > just disconnects, as it does today. There would be a possibility that we > could send the "I need to do CSA" event to userspace so it could > simultaneously switch the AP to the same channel, but I'm not convinced > it's worth it. I'd say it's debatable. :D > In case b), I'd say that the CSA should be refused. If needed, > wpa_supplicant could have disconnected the managed interfaces (it knows > about possible channel combinations), otherwise this will fail and it > needs to be prepared for failures anyway (and will likely shut down the > GO in that case.) This does, however, imply that we need multi-AP > switching like Micha? implemented. I don't really see how else to do it, > because if you want to keep this as multiple single-interface switches > you (i) can't enforce the switch time and (ii) have to add complex code > to handle the case where userspace 'forgets' to switch one of the many > interfaces. (i) we should somehow "merge" the switch time. We would have to do this anyway if the switch was triggered via a managed interface (ie. the BSS it's connected to sent the CSA). With either proposals, we would get a notification about this (either the "must switch" or the "CSA started") and would have to request a CSA for the AP interface with the same switch time. (ii) this is very simple: if the userspace "forgets" to switch any of the interfaces, we disconnect it. It's the same as if the userspace would "forget" to include one of the interfaces in the single-CSA-request-with-multiple-interfaces implementation. > IMHO the complexity of multi-switch is less than for trying to handle > that (ii) case and similar corner cases. As I see it, with the single-switch you need to treat lots of different corner cases separately. And force some outcomes (eg. refuse the CSA request in case b). With the multi-switch There Are No Corner Cases[TM]. :P -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 January 2014 10:52, Luca Coelho <luca@coelho.fi> wrote: > On Fri, 2014-01-24 at 09:40 +0100, Johannes Berg wrote: >> On Fri, 2014-01-24 at 09:41 +0200, Luca Coelho wrote: >> >> > Yeah, I agree that updating the IEs in mac80211 is not a good idea. >> > That's why I would prefer to have the notifications to the user space >> > ("your interface must switch channel") and wait for the userspace to >> > request a channel switch (with all the necessary information). >> >> Even in the sentence "your interface must switch channel" you're >> conflating multiple things though. Consider a managed+AP interface being >> concurrent, with the managed one receiving CSA from its BSS. >> >> 1) maybe it's a radar channel, and the AP detected radar, then we must >> switch >> (to make authorities happy); but in this case we should have >> detected radar >> as well on the AP we're running... > > Fine, the AP would have detected the radar. Then it gets the "must > switch" notification triggered by the managed interface and can sync the > count, so both switch at the same time (which must be the case anyway). What if the AP interface receives 'radar detected' before managed interfaces gets CSA? You'll need to consider both cases (either AP wants to switch first, or STA wants it first). (z) I wonder if we could have nl80211 channel_switch also work with STA? This way we could have single-request-for-multi-interface-channel-switch work for GO-follow-STA too, as the STA interface would simply send out "I got a CSA: params.." and wait for either a channel_switch or disconnect after a timeout. The timeout itself would have to be conservative, i.e. possibly longer than beacon_interval x csa count so that userspace has time to make a decision (to prevent disconnects when csa count is small and/or userspace lags a bit due to I/O, etc). This (delaying STA chswitch, i.e. going beyond the bcnint x count threshold) should be safe since you don't really need to channel switch STA so fast (if you lag a few beacons nobody cares). Perhaps mac80211 could even use connection loss work for the timeout itself. All that needs to be taken care of is to lock tx queues on STA. This obviously means you get eventually disconnected with STA interface if you get a CSA without wpa_s running (i.e. `iw connect`. But then again.. I don't think it makes much sense to run your wireless stack _without_ wpa_s. Maybe current STA CSA behaviour could be left for single-interface cases, but I'm not really sure there would be much use for it? The same would have to be applied for IBSS and mesh -- they must not switch implicitly in-kernel upon receiving CSA but notify userspace about it and let userspace make the decision. This also means "switch or die" policy is moved to userspace where it probably should've been from the start. >> The same cases exist if, like you suggested, we'd make such a >> notification when one AP interface starts to switch. I'm still mostly >> against that though. > > I think we're in a deadlock of I'm pro and you're against. :) > > >> "[Y]our interface must switch channel" is therefore not very well >> defined, and I'd hate to see a client interface CSA-started notification >> interpreted as such; > > We don't send the "must switch" notification to the interfaces that > *triggered* the channel switch. Either if it was a > client-interface-started CSA or if it was a remotely triggered CSA (ie. > the AP/GO our managed/client is connected to). We send the notification > to the *other* interfaces that are on the same channel. See (z). > > >> > you can see that there's at least one sub-case where other >> > interfaces don't have to switch. > > The notification is also just sent if it *must* switch (ie. when there's > no extra context available). In this case, even if the "other > interfaces don't have to switch", they must switch or die. Actually you could want to migrate your AP along with STA even if you can operate on two channels. E.g. you might want to maximize performance and such, no? > Let's say hostapd is managing two APs. It decides that AP1 needs to > switch channel. At this point it doesn't necessarily know that AP2 must > also switch because there are no extra channel contexts available to > keep them in separate channels. mac80211 knows and indicates that so > that hostapd decides to switch AP2 as well. With the > single-CSA-request-for-mutliple-interfaces proposal, it must switch all > of them at once even if it *would be* possible to keep them in separate > channels, because there's no way of know that beforehand. You could try to rely on "if (err == -EBUSY) ch_switch_all_the_things()". EBUSY at this point can either mean "iface comb impossible due to iftype/ifnum failure" or "num_diff_chans != available_num_chans". The former shouldn't be true since, well, you already got iftype/ifnum combo running, so you're only left with channel count. Or am I missing something? >> The question also is how to handle it if >> wpa_s doesn't respond (e.g. old wpa_s version that has no idea what's >> going on) - which interface should "lose"? > > If it doesn't respond, the AP/GO will get disconnected. That's where > the "switch or *die*" comes in. If we're using an old wpa_s, this > wouldn't work anyway. With my proposal and a *new* wpa_s, we could do > what I suggested above. > > How would the single-CSA-request-for-multiple-interfaces proposal help > in this case? See (z). Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-01-24 at 11:40 +0100, Michal Kazior wrote: > On 24 January 2014 10:52, Luca Coelho <luca@coelho.fi> wrote: > > On Fri, 2014-01-24 at 09:40 +0100, Johannes Berg wrote: > >> On Fri, 2014-01-24 at 09:41 +0200, Luca Coelho wrote: > >> > >> > Yeah, I agree that updating the IEs in mac80211 is not a good idea. > >> > That's why I would prefer to have the notifications to the user space > >> > ("your interface must switch channel") and wait for the userspace to > >> > request a channel switch (with all the necessary information). > >> > >> Even in the sentence "your interface must switch channel" you're > >> conflating multiple things though. Consider a managed+AP interface being > >> concurrent, with the managed one receiving CSA from its BSS. > >> > >> 1) maybe it's a radar channel, and the AP detected radar, then we must > >> switch > >> (to make authorities happy); but in this case we should have > >> detected radar > >> as well on the AP we're running... > > > > Fine, the AP would have detected the radar. Then it gets the "must > > switch" notification triggered by the managed interface and can sync the > > count, so both switch at the same time (which must be the case anyway). > > What if the AP interface receives 'radar detected' before managed > interfaces gets CSA? You'll need to consider both cases (either AP > wants to switch first, or STA wants it first). Hmmm... This should be okay, the only problem would be that the counts may conflict. If our AP decides to switch at count x and later our station gets a CSA with count y. The station cannot change the the count (since it's chosen externally) and for the AP it's too late to change (since the CSA has already been sent out). > (z) I wonder if we could have nl80211 channel_switch also work with > STA? This way we could have > single-request-for-multi-interface-channel-switch work for > GO-follow-STA too, as the STA interface would simply send out "I got a > CSA: params.." and wait for either a channel_switch or disconnect > after a timeout. I don't really see how this would help. It doesn't really make a difference if we send an "I got a CSA: params" to the userspace, because the userspace can't do anything but comply. It's the same as if we send a "you must switch or die" to the AP. > The timeout itself would have to be conservative, i.e. possibly longer > than beacon_interval x csa count so that userspace has time to make a > decision (to prevent disconnects when csa count is small and/or > userspace lags a bit due to I/O, etc). This (delaying STA chswitch, > i.e. going beyond the bcnint x count threshold) should be safe since > you don't really need to channel switch STA so fast (if you lag a few > beacons nobody cares). Perhaps mac80211 could even use connection loss > work for the timeout itself. All that needs to be taken care of is to > lock tx queues on STA. It's safeish for a station to switch later, but it will start losing packets. If it is not in the new channel by the time the AP told it to be, it will start missing stuff. It's possible for the AP to do some kind of check before starting to transmit to the station again, like waiting for a NULL packet or something, but this is not part of the specs, so we can't rely on it. > This obviously means you get eventually disconnected with STA > interface if you get a CSA without wpa_s running (i.e. `iw connect`. > But then again.. I don't think it makes much sense to run your > wireless stack _without_ wpa_s. Maybe current STA CSA behaviour could > be left for single-interface cases, but I'm not really sure there > would be much use for it? I think this case is not that important to justify complicating things. But still, it can be avoided if we don't do the "I got CSA: params" notification. > The same would have to be applied for IBSS and mesh -- they must not > switch implicitly in-kernel upon receiving CSA but notify userspace > about it and let userspace make the decision. More complications... Especially because with MESH we need to replicate the CSA packets. So would the userspace send two requests for mesh? One to say "it's okay to switch" and the other to say "trigger a switch"? > This also means "switch or die" policy is moved to userspace where it > probably should've been from the start. /me shouts '"switch or die" or die!' :P "switch or die" is not a policy, it's a fact. It's easy for the kernel to identify that and easy for the userspace to make the decision. That's why I think a notification started in the kernel is a good idea. > >> The same cases exist if, like you suggested, we'd make such a > >> notification when one AP interface starts to switch. I'm still mostly > >> against that though. > > > > I think we're in a deadlock of I'm pro and you're against. :) > > > > > >> "[Y]our interface must switch channel" is therefore not very well > >> defined, and I'd hate to see a client interface CSA-started notification > >> interpreted as such; > > > > We don't send the "must switch" notification to the interfaces that > > *triggered* the channel switch. Either if it was a > > client-interface-started CSA or if it was a remotely triggered CSA (ie. > > the AP/GO our managed/client is connected to). We send the notification > > to the *other* interfaces that are on the same channel. > > See (z). > > > > > > > >> > you can see that there's at least one sub-case where other > >> > interfaces don't have to switch. > > > > The notification is also just sent if it *must* switch (ie. when there's > > no extra context available). In this case, even if the "other > > interfaces don't have to switch", they must switch or die. > > Actually you could want to migrate your AP along with STA even if you > can operate on two channels. E.g. you might want to maximize > performance and such, no? Sure, but this is possible only when we have extra contexts. And it can be done later, as long as we have the "channel-switch done" notification that we all seem to agree is needed. No need to switch at the same time. > > Let's say hostapd is managing two APs. It decides that AP1 needs to > > switch channel. At this point it doesn't necessarily know that AP2 must > > also switch because there are no extra channel contexts available to > > keep them in separate channels. mac80211 knows and indicates that so > > that hostapd decides to switch AP2 as well. With the > > single-CSA-request-for-mutliple-interfaces proposal, it must switch all > > of them at once even if it *would be* possible to keep them in separate > > channels, because there's no way of know that beforehand. > > You could try to rely on "if (err == -EBUSY) > ch_switch_all_the_things()". EBUSY at this point can either mean > "iface comb impossible due to iftype/ifnum failure" or "num_diff_chans > != available_num_chans". The former shouldn't be true since, well, you > already got iftype/ifnum combo running, so you're only left with > channel count. Or am I missing something? You would need to rely on the response and keep trying different things until you got it right. The first case (iftype/ifnum) could also happen because there is already another interface of type X in the new channel. We have AP1 and AP2 in channel X and AP3 in channel Y. The maximum number of APs in the same channel is 2. With single-CSA-request, if AP1 needs to move to Y, hostapd would ask both AP1 and AP2 to move to channel Y. Then it would get "EBUSY". How does it know if the problem is the lack of extra context or iftype/ifnum? With a single interface per request this would happen: hostapd asks AP1 to move to channel Y. Channel why can have both AP1 and AP3, so the switch happens and we don't send a "switch or die" to AP2. Everyone is happy. Now if AP2 also needs to switch, hostapd tried to switch to channel Y. It gets EBUSY because of iftype/ifnum. It can then try something else, like moving to channel Z. > >> The question also is how to handle it if > >> wpa_s doesn't respond (e.g. old wpa_s version that has no idea what's > >> going on) - which interface should "lose"? > > > > If it doesn't respond, the AP/GO will get disconnected. That's where > > the "switch or *die*" comes in. If we're using an old wpa_s, this > > wouldn't work anyway. With my proposal and a *new* wpa_s, we could do > > what I suggested above. > > > > How would the single-CSA-request-for-multiple-interfaces proposal help > > in this case? > > See (z). (z) would still not work with older versions of wpa_s. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index ddb1c4d..29692f6 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1053,6 +1053,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, int err; sdata = IEEE80211_DEV_TO_SUB_IF(dev); + sdata_assert_lock(sdata); /* don't allow changing the beacon while CSA is in place - offset * of channel switch counter may change @@ -1080,15 +1081,19 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev) struct probe_resp *old_probe_resp; struct cfg80211_chan_def chandef; + sdata_assert_lock(sdata); + old_beacon = sdata_dereference(sdata->u.ap.beacon, sdata); if (!old_beacon) return -ENOENT; old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata); /* abort any running channel switch */ + mutex_lock(&local->mtx); sdata->vif.csa_active = false; kfree(sdata->u.ap.next_beacon); sdata->u.ap.next_beacon = NULL; + mutex_unlock(&local->mtx); cancel_work_sync(&sdata->u.ap.request_smps_work); @@ -2992,6 +2997,8 @@ static int ieee80211_ap_finish_csa(struct ieee80211_sub_if_data *sdata) { int err; + lockdep_assert_held(&sdata->local->mtx); + err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon); kfree(sdata->u.ap.next_beacon); sdata->u.ap.next_beacon = NULL; @@ -3017,10 +3024,12 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata) struct ieee80211_local *local = sdata->local; int err, changed = 0; - mutex_lock(&local->mtx); + sdata_assert_lock(sdata); + lockdep_assert_held(&local->mtx); + sdata->radar_required = sdata->csa_radar_required; + err = ieee80211_vif_change_channel(sdata, &changed); - mutex_unlock(&local->mtx); if (WARN_ON(err < 0)) return; @@ -3069,6 +3078,8 @@ void ieee80211_csa_finalize_work(struct work_struct *work) csa_finalize_work); sdata_lock(sdata); + mutex_lock(&sdata->local->mtx); + /* AP might have been stopped while waiting for the lock. */ if (!sdata->vif.csa_active) goto unlock; @@ -3079,6 +3090,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work) ieee80211_csa_finalize(sdata); unlock: + mutex_unlock(&sdata->local->mtx); sdata_unlock(sdata); } @@ -3092,7 +3104,8 @@ int __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, struct ieee80211_if_mesh __maybe_unused *ifmsh; int err, num_chanctx, changed = 0; - lockdep_assert_held(&sdata->wdev.mtx); + sdata_assert_lock(sdata); + lockdep_assert_held(&local->mtx); if (!list_empty(&local->roc_list) || local->scanning) return -EBUSY; @@ -3278,8 +3291,11 @@ int ieee80211_channel_switch(struct wiphy *wiphy, return -EOPNOTSUPP; sdata = IEEE80211_DEV_TO_SUB_IF(params[0].dev); + sdata_lock(sdata); + mutex_lock(&sdata->local->mtx); err = __ieee80211_channel_switch(wiphy, params[0].dev, ¶ms[0]); + mutex_unlock(&sdata->local->mtx); sdata_unlock(sdata); return err; diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c index 4854800..003434a 100644 --- a/net/mac80211/ibss.c +++ b/net/mac80211/ibss.c @@ -796,6 +796,12 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata, int err; u32 sta_flags; + sdata_assert_lock(sdata); + lockdep_assert_held(&sdata->local->mtx); + + if (sdata->vif.csa_active) + return true; + sta_flags = IEEE80211_STA_DISABLE_VHT; switch (ifibss->chandef.width) { case NL80211_CHAN_WIDTH_5: @@ -937,8 +943,9 @@ ieee80211_rx_mgmt_spectrum_mgmt(struct ieee80211_sub_if_data *sdata, if (len < required_len) return; - if (!sdata->vif.csa_active) - ieee80211_ibss_process_chanswitch(sdata, elems, false); + mutex_lock(&sdata->local->mtx); + ieee80211_ibss_process_chanswitch(sdata, elems, false); + mutex_unlock(&sdata->local->mtx); } static void ieee80211_rx_mgmt_deauth_ibss(struct ieee80211_sub_if_data *sdata, @@ -1119,9 +1126,12 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata, goto put_bss; /* process channel switch */ - if (sdata->vif.csa_active || - ieee80211_ibss_process_chanswitch(sdata, elems, true)) + mutex_lock(&local->mtx); + if (ieee80211_ibss_process_chanswitch(sdata, elems, true)) { + mutex_unlock(&local->mtx); goto put_bss; + } + mutex_unlock(&local->mtx); /* same BSSID */ if (ether_addr_equal(cbss->bssid, sdata->u.ibss.bssid)) diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 0aa9675..5583987 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -250,14 +250,19 @@ static inline int identical_mac_addr_allowed(int type1, int type2) type2 == NL80211_IFTYPE_AP_VLAN)); } -static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata, - enum nl80211_iftype iftype) +static int +__ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata, + enum nl80211_iftype iftype) { struct ieee80211_local *local = sdata->local; struct ieee80211_sub_if_data *nsdata; ASSERT_RTNL(); + /* access to vif.csa_active must be protected by sdata or local->mtx. + * this interates over interfaces so sdata lock won't do */ + lockdep_assert_held(&local->mtx); + /* we hold the RTNL here so can safely walk the list */ list_for_each_entry(nsdata, &local->interfaces, list) { if (nsdata != sdata && ieee80211_sdata_running(nsdata)) { @@ -308,6 +313,21 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata, return 0; } +static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata, + enum nl80211_iftype iftype) +{ + struct ieee80211_local *local = sdata->local; + int err; + + ASSERT_RTNL(); + + mutex_lock(&local->mtx); + err = __ieee80211_check_concurrent_iface(sdata, iftype); + mutex_unlock(&local->mtx); + + return err; +} + static int ieee80211_check_queues(struct ieee80211_sub_if_data *sdata, enum nl80211_iftype iftype) { @@ -822,7 +842,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, cancel_work_sync(&local->dynamic_ps_enable_work); cancel_work_sync(&sdata->recalc_smps); + sdata_lock(sdata); + mutex_lock(&local->mtx); sdata->vif.csa_active = false; + mutex_unlock(&local->mtx); + sdata_unlock(sdata); cancel_work_sync(&sdata->csa_finalize_work); cancel_delayed_work_sync(&sdata->dfs_cac_timer_work); diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index 98bcd2b..4617bf8 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -864,6 +864,12 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata, int err; u32 sta_flags; + sdata_assert_lock(sdata); + lockdep_assert_held(&sdata->local->mtx); + + if (sdata->vif.csa_active) + return true; + sta_flags = IEEE80211_STA_DISABLE_VHT; switch (sdata->vif.bss_conf.chandef.width) { case NL80211_CHAN_WIDTH_20_NOHT: @@ -933,6 +939,7 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata, return false; ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_REPEATER; + sdata->csa_chandef = params.chandef; if (__ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev, ¶ms) < 0) @@ -1048,9 +1055,11 @@ static void ieee80211_mesh_rx_bcn_presp(struct ieee80211_sub_if_data *sdata, ifmsh->sync_ops->rx_bcn_presp(sdata, stype, mgmt, &elems, rx_status); + mutex_lock(&sdata->local->mtx); if (ifmsh->csa_role != IEEE80211_MESH_CSA_ROLE_INIT && !sdata->vif.csa_active) ieee80211_mesh_process_chnswitch(sdata, &elems, true); + mutex_unlock(&sdata->local->mtx); } int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata) @@ -1148,6 +1157,7 @@ static void mesh_rx_csa_frame(struct ieee80211_sub_if_data *sdata, bool fwd_csa = true; size_t baselen; u8 *pos; + bool csa_ok = true; if (mgmt->u.action.u.measurement.action_code != WLAN_ACTION_SPCT_CHL_SWITCH) @@ -1168,8 +1178,12 @@ static void mesh_rx_csa_frame(struct ieee80211_sub_if_data *sdata, ifmsh->pre_value = pre_value; - if (!sdata->vif.csa_active && - !ieee80211_mesh_process_chnswitch(sdata, &elems, false)) { + mutex_lock(&sdata->local->mtx); + if (!sdata->vif.csa_active) + csa_ok = ieee80211_mesh_process_chnswitch(sdata, &elems, false); + mutex_unlock(&sdata->local->mtx); + + if (!csa_ok) { mcsa_dbg(sdata, "Failed to process CSA action frame"); return; } diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index bfb81cb..d898dc9 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -885,12 +885,11 @@ static void ieee80211_chswitch_work(struct work_struct *work) return; sdata_lock(sdata); + mutex_lock(&local->mtx); if (!ifmgd->associated) goto out; - mutex_lock(&local->mtx); ret = ieee80211_vif_change_channel(sdata, &changed); - mutex_unlock(&local->mtx); if (ret) { sdata_info(sdata, "vif channel switch failed, disconnecting\n"); @@ -923,6 +922,7 @@ static void ieee80211_chswitch_work(struct work_struct *work) out: sdata->vif.csa_active = false; ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED; + mutex_unlock(&local->mtx); sdata_unlock(sdata); } @@ -965,6 +965,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata, int res; sdata_assert_lock(sdata); + lockdep_assert_held(&sdata->local->mtx); if (!cbss) return; @@ -1987,10 +1988,9 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata) u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; sdata_lock(sdata); - if (!ifmgd->associated) { - sdata_unlock(sdata); - return; - } + mutex_lock(&sdata->local->mtx); + if (!ifmgd->associated) + goto out; ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY, @@ -2003,6 +2003,8 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata) cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf, IEEE80211_DEAUTH_FRAME_LEN); +out: + mutex_unlock(&sdata->local->mtx); sdata_unlock(sdata); } @@ -2969,8 +2971,10 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata, ieee80211_rx_bss_info(sdata, mgmt, len, rx_status, &elems); + mutex_lock(&local->mtx); ieee80211_sta_process_chanswitch(sdata, rx_status->mactime, &elems, true); + mutex_unlock(&local->mtx); if (!(ifmgd->flags & IEEE80211_STA_DISABLE_WMM) && ieee80211_sta_wmm_params(local, sdata, elems.wmm_param, @@ -3101,9 +3105,11 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, if (elems.parse_error) break; + mutex_lock(&sdata->local->mtx); ieee80211_sta_process_chanswitch(sdata, rx_status->mactime, &elems, false); + mutex_unlock(&sdata->local->mtx); } else if (mgmt->u.action.category == WLAN_CATEGORY_PUBLIC) { ies_len = skb->len - offsetof(struct ieee80211_mgmt, @@ -3123,9 +3129,11 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, elems.ext_chansw_ie = &mgmt->u.action.u.ext_chan_switch.data; + mutex_lock(&sdata->local->mtx); ieee80211_sta_process_chanswitch(sdata, rx_status->mactime, &elems, false); + mutex_unlock(&sdata->local->mtx); } break; }
The patch improves channel switch related locking (STA, IBSS, AP, mesh). Now read access to sdata->vif.csa_active is protected by wdev.mtx and local->mtx so holding either is enough for read access but both are required for write access. Keep in mind sdata lock must be taken before local->mtx. Taking them in reverse order creates a deadlock situation. The only exception is ieee80211_beacon_get_tim() but it's safe to leave it as is and it doesn't influence mac80211 state in any way. The patch adds a few lockdep assertions along for easier code/locking maintenance. This also prepares for multi-interface CSA. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/cfg.c | 22 +++++++++++++++++++--- net/mac80211/ibss.c | 18 ++++++++++++++---- net/mac80211/iface.c | 28 ++++++++++++++++++++++++++-- net/mac80211/mesh.c | 18 ++++++++++++++++-- net/mac80211/mlme.c | 20 ++++++++++++++------ 5 files changed, 89 insertions(+), 17 deletions(-)