On Wed, Jan 16, 2013 at 11:51:12PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-08 at 14:04 +0100, Simon Wunderlich wrote:
>
>
> > + * @radar_detect_timeout: this timeout indicates the end of the channel
> > + * availability check for radar channels (in jiffies), only after this
> > + * period the user may initiate the tx on the channel.
> > + * @cac_started: indicates that channel availability check is started for this
> > + * channel type.
>
> So you're enforcing a certain CAC time, but not the time we are allowed
> to treat the channel as clear? Shouldn't *that* be in each channel
> struct, rather than the other stuff?
>
> It also seems to me that "cac_started" isn't really all that relevant in
> the channel struct either. What seems relevant is the *result* of the
> CAC, and how long it's still valid, no?
>
Actually there is no limit how long a channel is considered "available", at
least in ETSI. ETSI EN 301-893 v1.4.1 had a limit of 24 hours for that,
but that was removed in v1.5.1 and didn't re-appear since then (current is
v1.7.1).
But we can move the CAC/timeout in the wdev and have keep a flag field in
the channel struct instead, marking the channel as available, unavailable, etc.
What do you think?
> > +++ b/net/wireless/chan.c
> > @@ -287,14 +287,18 @@ bool cfg80211_reg_can_beacon(struct wiphy *wiphy,
> > struct cfg80211_chan_def *chandef)
> > {
> > bool res;
> > + u32 prohibited_flags;
> >
> > trace_cfg80211_reg_can_beacon(wiphy, chandef);
> >
> > - res = cfg80211_chandef_usable(wiphy, chandef,
> > - IEEE80211_CHAN_DISABLED |
> > - IEEE80211_CHAN_PASSIVE_SCAN |
> > - IEEE80211_CHAN_NO_IBSS |
> > - IEEE80211_CHAN_RADAR);
> > + prohibited_flags = IEEE80211_CHAN_DISABLED;
> > +
> > + if (!(wiphy->features & NL80211_FEATURE_DFS))
> > + prohibited_flags |= IEEE80211_CHAN_PASSIVE_SCAN |
> > + IEEE80211_CHAN_NO_IBSS |
> > + IEEE80211_CHAN_RADAR;
>
> I have a feeling this change should take into account the channel width,
> and whether CAC completed successfully?
>
All channels used for operation are checked already in cfg80211_chandef_usable()
for the flags.
If the channel width is supported at all is checked with cfg80211_can_use_iftype_chan()
before/after.
So I don't see the neccesity for further checking width, or am I missing something?
The CAC completed check is performed outside right now, but when we introduce
the available/unavailable flags as suggested above we can as well move this check
into cfg80211_reg_can_beacon().
> > +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 (!(chandef.chan->flags & IEEE80211_CHAN_RADAR))
> > + return -EINVAL;
> > +
> > + if (chandef.chan->cac_started)
> > + return -EBUSY;
> > +
> > + 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;
> > + chandef.chan->cac_started = true;
> > + chandef.chan->radar_detect_timeout = jiffies +
> > + msecs_to_jiffies(NL80211_DFS_MIN_CAC_TIME_MS);
> > + }
> > +
> > + return err;
> > +}
>
> This still seems somewhat wrong. For the duration of the CAC, the
> channel should be "locked" in some way, no? As it stands now, nothing
> prevents userspace from adding another vif and using it for something
> entirely different, while cfg80211 thinks the CAC is actually running.
>
Hmm, we can put the "CAC state" in the wdev then, and use it for locking?
> > + if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
> > + nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
> > + nla_put_u32(msg, NL80211_ATTR_WIPHY_FREQ, chan->center_freq))
> > + goto nla_put_failure;
>
> That should be the entire chandef info, and possibly the WDEV_ID too.
OK
Attachment:
signature.asc
Description: Digital signature
