[PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Simon Wunderlich]

Performing spectral scans on 5 GHz channels may result in PHY errors
sent by the hardware, even if DFS support is not enabled in the driver
(e.g. channel scanning or passive monitoring). In that case channels may
falsely get marked as 'unusable'. To fix that, only process radar PHY
errors when radar is explicitly enabled in the driver.

Cc: Stable <stable@vger.kernel.org> [v3.10+]
Reported-by: Mathias Kretschmer <mathias.kretschmer@fokus.fraunhofer.de>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
 drivers/net/wireless/ath/ath9k/dfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c
index 726271c..3d04905 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -152,6 +152,12 @@ void ath9k_dfs_process_phyerr(struct ath_softc *sc, void *data,
 		return;
 	}
 
+	if (!sc->hw->conf.radar_enabled) {
+		ath_dbg(common, DFS,
+			"Error: received radar phyerr while radar was disabled\n");
+		return;
+	}
+
 	datalen = rs->rs_datalen;
 	if (datalen == 0) {
 		DFS_STAT_INC(sc, datalen_discards);
-- 
2.1.4

Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Arend van Spriel]

On 01/09/15 17:54, Simon Wunderlich wrote:
> Performing spectral scans on 5 GHz channels may result in PHY errors
> sent by the hardware, even if DFS support is not enabled in the driver
> (e.g. channel scanning or passive monitoring). In that case channels may
> falsely get marked as 'unusable'. To fix that, only process radar PHY
> errors when radar is explicitly enabled in the driver.

Hi Simon,

Not an ath9k expert, but I would think those channels would already be 
marked as unusable, because DFS is disabled in the driver. Or does this 
also affect 5G channels that do not require DFS.

Regards,
Arend

> Cc: Stable<stable@vger.kernel.org>  [v3.10+]
> Reported-by: Mathias Kretschmer<mathias.kretschmer@fokus.fraunhofer.de>
> Signed-off-by: Simon Wunderlich<sw@simonwunderlich.de>
> ---
>   drivers/net/wireless/ath/ath9k/dfs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c
> index 726271c..3d04905 100644
> --- a/drivers/net/wireless/ath/ath9k/dfs.c
> +++ b/drivers/net/wireless/ath/ath9k/dfs.c
> @@ -152,6 +152,12 @@ void ath9k_dfs_process_phyerr(struct ath_softc *sc, void *data,
>   		return;
>   	}
>
> +	if (!sc->hw->conf.radar_enabled) {
> +		ath_dbg(common, DFS,
> +			"Error: received radar phyerr while radar was disabled\n");
> +		return;
> +	}
> +
>   	datalen = rs->rs_datalen;
>   	if (datalen == 0) {
>   		DFS_STAT_INC(sc, datalen_discards);

Re: Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

On Friday 09 January 2015 19:57:37 Arend van Spriel wrote:
> On 01/09/15 17:54, Simon Wunderlich wrote:
> > Performing spectral scans on 5 GHz channels may result in PHY errors
> > sent by the hardware, even if DFS support is not enabled in the driver
> > (e.g. channel scanning or passive monitoring). In that case channels may
> > falsely get marked as 'unusable'. To fix that, only process radar PHY
> > errors when radar is explicitly enabled in the driver.
> 
> Hi Simon,
> 
> Not an ath9k expert, but I would think those channels would already be
> marked as unusable, because DFS is disabled in the driver. Or does this
> also affect 5G channels that do not require DFS.
> 
> Regards,
> Arend

Hey Arend,

maybe that was not really clear, but this is talking about the DFS state 
"unusable". By default, channels are marked in DFS state "usable", and after 
the clear channel assessment (which is done e.g. when starting AP mode) they 
are marked as "available". As soon as radar is detected they are marked as 
"unusable".

These DFS state changes should only happen while there is something operating 
with radar enabled, e.g. AP mode. It should not happen if we just have monitor 
mode or scan for channels. These channels should then stay in their previous 
DFS state (e.g. 'usable'). This was borked and this patch tries to fix it. :)

Cheers,
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Zefir Kurtisi]

[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]

On 01/10/2015 05:26 PM, Simon Wunderlich wrote:
> On Friday 09 January 2015 19:57:37 Arend van Spriel wrote:
>> On 01/09/15 17:54, Simon Wunderlich wrote:
>>> Performing spectral scans on 5 GHz channels may result in PHY errors
>>> sent by the hardware, even if DFS support is not enabled in the driver
>>> (e.g. channel scanning or passive monitoring). In that case channels may
>>> falsely get marked as 'unusable'. To fix that, only process radar PHY
>>> errors when radar is explicitly enabled in the driver.
>>
>> Hi Simon,
>>
>> Not an ath9k expert, but I would think those channels would already be
>> marked as unusable, because DFS is disabled in the driver. Or does this
>> also affect 5G channels that do not require DFS.
>>
>> Regards,
>> Arend
> 
> Hey Arend,
> 
> maybe that was not really clear, but this is talking about the DFS state 
> "unusable". By default, channels are marked in DFS state "usable", and after 
> the clear channel assessment (which is done e.g. when starting AP mode) they 
> are marked as "available". As soon as radar is detected they are marked as 
> "unusable".
> 
> These DFS state changes should only happen while there is something operating 
> with radar enabled, e.g. AP mode. It should not happen if we just have monitor 
> mode or scan for channels. These channels should then stay in their previous 
> DFS state (e.g. 'usable'). This was borked and this patch tries to fix it. :)
> 
> Cheers,
>      Simon
> 
Hi,

the issue here is that DFS and spectral use the same PHY_ERROR reporting
mechanism, and the dfs module is still in its initial state prior the spectral
support was added. With that, feeding the dfs detector with PHY_ERROR frames
generated by spectral scanner might cause false radar detections.

I did not dig how the hw->conf.radar_enabled flag is set in monitor mode, but if
it is same as for master (i.e. set for DFS channels), then it would be a better
approach to prevent calling ath9k_dfs_process_phyerr() altogether from
ath9k_rx_skb_preprocess() if not set.

And while you're at that: slaves do not need to scan for radar, might be worth
checking if it makes sense to selectively disable radar detection in STA mode. I
am using attached private OpenWRT patch for that - which still would interfere
with spectral scanning. Generally, the PHY_ERROR processing should be reworked but
becomes quite complicated when you take into account special use-cases. Think of
radar events being treated differently depending on whether a master or a monitor
detected them (OC-CAC vs. ISM).


Cheers,
Zefir


[-- Attachment #2: 999-0001-ath9k-do-not-enable-radar-pulse-detection-in-STA-mod.patch --]
[-- Type: text/x-patch, Size: 788 bytes --]

>From 089ab0d624d4b6f3a206ea8a81b4a3e061cf3edb Mon Sep 17 00:00:00 2001
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Thu, 30 Jan 2014 13:33:50 +0100
Subject: [PATCH] ath9k: do not enable radar pulse detection in STA mode

---
 drivers/net/wireless/ath/ath9k/recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -384,7 +384,7 @@ u32 ath_calcrxfilter(struct ath_softc *s
 		| ATH9K_RX_FILTER_MCAST;
 
 	/* if operating on a DFS channel, enable radar pulse detection */
-	if (sc->hw->conf.radar_enabled)
+	if (sc->hw->conf.radar_enabled && sc->sc_ah->opmode != NL80211_IFTYPE_STATION)
 		rfilt |= ATH9K_RX_FILTER_PHYRADAR | ATH9K_RX_FILTER_PHYERR;
 
 	spin_lock_bh(&sc->chan_lock);

Re: Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 3809 bytes --]

On Tuesday 13 January 2015 11:16:02 Zefir Kurtisi wrote:
> On 01/10/2015 05:26 PM, Simon Wunderlich wrote:
> > On Friday 09 January 2015 19:57:37 Arend van Spriel wrote:
> >> On 01/09/15 17:54, Simon Wunderlich wrote:
> >>> Performing spectral scans on 5 GHz channels may result in PHY errors
> >>> sent by the hardware, even if DFS support is not enabled in the driver
> >>> (e.g. channel scanning or passive monitoring). In that case channels may
> >>> falsely get marked as 'unusable'. To fix that, only process radar PHY
> >>> errors when radar is explicitly enabled in the driver.
> >> 
> >> Hi Simon,
> >> 
> >> Not an ath9k expert, but I would think those channels would already be
> >> marked as unusable, because DFS is disabled in the driver. Or does this
> >> also affect 5G channels that do not require DFS.
> >> 
> >> Regards,
> >> Arend
> > 
> > Hey Arend,
> > 
> > maybe that was not really clear, but this is talking about the DFS state
> > "unusable". By default, channels are marked in DFS state "usable", and
> > after the clear channel assessment (which is done e.g. when starting AP
> > mode) they are marked as "available". As soon as radar is detected they
> > are marked as "unusable".
> > 
> > These DFS state changes should only happen while there is something
> > operating with radar enabled, e.g. AP mode. It should not happen if we
> > just have monitor mode or scan for channels. These channels should then
> > stay in their previous DFS state (e.g. 'usable'). This was borked and
> > this patch tries to fix it. :)
> > 
> > Cheers,
> > 
> >      Simon
> 
> Hi,
> 
> the issue here is that DFS and spectral use the same PHY_ERROR reporting
> mechanism, and the dfs module is still in its initial state prior the
> spectral support was added. With that, feeding the dfs detector with
> PHY_ERROR frames generated by spectral scanner might cause false radar
> detections.

Yup, that's right - we noticed that too, and its written in various places 
that the FFT and DFS hardware logic is shared. :)
> 
> I did not dig how the hw->conf.radar_enabled flag is set in monitor mode,
> but if it is same as for master (i.e. set for DFS channels), then it would
> be a better approach to prevent calling ath9k_dfs_process_phyerr()
> altogether from ath9k_rx_skb_preprocess() if not set.

Hm, you mean like - if radar_enabled then dfs_process, otherwise fft_process? 
That would might be more elegant indeed ...

The monitor mode does not have the radar flag enabled, 
cfg80211_chandef_dfs_required() returns 0 in this case.

> 
> And while you're at that: slaves do not need to scan for radar, might be
> worth checking if it makes sense to selectively disable radar detection in
> STA mode. I am using attached private OpenWRT patch for that - which still
> would interfere with spectral scanning. Generally, the PHY_ERROR processing
> should be reworked but becomes quite complicated when you take into account
> special use-cases. Think of radar events being treated differently
> depending on whether a master or a monitor detected them (OC-CAC vs. ISM).

I didn't check if that is enforced correctly, but 
cfg80211_chandef_dfs_required() returns if radar is required for the various 
interface types - AP, Adhoc and Mesh have it enabled if its a DFS channel, 
client, monitor, etc don't have it enabled. That gets marked in the sdata-
>radar_required, and ieee80211_is_radar_required() checks all interfaces if 
there is any interface which needs radar. So that should have been taken care 
of.

Therefore I think that this is already handled in cfg80211/mac80211 and ath9k 
should not check the iftype at all, but only check the radar_enabled flag.

Off-channel CAC is certainly a different beast, but as far as I know we 
currently don't support that anyway. :)

Cheers,
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Zefir Kurtisi]

On 01/13/2015 12:04 PM, Simon Wunderlich wrote:
> On Tuesday 13 January 2015 11:16:02 Zefir Kurtisi wrote:
>> [...]
>>
>> I did not dig how the hw->conf.radar_enabled flag is set in monitor mode,
>> but if it is same as for master (i.e. set for DFS channels), then it would
>> be a better approach to prevent calling ath9k_dfs_process_phyerr()
>> altogether from ath9k_rx_skb_preprocess() if not set.
> 
> Hm, you mean like - if radar_enabled then dfs_process, otherwise fft_process? 
> That would might be more elegant indeed ...
> 
More concrete / restrictive:
* if radar_enabled
 - spectral must not be enabled
 - only ath9k_dfs_process_phyerr() has to be processed
* if !radar_enabled
 - don't process ath9k_dfs_process_phyerr()

> The monitor mode does not have the radar flag enabled, 
> cfg80211_chandef_dfs_required() returns 0 in this case.
> 
Ah, which then means you can not do (supplemental) CACs with a monitor
out-of-the-box? For that, radar_enabled would need to be set for monitor, which
basically should not harm for a fully passive interface.

>>
>> And while you're at that: slaves do not need to scan for radar, might be
>> worth checking if it makes sense to selectively disable radar detection in
>> STA mode. I am using attached private OpenWRT patch for that - which still
>> would interfere with spectral scanning. Generally, the PHY_ERROR processing
>> should be reworked but becomes quite complicated when you take into account
>> special use-cases. Think of radar events being treated differently
>> depending on whether a master or a monitor detected them (OC-CAC vs. ISM).
> 
> I didn't check if that is enforced correctly, but 
> cfg80211_chandef_dfs_required() returns if radar is required for the various 
> interface types - AP, Adhoc and Mesh have it enabled if its a DFS channel, 
> client, monitor, etc don't have it enabled. That gets marked in the sdata-
>> radar_required, and ieee80211_is_radar_required() checks all interfaces if 
> there is any interface which needs radar. So that should have been taken care 
> of.
> 
> Therefore I think that this is already handled in cfg80211/mac80211 and ath9k 
> should not check the iftype at all, but only check the radar_enabled flag.
> 
Ok, thanks for clarifying that - one private patch less to handle :)

Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Kalle Valo]

Simon Wunderlich <sw@simonwunderlich.de> writes:

>> I did not dig how the hw->conf.radar_enabled flag is set in monitor mode,
>> but if it is same as for master (i.e. set for DFS channels), then it would
>> be a better approach to prevent calling ath9k_dfs_process_phyerr()
>> altogether from ath9k_rx_skb_preprocess() if not set.
>
> Hm, you mean like - if radar_enabled then dfs_process, otherwise fft_process? 
> That would might be more elegant indeed ...
>
> The monitor mode does not have the radar flag enabled, 
> cfg80211_chandef_dfs_required() returns 0 in this case.

So are you going to send v2 or what's the plan? I didn't quite get the
conclusion from the discussion.

-- 
Kalle Valo

Re: Re: [PATCH] ath9k: ignore radar PHY errors when DFS is not enabled

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

Hey Kalle,

On Thursday 15 January 2015 16:30:51 Kalle Valo wrote:
> Simon Wunderlich <sw@simonwunderlich.de> writes:
> >> I did not dig how the hw->conf.radar_enabled flag is set in monitor mode,
> >> but if it is same as for master (i.e. set for DFS channels), then it
> >> would
> >> be a better approach to prevent calling ath9k_dfs_process_phyerr()
> >> altogether from ath9k_rx_skb_preprocess() if not set.
> > 
> > Hm, you mean like - if radar_enabled then dfs_process, otherwise
> > fft_process? That would might be more elegant indeed ...
> > 
> > The monitor mode does not have the radar flag enabled,
> > cfg80211_chandef_dfs_required() returns 0 in this case.
> 
> So are you going to send v2 or what's the plan? I didn't quite get the
> conclusion from the discussion.

sorry for the silence - yes, please drop this version, I'll send v2.

Thanks
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]