Re: [PATCHv6 3/6] nl80211/cfg80211: add radar detection command/event

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 4734 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]