Hi,
The patchset aims at making cfg80211 aware of
channel switches.
This makes it possible for more elegant channel
switching behavior by moving the decision up to
cfg80211.
Until now mac80211 could start channel switching
internally for STA, mesh and IBSS interfaces
without userspace interaction. This bypassed
interface combination checks at the very least.
Now mac80211 requests cfg80211 to channel switch
an interface, in a similar manner as userspace
asks cfg80211 for a channel switch. This makes it
possible to perform interface combination checks
(albeit it is not implemented yet).
The channel switch is split into two phases now -
start and finalize. This is required to make
cfg80211 in control of the whole channel
switching process. It also makes it apparent that
mac80211's STA CSA offload doesn't work quite well
here. Can we kill it?
The initial patchset has a lot of TODOs in the
code and the idea is to provide follow up patches
that implement missing functionality.
Major drawback now is cfg80211 will refuse a
channel switch if there's one in progress already,
although this shouldn't be much of a problem in
most use cases.
This approach should be suited for multi-interface
CSA as well as some other cases such as
"GO-follows-STA".
The patchset seemed to work in my limited testing
(ath9k as AP and iwldvm as STA).
Michal Kazior (2):
mac80211: merge STA CSA code
cfg80211: move channel switch logic to cfg80211
include/net/cfg80211.h | 80 ++++++++++++++++++++--
net/mac80211/cfg.c | 100 ++++++++++++++++++++-------
net/mac80211/ibss.c | 4 +-
net/mac80211/ieee80211_i.h | 6 +-
net/mac80211/mesh.c | 6 +-
net/mac80211/mlme.c | 133 +++++++++---------------------------
net/wireless/ap.c | 5 +-
net/wireless/chan.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
net/wireless/core.c | 23 +++++--
net/wireless/core.h | 18 +++++
net/wireless/ibss.c | 1 +
net/wireless/mesh.c | 5 +-
net/wireless/nl80211.c | 24 +++++--
net/wireless/rdev-ops.h | 21 ++++--
net/wireless/sme.c | 1 +
net/wireless/trace.h | 17 ++++-
net/wireless/util.c | 6 ++
17 files changed, 454 insertions(+), 159 deletions(-)
--
1.8.5.3
On Wed, 2014-02-05 at 15:37 +0100, Michal Kazior wrote:
> If the decision must be made at the beginning and you want mac80211 to
> start STA CSA then GO-follow-STA is impossible.
>
> What could work here is single-command-multi-interface channel switch
> IF you make STA CSA a userspace thing.
>
> If you want to keep STA CSA not a userspace thing you cannot make any
> decision at the start in cfg80211 and you must move the decision to
> the driver itself (or firmware, actually). In that case cfg80211
> remains highly reactive and you cannot guarantee interface combination
> state consistency at all times because driver may be incapable of
> notifying cfg80211 in a sane way about interface state changes (e.g.
> due to firmware design/ HW limitation/ cfg80211 locking issues/
> synchronization).
In the case of full-MAC drivers we pretty much have to assume that they
will correctly enforce their own interface combinations in the firmware
anyway, we can't even check in connect() today, so that doesn't seem
like a big issue.
I don't see how this all makes it impossible though, it seems you could
still do:
1) receive CSA from AP on STA interface
2) reserve a 'temporary' channel context that doesn't count towards the
limits,
since it'll be switched
3) notify cfg80211/userspace of the pending switch
4) when the time comes to do the switch, do a final capability check,
and
disconnect from the AP instead if it fails
If you have an GO-follows-STA scenario you'd have userspace react:
3a) notify cfg80211/userspace of the pending switch
3b) userspace starts (matching) CSA for the GO interface
3c) mac80211 matches that up with the STA interface switch and its
temporary
channel context [and this is the only tricky part]
4) when the time comes to do the switch, hand all the involved
interfaces to
cfg80211 for the final capability check, which should now not fail
unless
userspace specified only one of two GO interfaces or so
johannes
On Tue, 2014-02-04 at 13:14 +0100, Michal Kazior wrote:
> On 4 February 2014 11:07, Luca Coelho <[email protected]> wrote:
> > On Mon, 2014-02-03 at 14:41 +0100, Johannes Berg wrote:
> >> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> >> > * inform userspace what interfaces will be
> >> > possibly disconnected by a channel switch,
> >>
> >> Huh? Not sure I get that part, why would userspace ever be notified
> >> about something that *will* happen? Either the interface disconnects
> >> when the CSA is received, or it just switches and userspace gets a "CSA
> >> will happen" event?
>
> True. Userspace can probably deduce from interface combinations some
> interfaces require attention due to channel switching.
>
>
> > How do we get the "target" beacon from userspace if the interface just
> > switches?
>
> Do you really need the beacon? What for?
I meant if we would switch the AP/GO interfaces automatically. We need
the beacon that should be transmitted in the new channel (like the one
we need to pass in the existing channel switch request API).
> > Now a bit of brain burp: I think that the "count" decision should remain
> > in the userspace so it can decide to give more time for its stations to
> > switch. Eg. if the client interface got a CSA with count == x and a
> > host interface has dtim_interval > x, the userspace can send a "quiet"
> > CSA with count == dtim_interval + 1. The two requests would be "merged"
> > and the highest count would win. The client would be a bit late on the
> > new channel, but at least the AP wouldn't lose most of its clients.
> > Does this make any sense? I'm not sure myself. :)
>
> Makes sense.
Let's keep this in mind as an improvement for later.
> >> > * disconnect non-complying interfaces that were
> >> > sharing a channel that is being abandoned by
> >> > channel switching interface(s),
> >>
> >> Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> >> possible (userspace CSA) or cause the switching interface to disconnect,
> >> rather than *others*??
> >
> > It depends. And this logic is too complicated to stay in the kernel,
> > IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> > GO. Now, if you have an AP (with tons of STAs connected to it) and a
> > P2P client gets a CSA for whatever reason, do we really want to stop the
> > AP?
>
> Agreed.
>
> But doesn't this imply STA CSA shouldn't be started within kernel
> itself? Otherwise you leave a corner case when STA CSA is very short
> making it impossible for userspace to take any action.
I don't see how involving the userspace in the STA CSA decision would
make any difference. Doesn't it actually make it worse, because the STA
CSA *itself* may fail because userspace doesn't have the time to react?
> >> > - * @channel_switch: initiate channel-switch procedure (with CSA)
> >> > + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
> >> > + * prompt the driver to perform preparation work (start some timers,
> >> > + * include CS IEs in beacons, etc) but don't do an actual channel switch.
> >> > + * Once driver has completed all preparations and is ready for the actual
> >> > + * switch (after CSA counter is completed) it must call
> >> > + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
> >> > + * be called, but it doesn't necessarily happen immediately as cfg80211
> >> > + * may need to synchronize with other interfaces. If channel switch is
> >> > + * cancelled for some reason ch_switch_finalize() is not called and driver
> >> > + * should free up resources/cleanup state in interface disconnection flow.
> >> > + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
> >> > + * actual switch.
> >>
> >> I don't like this at all. This totally assumes that every driver behaves
> >> like mac80211, which clearly is not the case. The split between
> >> "starting" and "finalizing" it should not be part of the API.
> >
> > I agree, especially if the driver offloads the channel switch, in which
> > case it wouldn't be possible for cfg80211 to hold the finalize call to
> > sync all the interfaces.
>
> This implies we don't care if channel switching breaks interface
> combinations, right?
Those decisions need to be made *before* the channel switch starts.
--
Luca.
On 5 February 2014 08:39, Luca Coelho <[email protected]> wrote:
> On Tue, 2014-02-04 at 13:14 +0100, Michal Kazior wrote:
>> On 4 February 2014 11:07, Luca Coelho <[email protected]> wrote:
>> > On Mon, 2014-02-03 at 14:41 +0100, Johannes Berg wrote:
>> >> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>> >> > * inform userspace what interfaces will be
>> >> > possibly disconnected by a channel switch,
>> >>
>> >> Huh? Not sure I get that part, why would userspace ever be notified
>> >> about something that *will* happen? Either the interface disconnects
>> >> when the CSA is received, or it just switches and userspace gets a "CSA
>> >> will happen" event?
>>
>> True. Userspace can probably deduce from interface combinations some
>> interfaces require attention due to channel switching.
>>
>>
>> > How do we get the "target" beacon from userspace if the interface just
>> > switches?
>>
>> Do you really need the beacon? What for?
>
> I meant if we would switch the AP/GO interfaces automatically. We need
> the beacon that should be transmitted in the new channel (like the one
> we need to pass in the existing channel switch request API).
I think Johannes made it clear that switching AP/GO automatically
isn't possible because it requires too much information that mac80211
(nor cfg80211) doesn't have.
>> >> > * disconnect non-complying interfaces that were
>> >> > sharing a channel that is being abandoned by
>> >> > channel switching interface(s),
>> >>
>> >> Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
>> >> possible (userspace CSA) or cause the switching interface to disconnect,
>> >> rather than *others*??
>> >
>> > It depends. And this logic is too complicated to stay in the kernel,
>> > IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
>> > GO. Now, if you have an AP (with tons of STAs connected to it) and a
>> > P2P client gets a CSA for whatever reason, do we really want to stop the
>> > AP?
>>
>> Agreed.
>>
>> But doesn't this imply STA CSA shouldn't be started within kernel
>> itself? Otherwise you leave a corner case when STA CSA is very short
>> making it impossible for userspace to take any action.
>
> I don't see how involving the userspace in the STA CSA decision would
> make any difference. Doesn't it actually make it worse, because the STA
> CSA *itself* may fail because userspace doesn't have the time to react?
If you have STA CSA started inside mac80211 userspace may not have
enough time to react and request GO to switch in the GO-follows-STA.
This really depends how strongly you want to enforce interface
combinations and how much proactive/reactive CSA should be.
>> >> > - * @channel_switch: initiate channel-switch procedure (with CSA)
>> >> > + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
>> >> > + * prompt the driver to perform preparation work (start some timers,
>> >> > + * include CS IEs in beacons, etc) but don't do an actual channel switch.
>> >> > + * Once driver has completed all preparations and is ready for the actual
>> >> > + * switch (after CSA counter is completed) it must call
>> >> > + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
>> >> > + * be called, but it doesn't necessarily happen immediately as cfg80211
>> >> > + * may need to synchronize with other interfaces. If channel switch is
>> >> > + * cancelled for some reason ch_switch_finalize() is not called and driver
>> >> > + * should free up resources/cleanup state in interface disconnection flow.
>> >> > + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
>> >> > + * actual switch.
>> >>
>> >> I don't like this at all. This totally assumes that every driver behaves
>> >> like mac80211, which clearly is not the case. The split between
>> >> "starting" and "finalizing" it should not be part of the API.
>> >
>> > I agree, especially if the driver offloads the channel switch, in which
>> > case it wouldn't be possible for cfg80211 to hold the finalize call to
>> > sync all the interfaces.
>>
>> This implies we don't care if channel switching breaks interface
>> combinations, right?
>
> Those decisions need to be made *before* the channel switch starts.
The purpose of ch_switch_finalize() is to perform the final switch
atomically and make sure non-complying interfaces are disconnected
before channel is actually switched (this could include Ilan's GO case
in a clean way).
If you want to move this to drivers you'll need RTNL to check
interface combinations at least.
Or.. cfg80211 can simply not give a damn :)
Michał
On Mon, 2014-02-03 at 14:41 +0100, Johannes Berg wrote:
> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> > This introduces the following benefits:
> > * cfg80211 is now aware of channel switching
> > (although more work still needs to be done wrt
> > interface combinations & multi-interface CSA)
> > * fixes some channel switching failcases by
> > disconnecting offending interfaces
> > * STA CSA no longer modifies BSS channel from
> > within mac80211
>
> That's nice.
>
> > This aims to make the following possible later:
> > * defer channel switching decision to userspace
> > (if desired so),
>
> That's probably not needed, it can disconnect after the CSA event (that
> should be sent when the CSA is first received)
I agree that this shouldn't be needed for client (ie. P2P client or
managed) interfaces.
> > * inform userspace what interfaces will be
> > possibly disconnected by a channel switch,
>
> Huh? Not sure I get that part, why would userspace ever be notified
> about something that *will* happen? Either the interface disconnects
> when the CSA is received, or it just switches and userspace gets a "CSA
> will happen" event?
How do we get the "target" beacon from userspace if the interface just
switches?
Now a bit of brain burp: I think that the "count" decision should remain
in the userspace so it can decide to give more time for its stations to
switch. Eg. if the client interface got a CSA with count == x and a
host interface has dtim_interval > x, the userspace can send a "quiet"
CSA with count == dtim_interval + 1. The two requests would be "merged"
and the highest count would win. The client would be a bit late on the
new channel, but at least the AP wouldn't lose most of its clients.
Does this make any sense? I'm not sure myself. :)
> > * disconnect non-complying interfaces that were
> > sharing a channel that is being abandoned by
> > channel switching interface(s),
>
> Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> possible (userspace CSA) or cause the switching interface to disconnect,
> rather than *others*??
It depends. And this logic is too complicated to stay in the kernel,
IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
GO. Now, if you have an AP (with tons of STAs connected to it) and a
P2P client gets a CSA for whatever reason, do we really want to stop the
AP?
> > - * @channel_switch: initiate channel-switch procedure (with CSA)
> > + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
> > + * prompt the driver to perform preparation work (start some timers,
> > + * include CS IEs in beacons, etc) but don't do an actual channel switch.
> > + * Once driver has completed all preparations and is ready for the actual
> > + * switch (after CSA counter is completed) it must call
> > + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
> > + * be called, but it doesn't necessarily happen immediately as cfg80211
> > + * may need to synchronize with other interfaces. If channel switch is
> > + * cancelled for some reason ch_switch_finalize() is not called and driver
> > + * should free up resources/cleanup state in interface disconnection flow.
> > + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
> > + * actual switch.
>
> I don't like this at all. This totally assumes that every driver behaves
> like mac80211, which clearly is not the case. The split between
> "starting" and "finalizing" it should not be part of the API.
I agree, especially if the driver offloads the channel switch, in which
case it wouldn't be possible for cfg80211 to hold the finalize call to
sync all the interfaces.
--
Luca.
On Tue, 2014-02-04 at 13:42 +0200, Luca Coelho wrote:
> > Nothing! As far as that's permissible. Ilan's patchset might change
> > that, but that's mostly a regulatory thing, rather than a CSA thing.
>
> Ah, you have some insider information! :P
Not really, he posted those patches ... :)
> Sure, but how does the userspace know that the other interface *needs*
> to be moved, regardless of regulatory problems? It needs to know that
> there are not enough contexts to keep the interfaces in different
> channels...
It has all that information, it can know the channels, channel contexts,
etc.
> > > ...and, if the userspace doesn't react, we disconnect the GO. I think
> > > it's safer this way for the GO-follows-STA case.
> >
> > That would be a consequence of Ilan's work, yes.
>
> Okay, if that's going to be guaranteed, I'm fine with it.
Not yet, but Ilan probably needs to take care of that.
> > This typically won't happen, since userspace will do something else, but
> > we need to have some default policy. I don't think anything else makes
> > much sense?
>
> My point was that a CSA could have been triggered due to regulatory
> (which the AP/GO is monitoring), so the safest would be to move everyone
> out of the channel by default.
But the kernel can't do that, it needs userspace to do it. And if the
CSA was triggered due to regulatory, then the GO that follow the client
can only be on this channel with Ilan's work, otherwise it's allowed to
be standalone there.
johannes
On Tue, 2014-02-04 at 12:24 +0100, Johannes Berg wrote:
> On Tue, 2014-02-04 at 13:11 +0200, Luca Coelho wrote:
>
> > > Well, what I was describing was really only the default policy if
> > > userspace didn't do anything useful, which IMHO should really just be:
> > >
> > > * client receives CSA - disconnect if it can't be done
> > > * AP/GO wants CSA - refuse if it can't be done, let userspace sort it
> > > out
> > >
> > > In the first case, userspace still has the time between receiving the
> > > CSA and actually acting on it to make another decision.
> >
> > Right, this is okay, but the point is, what happens to the *other*
> > interfaces?
>
> Nothing! As far as that's permissible. Ilan's patchset might change
> that, but that's mostly a regulatory thing, rather than a CSA thing.
Ah, you have some insider information! :P
> > What does "it can't be done" mean for the client? If there's a GO in the
> > same context and no free contexts for the switch, do we simply
> > disconnect the client (and leave the GO hanging in the same channel)? We
> > should probably tell the userspace and let it decide.
>
> Yes, but we should only disconnect when the CSA actually needs to be
> done, rather than immediately on receiving it.
Sure, but how does the userspace know that the other interface *needs*
to be moved, regardless of regulatory problems? It needs to know that
there are not enough contexts to keep the interfaces in different
channels...
> > ...and, if the userspace doesn't react, we disconnect the GO. I think
> > it's safer this way for the GO-follows-STA case.
>
> That would be a consequence of Ilan's work, yes.
Okay, if that's going to be guaranteed, I'm fine with it.
> This typically won't happen, since userspace will do something else, but
> we need to have some default policy. I don't think anything else makes
> much sense?
My point was that a CSA could have been triggered due to regulatory
(which the AP/GO is monitoring), so the safest would be to move everyone
out of the channel by default.
But now I agree that this is an unrelated feature that also needs to be
handled when we do have free contexts to make the switch (and keep the
two interfaces in different channels).
--
Luca.
On 5 February 2014 13:11, Luca Coelho <[email protected]> wrote:
> On Wed, 2014-02-05 at 11:29 +0100, Michal Kazior wrote:
>> On 5 February 2014 08:39, Luca Coelho <[email protected]> wrote:
>> > I don't see how involving the userspace in the STA CSA decision would
>> > make any difference. Doesn't it actually make it worse, because the STA
>> > CSA *itself* may fail because userspace doesn't have the time to react?
>>
>> If you have STA CSA started inside mac80211 userspace may not have
>> enough time to react and request GO to switch in the GO-follows-STA.
>
> It's the same if the STA CSA would be "started" in the userspace. If
> the remote AP sets a too short count, the userspace won't have the
> chance to react. In either case, we would have to send a notification
> to the userspace and it would have to tell us what to do. If we wait
> for the userspace to react on our STA CSA, we will be too late on both
> interfaces. If mac80211 does the STA CSA directly, at least the STA CSA
> will be done on time and the GO can move a bit later (or get
> disconnected if we were short on chanctx's).
>
> Now, if you are thinking about my "delay the STA CSA even if it will be
> late" idea, then I think you have a point. The side-effect would be
> that if you have only a STA and get a CSA with short count, we will
> probably get delayed too.
Correct. Having STA CSA in userspace makes it a little more timing
tolerant but it makes it a bit "laggy".
[...]
>> The purpose of ch_switch_finalize() is to perform the final switch
>> atomically and make sure non-complying interfaces are disconnected
>> before channel is actually switched (this could include Ilan's GO case
>> in a clean way).
>
> I was thinking about eventual drivers that offload the whole channel
> switch process to the firmware. The driver just sends the count, the
> new channel and everything and the firmware takes care of it all. It
> will beacon for count TBTTs and perform the actual channel switch at the
> right time. In this case, all the decision making about what needs to
> be disconnected or switched must be made at the beginning.
>
> But I'll let Johannes comment more. ;)
If the decision must be made at the beginning and you want mac80211 to
start STA CSA then GO-follow-STA is impossible.
What could work here is single-command-multi-interface channel switch
IF you make STA CSA a userspace thing.
If you want to keep STA CSA not a userspace thing you cannot make any
decision at the start in cfg80211 and you must move the decision to
the driver itself (or firmware, actually). In that case cfg80211
remains highly reactive and you cannot guarantee interface combination
state consistency at all times because driver may be incapable of
notifying cfg80211 in a sane way about interface state changes (e.g.
due to firmware design/ HW limitation/ cfg80211 locking issues/
synchronization).
Michał
On Tue, 2014-02-04 at 12:29 +0100, Michal Kazior wrote:
> >> +void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
> >> +{
> >> + struct ieee80211_local *local = sdata->local;
> >> + struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
> >> +
> >> + switch (sdata->vif.type) {
> >> + case NL80211_IFTYPE_STATION:
> >> + case NL80211_IFTYPE_P2P_CLIENT:
> >> + ieee80211_queue_work(&local->hw,
> >> + &ifmgd->csa_connection_drop_work);
> >> + break;
> >> + default:
> >> + /* XXX: other iftypes should be halted too */
> >
> > Good point. This case would suck, but we need to do something because
> > the stations will think that we're on the different channel at this
> > point. But maybe instead the userspace should be notified instead of
> > halting here? It could retry with a count 0, for example.
>
> Ideally cfg80211 should be the one stopping interfaces. This way you'd
> get AP stopped event (recently done by Johannes) and userspace can do
> something about (e.g. re-start APs from scratch).
Well, if mac80211 ends up doing that, we just need an API call to send
the event and reset cfg80211's info. I didn't add one yet because the
locking might get a bit tricky, but others actually wanted such an API
for firmware bugs anyway.
johannes
On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> The patchset aims at making cfg80211 aware of
> channel switches.
That seems like a worthy goal :)
> This makes it possible for more elegant channel
> switching behavior by moving the decision up to
> cfg80211.
>
> Until now mac80211 could start channel switching
> internally for STA, mesh and IBSS interfaces
> without userspace interaction. This bypassed
> interface combination checks at the very least.
>
> Now mac80211 requests cfg80211 to channel switch
> an interface, in a similar manner as userspace
> asks cfg80211 for a channel switch. This makes it
> possible to perform interface combination checks
> (albeit it is not implemented yet).
Couldn't you just return the decision to mac80211? It would also have to
keep track of start/end of the CSA period, I guess, since a few things
that Ilan is working on shouldn't be allowed while the AP announces CSA.
> The channel switch is split into two phases now -
> start and finalize. This is required to make
> cfg80211 in control of the whole channel
> switching process. It also makes it apparent that
> mac80211's STA CSA offload doesn't work quite well
> here. Can we kill it?
No. Well, you could make cfg80211 be in charge, but not wpa_supplicant,
I think.
> The initial patchset has a lot of TODOs in the
> code and the idea is to provide follow up patches
> that implement missing functionality.
>
> Major drawback now is cfg80211 will refuse a
> channel switch if there's one in progress already,
> although this shouldn't be much of a problem in
> most use cases.
>
> This approach should be suited for multi-interface
> CSA as well as some other cases such as
> "GO-follows-STA".
>
> The patchset seemed to work in my limited testing
> (ath9k as AP and iwldvm as STA).
Nice. I hope Luca can take a look too :)
johannes
On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> + * @timestamp: value in microseconds of the 64-bit Time Synchronization
> + * Function (TSF) timer when the frame containing the channel switch
> + * announcement was received. This is simply the rx.mactime parameter
> + * the driver passed in.
That doesn't make a lot of sense, since "rx.mactime" is a mac80211
thing, so you should probably describe this. I'm not sure it really
needs to be mactime/TSF, does that even make sense? It seems more likely
you'd actually need the device time, i.e. something that works across
multiple vifs?
Not sure this even belongs into this patch anyway though?
johannes
On Tue, 2014-02-04 at 12:57 +0100, Johannes Berg wrote:
> On Tue, 2014-02-04 at 13:42 +0200, Luca Coelho wrote:
> > > Nothing! As far as that's permissible. Ilan's patchset might change
> > > that, but that's mostly a regulatory thing, rather than a CSA thing.
> >
> > Ah, you have some insider information! :P
>
> Not really, he posted those patches ... :)
Heh, I should read the mailing list more carefully instead of marking
everything as read without reading. ;)
> > Sure, but how does the userspace know that the other interface *needs*
> > to be moved, regardless of regulatory problems? It needs to know that
> > there are not enough contexts to keep the interfaces in different
> > channels...
>
> It has all that information, it can know the channels, channel contexts,
> etc.
Okay, so I assume it also knows how many vifs can be in the same context
and all the possible vif type combinations. And do the same checks that
cfg80211 already does...
> > > > ...and, if the userspace doesn't react, we disconnect the GO. I think
> > > > it's safer this way for the GO-follows-STA case.
> > >
> > > That would be a consequence of Ilan's work, yes.
> >
> > Okay, if that's going to be guaranteed, I'm fine with it.
>
> Not yet, but Ilan probably needs to take care of that.
>
> > > This typically won't happen, since userspace will do something else, but
> > > we need to have some default policy. I don't think anything else makes
> > > much sense?
> >
> > My point was that a CSA could have been triggered due to regulatory
> > (which the AP/GO is monitoring), so the safest would be to move everyone
> > out of the channel by default.
>
> But the kernel can't do that, it needs userspace to do it.
Right, but by "moving out of the channel by default" I meant
disconnecting those who don't so we are sure there's no one left.
> And if the
> CSA was triggered due to regulatory, then the GO that follow the client
> can only be on this channel with Ilan's work, otherwise it's allowed to
> be standalone there.
Okay, I'll check Ilan's patches.
--
Luca.
On Wed, 2014-02-05 at 11:29 +0100, Michal Kazior wrote:
> On 5 February 2014 08:39, Luca Coelho <[email protected]> wrote:
> > On Tue, 2014-02-04 at 13:14 +0100, Michal Kazior wrote:
> >> On 4 February 2014 11:07, Luca Coelho <[email protected]> wrote:
> >> > On Mon, 2014-02-03 at 14:41 +0100, Johannes Berg wrote:
> >> >> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> >> >> > * inform userspace what interfaces will be
> >> >> > possibly disconnected by a channel switch,
> >> >>
> >> >> Huh? Not sure I get that part, why would userspace ever be notified
> >> >> about something that *will* happen? Either the interface disconnects
> >> >> when the CSA is received, or it just switches and userspace gets a "CSA
> >> >> will happen" event?
> >>
> >> True. Userspace can probably deduce from interface combinations some
> >> interfaces require attention due to channel switching.
> >>
> >>
> >> > How do we get the "target" beacon from userspace if the interface just
> >> > switches?
> >>
> >> Do you really need the beacon? What for?
> >
> > I meant if we would switch the AP/GO interfaces automatically. We need
> > the beacon that should be transmitted in the new channel (like the one
> > we need to pass in the existing channel switch request API).
>
> I think Johannes made it clear that switching AP/GO automatically
> isn't possible because it requires too much information that mac80211
> (nor cfg80211) doesn't have.
Yes, that was clarified in the other thread. I just explained here what
I had meant in my original question. ;)
> >> >> > * disconnect non-complying interfaces that were
> >> >> > sharing a channel that is being abandoned by
> >> >> > channel switching interface(s),
> >> >>
> >> >> Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> >> >> possible (userspace CSA) or cause the switching interface to disconnect,
> >> >> rather than *others*??
> >> >
> >> > It depends. And this logic is too complicated to stay in the kernel,
> >> > IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> >> > GO. Now, if you have an AP (with tons of STAs connected to it) and a
> >> > P2P client gets a CSA for whatever reason, do we really want to stop the
> >> > AP?
> >>
> >> Agreed.
> >>
> >> But doesn't this imply STA CSA shouldn't be started within kernel
> >> itself? Otherwise you leave a corner case when STA CSA is very short
> >> making it impossible for userspace to take any action.
> >
> > I don't see how involving the userspace in the STA CSA decision would
> > make any difference. Doesn't it actually make it worse, because the STA
> > CSA *itself* may fail because userspace doesn't have the time to react?
>
> If you have STA CSA started inside mac80211 userspace may not have
> enough time to react and request GO to switch in the GO-follows-STA.
It's the same if the STA CSA would be "started" in the userspace. If
the remote AP sets a too short count, the userspace won't have the
chance to react. In either case, we would have to send a notification
to the userspace and it would have to tell us what to do. If we wait
for the userspace to react on our STA CSA, we will be too late on both
interfaces. If mac80211 does the STA CSA directly, at least the STA CSA
will be done on time and the GO can move a bit later (or get
disconnected if we were short on chanctx's).
Now, if you are thinking about my "delay the STA CSA even if it will be
late" idea, then I think you have a point. The side-effect would be
that if you have only a STA and get a CSA with short count, we will
probably get delayed too.
In any case, too short counts are a stupid idea and will (hopefully) not
happen very often in practice.
> This really depends how strongly you want to enforce interface
> combinations and how much proactive/reactive CSA should be.
We must enforce the interface combinations. And STA CSA is always
reactive. ;)
> >> >> > - * @channel_switch: initiate channel-switch procedure (with CSA)
> >> >> > + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
> >> >> > + * prompt the driver to perform preparation work (start some timers,
> >> >> > + * include CS IEs in beacons, etc) but don't do an actual channel switch.
> >> >> > + * Once driver has completed all preparations and is ready for the actual
> >> >> > + * switch (after CSA counter is completed) it must call
> >> >> > + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
> >> >> > + * be called, but it doesn't necessarily happen immediately as cfg80211
> >> >> > + * may need to synchronize with other interfaces. If channel switch is
> >> >> > + * cancelled for some reason ch_switch_finalize() is not called and driver
> >> >> > + * should free up resources/cleanup state in interface disconnection flow.
> >> >> > + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
> >> >> > + * actual switch.
> >> >>
> >> >> I don't like this at all. This totally assumes that every driver behaves
> >> >> like mac80211, which clearly is not the case. The split between
> >> >> "starting" and "finalizing" it should not be part of the API.
> >> >
> >> > I agree, especially if the driver offloads the channel switch, in which
> >> > case it wouldn't be possible for cfg80211 to hold the finalize call to
> >> > sync all the interfaces.
> >>
> >> This implies we don't care if channel switching breaks interface
> >> combinations, right?
> >
> > Those decisions need to be made *before* the channel switch starts.
>
> The purpose of ch_switch_finalize() is to perform the final switch
> atomically and make sure non-complying interfaces are disconnected
> before channel is actually switched (this could include Ilan's GO case
> in a clean way).
I was thinking about eventual drivers that offload the whole channel
switch process to the firmware. The driver just sends the count, the
new channel and everything and the firmware takes care of it all. It
will beacon for count TBTTs and perform the actual channel switch at the
right time. In this case, all the decision making about what needs to
be disconnected or switched must be made at the beginning.
But I'll let Johannes comment more. ;)
> If you want to move this to drivers you'll need RTNL to check
> interface combinations at least.
>
> Or.. cfg80211 can simply not give a damn :)
:)
--
Luca.
On Tue, 2014-02-04 at 13:11 +0200, Luca Coelho wrote:
> On Tue, 2014-02-04 at 11:33 +0100, Johannes Berg wrote:
> > On Tue, 2014-02-04 at 12:07 +0200, Luca Coelho wrote:
> >
> > > > Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> > > > possible (userspace CSA) or cause the switching interface to disconnect,
> > > > rather than *others*??
> > >
> > > It depends. And this logic is too complicated to stay in the kernel,
> > > IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> > > GO. Now, if you have an AP (with tons of STAs connected to it) and a
> > > P2P client gets a CSA for whatever reason, do we really want to stop the
> > > AP?
> >
> > Well, what I was describing was really only the default policy if
> > userspace didn't do anything useful, which IMHO should really just be:
> >
> > * client receives CSA - disconnect if it can't be done
> > * AP/GO wants CSA - refuse if it can't be done, let userspace sort it
> > out
> >
> > In the first case, userspace still has the time between receiving the
> > CSA and actually acting on it to make another decision.
>
> Right, this is okay, but the point is, what happens to the *other*
> interfaces?
>
> What does "it can't be done" mean for the client? If there's a GO in the
> same context and no free contexts for the switch, do we simply
> disconnect the client (and leave the GO hanging in the same channel)? We
> should probably tell the userspace and let it decide.
...and, if the userspace doesn't react, we disconnect the GO. I think
it's safer this way for the GO-follows-STA case.
--
Luca.
On Tue, 2014-02-04 at 11:33 +0100, Johannes Berg wrote:
> On Tue, 2014-02-04 at 12:07 +0200, Luca Coelho wrote:
>
> > > Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> > > possible (userspace CSA) or cause the switching interface to disconnect,
> > > rather than *others*??
> >
> > It depends. And this logic is too complicated to stay in the kernel,
> > IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> > GO. Now, if you have an AP (with tons of STAs connected to it) and a
> > P2P client gets a CSA for whatever reason, do we really want to stop the
> > AP?
>
> Well, what I was describing was really only the default policy if
> userspace didn't do anything useful, which IMHO should really just be:
>
> * client receives CSA - disconnect if it can't be done
> * AP/GO wants CSA - refuse if it can't be done, let userspace sort it
> out
>
> In the first case, userspace still has the time between receiving the
> CSA and actually acting on it to make another decision.
Right, this is okay, but the point is, what happens to the *other*
interfaces?
What does "it can't be done" mean for the client? If there's a GO in the
same context and no free contexts for the switch, do we simply
disconnect the client (and leave the GO hanging in the same channel)? We
should probably tell the userspace and let it decide.
--
Luca.
This introduces the following benefits:
* cfg80211 is now aware of channel switching
(although more work still needs to be done wrt
interface combinations & multi-interface CSA)
* fixes some channel switching failcases by
disconnecting offending interfaces
* STA CSA no longer modifies BSS channel from
within mac80211
This aims to make the following possible later:
* defer channel switching decision to userspace
(if desired so),
* inform userspace what interfaces will be
possibly disconnected by a channel switch,
* disconnect non-complying interfaces that were
sharing a channel that is being abandoned by
channel switching interface(s),
* take channel switching into account in
interface combination checks.
Channel switching processing is deferred to a
event_list/work for locking reasons to provide
safe interface list iteration for future interface
combination checks. The downside is if a driver
misbehaves in ch_switch_start/finalize callbacks
it might stall the whole networking subsystem.
Signed-off-by: Michal Kazior <[email protected]>
---
include/net/cfg80211.h | 75 ++++++++++++++++++++--
net/mac80211/cfg.c | 50 +++++----------
net/mac80211/ibss.c | 5 +-
net/mac80211/mesh.c | 9 ++-
net/mac80211/mlme.c | 5 +-
net/wireless/ap.c | 5 +-
net/wireless/chan.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++
net/wireless/core.c | 23 ++++---
net/wireless/core.h | 18 ++++++
net/wireless/ibss.c | 1 +
net/wireless/mesh.c | 5 +-
net/wireless/nl80211.c | 20 ++++--
net/wireless/rdev-ops.h | 21 +++++--
net/wireless/sme.c | 1 +
net/wireless/trace.h | 17 ++++-
net/wireless/util.c | 6 ++
16 files changed, 352 insertions(+), 72 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f6651a2..b4ea9d0 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2273,7 +2273,18 @@ struct cfg80211_qos_map {
* reliability. This operation can not fail.
* @set_coalesce: Set coalesce parameters.
*
- * @channel_switch: initiate channel-switch procedure (with CSA)
+ * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
+ * prompt the driver to perform preparation work (start some timers,
+ * include CS IEs in beacons, etc) but don't do an actual channel switch.
+ * Once driver has completed all preparations and is ready for the actual
+ * switch (after CSA counter is completed) it must call
+ * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
+ * be called, but it doesn't necessarily happen immediately as cfg80211
+ * may need to synchronize with other interfaces. If channel switch is
+ * cancelled for some reason ch_switch_finalize() is not called and driver
+ * should free up resources/cleanup state in interface disconnection flow.
+ * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
+ * actual switch.
*
* @set_qos_map: Set QoS mapping information to the driver
*/
@@ -2514,9 +2525,12 @@ struct cfg80211_ops {
int (*set_coalesce)(struct wiphy *wiphy,
struct cfg80211_coalesce *coalesce);
- int (*channel_switch)(struct wiphy *wiphy,
- struct net_device *dev,
- struct cfg80211_csa_settings *params);
+ int (*ch_switch_start)(struct wiphy *wiphy,
+ struct net_device *dev,
+ struct cfg80211_csa_settings *params);
+ int (*ch_switch_finalize)(struct wiphy *wiphy,
+ struct net_device *dev);
+
int (*set_qos_map)(struct wiphy *wiphy,
struct net_device *dev,
struct cfg80211_qos_map *qos_map);
@@ -2775,6 +2789,26 @@ struct wiphy_vendor_command {
};
/**
+ * enum cfg80211_chsw_state - interface channel switch state
+ *
+ * @CHSW_STATE_NONE: channel switch is not active
+ * @CHSW_STATE_REQUESTED: channel switch has been requested but not processed
+ * @CHSW_STATE_STARTED: channel switch has been processed and accepted
+ * @CHSW_STATE_COMPLETED: channel switch has been scheduled for finalization
+ * @CHSW_STATE_SYNCING: interface is ready for channel to be re-programmed in
+ * driver. Interface may remain in this state until some additional
+ * requirements are met, i.e. wait until other channel switch requests on other
+ * interfaces are synching as well.
+ */
+enum cfg80211_chsw_state {
+ CHSW_STATE_NONE,
+ CHSW_STATE_REQUESTED,
+ CHSW_STATE_STARTED,
+ CHSW_STATE_COMPLETED,
+ CHSW_STATE_SYNCING,
+};
+
+/**
* struct wiphy - wireless hardware description
* @reg_notifier: the driver's regulatory notification callback,
* note that if your driver uses wiphy_apply_custom_regulatory()
@@ -3233,6 +3267,9 @@ struct wireless_dev {
bool cac_started;
unsigned long cac_start_time;
+ enum cfg80211_chsw_state chsw_state;
+ struct cfg80211_chan_def chsw_chandef;
+
#ifdef CONFIG_CFG80211_WEXT
/* wext data */
struct {
@@ -4672,6 +4709,36 @@ void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp);
*/
unsigned int ieee80211_get_num_supported_channels(struct wiphy *wiphy);
+/**
+ * cfg80211_ch_switch_request - request a channel switch
+ *
+ * If the request is accepted start_channel_switch() driver callback is called.
+ * Otherwise interface is stopped/disconnected.
+ *
+ * This is a generic channel switch callback and can be used with any interface
+ * type.
+ *
+ * @wdev: the wireless device to schedule channel switch on
+ * @params: channel switch parameters
+ *
+ * Return: less than zero on failure
+ */
+int cfg80211_ch_switch_request(struct wireless_dev *wdev,
+ struct cfg80211_csa_settings *params);
+
+/**
+ * cfg80211_ch_switch_complete - notify cfg80211 that channel switch is ready
+ *
+ * Driver must call this after CSA beacon counter has been completed and is
+ * ready to switch channel. It must not switch the channel until it is asked to
+ * by ch_switch_finalize() op is called.
+ *
+ * @wdev: the wireless device to schedule channel switch on
+ *
+ * Return: less than zero on failure
+ */
+int cfg80211_ch_switch_complete(struct wireless_dev *wdev);
+
/* Logging, debugging and troubleshooting/diagnostic helpers. */
/* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index dff4d3a..6ccfc81 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3000,8 +3000,10 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
}
EXPORT_SYMBOL(ieee80211_csa_finish);
-static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_ch_switch_finalize(struct wiphy *wiphy,
+ struct net_device *dev)
{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = sdata->local;
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
int err, changed = 0;
@@ -3034,9 +3036,6 @@ static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
switch (sdata->vif.type) {
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_P2P_CLIENT:
- /* XXX: shouldn't really modify cfg80211-owned data! */
- ifmgd->associated->channel = sdata->csa_chandef.chan;
-
ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
break;
case NL80211_IFTYPE_AP:
@@ -3075,34 +3074,14 @@ static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
IEEE80211_MAX_QUEUE_MAP,
IEEE80211_QUEUE_STOP_REASON_CSA);
- cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
-
return 0;
}
-void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
-{
- struct ieee80211_local *local = sdata->local;
- struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
-
- switch (sdata->vif.type) {
- case NL80211_IFTYPE_STATION:
- case NL80211_IFTYPE_P2P_CLIENT:
- ieee80211_queue_work(&local->hw,
- &ifmgd->csa_connection_drop_work);
- break;
- default:
- /* XXX: other iftypes should be halted too */
- break;
- }
-}
-
void ieee80211_csa_finalize_work(struct work_struct *work)
{
struct ieee80211_sub_if_data *sdata =
container_of(work, struct ieee80211_sub_if_data,
csa_finalize_work);
- int err;
sdata_lock(sdata);
/* AP might have been stopped while waiting for the lock. */
@@ -3112,16 +3091,14 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
if (!ieee80211_sdata_running(sdata))
goto unlock;
- err = ieee80211_csa_finalize(sdata);
- if (err)
- ieee80211_csa_disconnect(sdata);
+ cfg80211_ch_switch_complete(&sdata->wdev);
unlock:
sdata_unlock(sdata);
}
-int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
- struct cfg80211_csa_settings *params)
+static int __ieee80211_ch_switch_start(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_csa_settings *params)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_chanctx_conf *chanctx_conf;
@@ -3306,14 +3283,18 @@ int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
ieee80211_bss_info_change_notify(sdata, changed);
drv_channel_switch_beacon(sdata, ¶ms->chandef);
} else {
- /* if the beacon didn't change, we can finalize immediately */
- ieee80211_csa_finalize(sdata);
+ /* If the beacon didn't change, we can finalize immediately.
+ *
+ * Can't call cfg80211_ch_switch_complete() directly since
+ * we're in cfg80211 callback that touches chsw state */
+ ieee80211_queue_work(&sdata->local->hw,
+ &sdata->csa_finalize_work);
}
return 0;
}
-static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+static int ieee80211_ch_switch_start(struct wiphy *wiphy, struct net_device *dev,
struct cfg80211_csa_settings *params)
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -3321,7 +3302,7 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
int err;
mutex_lock(&local->chanctx_mtx);
- err = __ieee80211_channel_switch(sdata, params);
+ err = __ieee80211_ch_switch_start(sdata, params);
mutex_unlock(&local->chanctx_mtx);
return err;
@@ -4071,6 +4052,7 @@ const struct cfg80211_ops mac80211_config_ops = {
.get_et_strings = ieee80211_get_et_strings,
.get_channel = ieee80211_cfg_get_channel,
.start_radar_detection = ieee80211_start_radar_detection,
- .channel_switch = ieee80211_channel_switch,
+ .ch_switch_start = ieee80211_ch_switch_start,
+ .ch_switch_finalize = ieee80211_ch_switch_finalize,
.set_qos_map = ieee80211_set_qos_map,
};
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index a86e278..ae71e76 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -896,10 +896,7 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
params.block_tx = !!csa_ie.mode;
- mutex_lock(&sdata->local->chanctx_mtx);
- err = __ieee80211_channel_switch(sdata, ¶ms);
- mutex_unlock(&sdata->local->chanctx_mtx);
-
+ err = cfg80211_ch_switch_request(&sdata->wdev, ¶ms);
if (err < 0)
goto disconnect;
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 43dc808..ffa00e2 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -936,12 +936,11 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_REPEATER;
- mutex_lock(&sdata->local->chanctx_mtx);
- err = __ieee80211_channel_switch(sdata, ¶ms);
- mutex_unlock(&sdata->local->chanctx_mtx);
-
- if (err < 0)
+ err = cfg80211_ch_switch_request(&sdata->wdev, ¶ms);
+ if (err < 0) {
+ /* XXX: should disconnect? */
return false;
+ }
return true;
}
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 3aaf2f2..d4d7c23 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -966,10 +966,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
params.timestamp = timestamp;
params.count = csa_ie.count;
- mutex_lock(&local->chanctx_mtx);
- res = __ieee80211_channel_switch(sdata, ¶ms);
- mutex_unlock(&local->chanctx_mtx);
-
+ res = cfg80211_ch_switch_request(&sdata->wdev, ¶ms);
if (res) {
sdata_info(sdata, "channel switch failed: %d, disconnecting",
res);
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index 4760d65..f0b2f37 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -6,8 +6,8 @@
#include "rdev-ops.h"
-static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
- struct net_device *dev)
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+ struct net_device *dev)
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
int err;
@@ -29,6 +29,7 @@ static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
wdev->beacon_interval = 0;
wdev->channel = NULL;
wdev->ssid_len = 0;
+ wdev->chsw_state = CHSW_STATE_NONE;
rdev_set_qos_map(rdev, dev, NULL);
nl80211_send_ap_stopped(wdev);
}
diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 78559b5..d6903c0 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -704,3 +704,166 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
return;
}
+
+void __cfg80211_ch_switch_request(struct wireless_dev *wdev,
+ struct cfg80211_csa_settings *params)
+{
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+ struct wireless_dev *wdev_iter;
+ int err;
+
+ ASSERT_RTNL();
+ ASSERT_WDEV_LOCK(wdev);
+
+ /* XXX: prevent multi-interface channel switching until cfg80211 can be
+ * smart about it */
+ list_for_each_entry(wdev_iter, &rdev->wdev_list, list) {
+ if (!wdev_iter->netdev)
+ continue;
+
+ if (wdev_iter == wdev)
+ continue;
+
+ /*
+ * We are holding the "wdev" mutex, but now need to lock
+ * wdev_iter. This is OK because once we get here wdev_iter
+ * is not wdev (tested above), but we need to use the nested
+ * locking for lockdep.
+ */
+ mutex_lock_nested(&wdev_iter->mtx, 1);
+ __acquire(wdev_iter->mtx);
+ if (wdev_iter->chsw_state != CHSW_STATE_NONE) {
+ pr_warn("%s: another interface already is switching channel, disconnecting\n",
+ wdev->netdev->name);
+ __cfg80211_leave(rdev, wdev);
+ wdev_unlock(wdev_iter);
+ return;
+ }
+ wdev_unlock(wdev_iter);
+ }
+
+ if (WARN_ON(wdev->chsw_state != CHSW_STATE_REQUESTED)) {
+ __cfg80211_leave(rdev, wdev);
+ return;
+ }
+
+ /* XXX: once interface combinations are aware of channel switching
+ * check if this channel switch request fits - or it will fit once
+ * channel switch is completed - in any supported combination */
+
+ /* XXX: could notify userspace about channel switch being scheduled and
+ * possibly hint what interfaces might be disconnected if they don't
+ * switch as well */
+
+ err = rdev_ch_switch_start(rdev, wdev->netdev, params);
+ if (err) {
+ pr_warn("%s: driver failed to start channel switch (err = %d), disconnecting\n",
+ wdev->netdev->name, err);
+ __cfg80211_leave(rdev, wdev);
+ return;
+ }
+
+ wdev->chsw_state = CHSW_STATE_STARTED;
+ wdev->chsw_chandef = params->chandef;
+}
+
+int cfg80211_ch_switch_request(struct wireless_dev *wdev,
+ struct cfg80211_csa_settings *params)
+{
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+ struct cfg80211_event *ev;
+ unsigned long flags;
+
+ ASSERT_WDEV_LOCK(wdev);
+
+ if (wdev->chsw_state != CHSW_STATE_NONE)
+ return -EALREADY;
+
+ wdev->chsw_chandef = params->chandef;
+ wdev->chsw_state = CHSW_STATE_REQUESTED;
+
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
+
+ ev->type = EVENT_CH_SWITCH_REQUESTED;
+ ev->csa = *params;
+
+ spin_lock_irqsave(&wdev->event_lock, flags);
+ list_add_tail(&ev->list, &wdev->event_list);
+ spin_unlock_irqrestore(&wdev->event_lock, flags);
+ queue_work(cfg80211_wq, &rdev->event_work);
+
+ return 0;
+}
+EXPORT_SYMBOL(cfg80211_ch_switch_request);
+
+void __cfg80211_ch_switch_complete(struct wireless_dev *wdev)
+{
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+ int err;
+
+ ASSERT_RTNL();
+ ASSERT_WDEV_LOCK(wdev);
+
+ if (WARN_ON(wdev->chsw_state != CHSW_STATE_COMPLETED)) {
+ __cfg80211_leave(rdev, wdev);
+ return;
+ }
+
+ wdev->chsw_state = CHSW_STATE_SYNCING;
+
+ /* XXX: Should wait for other interfaces that are channel switching as
+ * well to make the best of interface combinations, e.g. if
+ * multi-channel is supported then waiting for other interfaces may not
+ * be necessary. */
+
+ /* XXX: Interfaces that didn't comply (i.e. weren't scheduled to switch
+ * channel and use the same channel as interfaces that completed
+ * channel switch) should be stopped/disconnected here */
+
+ err = rdev_ch_switch_finalize(rdev, wdev->netdev);
+ if (err < 0) {
+ pr_warn("%s: driver failed to finalize channel (err = %d), disconnecting\n",
+ wdev->netdev->name, err);
+ __cfg80211_leave(rdev, wdev);
+ return;
+ }
+
+ cfg80211_ch_switch_notify(wdev->netdev, &wdev->chsw_chandef);
+ wdev->chsw_state = CHSW_STATE_NONE;
+
+ if (wdev->iftype == NL80211_IFTYPE_STATION ||
+ wdev->iftype == NL80211_IFTYPE_P2P_CLIENT) {
+ if (wdev->current_bss)
+ wdev->current_bss->pub.channel = wdev->chsw_chandef.chan;
+ }
+}
+
+int cfg80211_ch_switch_complete(struct wireless_dev *wdev)
+{
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+ struct cfg80211_event *ev;
+ unsigned long flags;
+
+ ASSERT_WDEV_LOCK(wdev);
+
+ if (WARN_ON(wdev->chsw_state != CHSW_STATE_STARTED))
+ return -EINVAL;
+
+ wdev->chsw_state = CHSW_STATE_COMPLETED;
+
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
+
+ ev->type = EVENT_CH_SWITCH_COMPLETED;
+
+ spin_lock_irqsave(&wdev->event_lock, flags);
+ list_add_tail(&ev->list, &wdev->event_list);
+ spin_unlock_irqrestore(&wdev->event_lock, flags);
+ queue_work(cfg80211_wq, &rdev->event_work);
+
+ return 0;
+}
+EXPORT_SYMBOL(cfg80211_ch_switch_complete);
diff --git a/net/wireless/core.c b/net/wireless/core.c
index b5ff39a..b622312 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -751,23 +751,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
rdev->num_running_monitor_ifaces += num;
}
-void cfg80211_leave(struct cfg80211_registered_device *rdev,
- struct wireless_dev *wdev)
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev)
{
struct net_device *dev = wdev->netdev;
ASSERT_RTNL();
+ ASSERT_WDEV_LOCK(wdev);
switch (wdev->iftype) {
case NL80211_IFTYPE_ADHOC:
- cfg80211_leave_ibss(rdev, dev, true);
+ __cfg80211_leave_ibss(rdev, dev, true);
break;
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_STATION:
if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
__cfg80211_stop_sched_scan(rdev, false);
- wdev_lock(wdev);
#ifdef CONFIG_CFG80211_WEXT
kfree(wdev->wext.ie);
wdev->wext.ie = NULL;
@@ -776,14 +776,13 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
#endif
cfg80211_disconnect(rdev, dev,
WLAN_REASON_DEAUTH_LEAVING, true);
- wdev_unlock(wdev);
break;
case NL80211_IFTYPE_MESH_POINT:
- cfg80211_leave_mesh(rdev, dev);
+ __cfg80211_leave_mesh(rdev, dev);
break;
case NL80211_IFTYPE_AP:
case NL80211_IFTYPE_P2P_GO:
- cfg80211_stop_ap(rdev, dev);
+ __cfg80211_stop_ap(rdev, dev);
break;
default:
break;
@@ -792,6 +791,16 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
wdev->beacon_interval = 0;
}
+void cfg80211_leave(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev)
+{
+ ASSERT_RTNL();
+
+ wdev_lock(wdev);
+ __cfg80211_leave(rdev, wdev);
+ wdev_unlock(wdev);
+}
+
static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
unsigned long state, void *ptr)
{
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 37ec16d..4c8fb5d 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -181,6 +181,8 @@ enum cfg80211_event_type {
EVENT_ROAMED,
EVENT_DISCONNECTED,
EVENT_IBSS_JOINED,
+ EVENT_CH_SWITCH_REQUESTED,
+ EVENT_CH_SWITCH_COMPLETED,
};
struct cfg80211_event {
@@ -211,6 +213,7 @@ struct cfg80211_event {
struct {
u8 bssid[ETH_ALEN];
} ij;
+ struct cfg80211_csa_settings csa;
};
};
@@ -272,6 +275,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
struct net_device *dev,
struct mesh_setup *setup,
const struct mesh_config *conf);
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+ struct net_device *dev);
int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
struct net_device *dev);
int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
@@ -279,6 +284,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
struct cfg80211_chan_def *chandef);
/* AP */
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+ struct net_device *dev);
int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
struct net_device *dev);
@@ -372,6 +379,9 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
void cfg80211_process_rdev_events(struct cfg80211_registered_device *rdev);
void cfg80211_process_wdev_events(struct wireless_dev *wdev);
+void __cfg80211_chswitch_request(struct wireless_dev *wdev,
+ struct cfg80211_csa_settings *params);
+
int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev,
enum nl80211_iftype iftype,
@@ -456,12 +466,20 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
enum nl80211_iftype iftype, int num);
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev);
+
void cfg80211_leave(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
void cfg80211_stop_p2p_device(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
+void __cfg80211_ch_switch_request(struct wireless_dev *wdev,
+ struct cfg80211_csa_settings *params);
+
+void __cfg80211_ch_switch_complete(struct wireless_dev *wdev);
+
#define CFG80211_MAX_NUM_DIFFERENT_CHANNELS 10
#ifdef CONFIG_CFG80211_DEVELOPER_WARNINGS
diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index f911c5f..a4130e5 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -198,6 +198,7 @@ static void __cfg80211_clear_ibss(struct net_device *dev, bool nowext)
cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
}
+ wdev->chsw_state = CHSW_STATE_NONE;
wdev->current_bss = NULL;
wdev->ssid_len = 0;
#ifdef CONFIG_CFG80211_WEXT
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 8858624..f7792df 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -256,8 +256,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
return 0;
}
-static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
- struct net_device *dev)
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+ struct net_device *dev)
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
int err;
@@ -277,6 +277,7 @@ static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
if (!err) {
wdev->mesh_id_len = 0;
wdev->channel = NULL;
+ wdev->chsw_state = CHSW_STATE_NONE;
rdev_set_qos_map(rdev, dev, NULL);
}
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 28ab3a9..989951b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1471,7 +1471,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
CMD(crit_proto_start, CRIT_PROTOCOL_START);
CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
if (dev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH)
- CMD(channel_switch, CHANNEL_SWITCH);
+ CMD(ch_switch_start, CHANNEL_SWITCH);
}
CMD(set_qos_map, SET_QOS_MAP);
@@ -5846,7 +5846,7 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
int err;
bool need_new_beacon = false;
- if (!rdev->ops->channel_switch ||
+ if (!rdev->ops->ch_switch_start ||
!(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
return -EOPNOTSUPP;
@@ -5957,9 +5957,21 @@ skip_beacons:
params.block_tx = true;
wdev_lock(wdev);
- err = rdev_channel_switch(rdev, dev, ¶ms);
- wdev_unlock(wdev);
+ if (wdev->chsw_state != CHSW_STATE_NONE) {
+ err = -EALREADY;
+ goto unlock;
+ }
+
+ err = rdev_ch_switch_start(rdev, dev, ¶ms);
+ if (err)
+ goto unlock;
+
+ wdev->chsw_state = CHSW_STATE_STARTED;
+ wdev->chsw_chandef = params.chandef;
+
+unlock:
+ wdev_unlock(wdev);
return err;
}
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index c8e2259..780de5e 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -920,14 +920,25 @@ static inline void rdev_crit_proto_stop(struct cfg80211_registered_device *rdev,
trace_rdev_return_void(&rdev->wiphy);
}
-static inline int rdev_channel_switch(struct cfg80211_registered_device *rdev,
- struct net_device *dev,
- struct cfg80211_csa_settings *params)
+static inline int rdev_ch_switch_start(struct cfg80211_registered_device *rdev,
+ struct net_device *dev,
+ struct cfg80211_csa_settings *params)
+{
+ int ret;
+
+ trace_rdev_ch_switch_start(&rdev->wiphy, dev, params);
+ ret = rdev->ops->ch_switch_start(&rdev->wiphy, dev, params);
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
+static inline int rdev_ch_switch_finalize(struct cfg80211_registered_device *rdev,
+ struct net_device *dev)
{
int ret;
- trace_rdev_channel_switch(&rdev->wiphy, dev, params);
- ret = rdev->ops->channel_switch(&rdev->wiphy, dev, params);
+ trace_rdev_ch_switch_finalize(&rdev->wiphy, dev);
+ ret = rdev->ops->ch_switch_finalize(&rdev->wiphy, dev);
trace_rdev_return_int(&rdev->wiphy, ret);
return ret;
}
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index c854f1c..3590c3c 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -977,6 +977,7 @@ int cfg80211_disconnect(struct cfg80211_registered_device *rdev,
kfree(wdev->connect_keys);
wdev->connect_keys = NULL;
+ wdev->chsw_state = CHSW_STATE_NONE;
if (wdev->conn)
err = cfg80211_sme_disconnect(wdev, reason);
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index fbcc23e..d3fd363 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1864,7 +1864,7 @@ TRACE_EVENT(rdev_crit_proto_stop,
WIPHY_PR_ARG, WDEV_PR_ARG)
);
-TRACE_EVENT(rdev_channel_switch,
+TRACE_EVENT(rdev_ch_switch_start,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
struct cfg80211_csa_settings *params),
TP_ARGS(wiphy, netdev, params),
@@ -1897,6 +1897,21 @@ TRACE_EVENT(rdev_channel_switch,
__entry->counter_offset_presp)
);
+TRACE_EVENT(rdev_ch_switch_finalize,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
+ TP_ARGS(wiphy, netdev),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT,
+ WIPHY_PR_ARG, NETDEV_PR_ARG)
+);
+
TRACE_EVENT(rdev_set_qos_map,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
struct cfg80211_qos_map *qos_map),
diff --git a/net/wireless/util.c b/net/wireless/util.c
index d39c371..350a244 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -822,6 +822,12 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
case EVENT_IBSS_JOINED:
__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid);
break;
+ case EVENT_CH_SWITCH_REQUESTED:
+ __cfg80211_ch_switch_request(wdev, &ev->csa);
+ break;
+ case EVENT_CH_SWITCH_COMPLETED:
+ __cfg80211_ch_switch_complete(wdev);
+ break;
}
wdev_unlock(wdev);
--
1.8.5.3
On Tue, 2014-02-04 at 13:26 +0100, Michal Kazior wrote:
> > I think mac80211 should keep the client CSA control and the userspace
> > should control the host interfaces. cfg80211 should keep track of the
> > channel switches and be asked if the switch is okay (so it can take the
> > interface combinations and regulatory into consideration).
>
> Why does mac80211 need to keep CSA control? Does it solve anything?
It makes more sense. You're too focused on ath10k. Think about ath6kl,
for example.
johannes
On 4 February 2014 10:09, Luca Coelho <[email protected]> wrote:
> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>> Managed interface channel switching code was
>> mostly redundant.
>>
>> This makes it easier to do more channel switching
>> code refactoring.
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>> ---
>
> Looks good! Just some nitpicks below (and I left the timstamp discussion
> between you and Johannes ;)
>
> [...]
>
>> @@ -3011,51 +3012,89 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>> sdata->radar_required = sdata->csa_radar_required;
>> err = ieee80211_vif_change_channel(sdata, &changed);
>> mutex_unlock(&local->mtx);
>> - if (WARN_ON(err < 0))
>> - return;
>> +
>> + sdata->vif.csa_active = false;
>> +
>> + if (WARN_ON(err < 0)) {
>> + sdata_info(sdata, "vif channel switch failed\n");
>> + return err;
>> + }
>
> Maybe WARN(err < 0, "vif channel switch failed\n") instead?
I don't mind :)
>> +void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
>> +{
>> + struct ieee80211_local *local = sdata->local;
>> + struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
>> +
>> + switch (sdata->vif.type) {
>> + case NL80211_IFTYPE_STATION:
>> + case NL80211_IFTYPE_P2P_CLIENT:
>> + ieee80211_queue_work(&local->hw,
>> + &ifmgd->csa_connection_drop_work);
>> + break;
>> + default:
>> + /* XXX: other iftypes should be halted too */
>
> Good point. This case would suck, but we need to do something because
> the stations will think that we're on the different channel at this
> point. But maybe instead the userspace should be notified instead of
> halting here? It could retry with a count 0, for example.
Ideally cfg80211 should be the one stopping interfaces. This way you'd
get AP stopped event (recently done by Johannes) and userspace can do
something about (e.g. re-start APs from scratch).
>> -int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>> - struct cfg80211_csa_settings *params)
>> +int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
>> + struct cfg80211_csa_settings *params)
>
> Why did you have to split this function? All cases where you call
> __ieee80211_channel_switch() you lock->call->unlock anyway.
It was shorter. The function does a bunch of return's so I'd have to
do err+goto. Since it asserts sdata lock is held I decided to assert
chanctx_mtx is held as well and lock the thing from call sites.
>> @@ -998,6 +944,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>> if (res)
>> return;
>>
>> + /* FIXME: This check should be moved to cfg80211 */
>
> This is not really related to this patch.
>
>
>> if (!cfg80211_chandef_usable(local->hw.wiphy, &csa_ie.chandef,
>> IEEE80211_CHAN_DISABLED)) {
>> sdata_info(sdata,
Ideally mac80211 shouldn't be the one checking that - it should be
cfg80211, shouldn't it?
>> static u32 ieee80211_handle_pwr_constr(struct ieee80211_sub_if_data *sdata,
>> @@ -1758,6 +1690,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>> del_timer_sync(&sdata->u.mgd.timer);
>> del_timer_sync(&sdata->u.mgd.chswitch_timer);
>>
>> + sdata->vif.csa_active = false;
>> sdata->vif.bss_conf.dtim_period = 0;
>> sdata->vif.bss_conf.beacon_rate = NULL;
>
> This is also not directly related. It's actually a bug fix for the
> existing code, isn't it?
Good point.
>
>
> [...]
>
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index 8b32a1f..28ab3a9 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -11240,7 +11240,9 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
>> if (WARN_ON(wdev->iftype != NL80211_IFTYPE_AP &&
>> wdev->iftype != NL80211_IFTYPE_P2P_GO &&
>> wdev->iftype != NL80211_IFTYPE_ADHOC &&
>> - wdev->iftype != NL80211_IFTYPE_MESH_POINT))
>> + wdev->iftype != NL80211_IFTYPE_MESH_POINT &&
>> + wdev->iftype != NL80211_IFTYPE_STATION &&
>> + wdev->iftype != NL80211_IFTYPE_P2P_CLIENT))
>> return;
>
> You should update the NL80211_CMD_CH_SWITCH_NOTIFY documentation in
> nl80211.h.
Good point. Thanks!
Michał
On Tue, 2014-02-04 at 12:07 +0200, Luca Coelho wrote:
> > Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
> > possible (userspace CSA) or cause the switching interface to disconnect,
> > rather than *others*??
>
> It depends. And this logic is too complicated to stay in the kernel,
> IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> GO. Now, if you have an AP (with tons of STAs connected to it) and a
> P2P client gets a CSA for whatever reason, do we really want to stop the
> AP?
Well, what I was describing was really only the default policy if
userspace didn't do anything useful, which IMHO should really just be:
* client receives CSA - disconnect if it can't be done
* AP/GO wants CSA - refuse if it can't be done, let userspace sort it
out
In the first case, userspace still has the time between receiving the
CSA and actually acting on it to make another decision.
johannes
On 4 February 2014 11:07, Luca Coelho <[email protected]> wrote:
> On Mon, 2014-02-03 at 14:41 +0100, Johannes Berg wrote:
>> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>> > This introduces the following benefits:
>> > * cfg80211 is now aware of channel switching
>> > (although more work still needs to be done wrt
>> > interface combinations & multi-interface CSA)
>> > * fixes some channel switching failcases by
>> > disconnecting offending interfaces
>> > * STA CSA no longer modifies BSS channel from
>> > within mac80211
>>
>> That's nice.
>>
>> > This aims to make the following possible later:
>> > * defer channel switching decision to userspace
>> > (if desired so),
>>
>> That's probably not needed, it can disconnect after the CSA event (that
>> should be sent when the CSA is first received)
>
> I agree that this shouldn't be needed for client (ie. P2P client or
> managed) interfaces.
>
>
>> > * inform userspace what interfaces will be
>> > possibly disconnected by a channel switch,
>>
>> Huh? Not sure I get that part, why would userspace ever be notified
>> about something that *will* happen? Either the interface disconnects
>> when the CSA is received, or it just switches and userspace gets a "CSA
>> will happen" event?
True. Userspace can probably deduce from interface combinations some
interfaces require attention due to channel switching.
> How do we get the "target" beacon from userspace if the interface just
> switches?
Do you really need the beacon? What for?
> Now a bit of brain burp: I think that the "count" decision should remain
> in the userspace so it can decide to give more time for its stations to
> switch. Eg. if the client interface got a CSA with count == x and a
> host interface has dtim_interval > x, the userspace can send a "quiet"
> CSA with count == dtim_interval + 1. The two requests would be "merged"
> and the highest count would win. The client would be a bit late on the
> new channel, but at least the AP wouldn't lose most of its clients.
> Does this make any sense? I'm not sure myself. :)
Makes sense.
>> > * disconnect non-complying interfaces that were
>> > sharing a channel that is being abandoned by
>> > channel switching interface(s),
>>
>> Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
>> possible (userspace CSA) or cause the switching interface to disconnect,
>> rather than *others*??
>
> It depends. And this logic is too complicated to stay in the kernel,
> IMHO. If we are in a GO-follows-STA scenario, we want to disconnect the
> GO. Now, if you have an AP (with tons of STAs connected to it) and a
> P2P client gets a CSA for whatever reason, do we really want to stop the
> AP?
Agreed.
But doesn't this imply STA CSA shouldn't be started within kernel
itself? Otherwise you leave a corner case when STA CSA is very short
making it impossible for userspace to take any action.
>> > - * @channel_switch: initiate channel-switch procedure (with CSA)
>> > + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
>> > + * prompt the driver to perform preparation work (start some timers,
>> > + * include CS IEs in beacons, etc) but don't do an actual channel switch.
>> > + * Once driver has completed all preparations and is ready for the actual
>> > + * switch (after CSA counter is completed) it must call
>> > + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
>> > + * be called, but it doesn't necessarily happen immediately as cfg80211
>> > + * may need to synchronize with other interfaces. If channel switch is
>> > + * cancelled for some reason ch_switch_finalize() is not called and driver
>> > + * should free up resources/cleanup state in interface disconnection flow.
>> > + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
>> > + * actual switch.
>>
>> I don't like this at all. This totally assumes that every driver behaves
>> like mac80211, which clearly is not the case. The split between
>> "starting" and "finalizing" it should not be part of the API.
>
> I agree, especially if the driver offloads the channel switch, in which
> case it wouldn't be possible for cfg80211 to hold the finalize call to
> sync all the interfaces.
This implies we don't care if channel switching breaks interface
combinations, right?
Michał
On Mon, 2014-02-03 at 13:43 +0100, Johannes Berg wrote:
> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>
> > The patchset aims at making cfg80211 aware of
> > channel switches.
>
> That seems like a worthy goal :)
I like the "awareness", but I don't like the control over channel switch
so much.
> > This makes it possible for more elegant channel
> > switching behavior by moving the decision up to
> > cfg80211.
> >
> > Until now mac80211 could start channel switching
> > internally for STA, mesh and IBSS interfaces
> > without userspace interaction. This bypassed
> > interface combination checks at the very least.
> >
> > Now mac80211 requests cfg80211 to channel switch
> > an interface, in a similar manner as userspace
> > asks cfg80211 for a channel switch. This makes it
> > possible to perform interface combination checks
> > (albeit it is not implemented yet).
>
> Couldn't you just return the decision to mac80211? It would also have to
> keep track of start/end of the CSA period, I guess, since a few things
> that Ilan is working on shouldn't be allowed while the AP announces CSA.
mac80211 could keep track of the start/finalize callbacks. But I don't
think that's a good idea, as I mentioned in the other email.
I think mac80211 should keep the client CSA control and the userspace
should control the host interfaces. cfg80211 should keep track of the
channel switches and be asked if the switch is okay (so it can take the
interface combinations and regulatory into consideration).
Maybe mac80211 could call a cfg80211_reserve_channel() function? If that
call fails, it means the switch cannot be done. If it succeeds,
cfg80211 keeps the reservation. Same thing if the switch is coming from
userspace, nl80211 could call cfg80211_reserve_channel() in the same
way.
> > The channel switch is split into two phases now -
> > start and finalize. This is required to make
> > cfg80211 in control of the whole channel
> > switching process. It also makes it apparent that
> > mac80211's STA CSA offload doesn't work quite well
> > here. Can we kill it?
>
> No. Well, you could make cfg80211 be in charge, but not wpa_supplicant,
> I think.
>
> > The initial patchset has a lot of TODOs in the
> > code and the idea is to provide follow up patches
> > that implement missing functionality.
> >
> > Major drawback now is cfg80211 will refuse a
> > channel switch if there's one in progress already,
> > although this shouldn't be much of a problem in
> > most use cases.
> >
> > This approach should be suited for multi-interface
> > CSA as well as some other cases such as
> > "GO-follows-STA".
> >
> > The patchset seemed to work in my limited testing
> > (ath9k as AP and iwldvm as STA).
>
> Nice. I hope Luca can take a look too :)
I did. :) Hopefully I have added good points and not only complicated
things further. :P
--
Luca.
On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> This introduces the following benefits:
> * cfg80211 is now aware of channel switching
> (although more work still needs to be done wrt
> interface combinations & multi-interface CSA)
> * fixes some channel switching failcases by
> disconnecting offending interfaces
> * STA CSA no longer modifies BSS channel from
> within mac80211
That's nice.
> This aims to make the following possible later:
> * defer channel switching decision to userspace
> (if desired so),
That's probably not needed, it can disconnect after the CSA event (that
should be sent when the CSA is first received)
> * inform userspace what interfaces will be
> possibly disconnected by a channel switch,
Huh? Not sure I get that part, why would userspace ever be notified
about something that *will* happen? Either the interface disconnects
when the CSA is received, or it just switches and userspace gets a "CSA
will happen" event?
> * disconnect non-complying interfaces that were
> sharing a channel that is being abandoned by
> channel switching interface(s),
Hmm, that sounds a bit the wrong way around? Shouldn't the CSA not be
possible (userspace CSA) or cause the switching interface to disconnect,
rather than *others*??
> - * @channel_switch: initiate channel-switch procedure (with CSA)
> + * @ch_switch_start: initiate channel-switch procedure (with CSA). This should
> + * prompt the driver to perform preparation work (start some timers,
> + * include CS IEs in beacons, etc) but don't do an actual channel switch.
> + * Once driver has completed all preparations and is ready for the actual
> + * switch (after CSA counter is completed) it must call
> + * cfg80211_ch_switch_complete(wdev). After that ch_switch_finalize() MAY
> + * be called, but it doesn't necessarily happen immediately as cfg80211
> + * may need to synchronize with other interfaces. If channel switch is
> + * cancelled for some reason ch_switch_finalize() is not called and driver
> + * should free up resources/cleanup state in interface disconnection flow.
> + * @ch_switch_finalize: finalize channel-switch procedure, i.e. perform the
> + * actual switch.
I don't like this at all. This totally assumes that every driver behaves
like mac80211, which clearly is not the case. The split between
"starting" and "finalizing" it should not be part of the API.
> /**
> + * enum cfg80211_chsw_state - interface channel switch state
> + *
> + * @CHSW_STATE_NONE: channel switch is not active
> + * @CHSW_STATE_REQUESTED: channel switch has been requested but not processed
> + * @CHSW_STATE_STARTED: channel switch has been processed and accepted
> + * @CHSW_STATE_COMPLETED: channel switch has been scheduled for finalization
> + * @CHSW_STATE_SYNCING: interface is ready for channel to be re-programmed in
> + * driver. Interface may remain in this state until some additional
> + * requirements are met, i.e. wait until other channel switch requests on other
> + * interfaces are synching as well.
> + */
This therefore doesn't belong into cfg80211, at least not all of it.
[snip rest that seemed pointless to look at now]
johannes
On 4 February 2014 11:27, Luca Coelho <[email protected]> wrote:
> On Mon, 2014-02-03 at 13:43 +0100, Johannes Berg wrote:
>> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>> > This makes it possible for more elegant channel
>> > switching behavior by moving the decision up to
>> > cfg80211.
>> >
>> > Until now mac80211 could start channel switching
>> > internally for STA, mesh and IBSS interfaces
>> > without userspace interaction. This bypassed
>> > interface combination checks at the very least.
>> >
>> > Now mac80211 requests cfg80211 to channel switch
>> > an interface, in a similar manner as userspace
>> > asks cfg80211 for a channel switch. This makes it
>> > possible to perform interface combination checks
>> > (albeit it is not implemented yet).
>>
>> Couldn't you just return the decision to mac80211? It would also have to
>> keep track of start/end of the CSA period, I guess, since a few things
>> that Ilan is working on shouldn't be allowed while the AP announces CSA.
>
> mac80211 could keep track of the start/finalize callbacks. But I don't
> think that's a good idea, as I mentioned in the other email.
>
> I think mac80211 should keep the client CSA control and the userspace
> should control the host interfaces. cfg80211 should keep track of the
> channel switches and be asked if the switch is okay (so it can take the
> interface combinations and regulatory into consideration).
Why does mac80211 need to keep CSA control? Does it solve anything?
> Maybe mac80211 could call a cfg80211_reserve_channel() function? If that
> call fails, it means the switch cannot be done. If it succeeds,
> cfg80211 keeps the reservation. Same thing if the switch is coming from
> userspace, nl80211 could call cfg80211_reserve_channel() in the same
> way.
The problem is you need RTNL for this to make any sense (interface
combinations need it). You can't make a directly callable cfg80211
function available in mac80211 that takes RTNL. It just won't work.
The only way I see it, is you need an asynchronous call with a
callback.
Michał
On Tue, 2014-02-04 at 13:11 +0200, Luca Coelho wrote:
> > Well, what I was describing was really only the default policy if
> > userspace didn't do anything useful, which IMHO should really just be:
> >
> > * client receives CSA - disconnect if it can't be done
> > * AP/GO wants CSA - refuse if it can't be done, let userspace sort it
> > out
> >
> > In the first case, userspace still has the time between receiving the
> > CSA and actually acting on it to make another decision.
>
> Right, this is okay, but the point is, what happens to the *other*
> interfaces?
Nothing! As far as that's permissible. Ilan's patchset might change
that, but that's mostly a regulatory thing, rather than a CSA thing.
> What does "it can't be done" mean for the client? If there's a GO in the
> same context and no free contexts for the switch, do we simply
> disconnect the client (and leave the GO hanging in the same channel)? We
> should probably tell the userspace and let it decide.
Yes, but we should only disconnect when the CSA actually needs to be
done, rather than immediately on receiving it.
> ...and, if the userspace doesn't react, we disconnect the GO. I think
> it's safer this way for the GO-follows-STA case.
That would be a consequence of Ilan's work, yes.
This typically won't happen, since userspace will do something else, but
we need to have some default policy. I don't think anything else makes
much sense?
johannes
Managed interface channel switching code was
mostly redundant.
This makes it easier to do more channel switching
code refactoring.
Signed-off-by: Michal Kazior <[email protected]>
---
include/net/cfg80211.h | 5 ++
net/mac80211/cfg.c | 110 +++++++++++++++++++++++++++++--------
net/mac80211/ibss.c | 7 ++-
net/mac80211/ieee80211_i.h | 6 ++-
net/mac80211/mesh.c | 7 ++-
net/mac80211/mlme.c | 132 +++++++++++----------------------------------
net/wireless/nl80211.c | 4 +-
7 files changed, 143 insertions(+), 128 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 459700e..f6651a2 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -686,6 +686,10 @@ struct cfg80211_ap_settings {
* @radar_required: whether radar detection is required on the new channel
* @block_tx: whether transmissions should be blocked while changing
* @count: number of beacons until switch
+ * @timestamp: value in microseconds of the 64-bit Time Synchronization
+ * Function (TSF) timer when the frame containing the channel switch
+ * announcement was received. This is simply the rx.mactime parameter
+ * the driver passed in.
*/
struct cfg80211_csa_settings {
struct cfg80211_chan_def chandef;
@@ -695,6 +699,7 @@ struct cfg80211_csa_settings {
bool radar_required;
bool block_tx;
u8 count;
+ u64 timestamp;
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 875e63d..dff4d3a 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3000,9 +3000,10 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
}
EXPORT_SYMBOL(ieee80211_csa_finish);
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_local *local = sdata->local;
+ struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
int err, changed = 0;
sdata_assert_lock(sdata);
@@ -3011,51 +3012,89 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
sdata->radar_required = sdata->csa_radar_required;
err = ieee80211_vif_change_channel(sdata, &changed);
mutex_unlock(&local->mtx);
- if (WARN_ON(err < 0))
- return;
+
+ sdata->vif.csa_active = false;
+
+ if (WARN_ON(err < 0)) {
+ sdata_info(sdata, "vif channel switch failed\n");
+ return err;
+ }
if (!local->use_chanctx) {
local->_oper_chandef = sdata->csa_chandef;
- ieee80211_hw_config(local, 0);
+ /* XXX: Needs more work for multi-interface support */
+ if (local->ops->channel_switch &&
+ (sdata->vif.type == NL80211_IFTYPE_STATION ||
+ sdata->vif.type == NL80211_IFTYPE_P2P_CLIENT))
+ local->hw.conf.chandef = local->_oper_chandef;
+ else
+ ieee80211_hw_config(local, 0);
}
- sdata->vif.csa_active = false;
switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ case NL80211_IFTYPE_P2P_CLIENT:
+ /* XXX: shouldn't really modify cfg80211-owned data! */
+ ifmgd->associated->channel = sdata->csa_chandef.chan;
+
+ ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+ break;
case NL80211_IFTYPE_AP:
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;
+ return err;
changed |= err;
break;
case NL80211_IFTYPE_ADHOC:
err = ieee80211_ibss_finish_csa(sdata);
if (err < 0)
- return;
+ return err;
changed |= err;
break;
#ifdef CONFIG_MAC80211_MESH
case NL80211_IFTYPE_MESH_POINT:
err = ieee80211_mesh_finish_csa(sdata);
if (err < 0)
- return;
+ return err;
changed |= err;
break;
#endif
default:
WARN_ON(1);
- return;
+ return -EINVAL;
}
ieee80211_bss_info_change_notify(sdata, changed);
+ /* XXX: wait for a beacon before continuing if there's at least one
+ * managed interface taking part in the switch? */
ieee80211_wake_queues_by_reason(&sdata->local->hw,
IEEE80211_MAX_QUEUE_MAP,
IEEE80211_QUEUE_STOP_REASON_CSA);
cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+ return 0;
+}
+
+void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ case NL80211_IFTYPE_P2P_CLIENT:
+ ieee80211_queue_work(&local->hw,
+ &ifmgd->csa_connection_drop_work);
+ break;
+ default:
+ /* XXX: other iftypes should be halted too */
+ break;
+ }
}
void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3063,6 +3102,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
struct ieee80211_sub_if_data *sdata =
container_of(work, struct ieee80211_sub_if_data,
csa_finalize_work);
+ int err;
sdata_lock(sdata);
/* AP might have been stopped while waiting for the lock. */
@@ -3072,16 +3112,17 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
if (!ieee80211_sdata_running(sdata))
goto unlock;
- ieee80211_csa_finalize(sdata);
+ err = ieee80211_csa_finalize(sdata);
+ if (err)
+ ieee80211_csa_disconnect(sdata);
unlock:
sdata_unlock(sdata);
}
-int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_csa_settings *params)
+int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_csa_settings *params)
{
- struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = sdata->local;
struct ieee80211_chanctx_conf *chanctx_conf;
struct ieee80211_chanctx *chanctx;
@@ -3089,6 +3130,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
int err, num_chanctx, changed = 0;
sdata_assert_lock(sdata);
+ lockdep_assert_held(&local->chanctx_mtx);
if (!list_empty(&local->roc_list) || local->scanning)
return -EBUSY;
@@ -3100,32 +3142,44 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
&sdata->vif.bss_conf.chandef))
return -EINVAL;
- rcu_read_lock();
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
- if (!chanctx_conf) {
- rcu_read_unlock();
+ if (WARN_ON(!chanctx_conf))
return -EBUSY;
- }
/* don't handle for multi-VIF cases */
chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
if (chanctx->refcount > 1) {
- rcu_read_unlock();
+ sdata_info(sdata,
+ "channel switch with multiple interfaces on the same channel\n");
return -EBUSY;
}
+
num_chanctx = 0;
list_for_each_entry_rcu(chanctx, &local->chanctx_list, list)
num_chanctx++;
- rcu_read_unlock();
- if (num_chanctx > 1)
- return -EBUSY;
+ if (num_chanctx > 1) {
+ if (sdata->vif.type == NL80211_IFTYPE_STATION ||
+ sdata->vif.type == NL80211_IFTYPE_P2P_CLIENT) {
+ if (!(local->hw.flags & IEEE80211_HW_CHANCTX_STA_CSA)) {
+ sdata_info(sdata,
+ "not handling chan-switch with channel contexts\n");
+ return -EBUSY;
+ }
+ } else {
+ return -EBUSY;
+ }
+ }
/* don't allow another channel switch if one is already active. */
if (sdata->vif.csa_active)
return -EBUSY;
switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ case NL80211_IFTYPE_P2P_CLIENT:
+ ieee80211_sta_chswitch_schedule(sdata, params);
+ break;
case NL80211_IFTYPE_AP:
sdata->u.ap.next_beacon =
cfg80211_beacon_dup(¶ms->beacon_after);
@@ -3259,6 +3313,20 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
return 0;
}
+static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+ struct cfg80211_csa_settings *params)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_local *local = sdata->local;
+ int err;
+
+ mutex_lock(&local->chanctx_mtx);
+ err = __ieee80211_channel_switch(sdata, params);
+ mutex_unlock(&local->chanctx_mtx);
+
+ return err;
+}
+
static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct cfg80211_mgmt_tx_params *params,
u64 *cookie)
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 8e44447..a86e278 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -896,8 +896,11 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
params.block_tx = !!csa_ie.mode;
- if (ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
- ¶ms))
+ mutex_lock(&sdata->local->chanctx_mtx);
+ err = __ieee80211_channel_switch(sdata, ¶ms);
+ mutex_unlock(&sdata->local->chanctx_mtx);
+
+ if (err < 0)
goto disconnect;
ieee80211_ibss_csa_mark_radar(sdata);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d37dc75..9a06580 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1460,8 +1460,10 @@ void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
/* channel switch handling */
void ieee80211_csa_finalize_work(struct work_struct *work);
-int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_csa_settings *params);
+int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_csa_settings *params);
+void ieee80211_sta_chswitch_schedule(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_csa_settings *params);
/* interface handling */
int ieee80211_iface_init(void);
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index f70e9cd..43dc808 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -936,8 +936,11 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_REPEATER;
- if (ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
- ¶ms) < 0)
+ mutex_lock(&sdata->local->chanctx_mtx);
+ err = __ieee80211_channel_switch(sdata, ¶ms);
+ mutex_unlock(&sdata->local->chanctx_mtx);
+
+ if (err < 0)
return false;
return true;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6c9ebca..3aaf2f2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -882,61 +882,6 @@ static void ieee80211_send_4addr_nullfunc(struct ieee80211_local *local,
ieee80211_tx_skb(sdata, skb);
}
-/* spectrum management related things */
-static void ieee80211_chswitch_work(struct work_struct *work)
-{
- struct ieee80211_sub_if_data *sdata =
- container_of(work, struct ieee80211_sub_if_data, u.mgd.chswitch_work);
- struct ieee80211_local *local = sdata->local;
- struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
- u32 changed = 0;
- int ret;
-
- if (!ieee80211_sdata_running(sdata))
- return;
-
- sdata_lock(sdata);
- 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");
- ieee80211_queue_work(&sdata->local->hw,
- &ifmgd->csa_connection_drop_work);
- goto out;
- }
-
- if (!local->use_chanctx) {
- local->_oper_chandef = sdata->csa_chandef;
- /* Call "hw_config" only if doing sw channel switch.
- * Otherwise update the channel directly
- */
- if (!local->ops->channel_switch)
- ieee80211_hw_config(local, 0);
- else
- local->hw.conf.chandef = local->_oper_chandef;
- }
-
- /* XXX: shouldn't really modify cfg80211-owned data! */
- ifmgd->associated->channel = sdata->csa_chandef.chan;
-
- /* XXX: wait for a beacon first? */
- ieee80211_wake_queues_by_reason(&local->hw,
- IEEE80211_MAX_QUEUE_MAP,
- IEEE80211_QUEUE_STOP_REASON_CSA);
-
- ieee80211_bss_info_change_notify(sdata, changed);
-
- out:
- sdata->vif.csa_active = false;
- ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
- sdata_unlock(sdata);
-}
-
void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success)
{
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
@@ -949,7 +894,8 @@ void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success)
ieee80211_queue_work(&sdata->local->hw,
&ifmgd->csa_connection_drop_work);
} else {
- ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
+ ieee80211_queue_work(&sdata->local->hw,
+ &sdata->csa_finalize_work);
}
}
EXPORT_SYMBOL(ieee80211_chswitch_done);
@@ -959,7 +905,7 @@ static void ieee80211_chswitch_timer(unsigned long data)
struct ieee80211_sub_if_data *sdata =
(struct ieee80211_sub_if_data *) data;
- ieee80211_queue_work(&sdata->local->hw, &sdata->u.mgd.chswitch_work);
+ ieee80211_queue_work(&sdata->local->hw, &sdata->csa_finalize_work);
}
static void
@@ -970,9 +916,9 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
struct ieee80211_local *local = sdata->local;
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
struct cfg80211_bss *cbss = ifmgd->associated;
- struct ieee80211_chanctx *chanctx;
enum ieee80211_band current_band;
struct ieee80211_csa_ie csa_ie;
+ struct cfg80211_csa_settings params;
int res;
sdata_assert_lock(sdata);
@@ -998,6 +944,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
if (res)
return;
+ /* FIXME: This check should be moved to cfg80211 */
if (!cfg80211_chandef_usable(local->hw.wiphy, &csa_ie.chandef,
IEEE80211_CHAN_DISABLED)) {
sdata_info(sdata,
@@ -1013,56 +960,41 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
- mutex_lock(&local->chanctx_mtx);
- if (local->use_chanctx) {
- u32 num_chanctx = 0;
- list_for_each_entry(chanctx, &local->chanctx_list, list)
- num_chanctx++;
+ memset(¶ms, 0, sizeof(params));
+ params.chandef = csa_ie.chandef;
+ params.block_tx = !!(csa_ie.mode);
+ params.timestamp = timestamp;
+ params.count = csa_ie.count;
- if (num_chanctx > 1 ||
- !(local->hw.flags & IEEE80211_HW_CHANCTX_STA_CSA)) {
- sdata_info(sdata,
- "not handling chan-switch with channel contexts\n");
- ieee80211_queue_work(&local->hw,
- &ifmgd->csa_connection_drop_work);
- mutex_unlock(&local->chanctx_mtx);
- return;
- }
- }
+ mutex_lock(&local->chanctx_mtx);
+ res = __ieee80211_channel_switch(sdata, ¶ms);
+ mutex_unlock(&local->chanctx_mtx);
- if (WARN_ON(!rcu_access_pointer(sdata->vif.chanctx_conf))) {
- ieee80211_queue_work(&local->hw,
- &ifmgd->csa_connection_drop_work);
- mutex_unlock(&local->chanctx_mtx);
- return;
- }
- chanctx = container_of(rcu_access_pointer(sdata->vif.chanctx_conf),
- struct ieee80211_chanctx, conf);
- if (chanctx->refcount > 1) {
- sdata_info(sdata,
- "channel switch with multiple interfaces on the same channel, disconnecting\n");
+ if (res) {
+ sdata_info(sdata, "channel switch failed: %d, disconnecting",
+ res);
ieee80211_queue_work(&local->hw,
&ifmgd->csa_connection_drop_work);
- mutex_unlock(&local->chanctx_mtx);
- return;
}
- mutex_unlock(&local->chanctx_mtx);
+}
- sdata->csa_chandef = csa_ie.chandef;
- sdata->vif.csa_active = true;
+void ieee80211_sta_chswitch_schedule(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_csa_settings *params)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+ struct cfg80211_bss *cbss = ifmgd->associated;
- if (csa_ie.mode)
- ieee80211_stop_queues_by_reason(&local->hw,
- IEEE80211_MAX_QUEUE_MAP,
- IEEE80211_QUEUE_STOP_REASON_CSA);
+ sdata_assert_lock(sdata);
+ /* XXX: This needs more work for multi-interface support */
if (local->ops->channel_switch) {
/* use driver's channel switch callback */
struct ieee80211_channel_switch ch_switch = {
- .timestamp = timestamp,
- .block_tx = csa_ie.mode,
- .chandef = csa_ie.chandef,
- .count = csa_ie.count,
+ .timestamp = params->timestamp,
+ .block_tx = params->block_tx,
+ .chandef = params->chandef,
+ .count = params->count,
};
drv_channel_switch(local, &ch_switch);
@@ -1070,11 +1002,11 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
}
/* channel switch handled in software */
- if (csa_ie.count <= 1)
+ if (params->count <= 1)
ieee80211_queue_work(&local->hw, &ifmgd->chswitch_work);
else
mod_timer(&ifmgd->chswitch_timer,
- TU_TO_EXP_TIME(csa_ie.count * cbss->beacon_interval));
+ TU_TO_EXP_TIME(params->count * cbss->beacon_interval));
}
static u32 ieee80211_handle_pwr_constr(struct ieee80211_sub_if_data *sdata,
@@ -1758,6 +1690,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
del_timer_sync(&sdata->u.mgd.timer);
del_timer_sync(&sdata->u.mgd.chswitch_timer);
+ sdata->vif.csa_active = false;
sdata->vif.bss_conf.dtim_period = 0;
sdata->vif.bss_conf.beacon_rate = NULL;
@@ -3523,7 +3456,6 @@ void ieee80211_sta_setup_sdata(struct ieee80211_sub_if_data *sdata)
ifmgd = &sdata->u.mgd;
INIT_WORK(&ifmgd->monitor_work, ieee80211_sta_monitor_work);
- INIT_WORK(&ifmgd->chswitch_work, ieee80211_chswitch_work);
INIT_WORK(&ifmgd->beacon_connection_loss_work,
ieee80211_beacon_connection_loss_work);
INIT_WORK(&ifmgd->csa_connection_drop_work,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8b32a1f..28ab3a9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11240,7 +11240,9 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
if (WARN_ON(wdev->iftype != NL80211_IFTYPE_AP &&
wdev->iftype != NL80211_IFTYPE_P2P_GO &&
wdev->iftype != NL80211_IFTYPE_ADHOC &&
- wdev->iftype != NL80211_IFTYPE_MESH_POINT))
+ wdev->iftype != NL80211_IFTYPE_MESH_POINT &&
+ wdev->iftype != NL80211_IFTYPE_STATION &&
+ wdev->iftype != NL80211_IFTYPE_P2P_CLIENT))
return;
wdev->channel = chandef->chan;
--
1.8.5.3
On Tue, 2014-02-04 at 12:29 +0100, Michal Kazior wrote:
> On 4 February 2014 10:09, Luca Coelho <[email protected]> wrote:
> > On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> >> +void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
> >> +{
> >> + struct ieee80211_local *local = sdata->local;
> >> + struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
> >> +
> >> + switch (sdata->vif.type) {
> >> + case NL80211_IFTYPE_STATION:
> >> + case NL80211_IFTYPE_P2P_CLIENT:
> >> + ieee80211_queue_work(&local->hw,
> >> + &ifmgd->csa_connection_drop_work);
> >> + break;
> >> + default:
> >> + /* XXX: other iftypes should be halted too */
> >
> > Good point. This case would suck, but we need to do something because
> > the stations will think that we're on the different channel at this
> > point. But maybe instead the userspace should be notified instead of
> > halting here? It could retry with a count 0, for example.
>
> Ideally cfg80211 should be the one stopping interfaces. This way you'd
> get AP stopped event (recently done by Johannes) and userspace can do
> something about (e.g. re-start APs from scratch).
Yeah, this is a tricky case and the best we can do is probably just stop
the interface.
> >> -int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> >> - struct cfg80211_csa_settings *params)
> >> +int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
> >> + struct cfg80211_csa_settings *params)
> >
> > Why did you have to split this function? All cases where you call
> > __ieee80211_channel_switch() you lock->call->unlock anyway.
>
> It was shorter. The function does a bunch of return's so I'd have to
> do err+goto. Since it asserts sdata lock is held I decided to assert
> chanctx_mtx is held as well and lock the thing from call sites.
Fair enough.
> >> @@ -998,6 +944,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
> >> if (res)
> >> return;
> >>
> >> + /* FIXME: This check should be moved to cfg80211 */
> >
> > This is not really related to this patch.
> >
> >
> >> if (!cfg80211_chandef_usable(local->hw.wiphy, &csa_ie.chandef,
> >> IEEE80211_CHAN_DISABLED)) {
> >> sdata_info(sdata,
>
> Ideally mac80211 shouldn't be the one checking that - it should be
> cfg80211, shouldn't it?
Yes, but this patch doesn't change anything regarding this. In any
case, it's an informative FIXME, so I guess it can go together with any
patch (it would be silly to have a separate patch just to add the
FIXME). So, fine by me. :)
--
Luca.
On Mon, 2014-02-03 at 13:43 +0100, Johannes Berg wrote:
> > The channel switch is split into two phases now -
> > start and finalize. This is required to make
> > cfg80211 in control of the whole channel
> > switching process. It also makes it apparent that
> > mac80211's STA CSA offload doesn't work quite well
> > here. Can we kill it?
>
> No. Well, you could make cfg80211 be in charge, but not wpa_supplicant,
> I think.
Actually, that might not make sense either, most non-mac80211 devices
would probably do that in firmware anyway and locking in the API to do
it "like mac80211" would thus probably be a bad idea.
johannes
On Wed, 2014-02-05 at 14:11 +0200, Luca Coelho wrote:
> It's the same if the STA CSA would be "started" in the userspace. If
> the remote AP sets a too short count, the userspace won't have the
> chance to react. In either case, we would have to send a notification
> to the userspace and it would have to tell us what to do. If we wait
> for the userspace to react on our STA CSA, we will be too late on both
> interfaces. If mac80211 does the STA CSA directly, at least the STA CSA
> will be done on time and the GO can move a bit later (or get
> disconnected if we were short on chanctx's).
I don't think the GO would get disconnected, rather the STA would, no?
> > The purpose of ch_switch_finalize() is to perform the final switch
> > atomically and make sure non-complying interfaces are disconnected
> > before channel is actually switched (this could include Ilan's GO case
> > in a clean way).
>
> I was thinking about eventual drivers that offload the whole channel
> switch process to the firmware. The driver just sends the count, the
> new channel and everything and the firmware takes care of it all. It
> will beacon for count TBTTs and perform the actual channel switch at the
> right time. In this case, all the decision making about what needs to
> be disconnected or switched must be made at the beginning.
>
> But I'll let Johannes comment more. ;)
Yes, I'm pretty sure that this kind of situation will happen sooner or
later, so preventing it in the APIs right now seems like a bad idea.
johannes
On 3 February 2014 14:18, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
>> + * @timestamp: value in microseconds of the 64-bit Time Synchronization
>> + * Function (TSF) timer when the frame containing the channel switch
>> + * announcement was received. This is simply the rx.mactime parameter
>> + * the driver passed in.
>
> That doesn't make a lot of sense, since "rx.mactime" is a mac80211
> thing, so you should probably describe this. I'm not sure it really
> needs to be mactime/TSF, does that even make sense? It seems more likely
> you'd actually need the device time, i.e. something that works across
> multiple vifs?
>
> Not sure this even belongs into this patch anyway though?
It does belong here. How else are you supposed to pass the timestamp
for STA CSA offload?
Would making this sort of a drv_priv thing work?
If we could just kill the offload, adding timestamp to
cfg80211_csa_settings wouldn't be necessary.
Michał
On Mon, 2014-02-03 at 10:58 +0100, Michal Kazior wrote:
> Managed interface channel switching code was
> mostly redundant.
>
> This makes it easier to do more channel switching
> code refactoring.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
Looks good! Just some nitpicks below (and I left the timstamp discussion
between you and Johannes ;)
[...]
> @@ -3011,51 +3012,89 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> sdata->radar_required = sdata->csa_radar_required;
> err = ieee80211_vif_change_channel(sdata, &changed);
> mutex_unlock(&local->mtx);
> - if (WARN_ON(err < 0))
> - return;
> +
> + sdata->vif.csa_active = false;
> +
> + if (WARN_ON(err < 0)) {
> + sdata_info(sdata, "vif channel switch failed\n");
> + return err;
> + }
Maybe WARN(err < 0, "vif channel switch failed\n") instead?
[...]
> +void ieee80211_csa_disconnect(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
> +
> + switch (sdata->vif.type) {
> + case NL80211_IFTYPE_STATION:
> + case NL80211_IFTYPE_P2P_CLIENT:
> + ieee80211_queue_work(&local->hw,
> + &ifmgd->csa_connection_drop_work);
> + break;
> + default:
> + /* XXX: other iftypes should be halted too */
Good point. This case would suck, but we need to do something because
the stations will think that we're on the different channel at this
point. But maybe instead the userspace should be notified instead of
halting here? It could retry with a count 0, for example.
[...]
> @@ -3072,16 +3112,17 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
> if (!ieee80211_sdata_running(sdata))
> goto unlock;
>
> - ieee80211_csa_finalize(sdata);
> + err = ieee80211_csa_finalize(sdata);
> + if (err)
> + ieee80211_csa_disconnect(sdata);
>
> unlock:
> sdata_unlock(sdata);
> }
>
> -int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> - struct cfg80211_csa_settings *params)
> +int __ieee80211_channel_switch(struct ieee80211_sub_if_data *sdata,
> + struct cfg80211_csa_settings *params)
Why did you have to split this function? All cases where you call
__ieee80211_channel_switch() you lock->call->unlock anyway.
[...]
> @@ -998,6 +944,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
> if (res)
> return;
>
> + /* FIXME: This check should be moved to cfg80211 */
This is not really related to this patch.
> if (!cfg80211_chandef_usable(local->hw.wiphy, &csa_ie.chandef,
> IEEE80211_CHAN_DISABLED)) {
> sdata_info(sdata,
[...]
> static u32 ieee80211_handle_pwr_constr(struct ieee80211_sub_if_data *sdata,
> @@ -1758,6 +1690,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> del_timer_sync(&sdata->u.mgd.timer);
> del_timer_sync(&sdata->u.mgd.chswitch_timer);
>
> + sdata->vif.csa_active = false;
> sdata->vif.bss_conf.dtim_period = 0;
> sdata->vif.bss_conf.beacon_rate = NULL;
This is also not directly related. It's actually a bug fix for the
existing code, isn't it?
[...]
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 8b32a1f..28ab3a9 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -11240,7 +11240,9 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
> if (WARN_ON(wdev->iftype != NL80211_IFTYPE_AP &&
> wdev->iftype != NL80211_IFTYPE_P2P_GO &&
> wdev->iftype != NL80211_IFTYPE_ADHOC &&
> - wdev->iftype != NL80211_IFTYPE_MESH_POINT))
> + wdev->iftype != NL80211_IFTYPE_MESH_POINT &&
> + wdev->iftype != NL80211_IFTYPE_STATION &&
> + wdev->iftype != NL80211_IFTYPE_P2P_CLIENT))
> return;
You should update the NL80211_CMD_CH_SWITCH_NOTIFY documentation in
nl80211.h.
--
Luca.