Hey Johannes,
thanks for the review!
On Thu, Jan 31, 2013 at 03:25:21PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 13:21 +0100, Simon Wunderlich wrote:
>
> > From: Victor Goldenshtein <victorg@xxxxxx>
>
> Probably time for you to claim ownership ... ;-)
>
maybe ... I've changed most of the original patch, actually. I'll keep
his name in the commit message though ..
> > Changes to PATCHv6:
>
> Thanks for your patience and the frequent posting :-)
>
Well, thanks for your patience ;)
> > /**
> > + * enum ieee80211_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 ieee80211_dfs_state {
> > + IEEE80211_DFS_USABLE,
> > + IEEE80211_DFS_UNAVAILABLE,
> > + IEEE80211_DFS_AVAILABLE,
>
> Should UNAVAILABLE be = 0, so that's the default?
>
yeah, good idea.
> > @@ -133,6 +151,9 @@ enum ieee80211_channel_flags {
> > * to enable this, this is useful only on 5 GHz band.
> > * @orig_mag: internal use
> > * @orig_mpwr: internal use
> > + * @dfs_state: current state of this channel. Only relevant if radar is required
> > + * on this channel.
> > + * @dfs_state_entered: time when the dfs state was entered.
>
> This is relevant for "unavailable", presumably, to make sure it stays
> there for long enough? What unit is that, and how long does it have to
> stay? "jiffies" can become unreliable after a long uptime so that might
> cause issues. It's unlikely, the issue would be that ~25 days after it
> was supposed to be available again it would be rejected as jiffies got
> back to the same value ... :)
> Also depends on your implementation which I haven't seen yet.
>
Right, this is used for unavailable - The Non-Occupancy period (NOP) time
is 30 minutes, for FCC it should be the same. All times in DFS are < 24 hours,
and we check this time with our own timers in kernel, so I don't think we will
ever hit the jiffies overrun. I would prefer using jiffies because it's independent
of system time changes and enough for this task.
I'll add that the time is jiffies in documentation.
> > @@ -412,6 +435,16 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
> > u32 prohibited_flags);
> >
> > /**
> > + * cfg80211_chandef_dfs_required - checks if radar detection
> > + * is required on any of the channels
> > + * @wiphy: the wiphy to validate against
> > + * @chandef: the channel definition to check
> > + * Return: 1 if radar detection is required, 0 if it is not, < 0 on error
> > + */
> > +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > + const struct cfg80211_chan_def *c);
>
> Why does that need to be exported to mac80211/drivers? Shouldn't
> cfg80211 be able to check everything?
>
I'm using it in mac80211/ieee80211_start_ap() to find out whether radar detection
is required. We could put into struct cfg80211_ap_settings *params if you prefer?
I guess similar params exist for IBSS.
> > struct wireless_dev {
> > struct wiphy *wiphy;
> > @@ -2638,6 +2678,8 @@ struct wireless_dev {
> >
> > u32 ap_unexpected_nlportid;
> >
> > + bool cac_started;
>
> Don't you need a cac_chandef or something?
>
The chandef is stored into wdev->preset_chandef anyway, so I didn't see any
need to save it twice?
> > /**
> > + * cfg80211_radar - radar detection event
> > + * @dev: network device
> > + * @chandef: chandef for the current channel
>
> Doesn't cfg80211 know what channel the device is operating/doing CAC on?
>
Whoops, typo in the description - forgot the _event :)
Anyway, the idea was that a driver can report a radar only for a certain part of
the currently used spectrum - e.g. we sense a radar on the extension channel in HT40+,
we would need to move but could still use the primary channel.
I don't know if this is overkill since we don't support any more than HT20 yet, but
that would be kind of future proof.
> > +#define NL80211_DFS_MIN_CAC_TIME_MS 60000
> > +#define NL80211_DFS_MIN_NOP_TIME_MS (30 * 60 * 1000)
>
> Why are those needed in the userspace API?
>
Hm, not needed anymore I guess, it's just a nice reference ... will move it to other header
files (probably cfg80211.h) then.
> > @@ -3221,6 +3240,7 @@ enum nl80211_feature_flags {
> > NL80211_FEATURE_P2P_GO_CTWIN = 1 << 11,
> > NL80211_FEATURE_P2P_GO_OPPPS = 1 << 12,
> > NL80211_FEATURE_FULL_AP_CLIENT_STATE = 1 << 13,
> > + NL80211_FEATURE_DFS = 1 << 14,
>
> I don't think we need this any more with the interface combinations
> thing?
>
Right, this is obsolete now. Will remove it.
> > +static void cfg80211_set_chans_dfs_state(struct wiphy *wiphy, u32 center_freq,
> > + u32 bandwidth,
> > + enum ieee80211_dfs_state dfs_state)
> > +{
> > + struct ieee80211_channel *c;
> > + u32 freq;
> > +
> > + for (freq = center_freq - bandwidth/2 + 10;
> > + freq <= center_freq + bandwidth/2 - 10;
> > + freq += 20) {
> > + c = ieee80211_get_channel(wiphy, freq);
> > + if (!c || !(c->flags & IEEE80211_CHAN_RADAR))
> > + continue;
> > +
> > + c->dfs_state = dfs_state;
> > + c->dfs_state_entered = jiffies;
>
> I wonder if it'd make sense to not skip if the radar flag isn't set,
> that could be relevant with regdom changes? OTOH, if the regdom changes
> (much) I *suspect* we need to invalidate all the radar measurements
> anyway since the rules might now be different?
>
IMHO the channels should be cleared on each regdom change. At least radar
patterns are different from FCC and ETSI (although I don't know if there is
a pattern in FCC which can be ignored in ETSI and vice versa). To be sure,
I would vote for complete reset.
> > +int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > + const struct cfg80211_chan_def *chandef)
> > +{
> > + u32 width;
> > + int r;
> > +
> > + if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> > + return -EINVAL;
> > +
> > + width = cfg80211_chandef_get_width(chandef);
> > + if (width < 0)
>
> never true with a u32, I think you might want to change the function
> prototype and the variable :)
>
Whoops, right, thanks!
> > @@ -344,7 +467,10 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
> > break;
> > case NL80211_IFTYPE_AP:
> > case NL80211_IFTYPE_P2P_GO:
> > - if (wdev->beacon_interval) {
> > + if (wdev->cac_started) {
> > + *chan = wdev->preset_chandef.chan;
> > + *chanmode = CHAN_MODE_SHARED;
>
> Ah, ok, so you're using the preset_chandef for CAC as well. I'm not
> entirely sure that is correct though, since it could be changed by
> userspace without much effect, e.g. by setting the channel with iw or
> iwconfig? Unless you added "if (!cac_started)" there everywhere?
>
Hmm, actually I've tried setting the frequency with iw and got a EINVAL back.
I'll look into it again if I missed something, but thought it would be good to
not have this stuff redundant.
> > +static inline void __cfg80211_dfs_clear_channel(struct ieee80211_channel *c,
>
> no reason for inline
>
OK
> > + bool *check_again,
> > + unsigned long *check_again_time)
> > +{
> > + unsigned long timeout;
> > +
> > + if (c->dfs_state == IEEE80211_DFS_UNAVAILABLE) {
>
> could save on indentation by returning early if it's not unavailable :-)
right, thanks.
>
> I guess this addresses the jiffies concern I had above.
>
OK - the state is entered via cfg80211_radar_event() when a radar is received.
> > + timeout = c->dfs_state_entered + NL80211_DFS_MIN_NOP_TIME_MS;
> > + if (time_after_eq(jiffies, timeout)) {
> > + /* TODO: we could notify userspace about that change */
> > + c->dfs_state = IEEE80211_DFS_USABLE;
> > + } else {
> > + if (!*check_again)
>
> should probably set it to false when the function starts, now you rely
> on it being set outside which is a bit odd? (or did I just snip that
> from my reply and don't see it any more?)
>
Actually I just have this function because I refactored
cfg80211_dfs_channels_update_work(), I had some trouble with the 80 characters
limit ... ;)
So yes, I rely on check_again set outside because I expect to only be used by
cfg80211_dfs_channels_update_work(). I'll try to write this nicer ...
> > + err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
> > + if (err < 1)
> > + return err;
>
> That doesn't make sense, if userspace starts CAC and that is successful
> it would expect to eventually receive an event that it completed? Thus
> if you return 0 here it would get confused, no?
>
Ah yes, I should probably return EINVAL in this case, or the appropriate
error code otherwise ...
> > + if (chandef.chan->dfs_state != IEEE80211_DFS_USABLE)
> > + return -EINVAL;
> > +
> > + 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);
>
> You need to keep holding the mutex until you've set cac_started to true
> (or failed), otherwise you introduce races.
>
OK
> > + if (err)
> > + return err;
> > +
> > + err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, &chandef);
> > + if (!err) {
> > + wdev->preset_chandef = chandef;
> > + wdev->cac_started = true;
> > + chandef.chan->dfs_state_entered = jiffies;
>
> No reason to assign dfs_state_entered since it won't be used in this
> state anyway, will it?
>
Hmm ... yeah, that's stupid. I'll need to add wdev->cac_entered or something like
that to track CAC time. Using dfs_state_entered is just wrong.
Thanks.
> > @@ -5575,6 +5623,10 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
> > if (!cfg80211_reg_can_beacon(&rdev->wiphy, &ibss.chandef))
> > return -EINVAL;
> >
> > + if (cfg80211_chandef_usable(&rdev->wiphy, &ibss.chandef,
> > + IEEE80211_CHAN_NO_IBSS))
> > + return -EINVAL;
>
> That seems like an unrelated change/fix?
>
Well, yeah, that is a survivor from some intermediate state that I forgot to remove.
I still have some confusion/questions regarding the other flags, there is a question
in the cover letter regarding this - we should discuss it there.
> > + /* reason is unspecified, just notify that CAC has failed. */
> > + if (nla_put_u8(msg, NL80211_ATTR_RADAR_EVENT, event))
>
> I think enums should generally be u32.
>
OK
> > +++ b/net/wireless/reg.c
> > @@ -884,6 +884,9 @@ static void handle_channel(struct wiphy *wiphy,
> > return;
> > }
> >
> > + chan->dfs_state = IEEE80211_DFS_USABLE;
> > + chan->dfs_state_entered = jiffies;
>
> Here also you don't really need the time assignment.
>
> (I skipped this before, so pasting here)
>
Hm, aren't channels initialized in this function? I wanted to set some
sane values here - although time is not relevant for the USABLE state,
I thought it might be useful if this info is exported to userspace or
for debugging.
> > +void cfg80211_radar_event(struct net_device *netdev,
> > + struct cfg80211_chan_def *chandef,
> > + enum nl80211_radar_event event,
> > + gfp_t gfp)
> > +{
> > + struct wireless_dev *wdev = netdev->ieee80211_ptr;
> > + struct wiphy *wiphy = wdev->wiphy;
> > + struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> > + unsigned long timeout;
> > +
> > + if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> > + return;
> > +
> > + trace_cfg80211_radar_event(netdev, chandef, event);
> > +
> > + switch (event) {
> > + case NL80211_RADAR_DETECTED:
> > + /*
> > + * only set the chandef supplied channel to unavailable, in
> > + * case the radar is detected on only one of multiple channels
> > + * spanned by the chandef.
> > + */
> > + cfg80211_set_dfs_state(wiphy, chandef,
> > + IEEE80211_DFS_UNAVAILABLE);
> > +
> > + timeout = msecs_to_jiffies(NL80211_DFS_MIN_NOP_TIME_MS);
> > + queue_delayed_work(cfg80211_wq, &rdev->dfs_update_channels_wk,
> > + timeout);
> > + break;
> > + case NL80211_CAC_FINISHED:
> > + timeout = wdev->preset_chandef.chan->dfs_state_entered;
> > + timeout = msecs_to_jiffies(timeout +
> > + NL80211_DFS_MIN_CAC_TIME_MS);
> > + WARN_ON(!time_after_eq(jiffies, timeout));
> > + cfg80211_set_dfs_state(wiphy, &wdev->preset_chandef,
> > + IEEE80211_DFS_AVAILABLE);
> > + break;
> > + case NL80211_CAC_ABORTED:
> > + /* Shouldn't happen if CAC was never started before. */
> > + WARN_ON(!wdev->cac_started);
> > + break;
> > + default:
> > + break;
>
> I think a warning (and maybe return) would be useful here.
>
OK
> > + }
> > +
> > + wdev->cac_started = false;
>
>
> Whew. :)
> Overall I think this is looking good, mostly minor comments.
I'm glad you say that! ;) Thanks as always for the comments. There
are a few questions in the cover letter I think we should discuss,
then I'll repost the patchset - hopefully quicker as most conceptional
changes have been included in this version already.
Cheers,
Simon
Attachment:
signature.asc
Description: Digital signature
