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 14:42:41 +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: <20191224114241.uxfdifi2suocp2sa@bars>
References: <20191222145449.15792-1-Orr.Mazor@tandemg.com>
 <20191223105234.lgsupxfapbmxuvc5@bars>
 <AM6PR02MB36219B15C4F70980804728B0EF290@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: <AM6PR02MB36219B15C4F70980804728B0EF290@AM6PR02MB3621.eurprd02.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi Orr,

Thanks for detailed use-case clarification.

> >> 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 ?


Regards,
Sergey