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

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

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

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