Thanks for the review and sorry for the delayed response.
On Mon, Sep 10, 2012 at 1:53 PM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> On Wed, 2012-08-08 at 14:53 +0300, Victor Goldenshtein wrote:
>
>> + * @NL80211_FEATURE_DFS: Radar detection is supported in the HW/driver.
>
> How will you know what kind of radar detection is supported? That is, HT
> 20, HT 40, in the future VHT80/160/80+80?
>
only 20 Mhz is supported at first stage, do you prefer to rename it to
something like: NL80211_FEATURE_20MHZ_DFS
>> + int (*start_radar_detection)(struct wiphy *wiphy,
>> + struct net_device *dev,
>> + struct ieee80211_channel *chan);
>
> Channel type/bandwidth might be needed?
>
np, will add also the channel type here.
>> +void cfg80211_radar_detected(struct net_device *dev,
>> + struct ieee80211_channel *chan, gfp_t gfp);
>
> Is the channel parameter useful? Only one detection can be triggered at
> any given time.
yes, for MC platforms and for sanity checks.
>
>> void cfg80211_send_rx_auth(struct net_device *dev, const u8 *buf, size_t len)
>> {
>> struct wireless_dev *wdev = dev->ieee80211_ptr;
>> @@ -1006,3 +1008,39 @@ bool cfg80211_rx_unexpected_4addr_frame(struct net_device *dev,
>> return nl80211_unexpected_4addr_frame(dev, addr, gfp);
>> }
>> EXPORT_SYMBOL(cfg80211_rx_unexpected_4addr_frame);
>> +
>> +int cfg80211_start_radar_detection(struct cfg80211_registered_device *rdev,
>> + struct net_device *dev,
>> + struct ieee80211_channel *chan)
>> +{
>> + int err;
>> +
>> + if (!rdev->ops->start_radar_detection)
>> + return -EOPNOTSUPP;
>> +
>> + err = rdev->ops->start_radar_detection(&rdev->wiphy, dev, chan);
>> + if (!err)
>> + chan->radar_detect_timeout = jiffies +
>> + msecs_to_jiffies(IEEE80211_DFS_MIN_CAC_TIME_MS);
>> + else {
>> + chan->radar_detect_timeout = 0;
>> + chan->cac_type = 0;
>> + }
>
> You're not setting cac_type in the good case, and also
> radar_detect_timeout can actually be 0 in the good case due to jiffies
> wrap. How is that handled?
>
I'm using time_is_after_jiffies() in nl80211_dfs_en_tx() which AFAIK
can handle jiffies wrap.
>> + chan->cac_type = 0;
>
> Also here and above you should use channel type enums. In fact, 0 is
> "NOHT". Oops. Need a boolean I guess that indicates validity.
>
right, since NOHT is 0 we probably need additional flag something like
"chan->cac_started"
alternatively we can add "__NL80211_CHAN_INVALID" at index 0 to the
enum nl80211_channel_type.
>> + bool dfs_supported = (rdev->wiphy.features & NL80211_FEATURE_DFS);
>
> why bother with the variable?
>
IMHO in that way the if expression is shorter and more readable.
>> + if (!chan || !(chan->flags & IEEE80211_CHAN_RADAR))
>> + return -EINVAL;
>> +
>> + if (chan->cac_type)
>> + return -EBUSY;
>> +
>> + chan->cac_type = cac_type;
>
> Aha. But these things probably should be in the function:
>
>> + return cfg80211_start_radar_detection(rdev, dev, chan);
>
> so it's actually a potentially useful function for other places.
> Otherwise you could just manually inline it here, I see no issues with
> that either.
>
will manually inline cfg80211_start_radar_detection() into
nl80211_start_radar_detection().
--
Thanks,
Victor.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
