diff mbox

ath9k: ignore radar PHY errors when DFS is not enabled

Message ID 54B4F062.2090301@neratec.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Zefir Kurtisi Jan. 13, 2015, 10:16 a.m. UTC
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

Comments

Simon Wunderlich Jan. 13, 2015, 11:04 a.m. UTC | #1
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
Zefir Kurtisi Jan. 13, 2015, 12:08 p.m. UTC | #2
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 :)


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kalle Valo Jan. 15, 2015, 2:30 p.m. UTC | #3
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.
Simon Wunderlich Jan. 15, 2015, 3:58 p.m. UTC | #4
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
diff mbox

Patch

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