On Thursday 26 January 2017 03:04 PM, Johannes Berg wrote:
>
>> + /* Should we apply the grace period during beaconing
>> interface
>> + * shutdown also?
>> + */
>> + cfg80211_sched_dfs_chan_update(rdev);
>
> It might make some sense, say if hostapd crashes and you restart it
> automatically or something?
Sure. Initially it looked tricky to handle this. But I guess we can store
the DFS channel and the time stamp (rdev specific) when the beaconing interface
is brought down. cfg80211_dfs_channels_update_work() can use these information
and apply the grace period before setting the DFS channel state back to 'usable'.
>
>> return err;
>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> index 5497d022..090309a 100644
>> --- a/net/wireless/chan.c
>> +++ b/net/wireless/chan.c
>> @@ -456,6 +456,102 @@ bool cfg80211_chandef_dfs_usable(struct wiphy
>> *wiphy,
>> return (r1 + r2 > 0);
>> }
>>
>> +static bool cfg80211_5ghz_sub_chan(struct cfg80211_chan_def
>> *chandef,
>> + struct ieee80211_channel *chan)
>
> This could use some explanation, and I don't see anything that's really
> 5 GHz specific in here, so why that in the function name?
Sure.
>
>> + u32 start_freq_seg0 = 0, end_freq_seg0 = 0;
>> + u32 start_freq_seg1 = 0, end_freq_seg1 = 0;
>> +
>> + if (chandef->chan->center_freq == chan->center_freq)
>> + return true;
>> +
>> + switch (chandef->width) {
>> + case NL80211_CHAN_WIDTH_40:
>> + start_freq_seg0 = chandef->center_freq1 - 20;
>> + end_freq_seg0 = chandef->center_freq1 + 20;
>> + break;
>> + case NL80211_CHAN_WIDTH_80P80:
>> + start_freq_seg1 = chandef->center_freq2 - 40;
>> + end_freq_seg1 = chandef->center_freq2 + 40;
>> + /* fall through */
>> + case NL80211_CHAN_WIDTH_80:
>> + start_freq_seg0 = chandef->center_freq1 - 40;
>> + end_freq_seg0 = chandef->center_freq1 + 40;
>> + break;
>> + case NL80211_CHAN_WIDTH_160:
>> + start_freq_seg0 = chandef->center_freq1 - 80;
>> + end_freq_seg0 = chandef->center_freq1 + 80;
>> + break;
>> + case NL80211_CHAN_WIDTH_20_NOHT:
>> + case NL80211_CHAN_WIDTH_20:
>> + case NL80211_CHAN_WIDTH_5:
>> + case NL80211_CHAN_WIDTH_10:
>> + break;
>> + }
>> +
>> + if (chan->center_freq > start_freq_seg0 &&
>> + chan->center_freq < end_freq_seg0)
>> + return true;
>> +
>> + return chan->center_freq > start_freq_seg1 &&
>> + chan->center_freq < end_freq_seg1;
>> +}
>
> It's also written pretty oddly... The 5/10/20 cases could return
> immediately, the start/end could be replaced by width, and the
> initializations wouldn't be needed at all ... I think we can do better
> here.
Sure, I'll improve this function.
>
>> +bool cfg80211_5ghz_any_wiphy_oper_chan(struct wiphy *wiphy,
>> + struct ieee80211_channel
>> *chan)
>
> Again, nothing 5 GHz specific.
Ok.
>
>> + struct wireless_dev *wdev;
>> +
>> + ASSERT_RTNL();
>> +
>> + if (!(chan->flags & IEEE80211_CHAN_RADAR))
>> + return false;
>> +
>> + list_for_each_entry(wdev, &wiphy->wdev_list, list) {
>> + if (!cfg80211_beaconing_iface_active(wdev))
>> + continue;
>> +
>> + if (cfg80211_5ghz_sub_chan(&wdev->chandef, chan))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>>
>> static bool cfg80211_get_chans_dfs_available(struct wiphy *wiphy,
>> u32 center_freq,
>> diff --git a/net/wireless/core.h b/net/wireless/core.h
>> index 58ca206..327fe95 100644
>> --- a/net/wireless/core.h
>> +++ b/net/wireless/core.h
>> @@ -459,6 +459,13 @@ void cfg80211_set_dfs_state(struct wiphy *wiphy,
>> cfg80211_chandef_dfs_cac_time(struct wiphy *wiphy,
>> const struct cfg80211_chan_def
>> *chandef);
>>
>> +void cfg80211_sched_dfs_chan_update(struct
>> cfg80211_registered_device *rdev);
>> +
>> +bool cfg80211_5ghz_any_wiphy_oper_chan(struct wiphy *wiphy,
>> + struct ieee80211_channel
>> *chan);
>> +
>> +bool cfg80211_beaconing_iface_active(struct wireless_dev *wdev);
>> +
>> static inline unsigned int elapsed_jiffies_msecs(unsigned long
>> start)
>> {
>> unsigned long end = jiffies;
>> diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
>> index 364f900..10bf040 100644
>> --- a/net/wireless/ibss.c
>> +++ b/net/wireless/ibss.c
>> @@ -190,6 +190,7 @@ static void __cfg80211_clear_ibss(struct
>> net_device *dev, bool nowext)
>> if (!nowext)
>> wdev->wext.ibss.ssid_len = 0;
>> #endif
>> + cfg80211_sched_dfs_chan_update(rdev);
>> }
>>
>> void cfg80211_clear_ibss(struct net_device *dev, bool nowext)
>> diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
>> index 2d8518a..ec0b1c2 100644
>> --- a/net/wireless/mesh.c
>> +++ b/net/wireless/mesh.c
>> @@ -262,6 +262,7 @@ int __cfg80211_leave_mesh(struct
>> cfg80211_registered_device *rdev,
>> wdev->beacon_interval = 0;
>> memset(&wdev->chandef, 0, sizeof(wdev->chandef));
>> rdev_set_qos_map(rdev, dev, NULL);
>> + cfg80211_sched_dfs_chan_update(rdev);
>> }
>>
>> return err;
>> diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
>> index 22b3d99..3c7e155 100644
>> --- a/net/wireless/mlme.c
>> +++ b/net/wireless/mlme.c
>> @@ -745,6 +745,12 @@ bool cfg80211_rx_mgmt(struct wireless_dev *wdev,
>> int freq, int sig_mbm,
>> }
>> EXPORT_SYMBOL(cfg80211_rx_mgmt);
>>
>> +void cfg80211_sched_dfs_chan_update(struct
>> cfg80211_registered_device *rdev)
>> +{
>> + cancel_delayed_work(&rdev->dfs_update_channels_wk);
>> + queue_delayed_work(cfg80211_wq, &rdev-
>>> dfs_update_channels_wk, 0);
>> +}
>
> This uses 0.
>
>> @@ -820,9 +844,7 @@ void cfg80211_radar_event(struct wiphy *wiphy,
>> */
>> cfg80211_set_dfs_state(wiphy, chandef,
>> NL80211_DFS_UNAVAILABLE);
>>
>> - timeout = msecs_to_jiffies(IEEE80211_DFS_MIN_NOP_TIME_MS);
>> - queue_delayed_work(cfg80211_wq, &rdev-
>>> dfs_update_channels_wk,
>> - timeout);
>> + cfg80211_sched_dfs_chan_update(rdev);
>
> But this didn't - why does that change?
Since cfg80211_dfs_channels_update_work() can be scheduled multiple times to run at
different point of time (2 secs - to expire cac for non-ETSI, 30 * 60 secs - to clear
NOL), cfg80211_sched_dfs_chan_update(rdev) is added to make sure the worker can also
be fired at nearer time stamp when it is already pending to run at after a relatively
later point of time. cfg80211_dfs_channels_update_work() uses the time stamp of channel
DFS state (dfs_state_entered) to set the next DFS state and/or re-schedule the worker
later.
>
>> +unsigned long regulatory_get_pre_cac_timeout(struct wiphy *wiphy)
>> +{
>> + if (!regulatory_pre_cac_allowed(wiphy))
>> + return REG_PRE_CAC_EXPIRY_GRACE_MS;
>> +
>> + /*
>> + * Return the maximum pre-CAC timeout when pre-CAC is
>> allowed
>> + * in the current dfs domain (ETSI).
>> + */
>> + return -1;
>> +}
>
> Don't ever return -1, that's -EPERM and not really what you want
> anyway.
>
Sure, since the return time is unsigned long I chose to use -1. I'll remove
this function as mentioned in below comment.
> In fact, this doesn't even make sense, since the only caller already
> checks regulatory_pre_cac_allowed() before calling this.
Sure. I originally thought a helper like this would be used multiple places.
But it is not the case now and being used in single place.
Thanks,
Vasanth
