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

[Simon Wunderlich <sw@simonwunderlich.de>]

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