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

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

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

On Wed, Feb 06, 2013 at 06:24:08PM +0100, Johannes Berg wrote:
> On Mon, 2013-02-04 at 13:49 +0100, Simon Wunderlich wrote:
> 
> >  /**
> > + * cfg80211_chandef_dfs_required - checks if radar detection
> > + *	is required on any of the channels
> 
> pretty sure this isn't valid kernel-doc, I think it has to fit into one
> line
> 
OK
> > @@ -3663,6 +3694,21 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
> >  			      gfp_t gfp);
> >  
> >  /**
> > + * cfg80211_radar_event - 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
> 
> occurred :)
> 
> 
OK
> > @@ -2014,6 +2029,7 @@ enum nl80211_band_attr {
> >   *	(100 * dBm).
> >   * @NL80211_FREQUENCY_ATTR_MAX: highest frequency attribute number
> >   *	currently defined
> > + * @NL80211_FREQUENCY_ATTR_DFS_STATE: current state for DFS
> 
> missed docs for _DFS_TIME, maybe should put it before _MAX :)
> also, what values does this take?
> 
Yeah, I'll fix that. States are the DFS states below.

> > +/**
> > + * enum nl80211_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 nl80211_dfs_state {
> > +	NL80211_DFS_USABLE	= 0,
> 
> I don't really see a reason for explicit values?
> 

You've suggested explicit values yourself last time [1], or did I misunderstand?

[1] http://article.gmane.org/gmane.linux.kernel.wireless.general/103278
"Should UNAVAILABLE be = 0, so that's the default?" (usable should be the default, btw)

> > +static inline int cfg80211_chandef_get_width(const struct cfg80211_chan_def *c)
> 
> no need for inline
> 
OK

> > +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > +				  const struct cfg80211_chan_def *chandef)
> > +{
> > +	int 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);
> > +}
> 
> If you don't export it why is it in the public header file -- either you
> need to export it as well or it doesn't need to be in cfg80211.h :)
> 

right, will move it into net/wireless/core.h

> > @@ -454,6 +462,16 @@ cfg80211_can_use_chan(struct cfg80211_registered_device *rdev,
> >  					    chan, chanmode, 0);
> >  }
> >  
> > +static inline unsigned int elapsed_jiffies_msecs(unsigned long start)
> > +{
> > +	unsigned long end = jiffies;
> > +
> > +	if (end >= start)
> > +		return jiffies_to_msecs(end - start);
> > +
> > +	return jiffies_to_msecs(end + (MAX_JIFFY_OFFSET - start) + 1);
> > +}
> 
> That seems a bit magic, like it should be hidden away in jiffies.h. Is
> there really no function already to do something like this?
> 

I've just moved this magic function from scan.c to core.h because I need it too.
It appears another driver (drivers/net/wireless/ipw2x00) also implements this
privately. We can suggest this patch for include/linux/jiffies.h if you prefer.

> > +static void cfg80211_notify_nop_ended(struct cfg80211_registered_device *rdev,
> > +					struct ieee80211_channel *c)
> 
> indentation :)
> 

OK

> > +{
> > +	struct wireless_dev *wdev;
> > +	struct cfg80211_chan_def chandef;
> > +
> > +	cfg80211_chandef_create(&chandef, c, NL80211_CHAN_NO_HT);
> > +
> > +	mutex_lock(&rdev->devlist_mtx);
> > +	list_for_each_entry(wdev, &rdev->wdev_list, list) {
> > +		if (!wdev->netdev)
> > +			continue;
> > +
> > +		if (!netif_running(wdev->netdev))
> > +			continue;
> > +
> > +		nl80211_radar_notify(rdev, &chandef,
> > +				     NL80211_RADAR_NOP_FINISHED,
> > +				     wdev->netdev, GFP_ATOMIC);
> > +	}
> 
> Why for all interfaces? This is multicast anyway, so why not just send
> the event without an interface index instead?
> 

Hmm. I'll try. ;)

> > --- a/net/wireless/scan.c
> > +++ b/net/wireless/scan.c
> > @@ -1155,16 +1155,6 @@ static void ieee80211_scan_add_ies(struct iw_request_info *info,
> >  	}
> >  }
> >  
> > -static inline unsigned int elapsed_jiffies_msecs(unsigned long start)
> > -{
> > -	unsigned long end = jiffies;
> > -
> > -	if (end >= start)
> > -		return jiffies_to_msecs(end - start);
> > -
> > -	return jiffies_to_msecs(end + (MAX_JIFFY_OFFSET - start) + 1);
> > -}
> 
> Oh, heh. Oh well ...

:P

> 
> I think I should probably look at locking too ...

Would be a good idea. I'll verify with lockdep enabled too, didn't do it so far.

Thanks!
	Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]