Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 24 Dec 2019 18:37:40 +0300
From:   Sergey Matyukevich <geomatsi@gmail.com>
To:     Orr Mazor <orr.mazor@tandemg.com>
Cc:     Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>,
        Johannes Berg <johannes@sipsolutions.net>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] subsystem: Fix radar event during another phy CAC
Message-ID: <20191224153740.c6gybca7sulmitzq@bars>
References: <20191222145449.15792-1-Orr.Mazor@tandemg.com>
 <20191223105234.lgsupxfapbmxuvc5@bars>
 <AM6PR02MB36219B15C4F70980804728B0EF290@AM6PR02MB3621.eurprd02.prod.outlook.com>
 <20191224114241.uxfdifi2suocp2sa@bars>
 <AM6PR02MB362166B19724CB89D587751DEF290@AM6PR02MB3621.eurprd02.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <AM6PR02MB362166B19724CB89D587751DEF290@AM6PR02MB3621.eurprd02.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> >> >> In case a radar event of CAC_FINISHED or RADAR_DETECTED happens
> >during
> >> >> another phy is during CAC we might need to cancel that CAC.
> >> >> If we got a radar in a channel that another phy is now doing CAC on
> >> >> then the CAC should be canceled.
> >> >> If, for example, 2 phys doing CAC on the same channels, or on
> >> >> comptable channels, once on of them will finish his CAC the other
> >> >> might need to cancel his CAC, since it is no longer relevant.
> >> >>
> >> >> To fix that the commit adds an callback and implement it in mac80211
> >> >> to end CAC.
> >> >> This commit also adds a call to said callback if after a radar event
> >> >> we see the cac is no longer relevant
> >> >
> >> >>  net/mac80211/cfg.c      | 23 +++++++++++++++++++++++
> >> >>  net/wireless/rdev-ops.h | 10 ++++++++++
> >> >>  net/wireless/reg.c      | 24 +++++++++++++++++++++++-
> >> >>  net/wireless/trace.h    |  5 +++++
> >> >>  5 files changed, 66 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index
> >> >> 4ab2c49423dc..68782ba8b6e8 100644
> >> >> --- a/include/net/cfg80211.h
> >> >> +++ b/include/net/cfg80211.h
> >> >> @@ -3537,6 +3537,9 @@ struct cfg80211_update_owe_info {
> >> >>   *
> >> >>   * @start_radar_detection: Start radar detection in the driver.
> >> >>   *
> >> >> + * @end_cac: End running CAC, probably because a related CAC
> >> >> + *   was finished on another phy.
> >> >> + *
> >> >
> >> >Maybe it makes sense to follow existing naming convention here and to
> >use
> >> >something like 'stop_radar_detection' ?
> >>
> >> I think 'stop_radar_detection' might be misleading as we don’t stop
> >radar_detection,
> >> we only end cac, normal radar detection will continue.
> >
> >Ok, good point.
> >
> >
> >> >>   * @update_ft_ies: Provide updated Fast BSS Transition information to
> >the
> >> >>   *   driver. If the SME is in the driver/firmware, this information can be
> >> >>   *   used in building Authentication and Reassociation Request frames.
> >> >> @@ -3863,6 +3866,8 @@ struct cfg80211_ops {
> >> >>                                        struct net_device *dev,
> >> >>                                        struct cfg80211_chan_def *chandef,
> >> >>                                        u32 cac_time_ms);
> >> >> +     void    (*end_cac)(struct wiphy *wiphy,
> >> >> +                             struct net_device *dev);
> >> >
> >> >...
> >> >
> >> >> +static void cfg80211_check_and_end_cac(struct
> >> >> +cfg80211_registered_device *rdev) {
> >> >> +     struct wireless_dev *wdev;
> >> >> +     /* If we finished CAC or received radar, we should end any
> >> >> +      * CAC running on the same channels.
> >> >> +      * the check !cfg80211_chandef_dfs_usable contain 2 options:
> >> >> +      * either all channels are available - those the CAC_FINISHED
> >> >> +      * event has effected another wdev state, or there is a channel
> >> >> +      * in unavailable state in wdev chandef - those the RADAR_DETECTED
> >> >> +      * event has effected another wdev state.
> >> >> +      * In both cases we should end the CAC on the wdev.
> >> >> +      *
> >> >> +      */
> >> >> +     list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list) {
> >> >> +             if (wdev->cac_started &&
> >> >> +                 !cfg80211_chandef_dfs_usable(&rdev->wiphy, &wdev-
> >> >>chandef))
> >> >> +                     rdev_end_cac(rdev, wdev->netdev);
> >> >> +     }
> >> >> +}
> >> >> +
> >> >
> >> >IIUC, this code does not match your commit message. You are stopping
> >CAC
> >> >on all the virtual wireless interfaces on the same PHY, but not CACs on
> >> >different PHYs. Meanwhile CAC does not need to be started on multiple
> >> >virtual interfaces. For instance, in multiple BSSID configuration, hostapd
> >> >performs CAC only on primary interface.
> >> >
> >>
> >> regulatory_propagate_dfs_state will call cfg80211_check_and_end_cac
> >> only on phys != current phy.
> >> So for each phy != current we will call mac80211 end_cac (if needed)
> >> which in turn will end the cac on all that phys’ interfaces.
> >
> >Ok, so regulatory_propagate_dfs_state iterates over other PHYs from the
> >same regulatory region updating state of DFS channel reported by the
> >original PHY. Unless I am missing something, in this case there are two
> >possible ways to proceed with CAC running on other PHYs:
> >
> >- to stop CAC on other PHYs with the same channel/bandwidth in both
> >  RADAR_DETECTION and CAC_FINISHED cases
> >- to stop CAC on other PHYs if their channel has just become unusable
> >  due to RADAR_DETECTION event reported by the original PHY
> >
> >So it looks like you have chosen a more conservative second option.
> >But then I don't quite understand your comment for the new function
> >cfg80211_check_and_end_cac: why do you say that CAC_FINISHED case
> >is also covered here ?
> >
> 
> CAC_FINISHED is covered here:
> If there is an unavailable channel cfg80211_chandef_dfs_usable will return false - this covers RADAR_DETECTED.
> If all channels are available cfg80211_chandef_dfs_usable will also return false,
> as they are not usable, they are available - this covers CAC_FINISHED.
> Basically cfg80211_chandef_dfs_usable checks if we need to do CAC,
> it is also used in nl80211_start_radar_detection in order to make sure we actually need CAC.

Ok, thanks for clarification. So in the end this is the first option:
CAC on other PHYs is stopped in both cases including 
RADAR_DETECTION and CAC_FINISHED.

Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>

Regards,
Sergey