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