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