Subject: Re: [PATCHv7 2/3] mac80211: add radar detection command/event

On Thu, 2013-01-31 at 17:31 +0100, Simon Wunderlich wrote:

> > > +void ieee80211_dfs_cac_timer_work(struct work_struct *work)
> > > +{
> > > + struct delayed_work *delayed_work =
> > > + container_of(work, struct delayed_work, work);
> > > + struct ieee80211_sub_if_data *sdata =
> > > + container_of(delayed_work, struct ieee80211_sub_if_data,
> > > + dfs_cac_timer_work);
> > > +
> > > + rtnl_lock();
> >
> > what part requires rtnl?
> >
> ieee80211_vif_release_channel() calls __ieee80211_vif_release_channel()
> and has ASSERT_RTNL() before parsing the AP VLAN list.
>
> cfg80211_radar_event() probably doesn't need it ... I should remove it
> from the rtnl lock, I guess?

Doesn't really matter, I was just wondering in general but the AP VLAN
list makes sense.

> > > +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp)
> > > +{
> > > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> > > +
> > > + trace_api_radar_detected(sdata);
> > > +
> > > + /* may happen to devices which have been added but are not up */
> > > + if (!cfg80211_chandef_valid(&sdata->vif.bss_conf.chandef))
> > > + return;
> >
> > Huh, what does device and up have to do with that?
> >
>
> What I've tried:
> * configure 2 SSIDs in hostapd, start it
> * both wlan0 and wlan0-1 got created
> * only wlan0 comes up, wlan0-1 was rejected because of missing channel combinations
> * now I've injected a radar - which should be sent to wlan0 and wlan0-1
> * wlan0 could send the event, but wlan0-1 had no bss configured and therefore no chandef
>
> I can change this comment to "may happen to devices which have currently no BSS configured",
> maybe that it is not so confusing ...

Not sure I understand, how would the radar detected event come to an
interface that doesn't really exist for the driver?

johannes

2013-01-31 14:25:11

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCHv7 1/3] nl80211/cfg80211: add radar detection command/event

On Tue, 2013-01-29 at 13:21 +0100, Simon Wunderlich wrote:

> From: Victor Goldenshtein <[email protected]>

Probably time for you to claim ownership ... ;-)

> Changes to PATCHv6:

Thanks for your patience and the frequent posting :-)

> /**
> + * enum ieee80211_dfs_state - DFS states for channels
> + *
> + * Channel states used by the DFS code.
> + *
> + * @IEEE80211_DFS_USABLE: The channel can be used, but channel availability
> + * check (CAC) must be performed before using it for AP or IBSS.
> + * @IEEE80211_DFS_UNAVAILABLE: A radar has been detected on this channel, it
> + * is therefore marked as not available.
> + * @IEEE80211_DFS_AVAILABLE: The channel has been CAC checked and is available.
> + */
> +
> +enum ieee80211_dfs_state {
> + IEEE80211_DFS_USABLE,
> + IEEE80211_DFS_UNAVAILABLE,
> + IEEE80211_DFS_AVAILABLE,

Should UNAVAILABLE be = 0, so that's the default?

> @@ -133,6 +151,9 @@ enum ieee80211_channel_flags {
> * to enable this, this is useful only on 5 GHz band.
> * @orig_mag: internal use
> * @orig_mpwr: internal use
> + * @dfs_state: current state of this channel. Only relevant if radar is required
> + * on this channel.
> + * @dfs_state_entered: time when the dfs state was entered.

This is relevant for "unavailable", presumably, to make sure it stays
there for long enough? What unit is that, and how long does it have to
stay? "jiffies" can become unreliable after a long uptime so that might
cause issues. It's unlikely, the issue would be that ~25 days after it
was supposed to be available again it would be rejected as jiffies got
back to the same value ... :)
Also depends on your implementation which I haven't seen yet.

> @@ -412,6 +435,16 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
> u32 prohibited_flags);
>
> /**
> + * cfg80211_chandef_dfs_required - checks if radar detection
> + * is required on any of the channels
> + * @wiphy: the wiphy to validate against
> + * @chandef: the channel definition to check
> + * Return: 1 if radar detection is required, 0 if it is not, < 0 on error
> + */
> +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> + const struct cfg80211_chan_def *c);

Why does that need to be exported to mac80211/drivers? Shouldn't
cfg80211 be able to check everything?

> struct wireless_dev {
> struct wiphy *wiphy;
> @@ -2638,6 +2678,8 @@ struct wireless_dev {
>
> u32 ap_unexpected_nlportid;
>
> + bool cac_started;

Don't you need a cac_chandef or something?

> /**
> + * cfg80211_radar - radar detection event
> + * @dev: network device
> + * @chandef: chandef for the current channel

Doesn't cfg80211 know what channel the device is operating/doing CAC on?

> +#define NL80211_DFS_MIN_CAC_TIME_MS 60000
> +#define NL80211_DFS_MIN_NOP_TIME_MS (30 * 60 * 1000)

Why are those needed in the userspace API?

> @@ -3221,6 +3240,7 @@ enum nl80211_feature_flags {
> NL80211_FEATURE_P2P_GO_CTWIN = 1 << 11,
> NL80211_FEATURE_P2P_GO_OPPPS = 1 << 12,
> NL80211_FEATURE_FULL_AP_CLIENT_STATE = 1 << 13,
> + NL80211_FEATURE_DFS = 1 << 14,

I don't think we need this any more with the interface combinations
thing?

> +static void cfg80211_set_chans_dfs_state(struct wiphy *wiphy, u32 center_freq,
> + u32 bandwidth,
> + enum ieee80211_dfs_state dfs_state)
> +{
> + struct ieee80211_channel *c;
> + u32 freq;
> +
> + for (freq = center_freq - bandwidth/2 + 10;
> + freq <= center_freq + bandwidth/2 - 10;
> + freq += 20) {
> + c = ieee80211_get_channel(wiphy, freq);
> + if (!c || !(c->flags & IEEE80211_CHAN_RADAR))
> + continue;
> +
> + c->dfs_state = dfs_state;
> + c->dfs_state_entered = jiffies;

I wonder if it'd make sense to not skip if the radar flag isn't set,
that could be relevant with regdom changes? OTOH, if the regdom changes
(much) I *suspect* we need to invalidate all the radar measurements
anyway since the rules might now be different?

> +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> + const struct cfg80211_chan_def *chandef)
> +{
> + u32 width;
> + int r;
> +
> + if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> + return -EINVAL;
> +
> + width = cfg80211_chandef_get_width(chandef);
> + if (width < 0)

never true with a u32, I think you might want to change the function
prototype and the variable :)

> @@ -344,7 +467,10 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
> break;
> case NL80211_IFTYPE_AP:
> case NL80211_IFTYPE_P2P_GO:
> - if (wdev->beacon_interval) {
> + if (wdev->cac_started) {
> + *chan = wdev->preset_chandef.chan;
> + *chanmode = CHAN_MODE_SHARED;

Ah, ok, so you're using the preset_chandef for CAC as well. I'm not
entirely sure that is correct though, since it could be changed by
userspace without much effect, e.g. by setting the channel with iw or
iwconfig? Unless you added "if (!cac_started)" there everywhere?

> +static inline void __cfg80211_dfs_clear_channel(struct ieee80211_channel *c,

no reason for inline

> + bool *check_again,
> + unsigned long *check_again_time)
> +{
> + unsigned long timeout;
> +
> + if (c->dfs_state == IEEE80211_DFS_UNAVAILABLE) {

could save on indentation by returning early if it's not unavailable :-)

I guess this addresses the jiffies concern I had above.

> + timeout = c->dfs_state_entered + NL80211_DFS_MIN_NOP_TIME_MS;
> + if (time_after_eq(jiffies, timeout)) {
> + /* TODO: we could notify userspace about that change */
> + c->dfs_state = IEEE80211_DFS_USABLE;
> + } else {
> + if (!*check_again)

should probably set it to false when the function starts, now you rely
on it being set outside which is a bit odd? (or did I just snip that
from my reply and don't see it any more?)

> + err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
> + if (err < 1)
> + return err;

That doesn't make sense, if userspace starts CAC and that is successful
it would expect to eventually receive an event that it completed? Thus
if you return 0 here it would get confused, no?

> + if (chandef.chan->dfs_state != IEEE80211_DFS_USABLE)
> + return -EINVAL;
> +
> + if (!rdev->ops->start_radar_detection)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&rdev->devlist_mtx);
> + err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
> + chandef.chan, CHAN_MODE_SHARED,
> + BIT(chandef.width));
> + mutex_unlock(&rdev->devlist_mtx);

You need to keep holding the mutex until you've set cac_started to true
(or failed), otherwise you introduce races.

> + if (err)
> + return err;
> +
> + err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, &chandef);
> + if (!err) {
> + wdev->preset_chandef = chandef;
> + wdev->cac_started = true;
> + chandef.chan->dfs_state_entered = jiffies;

No reason to assign dfs_state_entered since it won't be used in this
state anyway, will it?

> @@ -5575,6 +5623,10 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
> if (!cfg80211_reg_can_beacon(&rdev->wiphy, &ibss.chandef))
> return -EINVAL;
>
> + if (cfg80211_chandef_usable(&rdev->wiphy, &ibss.chandef,
> + IEEE80211_CHAN_NO_IBSS))
> + return -EINVAL;

That seems like an unrelated change/fix?

> + /* reason is unspecified, just notify that CAC has failed. */
> + if (nla_put_u8(msg, NL80211_ATTR_RADAR_EVENT, event))

I think enums should generally be u32.

> +++ b/net/wireless/reg.c
> @@ -884,6 +884,9 @@ static void handle_channel(struct wiphy *wiphy,
> return;
> }
>
> + chan->dfs_state = IEEE80211_DFS_USABLE;
> + chan->dfs_state_entered = jiffies;

Here also you don't really need the time assignment.

(I skipped this before, so pasting here)

> +void cfg80211_radar_event(struct net_device *netdev,
> + struct cfg80211_chan_def *chandef,
> + enum nl80211_radar_event event,
> + gfp_t gfp)
> +{
> + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> + struct wiphy *wiphy = wdev->wiphy;
> + struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> + unsigned long timeout;
> +
> + if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> + return;
> +
> + trace_cfg80211_radar_event(netdev, chandef, event);
> +
> + switch (event) {
> + case NL80211_RADAR_DETECTED:
> + /*
> + * only set the chandef supplied channel to unavailable, in
> + * case the radar is detected on only one of multiple channels
> + * spanned by the chandef.
> + */
> + cfg80211_set_dfs_state(wiphy, chandef,
> + IEEE80211_DFS_UNAVAILABLE);
> +
> + timeout = msecs_to_jiffies(NL80211_DFS_MIN_NOP_TIME_MS);
> + queue_delayed_work(cfg80211_wq, &rdev->dfs_update_channels_wk,
> + timeout);
> + break;
> + case NL80211_CAC_FINISHED:
> + timeout = wdev->preset_chandef.chan->dfs_state_entered;
> + timeout = msecs_to_jiffies(timeout +
> + NL80211_DFS_MIN_CAC_TIME_MS);
> + WARN_ON(!time_after_eq(jiffies, timeout));
> + cfg80211_set_dfs_state(wiphy, &wdev->preset_chandef,
> + IEEE80211_DFS_AVAILABLE);
> + break;
> + case NL80211_CAC_ABORTED:
> + /* Shouldn't happen if CAC was never started before. */
> + WARN_ON(!wdev->cac_started);
> + break;
> + default:
> + break;

I think a warning (and maybe return) would be useful here.

> + }
> +
> + wdev->cac_started = false;

Whew. :)
Overall I think this is looking good, mostly minor comments.

johannes

2013-01-29 14:52:36

by Simon Wunderlich

[permalink] [raw]

Subject: Re: [PATCHv7 0/3] Add DFS master ability

Hey Zefir,

On Tue, Jan 29, 2013 at 02:14:26PM +0100, Zefir Kurtisi wrote:
> On 01/29/2013 01:21 PM, Simon Wunderlich wrote:
> > [...]
> >
> > The channel states are now implemented in cfg80211. Shall we inform userspace
> > about channel changes? If yes, how should we do that? We could add channel states
> > to the channel list, and give "channel list changed" events to userspace as it
> > happens now, or define a new kind of event ("channel-available-again-event").
> > Suggestions welcome. :)
> >
> An event whenever a channel state changes is perfect, ideally provided with the
> time-stamp when this change happened.

Yeah. "channel list changed" events are already available (at least I get them in
hostapd). If we put the current state + timestamp when entered into the channel list,
we would have what you want, too.

>
> With the centralized channel state handling proposed here, the required
> modification to allow managed DFS operation can be minimized to a proprietary (or
> even upstreamed but CONFIG_CFG80211_CERTIFICATION_ONUS guarded) function to modify
> channel states.

Yes, the idea is that one can have a small proprietary patch or default-off command
(CONFIG_CFG80211_CERTIFICATION_ONUS sounds good) or something like that to change a
DFS state with the new cfg80211 function. I also thought about having a nl80211-command
without implementing an iw counter part or keeping a special "state change" program
somewhere with appropriate warnings. But this might still be too "liberal" for the
regulatory statement. :)
>
> Can't contribute much to code review, but full ACK for the updated concept.
>

Cool, also thanks for the code comments!

Cheers,
Simon

Attachments:

(No filename) (1.65 kB)
signature.asc (198.00 B)
Digital signature Download all attachments

2013-01-31 17:54:37

On Thu, Jan 31, 2013 at 03:44:08PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 13:21 +0100, Simon Wunderlich wrote:
>
> > +void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
> > + struct ieee80211_chanctx *chanctx)
> > +{
> > + struct ieee80211_sub_if_data *sdata;
> > + bool radar_enabled = false;
> > +
> > + lockdep_assert_held(&local->chanctx_mtx);
> > +
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > + if (sdata->radar_required)
> > + radar_enabled = true;
>
> I guess you could break here.

yes, OK.

> Technically though I'm not sure this is
> right, since it should only iterate interfaces on this chanctx. OTOH,
> multiple channel contexts aren't (currently?) allowed anyway.

I wonder of multiple channel contexts ever make sense for the DFS case.
If they do, we would have to change this anyway to detect radars on
one channel but (maybe) not on the other ... Anyway, I would leave that
to future strategists. ;)

>
> > + chanctx->conf.radar_enabled = radar_enabled;
> > + local->radar_detect_enabled = chanctx->conf.radar_enabled;
>
> What's the reason for "local->radar_detect_enabled"?

Interaction checking with ROC and scan.
>
> Btw, do we also make the driver responsible for turning off any
> powersave when radar detection is enabled? I guess so?
>
> > @@ -817,6 +817,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
> >
> > cancel_work_sync(&sdata->recalc_smps);
> >
> > + cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
> > +
> > + /* only inform about abort cac if it was started before. */
> > + if (sdata->wdev.cac_started) {
>
> I'm not entirely sure we should use the wdev data here, OTOH it seems
> safe, so why not.
>

If you don't mind, it's easier like that ... ;)

> > +void ieee80211_dfs_cac_timer_work(struct work_struct *work)
> > +{
> > + struct delayed_work *delayed_work =
> > + container_of(work, struct delayed_work, work);
> > + struct ieee80211_sub_if_data *sdata =
> > + container_of(delayed_work, struct ieee80211_sub_if_data,
> > + dfs_cac_timer_work);
> > +
> > + rtnl_lock();
>
> what part requires rtnl?
>
ieee80211_vif_release_channel() calls __ieee80211_vif_release_channel()
and has ASSERT_RTNL() before parsing the AP VLAN list.

cfg80211_radar_event() probably doesn't need it ... I should remove it
from the rtnl lock, I guess?

> > +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp)
> > +{
> > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> > +
> > + trace_api_radar_detected(sdata);
> > +
> > + /* may happen to devices which have been added but are not up */
> > + if (!cfg80211_chandef_valid(&sdata->vif.bss_conf.chandef))
> > + return;
>
> Huh, what does device and up have to do with that?
>

What I've tried:
* configure 2 SSIDs in hostapd, start it
* both wlan0 and wlan0-1 got created
* only wlan0 comes up, wlan0-1 was rejected because of missing channel combinations
* now I've injected a radar - which should be sent to wlan0 and wlan0-1
* wlan0 could send the event, but wlan0-1 had no bss configured and therefore no chandef

I can change this comment to "may happen to devices which have currently no BSS configured",
maybe that it is not so confusing ...

> > static bool ieee80211_can_scan(struct ieee80211_local *local,
> > struct ieee80211_sub_if_data *sdata)
> > {
> > + if (local->radar_detect_enabled)
> > + return false;
>
> Oh, ok. I guess that explains the reason for radar_detect_enabled :)

Yup. :)

Cheers,
Simon

Attachments:

(No filename) (3.50 kB)
signature.asc (198.00 B)
Digital signature Download all attachments

2013-01-31 14:43:58

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCHv7 2/3] mac80211: add radar detection command/event

On Tue, 2013-01-29 at 13:21 +0100, Simon Wunderlich wrote:

> +void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
> + struct ieee80211_chanctx *chanctx)
> +{
> + struct ieee80211_sub_if_data *sdata;
> + bool radar_enabled = false;
> +
> + lockdep_assert_held(&local->chanctx_mtx);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + if (sdata->radar_required)
> + radar_enabled = true;

I guess you could break here. Technically though I'm not sure this is
right, since it should only iterate interfaces on this chanctx. OTOH,
multiple channel contexts aren't (currently?) allowed anyway.

> + chanctx->conf.radar_enabled = radar_enabled;
> + local->radar_detect_enabled = chanctx->conf.radar_enabled;

What's the reason for "local->radar_detect_enabled"?

Btw, do we also make the driver responsible for turning off any
powersave when radar detection is enabled? I guess so?

> @@ -817,6 +817,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>
> cancel_work_sync(&sdata->recalc_smps);
>
> + cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
> +
> + /* only inform about abort cac if it was started before. */
> + if (sdata->wdev.cac_started) {

I'm not entirely sure we should use the wdev data here, OTOH it seems
safe, so why not.

> +void ieee80211_dfs_cac_timer_work(struct work_struct *work)
> +{
> + struct delayed_work *delayed_work =
> + container_of(work, struct delayed_work, work);
> + struct ieee80211_sub_if_data *sdata =
> + container_of(delayed_work, struct ieee80211_sub_if_data,
> + dfs_cac_timer_work);
> +
> + rtnl_lock();

what part requires rtnl?

> +void ieee80211_radar_detected(struct ieee80211_vif *vif, gfp_t gfp)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +
> + trace_api_radar_detected(sdata);
> +
> + /* may happen to devices which have been added but are not up */
> + if (!cfg80211_chandef_valid(&sdata->vif.bss_conf.chandef))
> + return;

Huh, what does device and up have to do with that?

> static bool ieee80211_can_scan(struct ieee80211_local *local,
> struct ieee80211_sub_if_data *sdata)
> {
> + if (local->radar_detect_enabled)
> + return false;

Oh, ok. I guess that explains the reason for radar_detect_enabled :)

johannes

2013-01-30 11:51:47

by Zefir Kurtisi

[permalink] [raw]

Subject: Re: [PATCHv7 1/3] nl80211/cfg80211: add radar detection command/event

On 01/29/2013 03:36 PM, Simon Wunderlich wrote:
> Hey Zefir,
>
> On Tue, Jan 29, 2013 at 02:48:53PM +0100, Zefir Kurtisi wrote:
>> On 01/29/2013 01:21 PM, Simon Wunderlich wrote:
>>> From: Victor Goldenshtein <[email protected]>
>>>
>>> [...]
>>>
>>> /**
>>> + * enum ieee80211_dfs_state - DFS states for channels
>>> + *
>>> + * Channel states used by the DFS code.
>>> + *
>>> + * @IEEE80211_DFS_USABLE: The channel can be used, but channel availability
>>> + * check (CAC) must be performed before using it for AP or IBSS.
>>> + * @IEEE80211_DFS_UNAVAILABLE: A radar has been detected on this channel, it
>>> + * is therefore marked as not available.
>>> + * @IEEE80211_DFS_AVAILABLE: The channel has been CAC checked and is available.
>>> + */
>>> +
>>> +enum ieee80211_dfs_state {
>>> + IEEE80211_DFS_USABLE,
>>> + IEEE80211_DFS_UNAVAILABLE,
>>> + IEEE80211_DFS_AVAILABLE,
>>> +};
>>> +
>> Not sure if IEEE80211_DFS_UNKNOWN is not missing here, i.e. whether a channel that
>> never passed a CAC (or the CAC has been aborted) is always USABLE. Once I realized
>> why ETSI defined an UNKNOWN state, but forgot meanwhile - so maybe only relevant
>> for managed operation (like an UNKNOWN state can be overridden by external
>> information, whereas e.g. UNAVAILABLE can't).
>
> I've seen the UNKNOWN state in your last mail and your last slides, but didn't see
> a purpose for it. If you can remember what we need it for, please tell me. :) We could
> reject changes for an UNAVAILABLE channel from outside access if this is required.

Double checked it and realized that ETSI removed that state between v1.5 and v1.7.
Before, it enabled the frequency manager to differentiate between untouched
channels and channels that passed a NOP (which is the only way to become USABLE).

>>
>> Furthermore, is there a reason to define an additional wireless_dev.cac_started
>> flag vs. adding a IEEE80211_DFS_CAC state?
>
> We have discussed that we want to move cac-information from channel into wdev, so I
> just did that.
>
Sorry, did not follow that discussion fully (due to my inability to follow).
Having a dedicated CAC state would provide a convenient way to directly retrieve
the state transition info, but I understand it makes no sense to have it redundant.

> The idea was that a CAC can always be started, and state transition is only performed
> after a succesful CAC. This simplifies the state machine and some corner cases. A
> channel stays USABLE through the CAC, and is only changed to AVAILABLE after CAC is
> completed. We also don't forbid checking the same channel on different WiFi modules,
> if any of them completes the channel is changed to AVAILABLE.
>
Really, a CAC is done whenever you switch to a DFS channel? I'd expect that
switching from an operating channel and back does not require a CAC (that's the
sole purpose of the AVAILABLE state and a centralized state handler).

> The same goes for the OPERATING state (defined in ETSI at least) - I don't have this
> as dfs state, because it is just the same as AVAILABLE plus the information that
> we currently use it, and that we know in the driver. Adding the state would just
> add management overhead with no practical gain, IMHO.
>
Would there be a benefit of an OPERATING state on systems with multiple wiphys?

> Cheers,
> Simon
>

2013-01-31 16:13:58

On Thu, Jan 31, 2013 at 05:46:00PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 17:13 +0100, Simon Wunderlich wrote:
>
> > > > +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > > > + const struct cfg80211_chan_def *c);
> > >
> > > Why does that need to be exported to mac80211/drivers? Shouldn't
> > > cfg80211 be able to check everything?
> > >
> > I'm using it in mac80211/ieee80211_start_ap() to find out whether radar detection
> > is required. We could put into struct cfg80211_ap_settings *params if you prefer?
> > I guess similar params exist for IBSS.
>
> I kinda think that would make more sense because then it's right there
> when you look at the parameters, rather than having to remember it.
>

OK, will change that.

> > > Don't you need a cac_chandef or something?
>
> > The chandef is stored into wdev->preset_chandef anyway, so I didn't see any
> > need to save it twice?
>
> (I'm just worried about that changing, see below. We could use
> "wdev->channel" though, which I need to change to chandef anyway)
>

I'll have a look at wdev->channel then. :)

> > Anyway, the idea was that a driver can report a radar only for a certain part of
> > the currently used spectrum - e.g. we sense a radar on the extension channel in HT40+,
> > we would need to move but could still use the primary channel.
> >
> > I don't know if this is overkill since we don't support any more than HT20 yet, but
> > that would be kind of future proof.
>
> I guess I'm not worried about internal APIs much. Does that even make
> sense? Can you detect radar on one 20 MHz subchannel and then still use
> the other subchannel?
>

According to ETSI, that is possible. To quote from the EN 301 893 v1.7.1:

"If the master device has detected a radar signal on an Operating Channel during In-Service Monitoring, the
master device shall instruct all its associated slave devices to stop transmitting on this channel which becomes
an Unavailable Channel. For devices operating on multiple (adjacent or non-adjacent) Operating Channels
simultaneously, only the Operating Channel containing the frequency on which radar was detected shall
become an Unavailable Channel."

At least in ath9k it appears that the radar header contains some information whether
the radar was received on the primary or extension channel.

I don't know how upcoming 80 MHz devices handle that though.

We can remove it now and re-add it later if you prefer?

> > > > @@ -344,7 +467,10 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
> > > > break;
> > > > case NL80211_IFTYPE_AP:
> > > > case NL80211_IFTYPE_P2P_GO:
> > > > - if (wdev->beacon_interval) {
> > > > + if (wdev->cac_started) {
> > > > + *chan = wdev->preset_chandef.chan;
> > > > + *chanmode = CHAN_MODE_SHARED;
> > >
> > > Ah, ok, so you're using the preset_chandef for CAC as well. I'm not
> > > entirely sure that is correct though, since it could be changed by
> > > userspace without much effect, e.g. by setting the channel with iw or
> > > iwconfig? Unless you added "if (!cac_started)" there everywhere?
> > >
> >
> > Hmm, actually I've tried setting the frequency with iw and got a EINVAL back.
> > I'll look into it again if I missed something, but thought it would be good to
> > not have this stuff redundant.
>
> Ok. Hmm. EINVAL? Maybe you tried setting to a radar frequency or
> something? Can you try setting to say channel 1? I don't think you
> changed __nl80211_set_channel() to check cac_started, so ...
>

I can try again ... but maybe this is obsolete when using wdev->channel.

> > > > + err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
> > > > + if (err < 1)
> > > > + return err;
> > >
> > > That doesn't make sense, if userspace starts CAC and that is successful
> > > it would expect to eventually receive an event that it completed? Thus
> > > if you return 0 here it would get confused, no?
> > >
> >
> > Ah yes, I should probably return EINVAL in this case, or the appropriate
> > error code otherwise ...
>
> Maybe return some more useful error code? Can't really find any one that
> is appropriate though.

We should add EUSELESS. :D
Can't think of anything better, so will keep it at EINVAL until someone has
a better idea.

>
> Do we export the state yet when you do try to get the channel list? That
> would be helpful for userspace, particularly in this case, I think.
>

No, not yet, I've just put that on my TODO.

> > > > + err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, &chandef);
> > > > + if (!err) {
> > > > + wdev->preset_chandef = chandef;
> > > > + wdev->cac_started = true;
> > > > + chandef.chan->dfs_state_entered = jiffies;
> > >
> > > No reason to assign dfs_state_entered since it won't be used in this
> > > state anyway, will it?
> > >
> >
> > Hmm ... yeah, that's stupid. I'll need to add wdev->cac_entered or something like
> > that to track CAC time. Using dfs_state_entered is just wrong.
>
> I didn't see you read that, or did I miss that?
>

chan->dfs_state_entered was read in cfg80211_radar_event() for the CAC_FINISHED event,
which only worked "by accident" because we set the state entered above. :) I'll change
this part too.

> [...]
> > > > @@ -884,6 +884,9 @@ static void handle_channel(struct wiphy *wiphy,
> > > > return;
> > > > }
> > > >
> > > > + chan->dfs_state = IEEE80211_DFS_USABLE;
> > > > + chan->dfs_state_entered = jiffies;
> > >
> > > Here also you don't really need the time assignment.
> > >
> > > (I skipped this before, so pasting here)
> > >
> >
> > Hm, aren't channels initialized in this function? I wanted to set some
> > sane values here - although time is not relevant for the USABLE state,
> > I thought it might be useful if this info is exported to userspace or
> > for debugging.
>
> Maybe so, I just don't think you need the time there since it won't be
> of relevance in the USABLE state. The "state entered" time is only used
> for UNAVAILABLE.
>
> Maybe therefore state_entered should be renamed to "unavailable_until"
> with the corresponding change in the logic of adding the time when it's
> set to that state?

Can do that, if no one is interested in when we, say, change from unavailable
to usable (after NOP). This is what Zefir asked for.

Personally I don't care at all, and we discuss that in the cover letter thread
anyway. Let's see what Zefir says or if anyone else objects, I put that onto
the "TODO if nobody objects list". :)

Thanks!
Simon

Attachments:

(No filename) (6.34 kB)
signature.asc (198.00 B)
Digital signature Download all attachments

2013-01-29 12:24:15

by Simon Wunderlich

[permalink] [raw]

Subject: [PATCHv7 1/3] nl80211/cfg80211: add radar detection command/event

From: Victor Goldenshtein <[email protected]>

Add new NL80211_CMD_RADAR_DETECT, which starts the Channel Availability
Check (CAC). This command will also notify the usermode about events
(CAC finished, CAC aborted, radar detected).
Once radar detection has started it should continuously
monitor for radars as long as the channel is active.

Add new NL80211_FEATURE_DFS attribute which
indicates that driver/HW supports radar detection.

Signed-off-by: Victor Goldenshtein <[email protected]>
[[email protected]: change to chandef, add radar checking function
to cfg80211, handle CAC and Non-Occupancy Period (NOP), add
tracing, various fixes]
Signed-off-by: Simon Wunderlich <[email protected]>
---
Changes to PATCHv6:
* use chandef in radar_detected_notify()
* add channel states, and handle them in cfg80211
* add Channel Switch Announcement (CAC) and Non-Occupancy Period (NOP)
handling to cfg80211
* add cfg80211_chandef_dfs_required function
* add tracing for wireless event/functions

Changes to PATCHv5:
* remove unused cac_type
* fix doc for cac_started
* remove dfs_nlportid
---
include/net/cfg80211.h | 57 ++++++++++++++++++
include/uapi/linux/nl80211.h | 31 ++++++++++
net/wireless/chan.c | 130 +++++++++++++++++++++++++++++++++++++++++-
net/wireless/core.c | 3 +
net/wireless/core.h | 8 +++
net/wireless/mlme.c | 111 ++++++++++++++++++++++++++++++++++++
net/wireless/nl80211.c | 106 ++++++++++++++++++++++++++++++++++
net/wireless/nl80211.h | 7 +++
net/wireless/reg.c | 3 +
net/wireless/trace.h | 33 +++++++++++
10 files changed, 487 insertions(+), 2 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 970da44..2ee2eb6 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -114,6 +114,24 @@ enum ieee80211_channel_flags {
(IEEE80211_CHAN_NO_HT40PLUS | IEEE80211_CHAN_NO_HT40MINUS)

/**
+ * enum ieee80211_dfs_state - DFS states for channels
+ *
+ * Channel states used by the DFS code.
+ *
+ * @IEEE80211_DFS_USABLE: The channel can be used, but channel availability
+ * check (CAC) must be performed before using it for AP or IBSS.
+ * @IEEE80211_DFS_UNAVAILABLE: A radar has been detected on this channel, it
+ * is therefore marked as not available.
+ * @IEEE80211_DFS_AVAILABLE: The channel has been CAC checked and is available.
+ */
+
+enum ieee80211_dfs_state {
+ IEEE80211_DFS_USABLE,
+ IEEE80211_DFS_UNAVAILABLE,
+ IEEE80211_DFS_AVAILABLE,
+};
+
+/**
* struct ieee80211_channel - channel definition
*
* This structure describes a single channel for use
@@ -133,6 +151,9 @@ enum ieee80211_channel_flags {
* to enable this, this is useful only on 5 GHz band.
* @orig_mag: internal use
* @orig_mpwr: internal use
+ * @dfs_state: current state of this channel. Only relevant if radar is required
+ * on this channel.
+ * @dfs_state_entered: time when the dfs state was entered.
*/
struct ieee80211_channel {
enum ieee80211_band band;
@@ -145,6 +166,8 @@ struct ieee80211_channel {
bool beacon_found;
u32 orig_flags;
int orig_mag, orig_mpwr;
+ enum ieee80211_dfs_state dfs_state;
+ unsigned long dfs_state_entered;
};

/**
@@ -412,6 +435,16 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
u32 prohibited_flags);

/**
+ * cfg80211_chandef_dfs_required - checks if radar detection
+ * is required on any of the channels
+ * @wiphy: the wiphy to validate against
+ * @chandef: the channel definition to check
+ * Return: 1 if radar detection is required, 0 if it is not, < 0 on error
+ */
+int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
+ const struct cfg80211_chan_def *c);
+
+/**
* enum survey_info_flags - survey information flags
*
* @SURVEY_INFO_NOISE_DBM: noise (in dBm) was filled in
@@ -1796,6 +1829,8 @@ struct cfg80211_gtk_rekey_data {
*
* @start_p2p_device: Start the given P2P device.
* @stop_p2p_device: Stop the given P2P device.
+ *
+ * @start_radar_detection: Start radar detection in the driver.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2016,6 +2051,10 @@ struct cfg80211_ops {
struct wireless_dev *wdev);
void (*stop_p2p_device)(struct wiphy *wiphy,
struct wireless_dev *wdev);
+
+ int (*start_radar_detection)(struct wiphy *wiphy,
+ struct net_device *dev,
+ struct cfg80211_chan_def *chandef);
};

/*
@@ -2587,6 +2626,7 @@ struct cfg80211_cached_keys;
* beacons, 0 when not valid
* @address: The address for this device, valid only if @netdev is %NULL
* @p2p_started: true if this is a P2P Device that has been started
+ * @cac_started: true if DFS channel availability check has been started
*/
struct wireless_dev {
struct wiphy *wiphy;
@@ -2638,6 +2678,8 @@ struct wireless_dev {

u32 ap_unexpected_nlportid;

+ bool cac_started;
+
#ifdef CONFIG_CFG80211_WEXT
/* wext data */
struct {
@@ -3629,6 +3671,21 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
gfp_t gfp);

/**
+ * cfg80211_radar - radar detection event
+ * @dev: network device
+ * @chandef: chandef for the current channel
+ * @event: type of event
+ * @gfp: context flags
+ *
+ * This function is called when a radar is detected or a CAC event occured
+ * on the current channel. This must be called to notify the completion of
+ * a CAC process, also by full-MAC drivers.
+ */
+void cfg80211_radar_event(struct net_device *dev,
+ struct cfg80211_chan_def *chandef,
+ enum nl80211_radar_event event, gfp_t gfp);
+
+/**
* cfg80211_cqm_pktloss_notify - notify userspace about packetloss to peer
* @dev: network device
* @peer: peer's MAC address
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index e6eeb4b..6d27d413 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -586,6 +586,14 @@
* @NL80211_CMD_SET_MCAST_RATE: Change the rate used to send multicast frames
* for IBSS or MESH vif.
*
+ * @NL80211_CMD_RADAR_DETECT: Start a Channel availability check (CAC). Once
+ * a radar is detected or the channel availability scan (CAC) has finished
+ * or was aborted, or a radar was detected, usermode will be notified with
+ * this event. This command is also used to notify userspace about radars
+ * while operating on this channel.
+ * %NL80211_ATTR_RADAR_EVENT is used to inform about the type of the
+ * event.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -736,6 +744,8 @@ enum nl80211_commands {

NL80211_CMD_SET_MCAST_RATE,

+ NL80211_CMD_RADAR_DETECT,
+
/* add new commands above here */

/* used to define NL80211_CMD_MAX below */
@@ -1313,6 +1323,9 @@ enum nl80211_commands {
* @NL80211_ATTR_LOCAL_MESH_POWER_MODE: local mesh STA link-specific power mode
* defined in &enum nl80211_mesh_power_mode.
*
+ * @NL80211_ATTR_RADAR_EVENT: Type of radar event for notification to userspace,
+ * contains a value of enum nl80211_radar_event (u8).
+ *
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
@@ -1585,6 +1598,8 @@ enum nl80211_attrs {

NL80211_ATTR_LOCAL_MESH_POWER_MODE,

+ NL80211_ATTR_RADAR_EVENT,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -1640,6 +1655,9 @@ enum nl80211_attrs {

#define NL80211_CQM_TXE_MAX_INTVL 1800

+#define NL80211_DFS_MIN_CAC_TIME_MS 60000
+#define NL80211_DFS_MIN_NOP_TIME_MS (30 * 60 * 1000)
+
/**
* enum nl80211_iftype - (virtual) interface types
*
@@ -3205,6 +3223,7 @@ enum nl80211_ap_sme_features {
* Note that even for drivers that support this, the default is to add
* stations in authenticated/associated state, so to add unauthenticated
* stations the authenticated/associated bits have to be set in the mask.
+ * @NL80211_FEATURE_DFS: Radar detection is supported in the HW/driver.
*/
enum nl80211_feature_flags {
NL80211_FEATURE_SK_TX_STATUS = 1 << 0,
@@ -3221,6 +3240,7 @@ enum nl80211_feature_flags {
NL80211_FEATURE_P2P_GO_CTWIN = 1 << 11,
NL80211_FEATURE_P2P_GO_OPPPS = 1 << 12,
NL80211_FEATURE_FULL_AP_CLIENT_STATE = 1 << 13,
+ NL80211_FEATURE_DFS = 1 << 14,
};

/**
@@ -3276,4 +3296,15 @@ enum nl80211_scan_flags {
NL80211_SCAN_FLAG_AP = 1<<2,
};

+/**
+ * enum nl80211_radar_event - type of radar event for DFS operation
+ *
+ * Type of event to be used with NL80211_ATTR_RADAR_EVENT to inform userspace
+ * about detected radars or success of the channel available check (CAC)
+ */
+enum nl80211_radar_event {
+ NL80211_RADAR_DETECTED,
+ NL80211_CAC_FINISHED,
+ NL80211_CAC_ABORTED,
+};
#endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 396373f..c95b142 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -147,6 +147,32 @@ static void chandef_primary_freqs(const struct cfg80211_chan_def *c,
}
}

+static inline u32 cfg80211_chandef_get_width(const struct cfg80211_chan_def *c)
+{
+ u32 width;
+
+ switch (c->width) {
+ case NL80211_CHAN_WIDTH_20:
+ case NL80211_CHAN_WIDTH_20_NOHT:
+ width = 20;
+ break;
+ case NL80211_CHAN_WIDTH_40:
+ width = 40;
+ break;
+ case NL80211_CHAN_WIDTH_80P80:
+ case NL80211_CHAN_WIDTH_80:
+ width = 80;
+ break;
+ case NL80211_CHAN_WIDTH_160:
+ width = 160;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return -1;
+ }
+ return width;
+}
+
const struct cfg80211_chan_def *
cfg80211_chandef_compatible(const struct cfg80211_chan_def *c1,
const struct cfg80211_chan_def *c2)
@@ -192,6 +218,94 @@ cfg80211_chandef_compatible(const struct cfg80211_chan_def *c1,
}
EXPORT_SYMBOL(cfg80211_chandef_compatible);

+static void cfg80211_set_chans_dfs_state(struct wiphy *wiphy, u32 center_freq,
+ u32 bandwidth,
+ enum ieee80211_dfs_state dfs_state)
+{
+ struct ieee80211_channel *c;
+ u32 freq;
+
+ for (freq = center_freq - bandwidth/2 + 10;
+ freq <= center_freq + bandwidth/2 - 10;
+ freq += 20) {
+ c = ieee80211_get_channel(wiphy, freq);
+ if (!c || !(c->flags & IEEE80211_CHAN_RADAR))
+ continue;
+
+ c->dfs_state = dfs_state;
+ c->dfs_state_entered = jiffies;
+ }
+}
+
+void cfg80211_set_dfs_state(struct wiphy *wiphy,
+ const struct cfg80211_chan_def *chandef,
+ enum ieee80211_dfs_state dfs_state)
+{
+ u32 width;
+
+ if (WARN_ON(!cfg80211_chandef_valid(chandef)))
+ return;
+
+ width = cfg80211_chandef_get_width(chandef);
+ if (width < 0)
+ return;
+
+ cfg80211_set_chans_dfs_state(wiphy, chandef->center_freq1,
+ width, dfs_state);
+
+ if (!chandef->center_freq2)
+ return;
+ cfg80211_set_chans_dfs_state(wiphy, chandef->center_freq2,
+ width, dfs_state);
+}
+
+static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
+ u32 center_freq,
+ u32 bandwidth)
+{
+ struct ieee80211_channel *c;
+ u32 freq;
+
+ for (freq = center_freq - bandwidth/2 + 10;
+ freq <= center_freq + bandwidth/2 - 10;
+ freq += 20) {
+ c = ieee80211_get_channel(wiphy, freq);
+ if (!c)
+ return -EINVAL;
+
+ if (c->flags & IEEE80211_CHAN_RADAR)
+ return 1;
+ }
+ return 0;
+}
+
+
+int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
+ const struct cfg80211_chan_def *chandef)
+{
+ u32 width;
+ int r;
+
+ if (WARN_ON(!cfg80211_chandef_valid(chandef)))
+ return -EINVAL;
+
+ width = cfg80211_chandef_get_width(chandef);
+ if (width < 0)
+ return -EINVAL;
+
+ r = cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq1,
+ width);
+ if (r)
+ return r;
+
+ if (!chandef->center_freq2)
+ return 0;
+
+ return cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq2,
+ width);
+}
+EXPORT_SYMBOL(cfg80211_chandef_dfs_required);
+
static bool cfg80211_secondary_chans_ok(struct wiphy *wiphy,
u32 center_freq, u32 bandwidth,
u32 prohibited_flags)
@@ -203,7 +317,16 @@ static bool cfg80211_secondary_chans_ok(struct wiphy *wiphy,
freq <= center_freq + bandwidth/2 - 10;
freq += 20) {
c = ieee80211_get_channel(wiphy, freq);
- if (!c || c->flags & prohibited_flags)
+ if (!c)
+ return false;
+
+ /* check for radar flags */
+ if ((prohibited_flags & c->flags & IEEE80211_CHAN_RADAR) &&
+ (c->dfs_state != IEEE80211_DFS_AVAILABLE))
+ return false;
+
+ /* check for the other flags */
+ if (c->flags & prohibited_flags & ~IEEE80211_CHAN_RADAR)
return false;
}

@@ -344,7 +467,10 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
break;
case NL80211_IFTYPE_AP:
case NL80211_IFTYPE_P2P_GO:
- if (wdev->beacon_interval) {
+ if (wdev->cac_started) {
+ *chan = wdev->preset_chandef.chan;
+ *chanmode = CHAN_MODE_SHARED;
+ } else if (wdev->beacon_interval) {
*chan = wdev->channel;
*chanmode = CHAN_MODE_SHARED;
}
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 9245729..2da47eb 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -324,6 +324,8 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
INIT_LIST_HEAD(&rdev->bss_list);
INIT_WORK(&rdev->scan_done_wk, __cfg80211_scan_done);
INIT_WORK(&rdev->sched_scan_results_wk, __cfg80211_sched_scan_results);
+ INIT_DELAYED_WORK(&rdev->dfs_update_channels_wk,
+ cfg80211_dfs_channels_update_work);
#ifdef CONFIG_CFG80211_WEXT
rdev->wiphy.wext = &cfg80211_wext_handler;
#endif
@@ -690,6 +692,7 @@ void wiphy_unregister(struct wiphy *wiphy)
flush_work(&rdev->scan_done_wk);
cancel_work_sync(&rdev->conn_work);
flush_work(&rdev->event_work);
+ cancel_delayed_work_sync(&rdev->dfs_update_channels_wk);

if (rdev->wowlan && rdev->ops->set_wakeup)
rdev_set_wakeup(rdev, false);
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 8396f76..fb5d630 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -87,6 +87,8 @@ struct cfg80211_registered_device {

struct cfg80211_wowlan *wowlan;

+ struct delayed_work dfs_update_channels_wk;
+
/* must be last because of the way we do wiphy_priv(),
* and it should at least be aligned to NETDEV_ALIGN */
struct wiphy wiphy __aligned(NETDEV_ALIGN);
@@ -427,6 +429,12 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
struct ieee80211_channel *chan,
enum cfg80211_chan_mode chanmode,
u8 radar_detect);
+void cfg80211_set_dfs_state(struct wiphy *wiphy,
+ const struct cfg80211_chan_def *chandef,
+ enum ieee80211_dfs_state dfs_state);
+
+void cfg80211_dfs_channels_update_work(struct work_struct *work);
+

static inline int
cfg80211_can_change_interface(struct cfg80211_registered_device *rdev,
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index 461e692..17753f0 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -987,3 +987,114 @@ void cfg80211_pmksa_candidate_notify(struct net_device *dev, int index,
nl80211_pmksa_candidate_notify(rdev, dev, index, bssid, preauth, gfp);
}
EXPORT_SYMBOL(cfg80211_pmksa_candidate_notify);
+
+static inline void __cfg80211_dfs_clear_channel(struct ieee80211_channel *c,
+ bool *check_again,
+ unsigned long *check_again_time)
+{
+ unsigned long timeout;
+
+ if (c->dfs_state == IEEE80211_DFS_UNAVAILABLE) {
+ timeout = c->dfs_state_entered + NL80211_DFS_MIN_NOP_TIME_MS;
+ if (time_after_eq(jiffies, timeout)) {
+ /* TODO: we could notify userspace about that change */
+ c->dfs_state = IEEE80211_DFS_USABLE;
+ } else {
+ if (!*check_again)
+ *check_again_time = timeout - jiffies;
+ else
+ *check_again_time = min(*check_again_time,
+ timeout - jiffies);
+ *check_again = true;
+ }
+ }
+}
+
+void cfg80211_dfs_channels_update_work(struct work_struct *work)
+{
+ struct delayed_work *delayed_work;
+ struct cfg80211_registered_device *rdev;
+ struct ieee80211_supported_band *sband;
+ struct ieee80211_channel *c;
+ struct wiphy *wiphy;
+ bool check_again = false;
+ unsigned long check_again_time = 0;
+ int bandid, i;
+
+ delayed_work = container_of(work, struct delayed_work, work);
+ rdev = container_of(delayed_work, struct cfg80211_registered_device,
+ dfs_update_channels_wk);
+ wiphy = &rdev->wiphy;
+
+ mutex_lock(&cfg80211_mutex);
+ for (bandid = 0; bandid < IEEE80211_NUM_BANDS; bandid++) {
+ sband = wiphy->bands[bandid];
+ if (!sband)
+ continue;
+
+ for (i = 0; i < sband->n_channels; i++) {
+ c = &sband->channels[i];
+
+ __cfg80211_dfs_clear_channel(c, &check_again,
+ &check_again_time);
+ }
+ }
+ mutex_unlock(&cfg80211_mutex);
+
+ /* reschedule if there are other channels waiting to be cleared again */
+ if (check_again)
+ queue_delayed_work(cfg80211_wq, &rdev->dfs_update_channels_wk,
+ check_again_time);
+}
+
+
+void cfg80211_radar_event(struct net_device *netdev,
+ struct cfg80211_chan_def *chandef,
+ enum nl80211_radar_event event,
+ gfp_t gfp)
+{
+ struct wireless_dev *wdev = netdev->ieee80211_ptr;
+ struct wiphy *wiphy = wdev->wiphy;
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+ unsigned long timeout;
+
+ if (WARN_ON(!cfg80211_chandef_valid(chandef)))
+ return;
+
+ trace_cfg80211_radar_event(netdev, chandef, event);
+
+ switch (event) {
+ case NL80211_RADAR_DETECTED:
+ /*
+ * only set the chandef supplied channel to unavailable, in
+ * case the radar is detected on only one of multiple channels
+ * spanned by the chandef.
+ */
+ cfg80211_set_dfs_state(wiphy, chandef,
+ IEEE80211_DFS_UNAVAILABLE);
+
+ timeout = msecs_to_jiffies(NL80211_DFS_MIN_NOP_TIME_MS);
+ queue_delayed_work(cfg80211_wq, &rdev->dfs_update_channels_wk,
+ timeout);
+ break;
+ case NL80211_CAC_FINISHED:
+ timeout = wdev->preset_chandef.chan->dfs_state_entered;
+ timeout = msecs_to_jiffies(timeout +
+ NL80211_DFS_MIN_CAC_TIME_MS);
+ WARN_ON(!time_after_eq(jiffies, timeout));
+ cfg80211_set_dfs_state(wiphy, &wdev->preset_chandef,
+ IEEE80211_DFS_AVAILABLE);
+ break;
+ case NL80211_CAC_ABORTED:
+ /* Shouldn't happen if CAC was never started before. */
+ WARN_ON(!wdev->cac_started);
+ break;
+ default:
+ break;
+ }
+
+ wdev->cac_started = false;
+
+ nl80211_radar_notify(rdev, &wdev->preset_chandef, event, netdev, gfp);
+}
+EXPORT_SYMBOL(cfg80211_radar_event);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 33de803..2e0216a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4869,6 +4869,54 @@ static int nl80211_stop_sched_scan(struct sk_buff *skb,
return err;
}

+static int nl80211_start_radar_detection(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct net_device *dev = info->user_ptr[1];
+ struct wireless_dev *wdev = dev->ieee80211_ptr;
+ struct cfg80211_chan_def chandef;
+ int err;
+
+ if (!(rdev->wiphy.features & NL80211_FEATURE_DFS))
+ return -EOPNOTSUPP;
+
+ err = nl80211_parse_chandef(rdev, info, &chandef);
+ if (err)
+ return err;
+
+ if (wdev->cac_started)
+ return -EBUSY;
+
+ err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
+ if (err < 1)
+ return err;
+
+ if (chandef.chan->dfs_state != IEEE80211_DFS_USABLE)
+ return -EINVAL;
+
+ if (!rdev->ops->start_radar_detection)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&rdev->devlist_mtx);
+ err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
+ chandef.chan, CHAN_MODE_SHARED,
+ BIT(chandef.width));
+ mutex_unlock(&rdev->devlist_mtx);
+
+ if (err)
+ return err;
+
+ err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, &chandef);
+ if (!err) {
+ wdev->preset_chandef = chandef;
+ wdev->cac_started = true;
+ chandef.chan->dfs_state_entered = jiffies;
+ }
+
+ return err;
+}
+
static int nl80211_send_bss(struct sk_buff *msg, struct netlink_callback *cb,
u32 seq, int flags,
struct cfg80211_registered_device *rdev,
@@ -5575,6 +5623,10 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
if (!cfg80211_reg_can_beacon(&rdev->wiphy, &ibss.chandef))
return -EINVAL;

+ if (cfg80211_chandef_usable(&rdev->wiphy, &ibss.chandef,
+ IEEE80211_CHAN_NO_IBSS))
+ return -EINVAL;
+
if (ibss.chandef.width > NL80211_CHAN_WIDTH_40)
return -EINVAL;
if (ibss.chandef.width != NL80211_CHAN_WIDTH_20_NOHT &&
@@ -7876,6 +7928,14 @@ static struct genl_ops nl80211_ops[] = {
.internal_flags = NL80211_FLAG_NEED_NETDEV |
NL80211_FLAG_NEED_RTNL,
},
+ {
+ .cmd = NL80211_CMD_RADAR_DETECT,
+ .doit = nl80211_start_radar_detection,
+ .policy = nl80211_policy,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+ NL80211_FLAG_NEED_RTNL,
+ },
};

static struct genl_multicast_group nl80211_mlme_mcgrp = {
@@ -9073,6 +9133,52 @@ nl80211_send_cqm_txe_notify(struct cfg80211_registered_device *rdev,
}

void
+nl80211_radar_notify(struct cfg80211_registered_device *rdev,
+ struct cfg80211_chan_def *chandef,
+ enum nl80211_radar_event event,
+ struct net_device *netdev, gfp_t gfp)
+{
+ struct sk_buff *msg;
+ struct wireless_dev *wdev = netdev->ieee80211_ptr;
+ void *hdr;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ if (!msg)
+ return;
+
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_RADAR_DETECT);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
+ nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
+ nla_put_u64(msg, NL80211_ATTR_WDEV, wdev_id(wdev)))
+ goto nla_put_failure;
+
+ /* reason is unspecified, just notify that CAC has failed. */
+ if (nla_put_u8(msg, NL80211_ATTR_RADAR_EVENT, event))
+ goto nla_put_failure;
+
+ if (nl80211_send_chandef(msg, chandef))
+ goto nla_put_failure;
+
+ if (genlmsg_end(msg, hdr) < 0) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ genlmsg_multicast_netns(wiphy_net(&rdev->wiphy), msg, 0,
+ nl80211_mlme_mcgrp.id, gfp);
+ return;
+
+ nla_put_failure:
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+}
+
+void
nl80211_send_cqm_pktloss_notify(struct cfg80211_registered_device *rdev,
struct net_device *netdev, const u8 *peer,
u32 num_packets, gfp_t gfp)
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index 2acba84..b061da4 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -108,6 +108,13 @@ nl80211_send_cqm_rssi_notify(struct cfg80211_registered_device *rdev,
struct net_device *netdev,
enum nl80211_cqm_rssi_threshold_event rssi_event,
gfp_t gfp);
+
+void
+nl80211_radar_notify(struct cfg80211_registered_device *rdev,
+ struct cfg80211_chan_def *chandef,
+ enum nl80211_radar_event event,
+ struct net_device *netdev, gfp_t gfp);
+
void
nl80211_send_cqm_pktloss_notify(struct cfg80211_registered_device *rdev,
struct net_device *netdev, const u8 *peer,
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index de02d63..2c441dc 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -884,6 +884,9 @@ static void handle_channel(struct wiphy *wiphy,
return;
}

+ chan->dfs_state = IEEE80211_DFS_USABLE;
+ chan->dfs_state_entered = jiffies;
+
chan->beacon_found = false;
chan->flags = flags | bw_flags | map_regdom_flags(reg_rule->flags);
chan->max_antenna_gain =
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 2134576..3eadb34 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2033,6 +2033,21 @@ TRACE_EVENT(cfg80211_reg_can_beacon,
WIPHY_PR_ARG, CHAN_DEF_PR_ARG)
);

+TRACE_EVENT(cfg80211_chandef_dfs_required,
+ TP_PROTO(struct wiphy *wiphy, struct cfg80211_chan_def *chandef),
+ TP_ARGS(wiphy, chandef),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ CHAN_DEF_ENTRY
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ CHAN_DEF_ASSIGN(chandef);
+ ),
+ TP_printk(WIPHY_PR_FMT ", " CHAN_DEF_PR_FMT,
+ WIPHY_PR_ARG, CHAN_DEF_PR_ARG)
+);
+
TRACE_EVENT(cfg80211_ch_switch_notify,
TP_PROTO(struct net_device *netdev,
struct cfg80211_chan_def *chandef),
@@ -2049,6 +2064,24 @@ TRACE_EVENT(cfg80211_ch_switch_notify,
NETDEV_PR_ARG, CHAN_DEF_PR_ARG)
);

+TRACE_EVENT(cfg80211_radar_event,
+ TP_PROTO(struct net_device *netdev, struct cfg80211_chan_def *chandef,
+ enum nl80211_radar_event evt),
+ TP_ARGS(netdev, chandef, evt),
+ TP_STRUCT__entry(
+ NETDEV_ENTRY
+ CHAN_DEF_ENTRY
+ __field(enum nl80211_radar_event, evt)
+ ),
+ TP_fast_assign(
+ NETDEV_ASSIGN;
+ CHAN_DEF_ASSIGN(chandef);
+ __entry->evt = evt;
+ ),
+ TP_printk(NETDEV_PR_FMT ", " CHAN_DEF_PR_FMT " event: %d",
+ NETDEV_PR_ARG, CHAN_DEF_PR_ARG, __entry->evt)
+);
+
DECLARE_EVENT_CLASS(cfg80211_rx_evt,
TP_PROTO(struct net_device *netdev, const u8 *addr),
TP_ARGS(netdev, addr),
--
1.7.10.4

2013-01-31 17:22:05

On Fri, Feb 01, 2013 at 10:57:00AM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 18:47 +0100, Simon Wunderlich wrote:
>
> > > > What I've tried:
> > > > * configure 2 SSIDs in hostapd, start it
> > > > * both wlan0 and wlan0-1 got created
> > > > * only wlan0 comes up, wlan0-1 was rejected because of missing channel combinations
> > > > * now I've injected a radar - which should be sent to wlan0 and wlan0-1
> > > > * wlan0 could send the event, but wlan0-1 had no bss configured and therefore no chandef
> > > >
> > > > I can change this comment to "may happen to devices which have currently no BSS configured",
> > > > maybe that it is not so confusing ...
> > >
> > > Not sure I understand, how would the radar detected event come to an
> > > interface that doesn't really exist for the driver?
> >
> > wlan0-1 exists and was created, but no AP was ever started - because hostapd tried
> > to start the AP on a DFS channel when wlan0 was already active, and thanks to our
> > interface combinations this is not allowed. Therefore, the vif.bss_conf.chandef is empty.
> >
> > The interface does exist for the driver (interface add succeeded), but start_ap failed,
> > so it is a virgin AP interface.
> >
> > I think this behaviour is correct like that ...
>
> So ... starting the AP failed because it was a different channel, it was
> added to the driver because multiple AP interfaces were allowed but the
> specific channel wasn't allowed (in addition) when it was started? But I
> still don't see why that interface should get an event since it doesn't
> even have a channel yet, except maybe preset_chan which is really only
> for backward compatibility reasons?
>
> What am I missing? Where does the event on wlan0-1 come from anyway?

In the (dummy) ath9k part I'm handling the radar by simply sending radar events
to all ieee80211_vifs which are registered on this phy - regardless their
status, if they are up or not. That's why both wlan0 and wlan0-1 get the
event in my example. Of course, sending the event to wlan0-1 is pretty
useless in my case, but we should better check. :)

Cheers,
Simon
>

Attachments:

(No filename) (2.08 kB)
signature.asc (198.00 B)
Digital signature Download all attachments

2013-02-01 09:53:55

by Johannes Berg

On Thu, 2013-01-31 at 18:47 +0100, Simon Wunderlich wrote:

> > > What I've tried:
> > > * configure 2 SSIDs in hostapd, start it
> > > * both wlan0 and wlan0-1 got created
> > > * only wlan0 comes up, wlan0-1 was rejected because of missing channel combinations
> > > * now I've injected a radar - which should be sent to wlan0 and wlan0-1
> > > * wlan0 could send the event, but wlan0-1 had no bss configured and therefore no chandef
> > >
> > > I can change this comment to "may happen to devices which have currently no BSS configured",
> > > maybe that it is not so confusing ...
> >
> > Not sure I understand, how would the radar detected event come to an
> > interface that doesn't really exist for the driver?
>
> wlan0-1 exists and was created, but no AP was ever started - because hostapd tried
> to start the AP on a DFS channel when wlan0 was already active, and thanks to our
> interface combinations this is not allowed. Therefore, the vif.bss_conf.chandef is empty.
>
> The interface does exist for the driver (interface add succeeded), but start_ap failed,
> so it is a virgin AP interface.
>
> I think this behaviour is correct like that ...

So ... starting the AP failed because it was a different channel, it was
added to the driver because multiple AP interfaces were allowed but the
specific channel wasn't allowed (in addition) when it was started? But I
still don't see why that interface should get an event since it doesn't
even have a channel yet, except maybe preset_chan which is really only
for backward compatibility reasons?

What am I missing? Where does the event on wlan0-1 come from anyway?

johannes