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:
signature.asc
Description: Digital signature
