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 <[email protected]> [v3.10+]
Reported-by: Mathias Kretschmer <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
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
Hey Kalle,
On Thursday 15 January 2015 16:30:51 Kalle Valo wrote:
> Simon Wunderlich <[email protected]> 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
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<[email protected]> [v3.10+]
> Reported-by: Mathias Kretschmer<[email protected]>
> Signed-off-by: Simon Wunderlich<[email protected]>
> ---
> 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);
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
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
Simon Wunderlich <[email protected]> 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
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
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 :)