[PATCH] subsystem: Fix radar event during another phy CAC

[PATCH] subsystem: Fix radar event during another phy CAC

[Orr Mazor]

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

Signed-off-by: Orr Mazor <Orr.Mazor@tandemg.com>
---
 include/net/cfg80211.h  |  5 +++++
 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.
+ *
  * @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);
 	int	(*update_ft_ies)(struct wiphy *wiphy, struct net_device *dev,
 				 struct cfg80211_update_ft_ies_params *ftie);
 	int	(*crit_proto_start)(struct wiphy *wiphy,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..0daaf7e37a21 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2954,6 +2954,28 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
 	return err;
 }
 
+static void ieee80211_end_cac(struct wiphy *wiphy,
+			      struct net_device *dev)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_local *local = sdata->local;
+
+	mutex_lock(&local->mtx);
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		/* it might be waiting for the local->mtx, but then
+		 * by the time it gets it, sdata->wdev.cac_started
+		 * will no longer be true
+		 */
+		cancel_delayed_work(&sdata->dfs_cac_timer_work);
+
+		if (sdata->wdev.cac_started) {
+			ieee80211_vif_release_channel(sdata);
+			sdata->wdev.cac_started = false;
+		}
+	}
+	mutex_unlock(&local->mtx);
+}
+
 static struct cfg80211_beacon_data *
 cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
 {
@@ -4023,6 +4045,7 @@ const struct cfg80211_ops mac80211_config_ops = {
 #endif
 	.get_channel = ieee80211_cfg_get_channel,
 	.start_radar_detection = ieee80211_start_radar_detection,
+	.end_cac = ieee80211_end_cac,
 	.channel_switch = ieee80211_channel_switch,
 	.set_qos_map = ieee80211_set_qos_map,
 	.set_ap_chanwidth = ieee80211_set_ap_chanwidth,
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index e853a4fe6f97..663c0d3127a4 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1167,6 +1167,16 @@ rdev_start_radar_detection(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline void
+rdev_end_cac(struct cfg80211_registered_device *rdev,
+	     struct net_device *dev)
+{
+	trace_rdev_end_cac(&rdev->wiphy, dev);
+	if (rdev->ops->end_cac)
+		rdev->ops->end_cac(&rdev->wiphy, dev);
+	trace_rdev_return_void(&rdev->wiphy);
+}
+
 static inline int
 rdev_set_mcast_rate(struct cfg80211_registered_device *rdev,
 		    struct net_device *dev,
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 446c76d44e65..d18cc05061a0 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -3885,6 +3885,26 @@ bool regulatory_pre_cac_allowed(struct wiphy *wiphy)
 }
 EXPORT_SYMBOL(regulatory_pre_cac_allowed);
 
+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);
+	}
+}
+
 void regulatory_propagate_dfs_state(struct wiphy *wiphy,
 				    struct cfg80211_chan_def *chandef,
 				    enum nl80211_dfs_state dfs_state,
@@ -3911,8 +3931,10 @@ void regulatory_propagate_dfs_state(struct wiphy *wiphy,
 		cfg80211_set_dfs_state(&rdev->wiphy, chandef, dfs_state);
 
 		if (event == NL80211_RADAR_DETECTED ||
-		    event == NL80211_RADAR_CAC_FINISHED)
+		    event == NL80211_RADAR_CAC_FINISHED) {
 			cfg80211_sched_dfs_chan_update(rdev);
+			cfg80211_check_and_end_cac(rdev);
+		}
 
 		nl80211_radar_notify(rdev, chandef, event, NULL, GFP_KERNEL);
 	}
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index d98ad2b3143b..8677d7ab7d69 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -646,6 +646,11 @@ DEFINE_EVENT(wiphy_netdev_evt, rdev_flush_pmksa,
 	TP_ARGS(wiphy, netdev)
 );
 
+DEFINE_EVENT(wiphy_netdev_evt, rdev_end_cac,
+	     TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
+	     TP_ARGS(wiphy, netdev)
+);
+
 DECLARE_EVENT_CLASS(station_add_change,
 	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, u8 *mac,
 		 struct station_parameters *params),
-- 
2.17.1

Re: [PATCH] subsystem: Fix radar event during another phy CAC

[Sergey Matyukevich]

Hi Orr,

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

>   * @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.

Could you please clarify the use-case which requires this functionality ?

Regards,
Sergey


This email, including its contents and any attachment(s), may contain confidential information of ON Semiconductor and is solely for the intended recipient(s). If you may have received this in error, please contact the sender and permanently delete this email, its contents and any attachment(s).

RE: [PATCH] subsystem: Fix radar event during another phy CAC

[Orr Mazor]

Hi Sergey,

>Hi Orr,
>
>> 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. 

>
>>   * @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.

>Could you please clarify the use-case which requires this functionality ?
>


I will explain more on the use-case:
Let say we have 2 phys on 5ghz: phy0, phy1
And 2 interfaces accordingly: wlan0, wlan1
We start hostapd with wlan0 in channel 60,
5 seconds later we start hostapd with wlan1 on channel 60.

What will happen is that when phy0 finishes CAC,
It will propagate it to phy1 and to the other hostapd, 
which causes it to start the ap fully.
However the CAC timer on wlan1 is still running,
The propagate did not stop it.

When wlan1 CAC is finished we will get the following:
WARNING: CPU: 0 PID: 4406 at net/mac80211/chan.c:1753 ieee80211_vif_release_channel+0x21/0x60 [mac80211]
From
[  +0.000002] Call Trace:
[  +0.000044]  ieee80211_dfs_cac_timer_work+0x74/0xc0 [mac80211]
Since we are trying to release the channel when the interface is active.

Also, from that point on, we will be unable to do channel switch on wlan1,
probably because we released the channel. 

A few minutes later this also shows up:
WARNING: CPU: 2 PID: 6017 at net/mac80211/ieee80211_i.h:1435 ieee80211_change_bss+0x1a6/0x1c0 [mac80211]

The idea behind this patch is to improve the regulatory_propagate_dfs_state,
so it will also consider cases in which the radar event effects other phys CAC,
and won’t get that warnings and issues in that case.

>Regards,
>Sergey
>
>
>This email, including its contents and any attachment(s), may contain
>confidential information of ON Semiconductor and is solely for the intended
>recipient(s). If you may have received this in error, please contact the sender
>and permanently delete this email, its contents and any attachment(s).

Regards,
Orr

Re: [PATCH] subsystem: Fix radar event during another phy CAC

[Sergey Matyukevich]

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

RE: [PATCH] subsystem: Fix radar event during another phy CAC

[Orr Mazor]

Hi Sergey,

>Hi Orr,
>
>Thanks for detailed use-case clarification.

Sure thing.
Thanks for reviewing my patch.

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

Regards,
Orr

Re: [PATCH] subsystem: Fix radar event during another phy CAC

[Sergey Matyukevich]

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

Re: [PATCH] subsystem: Fix radar event during another phy CAC

[Kalle Valo]

Orr Mazor <orr.mazor@tandemg.com> writes:

> 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
>
> Signed-off-by: Orr Mazor <Orr.Mazor@tandemg.com>

The title prefix should be "cfg80211: ", not "subsystem: ".

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] subsystem: Fix radar event during another phy CAC

[Johannes Berg]

On Wed, 2020-01-15 at 09:40 +0200, Kalle Valo wrote:
> Orr Mazor <orr.mazor@tandemg.com> writes:
> 
> > 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
> > 
> > Signed-off-by: Orr Mazor <Orr.Mazor@tandemg.com>
> 
> The title prefix should be "cfg80211: ", not "subsystem: ".

Yeah, but I fixed it when I just applied it :)

johannes