MIME-Version: 1.0
In-Reply-To: <1347274394.4272.8.camel@jlt4.sipsolutions.net>
References: <1344426823-1795-1-git-send-email-victorg@ti.com>
	<1344426823-1795-2-git-send-email-victorg@ti.com>
	<1347274394.4272.8.camel@jlt4.sipsolutions.net>
Date: Wed, 3 Oct 2012 15:53:45 +0200
Message-ID: <CAK80WHYAH3FdoTDGQh54NdLkYMpv7heGsNjn7venC9fHj65RFw@mail.gmail.com> (sfid-20121003_155352_471166_7BFDE32C)
Subject: Re: [PATCH v3 1/7] nl80211/cfg80211: add radar detection command/event
From: "Goldenshtein, Victor" <victorg@ti.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org, kgiori@qca.qualcomm.com,
	mcgrof@frijolero.org, zefir.kurtisi@neratec.com,
	adrian.chadd@gmail.com, j@w1.fi, coelho@ti.com, assaf@ti.com,
	yoni.divinsky@ti.com, igalc@ti.com, adrian@freebsd.org,
	nbd@nbd.name, simon.wunderlich@s2003.tu-chemnitz.de
Content-Type: text/plain; charset=UTF-8
Sender: linux-wireless-owner@vger.kernel.org

Thanks for the review and sorry for the delayed response.

On Mon, Sep 10, 2012 at 1:53 PM, Johannes Berg
<johannes@sipsolutions.net> 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.