Hi,
This cleans up some CSA related stuff and notably
tries to fix locking issues.
Now local->mtx is used along with sdata lock to
secure vif->csa_active modification. Addition of
local->mtx makes it possible to check
vif->csa_active on all interfaces safely.
The last patch (the revert) is more of a
suggestion.
I've decided to include the radar patch here as
well.
This patchset is required to support
multi-interface CSA in mac80211.
Changes since RFC:
* rebase on top of mac80211-next and adjust to
some IBSS/mesh changes
* fix typo in commit message of `move
csa_active seting in STA CSA`
* remove `track CSA globally`
* add `deny attempts at using chanctx during CSA`
(it takes a little bit of code from the removed
`track CSA globally` and `implement
multi-interface CSA`)
Michal Kazior (7):
mac80211: fix possible memory leak on AP CSA failure
mac80211: treat IBSS CSA finish failure seriously
mac80211: move csa_active setting in STA CSA
mac80211: fix sdata->radar_required locking
mac80211: improve CSA locking
mac80211: deny attempts at using chanctx during CSA
Revert "cfg80211: disable CSA for all drivers"
net/mac80211/cfg.c | 51 +++++++++++++++++++++++++++++++++++-----------
net/mac80211/chan.c | 7 +++++++
net/mac80211/ibss.c | 20 +++++++++++++-----
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/iface.c | 28 +++++++++++++++++++++++--
net/mac80211/mesh.c | 18 ++++++++++++++--
net/mac80211/mlme.c | 22 +++++++++++++-------
net/mac80211/util.c | 18 ++++++++++++++++
net/wireless/core.c | 6 ------
9 files changed, 137 insertions(+), 34 deletions(-)
--
1.8.4.rc3
On Thu, 2014-01-23 at 13:38 +0100, Michal Kazior wrote:
> On 23 January 2014 13:20, Johannes Berg <[email protected]> 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.
On 23 January 2014 10:50, Luca Coelho <[email protected]> wrote:
> On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote:
>> On 23 January 2014 08:31, Luca Coelho <[email protected]> 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 <[email protected]> wrote:
>> >> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
>> >> >> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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ł
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> The sdata->vif.csa_active could be left set after,
> e.g. channel context constraints check fail in STA
> mode leaving the interface in a strange state for
> a brief period of time until it is disconnected.
> This was harmless but ugly.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> net/mac80211/mlme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index fc1d824..bfb81cb 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -1001,7 +1001,6 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
> }
>
> ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
> - sdata->vif.csa_active = true;
>
> mutex_lock(&local->chanctx_mtx);
> if (local->use_chanctx) {
> @@ -1039,6 +1038,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
> mutex_unlock(&local->chanctx_mtx);
>
> sdata->csa_chandef = csa_ie.chandef;
> + sdata->vif.csa_active = true;
>
> if (csa_ie.mode)
> ieee80211_stop_queues_by_reason(&local->hw,
Looks better indeed.
--
Luca.
This reverts commit dda444d52496aa8ddc501561bca580f1374a96a9.
Re-enable CSA as the locking issues have been solved.
Signed-off-by: Michal Kazior <[email protected]>
---
net/wireless/core.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index d89dee2..aa3def7 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -443,12 +443,6 @@ int wiphy_register(struct wiphy *wiphy)
/* support for 5/10 MHz is broken due to nl80211 API mess - disable */
wiphy->flags &= ~WIPHY_FLAG_SUPPORTS_5_10_MHZ;
- /*
- * There are major locking problems in nl80211/mac80211 for CSA,
- * disable for all drivers until this has been reworked.
- */
- wiphy->flags &= ~WIPHY_FLAG_HAS_CHANNEL_SWITCH;
-
#ifdef CONFIG_PM
if (WARN_ON(wiphy->wowlan &&
(wiphy->wowlan->flags & WIPHY_WOWLAN_GTK_REKEY_FAILURE) &&
--
1.8.4.rc3
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> Other interface modes are checked against failure.
> This should avoid false-positive channel switch
> events where IBSS CSA actually failed.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> net/mac80211/cfg.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 62bf6c4..76fe1f90 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3039,7 +3039,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> return;
> break;
> case NL80211_IFTYPE_ADHOC:
> - ieee80211_ibss_finish_csa(sdata);
> + err = ieee80211_ibss_finish_csa(sdata);
> + if (err < 0)
> + return;
Looks fine, though I suppose this should then perhaps also return a
changed bitmap to be used later. Ditto for mesh, I guess.
johannes
On Thu, 2014-01-23 at 13:41 +0100, Michal Kazior wrote:
> On 23 January 2014 13:09, Johannes Berg <[email protected]> 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.
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
On Tue, 2014-01-21 at 16:00 +0100, Johannes Berg wrote:
> On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> > Other interface modes are checked against failure.
> > This should avoid false-positive channel switch
> > events where IBSS CSA actually failed.
> >
> > Signed-off-by: Michal Kazior <[email protected]>
> > ---
> > net/mac80211/cfg.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> > index 62bf6c4..76fe1f90 100644
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -3039,7 +3039,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> > return;
> > break;
> > case NL80211_IFTYPE_ADHOC:
> > - ieee80211_ibss_finish_csa(sdata);
> > + err = ieee80211_ibss_finish_csa(sdata);
> > + if (err < 0)
> > + return;
>
> Looks fine, though I suppose this should then perhaps also return a
> changed bitmap to be used later. Ditto for mesh, I guess.
Yes, I agree. I actually had started to work on some patches to align
all this, but I'll leave it to Michal. ;)
--
Luca.
On Thu, 2014-01-23 at 08:57 +0100, Michal Kazior wrote:
> On 23 January 2014 08:31, Luca Coelho <[email protected]> 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 <[email protected]> wrote:
> >> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
> >> >> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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.
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> It was possible to connect STA when AP CSA was in
> progress which clearly was a bug.
I understand that preventing this simplifies things, but I don't think
it's a bug. If the CSA process takes a long time (ie. several TBTTs),
why not let a new STA associate meanwhile? The new STA knows when to
switch, since it can see the count in the beacons (and probe_responses).
--
Luca.
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.
The sdata->vif.csa_active could be left set after,
e.g. channel context constraints check fail in STA
mode leaving the interface in a strange state for
a brief period of time until it is disconnected.
This was harmless but ugly.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/mlme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index fc1d824..bfb81cb 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1001,7 +1001,6 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
}
ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
- sdata->vif.csa_active = true;
mutex_lock(&local->chanctx_mtx);
if (local->use_chanctx) {
@@ -1039,6 +1038,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
mutex_unlock(&local->chanctx_mtx);
sdata->csa_chandef = csa_ie.chandef;
+ sdata->vif.csa_active = true;
if (csa_ie.mode)
ieee80211_stop_queues_by_reason(&local->hw,
--
1.8.4.rc3
On 20 January 2014 15:21, Michal Kazior <[email protected]> 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 <[email protected]>
> ---
> 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ł
On Fri, 2014-01-24 at 11:40 +0100, Michal Kazior wrote:
> On 24 January 2014 10:52, Luca Coelho <[email protected]> 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.
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
SSBhZ3JlZSB3aXRoIEx1Y2EgdGhhdCB3ZSBuZWVkIHRvIGxlYXZlIGFuIG9wdGlvbiB0byBzd2l0
Y2ggbXVsdGlwbGUgaW50ZXJmYWNlIHNlcGFyYXRlbHkuDQpTdXBwb3NlIHRoZSBmb2xsb3dpbmcg
c2NlbmFyaW86IEJTU18xIGFuZCBHT18xIG9uIGNoYW5uZWwgWCwgYW5kIEJTU18yIGFuZCBHT18y
IG9uIGNoYW5uZWwgWS4NCk5vdyBCU1NfMSByZWNlaXZlcyBjaGFubmVsIHN3aXRjaCAoY291bnQg
NSAtPiBjaGFubmVsIFopIGFuZCB0aGUgdXNlciBzcGFjZSBkZWNpZGVzIHRvIG1vdmUgR09fMSAo
b25seSkuDQpTbGlnaHRseSBsYXRlciwgd2hlbiB0aGUgcHJldmlvdXMgY2hhbm5lbCBzd2l0Y2gg
aXMgc3RpbGwgaW4gcHJvZ3Jlc3MgQlNTXzIgYWxzbyByZWNlaXZlcyBhIGNoYW5uZWwgc3dpdGNo
LCBzbyB3cGFfcw0Kd291bGQgdHJ5IHRvIHRyaWdnZXIgR09fMidzIENTQSwgd2hpY2ggaXMgaW1w
b3NzaWJsZSBiZWNhdXNlIHRoZXJlJ3MgYWxyZWFkeSBhICJtZXJnZWQgQ1NBIiBpbiBwcm9ncmVz
cy4NCg0KU28gdGhlIGJlc3Qgd2F5IGhlcmUgaXMgdG8gc2VuZCBhIHNlcGFyYXRlIENTQSBjb21t
YW5kIGZvciBlYWNoIGludGVyZmFjZSBhbmQgbGV0IHRoZQ0Ka2VybmVsIGRlY2lkZSBob3cgdG8g
cGVyZm9ybSB0aGUgc3dpdGNoLg0KDQpUaGUgbWVyZ2VkIEFQSSBpcyBvayB3aXRoIG1lLCBidXQg
aXQgc2hvdWxkIGxlYXZlIHRoZSBvcHRpb24gdG8gc3dpdGNoIHNlcGFyYXRlbHkgKHdpdGggbGl0
dGxlIGRlbGF5cykuDQoNCi0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQpGcm9tOiBMdWNhIENv
ZWxobyBbbWFpbHRvOmx1Y2FAY29lbGhvLmZpXSANClNlbnQ6IFRodXJzZGF5LCBKYW51YXJ5IDIz
LCAyMDE0IDA5OjMyDQpUbzogTWljaGFsIEthemlvcg0KQ2M6IEpvaGFubmVzIEJlcmc7IGxpbnV4
LXdpcmVsZXNzOyBPdGNoZXJldGlhbnNraSwgQW5kcmVpDQpTdWJqZWN0OiBSZTogW1BBVENIIDUv
N10gbWFjODAyMTE6IGltcHJvdmUgQ1NBIGxvY2tpbmcNCg0KKGFkZGluZyBBbmRyZWksIHNpbmNl
IGhlIHNob3VsZCBiZSBhd2FyZSBvZiB0aGVzZSBkaXNjdXNzaW9ucyBhcyB3ZWxsIDspDQoNCk9u
IFRodSwgMjAxNC0wMS0yMyBhdCAwNzo0MSArMDEwMCwgTWljaGFsIEthemlvciB3cm90ZToNCj4g
T24gMjMgSmFudWFyeSAyMDE0IDA3OjMxLCBMdWNhIENvZWxobyA8bHVjYUBjb2VsaG8uZmk+IHdy
b3RlOg0KPiA+IE9uIFRodSwgMjAxNC0wMS0yMyBhdCAwNzoyMiArMDEwMCwgTWljaGFsIEthemlv
ciB3cm90ZToNCj4gPj4gT24gMjIgSmFudWFyeSAyMDE0IDE2OjEzLCBMdWNhIENvZWxobyA8bHVj
YUBjb2VsaG8uZmk+IHdyb3RlOg0KPiA+PiA+IE9uIFdlZCwgMjAxNC0wMS0yMiBhdCAxNjoxMCAr
MDEwMCwgSm9oYW5uZXMgQmVyZyB3cm90ZToNCj4gPj4gPj4gT24gV2VkLCAyMDE0LTAxLTIyIGF0
IDE0OjM2ICswMjAwLCBMdWNhIENvZWxobyB3cm90ZToNCj4gPj4gPj4NCj4gPj4gPj4gPiBJIGRv
bid0IHRoaW5rIHdlIHNob3VsZCB0cnkgdG8gbWVyZ2UgdGhlIGNoYW5uZWwgc3dpdGNoZXMuICBX
ZSANCj4gPj4gPj4gPiBzaG91bGQganVzdCBwZXJmb3JtIHRoZW0gc2VwYXJhdGVseSwgZXNwZWNp
YWxseSBiZWNhdXNlIHRoZSANCj4gPj4gPj4gPiBleGFjdCB0aW1lIG9mIHRoZSBzd2l0Y2ggd2ls
bCBtb3N0IGxpa2VseSBub3QgYmUgdGhlIHNhbWUgDQo+ID4+ID4+ID4gKHNpbmNlIHRoZSBUQlRU
cyBhcmUgbm90IGluIHN5bmMpLg0KPiA+PiA+Pg0KPiA+PiA+PiBEbyB5b3UgbWVhbiB0aGF0IHdl
IHNob3VsZG4ndCBldmVuIGhhdmUgYWxsIHRoYXQgbmV3IEFQSSB0byANCj4gPj4gPj4gc3dpdGNo
IG11bHRpcGxlIGludGVyZmFjZXMgc2ltdWx0YW5lb3VzbHk/DQo+ID4+ID4NCj4gPj4gPiBSaWdo
dCwgSSdtIG5vdCByZWFsbHkgc3VyZSBpdCdzIG5lY2Vzc2FyeS4gIFBFcmhhcHMgd2l0aCANCj4g
Pj4gPiBub24tY2hhbmN0eCB3ZSBuZWVkIHNvbWV0aGluZyBsaWtlIHRoYXQsIGJ1dCBtYXliZSBp
dCB3b3VsZCBzdGlsbCANCj4gPj4gPiBiZSBiZXR0ZXIgbm90IHRvIGRvIHRoaXMgaW4gdGhlIG5s
ODAyMTEgQVBJLCBidXQgc3luYy9tZXJnZSBpbiBjZmc4MDIxMS9tYWM4MDIxMT8NCj4gPj4NCj4g
Pj4gSSB3YXMgdGhpbmtpbmcgYWJvdXQgaXQuIFRoaXMgc2hvdWxkIHdvcmssIG1vc3RseSwgYXMg
bG9uZyBhcyANCj4gPj4geW91J3JlIGFibGUgdG8gc3VibWl0IENTQSByZXF1ZXN0cyBmYXN0IGVu
b3VnaCBhbmQgeW91IGRvbid0IHVzZSANCj4gPj4gY291bnQgMCBvciAxLCBpbiB3aGljaCBjYXNl
IGl0IGJlY29tZXMgcmFjeS4NCj4gPg0KPiA+IENTQSB3aXRoIGNvdW50IDAgb3IgMSBhcmUgcmVh
bGx5IHRyaWNreSBpbiBtYW55IHJlc3BlY3RzLiAgQnV0IHN0aWxsIA0KPiA+IEkgZG9uJ3Qgc2Vl
IHdoeSBpdCB3b3VsZCBnZXQgcmFjeS4gIFRoZSBpbnRlcmZhY2VzIHdpbGwgc3dpdGNoIA0KPiA+
IGluZGVwZW5kZW50bHksIG1ha2luZyB0aGVpciBvd24gY2hhbmN0eCByZXNlcnZhdGlvbnMgYW5k
IHNvIG9uLi4uDQo+IA0KPiBZb3UgY2FuJ3QgcmVhbGx5IHN3aXRjaCBtdWx0aXBsZSBpbnRlcmZh
Y2VzIGluZGVwZW5kZW50bHkgd2l0aCBhIA0KPiBzaW5nbGUtY2hhbm5lbCBoYXJkd2FyZS4gWW91
J2xsIGVpdGhlciBlbmQgdXAgd2l0aCBzb21lIGludGVyZmFjZXMgDQo+IGRpc2Nvbm5lY3RlZCBv
ciBkcmFnZ2VkIHRvIGEgZGlmZmVyZW50IGNoYW5uZWwgZm9yY2VmdWxseS4NCg0KUmlnaHQuLi4g
SSB0aGluayB0aGVyZSdzIG5vIHdheSBhcm91bmQgZHJhZ2dpbmcgdGhlbSBhbGwgZm9yY2VmdWxs
eS4NCg0KV2hhdCBhYm91dCB0aGlzOiB3aGVuIG1hYzgwMjExIHNlZXMgdGhhdCBvbmUgaW50ZXJm
YWNlIGlzIGFib3V0IHRvIHN3aXRjaCBjaGFubmVsIGFuZCB0aGVyZSBhcmUgb3RoZXIgaW50ZXJm
YWNlcyBpbiBhIHNpbmdsZS1jaGFubmVsIGRldmljZSwgaXQgYXV0b21hdGljYWxseSBzdGFydHMg
Y2hhbm5lbCBzd2l0Y2ggb24gdGhlIG90aGVyIGludGVyZmFjZXMgYXMgd2VsbCAocmVhbGx5LCB0
aGVyZSdzIG5vIHdheSBhcm91bmQgaXQpLiAgT2J2aW91c2x5IGl0IG5lZWRzIHRvIGluZm9ybSB1
c2Vyc3BhY2UgaW4gdGhpcyBjYXNlIGFuZCB0aGVuIGl0J3MgdXAgdG8gdGhlIHVzZXJzcGFjZSB0
byBkZWNpZGUgd2hhdCB0byBkbyAoZHJvcCB0aGUgY29ubmVjdGlvbiwgb3IgYWNjZXB0IHRoZSBj
aGFuZ2UpLg0KDQpJbiBtdWx0aS1jaGFubmVsIGludGVyZmFjZXMsIHdlIGRvbid0IHN3aXRjaCBh
dXRvbWF0aWNhbGx5LCBidXQgbGV0IHRoZSB1c2Vyc3BhY2UgZGVjaWRlIHdoYXQgdG8gZG8gd2l0
aCB0aGUgb3RoZXIgaW50ZXJmYWNlcy4gIElmIGl0IGRvZXNuJ3QgZG8gYW55dGhpbmcsIHdlIGVu
ZCB1cCB3aXRoIGVhY2ggaW50ZXJmYWNlIG9uIGEgZGlmZmVyZW50IGNoYW5uZWwuDQoNCkFjdHVh
bGx5IHRoaXMgY291bGQgYmUgZ2VuZXJhbGl6ZWQgdG9vLiAgV2UgZG9uJ3Qgc3dpdGNoIGF1dG9t
YXRpY2FsbHkgb25seSB3aGVuIHdlIGhhdmUgc2luZ2xlLWNoYW5uZWwgaW50ZXJmYWNlcywgYnV0
IHdoZW4gd2UgZG9uJ3QgaGF2ZSBhIGZyZWUgY29udGV4dCBmb3IgdGhlIG5ldyBjaGFubmVsLg0K
DQotLQ0KTHVjYS4NCg0KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCkEgbWVtYmVyIG9mIHRoZSBJbnRlbCBDb3Jwb3Jh
dGlvbiBncm91cCBvZiBjb21wYW5pZXMKClRoaXMgZS1tYWlsIGFuZCBhbnkgYXR0YWNobWVudHMg
bWF5IGNvbnRhaW4gY29uZmlkZW50aWFsIG1hdGVyaWFsIGZvcgp0aGUgc29sZSB1c2Ugb2YgdGhl
IGludGVuZGVkIHJlY2lwaWVudChzKS4gQW55IHJldmlldyBvciBkaXN0cmlidXRpb24KYnkgb3Ro
ZXJzIGlzIHN0cmljdGx5IHByb2hpYml0ZWQuIElmIHlvdSBhcmUgbm90IHRoZSBpbnRlbmRlZApy
ZWNpcGllbnQsIHBsZWFzZSBjb250YWN0IHRoZSBzZW5kZXIgYW5kIGRlbGV0ZSBhbGwgY29waWVz
Lgo=
(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 <[email protected]> wrote:
> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
> >> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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.
On 22 January 2014 13:36, Luca Coelho <[email protected]> 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?
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> 9 files changed, 137 insertions(+), 34 deletions(-)
This is fairly large - are you or is anyone else going to scream and
shout if I don't take it into 3.14?
johannes
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
On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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.
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
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
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
On 22 January 2014 10:13, Johannes Berg <[email protected]> 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ł
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
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
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
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
On 24 January 2014 10:52, Luca Coelho <[email protected]> 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ł
It was possible to connect STA when AP CSA was in
progress which clearly was a bug.
Deny attempts to increase chanctx refcount or
create chanctx.
This effectively denies starting a new interface
and radar detection while CSA is in progress.
Existing interfaces (including those with active
CSA) can be stopped though.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 7 +++++++
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/util.c | 18 ++++++++++++++++++
3 files changed, 26 insertions(+)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f43613a..9057b66 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -517,6 +517,13 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
+ /* Do not allow an interface to bind to channel contexts during CSA as
+ * it would end up with the interface being dragged to a different
+ * channel once CSA completes. It's safe to unbind from channel
+ * contexts though. */
+ if (ieee80211_is_csa_active(local))
+ return -EBUSY;
+
mutex_lock(&local->chanctx_mtx);
__ieee80211_vif_release_channel(sdata);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a6cda02..763a0ca 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1465,6 +1465,7 @@ int __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
int ieee80211_channel_switch(struct wiphy *wiphy,
struct cfg80211_csa_settings *params,
int num_params);
+bool ieee80211_is_csa_active(struct ieee80211_local *local);
/* interface handling */
int ieee80211_iface_init(void);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 676dc09..3e77c41 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2775,3 +2775,21 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
ps->dtim_count = dtim_count;
}
+
+bool ieee80211_is_csa_active(struct ieee80211_local *local)
+{
+ struct ieee80211_sub_if_data *sdata;
+
+ lockdep_assert_held(&local->mtx);
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->vif.csa_active) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
--
1.8.4.rc3
On 20 January 2014 22:41, Luca Coelho <[email protected]> wrote:
> On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
>> It was possible to connect STA when AP CSA was in
>> progress which clearly was a bug.
>
> I understand that preventing this simplifies things, but I don't think
> it's a bug. If the CSA process takes a long time (ie. several TBTTs),
> why not let a new STA associate meanwhile? The new STA knows when to
> switch, since it can see the count in the beacons (and probe_responses).
This patch prevents a station interface from connecting/associating
somewhere. It does not prevent a station from connecting to your AP
while it's in CSA. I probably should rephrase the commit message to
make it clear.
Michał
On 23 January 2014 07:31, Luca Coelho <[email protected]> wrote:
> On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
>> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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ł
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.
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> It was possible to connect STA when AP CSA was in
> progress which clearly was a bug.
>
> Deny attempts to increase chanctx refcount or
> create chanctx.
>
> This effectively denies starting a new interface
> and radar detection while CSA is in progress.
>
> Existing interfaces (including those with active
> CSA) can be stopped though.
I think this is the wrong approach. This makes more special cases for
non-chanctx code, whereas I think the code should gain more channel
context awareness. Luca has a patch to make channel switch work with
channel contexts (by creating one on the new channel and then later
switching to it), and I think this mechanism should be subsumed into the
channel context reservation code, rather than being CSA specific.
However, if there's only one channel context (or no support for channel
contexts) then CSA becomes a special case in Luca's patch, I guess,
because we can't be reserving a new one?
Luca, this is probably something you need to look into - this goes back
to the exact timing thing we just talked about. If we don't support
channel contexts, then we can create a fake one to switch to, but then
this fake one can't accept any further interfaces being bound to it
since it will not be able to coexist with the other one.
As far as this particular behaviour is concerned, I would then say that
then the old channel context should also be marked as incompatible for
the time of the CSA, so that no other (station) vif can attempt to use
it.
That'll result in a better solution overall, I think.
johannes
On 23 January 2014 13:20, Johannes Berg <[email protected]> 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ł
On 23 January 2014 13:09, Johannes Berg <[email protected]> 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ł
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.
On 23 January 2014 08:31, Luca Coelho <[email protected]> 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 <[email protected]> wrote:
>> > On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
>> >> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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ł
On 22 January 2014 09:52, Johannes Berg <[email protected]> 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ł
On 22 January 2014 16:13, Luca Coelho <[email protected]> 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ł
radar_required setting wasn't protected by
local->mtx in some places. This should prevent
from scanning/radar detection/roc colliding.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/cfg.c | 4 ++--
net/mac80211/ibss.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 76fe1f90..ddb1c4d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -970,9 +970,9 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
/* TODO: make hostapd tell us what it wants */
sdata->smps_mode = IEEE80211_SMPS_OFF;
sdata->needed_rx_chains = sdata->local->rx_chains;
- sdata->radar_required = params->radar_required;
mutex_lock(&local->mtx);
+ sdata->radar_required = params->radar_required;
err = ieee80211_vif_use_channel(sdata, ¶ms->chandef,
IEEE80211_CHANCTX_SHARED);
mutex_unlock(&local->mtx);
@@ -3017,8 +3017,8 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
struct ieee80211_local *local = sdata->local;
int err, changed = 0;
- sdata->radar_required = sdata->csa_radar_required;
mutex_lock(&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))
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 5ca5834..4854800 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -303,6 +303,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
mutex_unlock(&local->mtx);
return;
}
+ sdata->radar_required = radar_required;
mutex_unlock(&local->mtx);
memcpy(ifibss->bssid, bssid, ETH_ALEN);
@@ -318,7 +319,6 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
rcu_assign_pointer(ifibss->presp, presp);
mgmt = (void *)presp->head;
- sdata->radar_required = radar_required;
sdata->vif.bss_conf.enable_beacon = true;
sdata->vif.bss_conf.beacon_int = beacon_int;
sdata->vif.bss_conf.basic_rates = basic_rates;
--
1.8.4.rc3
On 21 January 2014 15:55, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
>> If CSA for AP interface failed and the interface
>> was not stopped afterwards another CSA request
>> would leak sdata->u.ap.next_beacon.
>
>> void ieee80211_csa_finish(struct ieee80211_vif *vif)
>> {
>> struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> @@ -3019,15 +3034,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>> sdata->vif.csa_active = false;
>> switch (sdata->vif.type) {
>> case NL80211_IFTYPE_AP:
>> - err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
>> + err = ieee80211_ap_finish_csa(sdata);
>> if (err < 0)
>> return;
>> -
>> - changed |= err;
>
> This looks a bit like somebody had intended to batch the
> ieee80211_bss_info_change_notify() calls, which would probably be a good
> thing. You're breaking them apart even further - maybe we should
> actually batch them instead by moving ieee80211_bss_info_change_notify()
> after the switch()?
Sounds good. I'll fix it.
Michał
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> If CSA for AP interface failed and the interface
> was not stopped afterwards another CSA request
> would leak sdata->u.ap.next_beacon.
> void ieee80211_csa_finish(struct ieee80211_vif *vif)
> {
> struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> @@ -3019,15 +3034,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> sdata->vif.csa_active = false;
> switch (sdata->vif.type) {
> case NL80211_IFTYPE_AP:
> - err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
> + err = ieee80211_ap_finish_csa(sdata);
> if (err < 0)
> return;
> -
> - changed |= err;
This looks a bit like somebody had intended to batch the
ieee80211_bss_info_change_notify() calls, which would probably be a good
thing. You're breaking them apart even further - maybe we should
actually batch them instead by moving ieee80211_bss_info_change_notify()
after the switch()?
johannes
If CSA for AP interface failed and the interface
was not stopped afterwards another CSA request
would leak sdata->u.ap.next_beacon.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/cfg.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 65dac7f..62bf6c4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2988,6 +2988,21 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
return new_beacon;
}
+static int ieee80211_ap_finish_csa(struct ieee80211_sub_if_data *sdata)
+{
+ int err;
+
+ err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
+ kfree(sdata->u.ap.next_beacon);
+ sdata->u.ap.next_beacon = NULL;
+
+ if (err < 0)
+ return err;
+
+ ieee80211_bss_info_change_notify(sdata, err);
+ return 0;
+}
+
void ieee80211_csa_finish(struct ieee80211_vif *vif)
{
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
@@ -3019,15 +3034,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
sdata->vif.csa_active = false;
switch (sdata->vif.type) {
case NL80211_IFTYPE_AP:
- err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
+ err = ieee80211_ap_finish_csa(sdata);
if (err < 0)
return;
-
- changed |= err;
- kfree(sdata->u.ap.next_beacon);
- sdata->u.ap.next_beacon = NULL;
-
- ieee80211_bss_info_change_notify(sdata, err);
break;
case NL80211_IFTYPE_ADHOC:
ieee80211_ibss_finish_csa(sdata);
--
1.8.4.rc3
On Thu, 2014-01-23 at 07:22 +0100, Michal Kazior wrote:
> On 22 January 2014 16:13, Luca Coelho <[email protected]> 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.
On 21 January 2014 16:06, Johannes Berg <[email protected]> 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ł
Other interface modes are checked against failure.
This should avoid false-positive channel switch
events where IBSS CSA actually failed.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/cfg.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 62bf6c4..76fe1f90 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3039,7 +3039,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
return;
break;
case NL80211_IFTYPE_ADHOC:
- ieee80211_ibss_finish_csa(sdata);
+ err = ieee80211_ibss_finish_csa(sdata);
+ if (err < 0)
+ return;
break;
#ifdef CONFIG_MAC80211_MESH
case NL80211_IFTYPE_MESH_POINT:
--
1.8.4.rc3
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 <[email protected]>
---
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/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;
}
--
1.8.4.rc3