Discussion:
[ath9k-devel] [RFC 0/6] ath9k: DFS pattern detection
Zefir Kurtisi
2011-10-03 10:11:17 UTC
Permalink
This patch series proposes DFS pattern detection for ath9k.

With the source code for the proprietary wireless driver provided by
Atheros, the basic functionality to inspect radar pulses detected by
the HW and to post-filter false pulses has been re-implemented
for ath9k.



Zefir Kurtisi (6):
ath9k: add DFS statistics to debugfs
ath9k: add DFS debug flag
ath9k: initial radar pulse detection for DFS
ath9k: add DFS build parameter
ath9k: enable DFS pulse detection
ath9k: handle pulse data reported by DFS HW

drivers/net/wireless/ath/ath.h | 34 +++---
drivers/net/wireless/ath/ath9k/Kconfig | 7 +
drivers/net/wireless/ath/ath9k/Makefile | 2 +
drivers/net/wireless/ath/ath9k/debug.c | 51 ++++++++
drivers/net/wireless/ath/ath9k/debug.h | 29 +++++
drivers/net/wireless/ath/ath9k/dfs.c | 192 +++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath9k/dfs.h | 24 ++++
drivers/net/wireless/ath/ath9k/main.c | 12 ++
drivers/net/wireless/ath/ath9k/recv.c | 22 +++-
9 files changed, 352 insertions(+), 21 deletions(-)
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.c
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.h
--
1.7.4.1
Zefir Kurtisi
2011-10-03 10:29:12 UTC
Permalink
(now with patches ;))

This patch series proposes DFS pattern detection for ath9k.

With the source code for the proprietary wireless driver provided by
Atheros, the basic functionality to inspect radar pulses detected by
the HW and to post-filter false pulses has been re-implemented
for ath9k.



Zefir Kurtisi (6):
ath9k: add DFS statistics to debugfs
ath9k: add DFS debug flag
ath9k: initial radar pulse detection for DFS
ath9k: add DFS build parameter
ath9k: enable DFS pulse detection
ath9k: handle pulse data reported by DFS HW

drivers/net/wireless/ath/ath.h | 34 +++---
drivers/net/wireless/ath/ath9k/Kconfig | 7 +
drivers/net/wireless/ath/ath9k/Makefile | 2 +
drivers/net/wireless/ath/ath9k/debug.c | 51 ++++++++
drivers/net/wireless/ath/ath9k/debug.h | 29 +++++
drivers/net/wireless/ath/ath9k/dfs.c | 192 +++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath9k/dfs.h | 24 ++++
drivers/net/wireless/ath/ath9k/main.c | 12 ++
drivers/net/wireless/ath/ath9k/recv.c | 22 +++-
9 files changed, 352 insertions(+), 21 deletions(-)
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.c
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.h
--
1.7.4.1
Zefir Kurtisi
2011-10-03 10:29:14 UTC
Permalink
Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath.h | 34 ++++++++++++++++++----------------
1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 6d56532..34d4da1 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -211,6 +211,7 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
* @ATH_DBG_HWTIMER: hardware timer handling
* @ATH_DBG_BTCOEX: bluetooth coexistance
* @ATH_DBG_BSTUCK: stuck beacons
+ * @ATH_DBG_DFS: radar datection
* @ATH_DBG_ANY: enable all debugging
*
* The debug level is used to control the amount and type of debugging output
@@ -220,22 +221,23 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
* entry.
*/
enum ATH_DEBUG {
- ATH_DBG_RESET = 0x00000001,
- ATH_DBG_QUEUE = 0x00000002,
- ATH_DBG_EEPROM = 0x00000004,
- ATH_DBG_CALIBRATE = 0x00000008,
- ATH_DBG_INTERRUPT = 0x00000010,
- ATH_DBG_REGULATORY = 0x00000020,
- ATH_DBG_ANI = 0x00000040,
- ATH_DBG_XMIT = 0x00000080,
- ATH_DBG_BEACON = 0x00000100,
- ATH_DBG_CONFIG = 0x00000200,
- ATH_DBG_FATAL = 0x00000400,
- ATH_DBG_PS = 0x00000800,
- ATH_DBG_HWTIMER = 0x00001000,
- ATH_DBG_BTCOEX = 0x00002000,
- ATH_DBG_WMI = 0x00004000,
- ATH_DBG_BSTUCK = 0x00008000,
+ ATH_DBG_RESET = BIT(0),
+ ATH_DBG_QUEUE = BIT(1),
+ ATH_DBG_EEPROM = BIT(2),
+ ATH_DBG_CALIBRATE = BIT(3),
+ ATH_DBG_INTERRUPT = BIT(4),
+ ATH_DBG_REGULATORY = BIT(5),
+ ATH_DBG_ANI = BIT(6),
+ ATH_DBG_XMIT = BIT(7),
+ ATH_DBG_BEACON = BIT(8),
+ ATH_DBG_CONFIG = BIT(9),
+ ATH_DBG_FATAL = BIT(10),
+ ATH_DBG_PS = BIT(11),
+ ATH_DBG_HWTIMER = BIT(12),
+ ATH_DBG_BTCOEX = BIT(13),
+ ATH_DBG_WMI = BIT(14),
+ ATH_DBG_BSTUCK = BIT(15),
+ ATH_DBG_DFS = BIT(16),
ATH_DBG_ANY = 0xffffffff
};
--
1.7.4.1
Luis R. Rodriguez
2011-10-03 18:15:46 UTC
Permalink
---
 drivers/net/wireless/ath/ath.h |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 6d56532..34d4da1 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -211,6 +211,7 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
 *
 * The debug level is used to control the amount and type of debugging output
@@ -220,22 +221,23 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
 * entry.
 */
 enum ATH_DEBUG {
-       ATH_DBG_RESET           = 0x00000001,
-       ATH_DBG_QUEUE           = 0x00000002,
-       ATH_DBG_EEPROM          = 0x00000004,
-       ATH_DBG_CALIBRATE       = 0x00000008,
-       ATH_DBG_INTERRUPT       = 0x00000010,
-       ATH_DBG_REGULATORY      = 0x00000020,
-       ATH_DBG_ANI             = 0x00000040,
-       ATH_DBG_XMIT            = 0x00000080,
-       ATH_DBG_BEACON          = 0x00000100,
-       ATH_DBG_CONFIG          = 0x00000200,
-       ATH_DBG_FATAL           = 0x00000400,
-       ATH_DBG_PS              = 0x00000800,
-       ATH_DBG_HWTIMER         = 0x00001000,
-       ATH_DBG_BTCOEX          = 0x00002000,
-       ATH_DBG_WMI             = 0x00004000,
-       ATH_DBG_BSTUCK          = 0x00008000,
+       ATH_DBG_RESET           = BIT(0),
+       ATH_DBG_QUEUE           = BIT(1),
+       ATH_DBG_EEPROM          = BIT(2),
+       ATH_DBG_CALIBRATE       = BIT(3),
+       ATH_DBG_INTERRUPT       = BIT(4),
+       ATH_DBG_REGULATORY      = BIT(5),
+       ATH_DBG_ANI             = BIT(6),
+       ATH_DBG_XMIT            = BIT(7),
+       ATH_DBG_BEACON          = BIT(8),
+       ATH_DBG_CONFIG          = BIT(9),
+       ATH_DBG_FATAL           = BIT(10),
+       ATH_DBG_PS              = BIT(11),
+       ATH_DBG_HWTIMER         = BIT(12),
+       ATH_DBG_BTCOEX          = BIT(13),
+       ATH_DBG_WMI             = BIT(14),
+       ATH_DBG_BSTUCK          = BIT(15),
+       ATH_DBG_DFS             = BIT(16),
       ATH_DBG_ANY             = 0xffffffff
Split this into two patches, one to convert stuff to
Zefir Kurtisi
2011-10-04 08:31:39 UTC
Permalink
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath.h | 34 ++++++++++++++++++----------------
1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 6d56532..34d4da1 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -211,6 +211,7 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
*
* The debug level is used to control the amount and type of debugging output
@@ -220,22 +221,23 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
* entry.
*/
enum ATH_DEBUG {
- ATH_DBG_RESET = 0x00000001,
- ATH_DBG_QUEUE = 0x00000002,
- ATH_DBG_EEPROM = 0x00000004,
- ATH_DBG_CALIBRATE = 0x00000008,
- ATH_DBG_INTERRUPT = 0x00000010,
- ATH_DBG_REGULATORY = 0x00000020,
- ATH_DBG_ANI = 0x00000040,
- ATH_DBG_XMIT = 0x00000080,
- ATH_DBG_BEACON = 0x00000100,
- ATH_DBG_CONFIG = 0x00000200,
- ATH_DBG_FATAL = 0x00000400,
- ATH_DBG_PS = 0x00000800,
- ATH_DBG_HWTIMER = 0x00001000,
- ATH_DBG_BTCOEX = 0x00002000,
- ATH_DBG_WMI = 0x00004000,
- ATH_DBG_BSTUCK = 0x00008000,
+ ATH_DBG_RESET = BIT(0),
+ ATH_DBG_QUEUE = BIT(1),
+ ATH_DBG_EEPROM = BIT(2),
+ ATH_DBG_CALIBRATE = BIT(3),
+ ATH_DBG_INTERRUPT = BIT(4),
+ ATH_DBG_REGULATORY = BIT(5),
+ ATH_DBG_ANI = BIT(6),
+ ATH_DBG_XMIT = BIT(7),
+ ATH_DBG_BEACON = BIT(8),
+ ATH_DBG_CONFIG = BIT(9),
+ ATH_DBG_FATAL = BIT(10),
+ ATH_DBG_PS = BIT(11),
+ ATH_DBG_HWTIMER = BIT(12),
+ ATH_DBG_BTCOEX = BIT(13),
+ ATH_DBG_WMI = BIT(14),
+ ATH_DBG_BSTUCK = BIT(15),
+ ATH_DBG_DFS = BIT(16),
ATH_DBG_ANY = 0xffffffff
Split this into two patches, one to convert stuff to BIT(foo) and the
other one to add ATH_DBG_DFS
In fact, the BIT values will be reverted, since it is easier to have the hex values when setting debug mask than the bit position (for me at least).
Luis
Thanks,
Zefir
Mohammed Shafi
2011-10-04 09:40:42 UTC
Permalink
Post by Zefir Kurtisi
---
 drivers/net/wireless/ath/ath.h |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 6d56532..34d4da1 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -211,6 +211,7 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
 *
 * The debug level is used to control the amount and type of debugging output
@@ -220,22 +221,23 @@ ath_printk(const char *level, struct ath_common *common, const char *fmt, ...);
 * entry.
 */
 enum ATH_DEBUG {
-       ATH_DBG_RESET           = 0x00000001,
-       ATH_DBG_QUEUE           = 0x00000002,
-       ATH_DBG_EEPROM          = 0x00000004,
-       ATH_DBG_CALIBRATE       = 0x00000008,
-       ATH_DBG_INTERRUPT       = 0x00000010,
-       ATH_DBG_REGULATORY      = 0x00000020,
-       ATH_DBG_ANI             = 0x00000040,
-       ATH_DBG_XMIT            = 0x00000080,
-       ATH_DBG_BEACON          = 0x00000100,
-       ATH_DBG_CONFIG          = 0x00000200,
-       ATH_DBG_FATAL           = 0x00000400,
-       ATH_DBG_PS              = 0x00000800,
-       ATH_DBG_HWTIMER         = 0x00001000,
-       ATH_DBG_BTCOEX          = 0x00002000,
-       ATH_DBG_WMI             = 0x00004000,
-       ATH_DBG_BSTUCK          = 0x00008000,
+       ATH_DBG_RESET           = BIT(0),
+       ATH_DBG_QUEUE           = BIT(1),
+       ATH_DBG_EEPROM          = BIT(2),
+       ATH_DBG_CALIBRATE       = BIT(3),
+       ATH_DBG_INTERRUPT       = BIT(4),
+       ATH_DBG_REGULATORY      = BIT(5),
+       ATH_DBG_ANI             = BIT(6),
+       ATH_DBG_XMIT            = BIT(7),
+       ATH_DBG_BEACON          = BIT(8),
+       ATH_DBG_CONFIG          = BIT(9),
+       ATH_DBG_FATAL           = BIT(10),
+       ATH_DBG_PS              = BIT(11),
+       ATH_DBG_HWTIMER         = BIT(12),
+       ATH_DBG_BTCOEX          = BIT(13),
+       ATH_DBG_WMI             = BIT(14),
+       ATH_DBG_BSTUCK          = BIT(15),
+       ATH_DBG_DFS             = BIT(16),
       ATH_DBG_ANY             = 0xffffffff
Split this into two patches, one to convert stuff to BIT(foo) and the
other one to add ATH_DBG_DFS
In fact, the BIT values will be reverted, since it is easier to have the hex values when setting debug mask than the bit position (for me at least).
yes, i agree having hex values is much better. especially when we
have a combination of 2 or 3 debug masks for debugging issues.
thanks!
Post by Zefir Kurtisi
  Luis
Thanks,
Zefir
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
shafi
Zefir Kurtisi
2011-10-03 10:29:17 UTC
Permalink
Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
+ /**
+ * enable radar pulse detection
+ *
+ * TODO: do this only for DFS channels
+ */
+ ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+ ath9k_hw_setrxfilter(ah,
+ ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+ ath_dbg(common, ATH_DBG_DFS,
+ "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif

if (!ath_complete_reset(sc, true))
r = -EIO;
--
1.7.4.1
Luis R. Rodriguez
2011-10-03 18:27:39 UTC
Permalink
---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
                       "Unable to reset channel, reset status %d\n", r);
               goto out;
       }
+#ifdef CONFIG_ATH9K_DFS
+       /**
+        * enable radar pulse detection
+        *
+        * TODO: do this only for DFS channels
+        */
+       ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+       ath9k_hw_setrxfilter(ah,
+                       ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+       ath_dbg(common, ATH_DBG_DFS,
+               "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.

Luis
Luis R. Rodriguez
2011-10-03 19:31:12 UTC
Permalink
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
                       "Unable to reset channel, reset status %d\n", r);
               goto out;
       }
+#ifdef CONFIG_ATH9K_DFS
+       /**
+        * enable radar pulse detection
+        *
+        * TODO: do this only for DFS channels
+        */
+       ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+       ath9k_hw_setrxfilter(ah,
+                       ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+       ath_dbg(common, ATH_DBG_DFS,
+               "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled. You
may also want to simply disable DFS if you do not want to deal with
the regulatory test implications of having it enabled.

Luis
Christian Lamparter
2011-10-04 13:38:12 UTC
Permalink
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.

Regards,
Chr
Zefir Kurtisi
2011-10-04 14:17:35 UTC
Permalink
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
Yes on both. Work on the management part of the DFS module has just been started by TI guys. When this is in, hostapd will be able to query the driver's DFS detection capabilities and leave DFS channels disabled for those devices with no (or insufficient) support (like it is generally done today for DFS channels).

The proper way for a driver's OFF switch would then be to just announce missing DFS capabilities.
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
There might be a demand for conditional compiling in addition to DFS capabilities announcements to reduce memory footprint, since (especially) pattern matching algorithms will increase it significantly.
Post by Christian Lamparter
Regards,
Chr
Zefir
Adrian Chadd
2011-10-04 14:34:58 UTC
Permalink
.. erm, how complicated is the pattern matching code when it's compiled?

The port of the fusion radar detection module didn't end up being all that big.



Adrian
Luis R. Rodriguez
2011-10-05 22:31:19 UTC
Permalink
Post by Adrian Chadd
.. erm, how complicated is the pattern matching code when it's compiled?
The port of the fusion radar detection module didn't end up being all that big.
Adrian, please keep terms like "fusion codebase" out of public lists.

Luis
Peter Stuge
2011-10-05 22:53:25 UTC
Permalink
Post by Luis R. Rodriguez
Post by Adrian Chadd
The port of the fusion radar detection module didn't end up being all that big.
Adrian, please keep terms like "fusion codebase" out of public
lists.
I think that's silly. But you already knew that.


//Peter
Luis R. Rodriguez
2011-10-05 23:02:55 UTC
Permalink
Post by Peter Stuge
Post by Luis R. Rodriguez
Post by Adrian Chadd
The port of the fusion radar detection module didn't end up being all that big.
Adrian, please keep terms like "fusion codebase" out of public lists.
I think that's silly. But you already knew that.
Silly, but it confuses people who have not signed proper agreements to
contribute upstream with access to alternative codebases. We have
proper agreements in place with key active developers to do this.
Making broad statements like the above sometimes gives hints to some
developers under other agreements they can simply contribute upstream
with the code they received under other agreements.

Luis
Christian Lamparter
2011-10-04 14:42:54 UTC
Permalink
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
Yes on both. Work on the management part of the DFS module has just
been started by TI guys. When this is in, hostapd will be able to
query the driver's DFS detection capabilities and leave DFS channels
disabled for those devices with no (or insufficient) support
(like it is generally done today for DFS channels).
The proper way for a driver's OFF switch would then be to just
announce missing DFS capabilities.
Actually, I think we already have a flag for such a
purpose:
* @IEEE80211_HW_SPECTRUM_MGMT:
* Hardware supports spectrum management defined in 802.11h
AFAIK 802.11h[now integrated in 802.11-2007] included DFS, right?
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
There might be a demand for conditional compiling in addition to
DFS capabilities announcements to reduce memory footprint, since
(especially) pattern matching algorithms will increase it significantly.
I don't think memory footprint is such a big problem. After all ath9k
has around 170 kb [please correct me if I'm wrong here] of initvals
for all supported solutions since AR5008.

Regards,
Chr
Adrian Chadd
2011-10-04 14:50:42 UTC
Permalink
Also whilst I'm at it, "SPECTRUM_MANAGEMENT" is a very broad flag to set.

For example: you may not want to do DFS on the AR5416 NICs because (as
documented in the open hal and earlier ath9k bits) there isn't support
for radar pulses on the ext channel. So even if you had a successful
DFS algorithm for this NIC, you'd have to somehow tell the DFS
machinery that HT40+DFS channels aren't supported but HT20+DFS
channels are.

But then, the AR5416 supports per-packet TPC, so you could use it in
STA mode perfectly fine and it'd support that part of spectrum
management. Since you get per-frame RSSI of RX'ed frames, you can
support the spectrum power histogram IE.

And since it supports quiet time stuff, you can use it in STA and
hostap mode for supporting the quiet time IE.

(Yes, I'm looking at how to make all of this work in FreeBSD net80211,
as some patches have been supplied to start fleshing out these
functions. :)

I'm not saying this needs to be solved now, but I think it's worth
thinking about how to encapsulate exactly what it is that NICs
support, rather than simply saying "yup, 11h is here, all good mate."


Adrian
Christian Lamparter
2011-10-04 15:26:40 UTC
Permalink
Post by Adrian Chadd
Also whilst I'm at it, "SPECTRUM_MANAGEMENT" is a very broad flag to set.
Hm, 802.11-2007 5.4.4 "Spectrum management services" explicitly
mentions that both services TPC and DFS are required in some
regulatory domains for operation in the 5 GHz band.
Post by Adrian Chadd
For example: you may not want to do DFS on the AR5416 NICs because
(as documented in the open hal and earlier ath9k bits) there isn't
support for radar pulses on the ext channel. So even if you had a
successful DFS algorithm for this NIC, you'd have to somehow tell
the DFS machinery that HT40+DFS channels aren't supported but
HT20+DFS channels are.
well, that's a pity. We do have seperated HT feature sets for each
band but they are not seperated by operation mode...

[ Just for fun: Do you know how the AR5418 handled radars in
the proprietary turbo mode in the 5 GHz band? And how does
Atheros own driver/stack/etc. deals with this limitation?
After all Unex sold/sells the DNBA-81 on their product page
as: <http://www.unex.com.tw/product/dnba-81>

"DNBA-81 is a 802.11n 2x3 2-stream a/b/g dual band wifi
Cardbus designed specifically for laptops and *access points*/
home gateways/consumer electronics/multimedia entertainment
devices/peripherals with standard Cardbus slot." ]

Regards,
Chr
Adrian Chadd
2011-10-04 15:57:32 UTC
Permalink
Post by Christian Lamparter
Post by Adrian Chadd
Also whilst I'm at it, "SPECTRUM_MANAGEMENT" is a very broad flag to set.
Hm, 802.11-2007 5.4.4 "Spectrum management services" explicitly
mentions that both services TPC and DFS are required in some
regulatory domains for operation in the 5 GHz band.
Right, but just for hostap mode, correct? I'm also thinking about
non-hostap mode support.
Post by Christian Lamparter
[ Just for fun: Do you know how the AR5418 handled radars in
the proprietary turbo mode in the 5 GHz band? And how does
Atheros own driver/stack/etc. deals with this limitation?
After all Unex sold/sells the DNBA-81 on their product page
as: <http://www.unex.com.tw/product/dnba-81>
"DNBA-81 is a 802.11n 2x3 2-stream a/b/g dual band wifi
Cardbus designed specifically for laptops and *access points*/
home gateways/consumer electronics/multimedia entertainment
devices/peripherals with standard Cardbus slot." ]
I haven't sat down and looked at exactly what the AR5416/AR5418
support is like save what I've seen when digging into the meaning
behind the extended EEPROM bits. In any case, the easiest solution is
to just disable HT40 for those (which, incidentally, is exactly what
FreeBSD's ath and net80211 regulatory code did until recently.)

And as I said, that isn't needed for station mode operation, so you
can just use HT/40 channels. :)

Besides, you realise that vendors say the darnest things at times? :)
For example, one well-known vendor lists their AR9160 NICs as DFS
ready (and thus people who buy them think they're DFS ready), but if
you buy some of their MIPS SOC hardware, they only support
OpenWRT+ath9k on it. I'm sure more than a few people fell into that
trap. :)



Adrian
Christian Lamparter
2011-10-04 16:42:04 UTC
Permalink
Post by Adrian Chadd
Post by Christian Lamparter
Post by Adrian Chadd
Also whilst I'm at it, "SPECTRUM_MANAGEMENT" is a very broad flag to set.
Hm, 802.11-2007 5.4.4 "Spectrum management services" explicitly
mentions that both services TPC and DFS are required in some
regulatory domains for operation in the 5 GHz band.
Right, but just for hostap mode, correct? I'm also thinking about
non-hostap mode support.
I don't think it's just hostap mode. More like hostap, IBSS and MESH.
In fact, with P2P/Wifi-Direct, the station mode might becoming a
thing of the past, right?
Post by Adrian Chadd
Post by Christian Lamparter
[ Just for fun: Do you know how the AR5418 handled radars in
the proprietary turbo mode in the 5 GHz band? And how does
Atheros own driver/stack/etc. deals with this limitation?
After all Unex sold/sells the DNBA-81 on their product page
as: <http://www.unex.com.tw/product/dnba-81>
"DNBA-81 is a 802.11n 2x3 2-stream a/b/g dual band wifi
Cardbus designed specifically for laptops and *access points*/
home gateways/consumer electronics/multimedia entertainment
devices/peripherals with standard Cardbus slot." ]
Besides, you realise that vendors say the darnest things at times? :)
For example, one well-known vendor lists their AR9160 NICs as DFS
ready (and thus people who buy them think they're DFS ready), but if
you buy some of their MIPS SOC hardware, they only support
OpenWRT+ath9k on it. I'm sure more than a few people fell into that
trap. :)
Well, ok. I found a better example:
http://www.wikidevi.com/wiki/Apple_AirPort_Extreme_Base_Station_A1143_%28MA073LL/A%29

You see, they put an AR5418(or 6?) with an AR5133 into the first draft-n
Apple Airport Base Station. If this much money was involved, do you
think that an American Company would "lie" to another American Company?

Regards,
Chr
Adrian Chadd
2011-10-04 17:03:10 UTC
Permalink
Nope, what I'm saying is that someone's likely just copy/paste'd the
"we support x and y!" from some marketing material and then suggested
people use linux/openwrt/ath9k without really looking at it.

There were plenty of people advertising the AR9160 as a "3x3 MIMO"
when it's a 2x2 stream device with 3 antennas. I know this confused me
when I started tinkering with wifi stuff.

Anyway, we're getting off-track here.


Adrian
Christian Lamparter
2011-10-04 17:49:35 UTC
Permalink
Post by Adrian Chadd
Nope, what I'm saying is that someone's likely just copy/paste'd the
"we support x and y!" from some marketing material and then suggested
people use linux/openwrt/ath9k without really looking at it.
I'm still curious how they got the Apple access point through the FCC
testing back then. Well, I guess we'll never know for sure.
Post by Adrian Chadd
There were plenty of people advertising the AR9160 as a "3x3 MIMO"
when it's a 2x2 stream device with 3 antennas. I know this confused me
when I started tinkering with wifi stuff.
AFAIK the 3x3 really just stands for the amount of tx X rx chains [and
not antennas]. So if the hardware has 3 tx and rx chains it can be sold
as a "3x3" device even if does not support 3 streams.
Post by Adrian Chadd
Anyway, we're getting off-track here.
we can stop anytime.
Luis R. Rodriguez
2011-10-05 22:37:57 UTC
Permalink
Post by Adrian Chadd
Also whilst I'm at it, "SPECTRUM_MANAGEMENT" is a very broad flag to set.
For example: you may not want to do DFS on the AR5416 NICs because (as
documented in the open hal and earlier ath9k bits) there isn't support
for radar pulses on the ext channel. So even if you had a successful
DFS algorithm for this NIC, you'd have to somehow tell the DFS
machinery that HT40+DFS channels aren't supported but HT20+DFS
channels are.
Good point. I simply rather start out with the best possible DFS
support on Linux and go with the best hardware we have instead of
dealing with old hardware. Think about the support issues that can
come up with supporting the above. I rather simply not deal with it as
I also do not care about Turbo crap. Let legacy crap die.
Post by Adrian Chadd
But then, the AR5416 supports per-packet TPC, so you could use it in
STA mode perfectly fine and it'd support that part of spectrum
management. Since you get per-frame RSSI of RX'ed frames, you can
support the spectrum power histogram IE.
TPC is not implemented even in a lot of proprietary code bases,
although TPC is part of 802.11h its requirements are me by statically
reducing the maximum EIRP by 3 dB in frequency bands requiring TPC in
consideration for interference with satellites. In my latest
evaluation of TPC the only thing we want to do is simply enable the
option to explicitly state the max delta on power in consideration for
TPC in such a way that *if* TPC is implemented we can reduce the
reduction. But given that this is hardware specific and vendor
specific and not many people implement it right now I frankly don't
care too much about it. DFS is a bigger aspect.

Luis
Zefir Kurtisi
2011-10-04 16:26:14 UTC
Permalink
Post by Christian Lamparter
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
Yes on both. Work on the management part of the DFS module has just
been started by TI guys. When this is in, hostapd will be able to
query the driver's DFS detection capabilities and leave DFS channels
disabled for those devices with no (or insufficient) support
(like it is generally done today for DFS channels).
The proper way for a driver's OFF switch would then be to just
announce missing DFS capabilities.
Actually, I think we already have a flag for such a
* Hardware supports spectrum management defined in 802.11h
AFAIK 802.11h[now integrated in 802.11-2007] included DFS, right?
Yes, 802.11h includes DFS and TPC.

But this flag is already used (i.e. is set by ath9k) without having DFS support so far. Either it is having some different interpretation, or it is just set wrong. This was irrelevant so far, since there is no DFS support in mac80211 yet and might require to clarify the specs for this flag.
Post by Christian Lamparter
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
There might be a demand for conditional compiling in addition to
DFS capabilities announcements to reduce memory footprint, since
(especially) pattern matching algorithms will increase it significantly.
I don't think memory footprint is such a big problem. After all ath9k
has around 170 kb [please correct me if I'm wrong here] of initvals
for all supported solutions since AR5008.
It might be even more. But this does not imply that it is not worth to save some bytes, IMHO.
Post by Christian Lamparter
Regards,
Chr
Zefir
Luis R. Rodriguez
2011-10-05 22:30:18 UTC
Permalink
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
                       "Unable to reset channel, reset status %d\n", r);
               goto out;
       }
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
Yes on both. Work on the management part of the DFS module has just been started by TI guys. When this is in, hostapd will be able to query the driver's DFS detection capabilities and leave DFS channels disabled for those devices with no (or insufficient) support (like it is generally done today for DFS channels).
The proper way for a driver's OFF switch would then be to just announce missing DFS capabilities.
And this is what I meant by leaving a kernel build option available
for each driver to enable/disable DFS. If a vendor does not want to
deal with the question of enabling DFS they can disable it on the
driver front.
Post by Zefir Kurtisi
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
There might be a demand for conditional compiling in addition to DFS capabilities announcements to reduce memory footprint, since (especially) pattern matching algorithms will increase it significantly.
I doubt space considerations will be that much given that we don't
even have build options to disable hardware families, at least nbd had
determine already that separating families at build time wouldn't save
us much. Given that -- I suspect using build size as an argument for
introducing a flag here doesn't carry much weight. The argument I'm
using is to simply enable integrators to decide whether or not to have
to deal with testing such features.

Luis
Luis R. Rodriguez
2011-10-05 22:27:03 UTC
Permalink
On Tue, Oct 4, 2011 at 6:38 AM, Christian Lamparter
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
                       "Unable to reset channel, reset status %d\n", r);
               goto out;
       }
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
You do have a good point, but I disagree that you do not need to test
/ regress test hardware / driver code for DFS. This is what I'm
talking about. But yes, userspace also submits itself to the same
criteria.
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
No, DFS is set for certain channels on wireless-regdb/CRDA, I just
posted DFS master region support for wireless-regdb and CRDA. Apart
from this we then need driver support. To get DFS you need all of
these + hostapd part. Each one has its own set of components and does
deserve its own set of tests and review.

Luis
Christian Lamparter
2011-10-06 16:49:48 UTC
Permalink
Post by Luis R. Rodriguez
On Tue, Oct 4, 2011 at 6:38 AM, Christian Lamparter
Post by Christian Lamparter
Post by Luis R. Rodriguez
On Mon, Oct 3, 2011 at 12:24 PM, Christian Lamparter
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
[...]
+#ifdef CONFIG_ATH9K_DFS
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".
Because DFS requires to be properly tested before being enabled.
Testing if a driver detects a pulse is "trivial" compared to the
stuff mac80211/cfg80211 and hostapd will have to do to make a
channel-change as smooth as possible. I think if there's a DFS
"OFF" switch, it should be in hostapd and I hope more people
agree on this one.
You do have a good point, but I disagree that you do not need to test
/ regress test hardware / driver code for DFS.
Actually, you are sort of contradicting yourself here.

Take a look at your "wireless: add DFS master support" patch
series. I don't see any IFDEFs to select between the old
and the new "way" even though you know full well that there's
some black magic going on.

http://permalink.gmane.org/gmane.linux.kernel.wireless.general/78455
Quote:
"Here's a puzzle though... If we change this series to use the other
pad byte that was available, the first pad byte, instead of the last
one, we loose backward compatibility support and I cannot figure out
why."

[Note: This is just an recent enough example. I do think I could come
up with a better one, e.g.: why didn't athXk have a compile-time
switch to disable ANI when it was introduced because it can(and has?)
caused some regressions as well?]

so, why do you want a useless compile-time for *this option* *now*?
Is this something about politics/laws I don't know about [I'm just
curious, because I don't really buy "testing" here, since Zefir
obviously has a working prototype and Atheros has a working and
certificated codebase as well which he can access and base his
work on. So I don't think its that unstable and needs added
ugliness.]
Post by Luis R. Rodriguez
This is what I'm talking about. But yes, userspace also submits
itself to the same criteria.
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
No, DFS is set for certain channels on wireless-regdb/CRDA, I just
posted DFS master region support for wireless-regdb and CRDA. Apart
from this we then need driver support. To get DFS you need all of
these + hostapd part. Each one has its own set of components and does
deserve its own set of tests and review.
This "deserve its own set of tests and review". Does it translate in:
"ath9k [every driver], mac80211, cfg80211 and hostapd need extra
DFS IFDEFS?". In fact, ifdefs make it harder to do reviews, because
sometimes you just forget the IFDEF/ELSIF/ELSE context of the code.
And regression testing can be done by "git bisect". In fact, isn't
this what git bisect is for?

Regards,
Chr
Luis R. Rodriguez
2011-10-06 18:36:49 UTC
Permalink
On Thu, Oct 6, 2011 at 9:49 AM, Christian Lamparter
Post by Christian Lamparter
Post by Luis R. Rodriguez
You do have a good point, but I disagree that you do not need to test
/ regress test hardware / driver code for DFS.
Actually, you are sort of contradicting yourself here.
Take a look at your "wireless: add DFS master support" patch
series. I don't see any IFDEFs to select between the old
and the new "way" even though you know full well that there's
some black magic going on.
Well its true, but the regdb and CRDA stuff is can have the DFS master
region support, its just the mapping, not a technical implementation.
IMHO DFS support should be a kconfig on both the 802.11 stack and
driver part, and the driver part depend on the 802.11 stack option.
Post by Christian Lamparter
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/78455
"Here's a puzzle though... If we change this series to use the other
pad byte that was available, the first pad byte, instead of the last
one, we loose backward compatibility support and I cannot figure out
why."
That's an implementation weirdness with python pack on generating the
binary output, not a DFS issue per se.
Post by Christian Lamparter
[Note: This is just an recent enough example. I do think I could come
up with a better one, e.g.: why didn't athXk have a compile-time
switch to disable ANI when it was introduced because it can(and has?)
caused some regressions as well?]
No ANI is *required*, without it the cards are useless. ANI is also
properly tested and validated by our systems team. Have you tried
disabling ANI? When we introduced a revamp of the new ANI though we
did enable users to use the new ANI for older generation cards, the
module parameter is still there.

DFS is a different beast. Testing DFS cannot be compared to testing
ANI, DFS has a slew of different tests you need to run against, and
then *if* you do want to sell cards in certain geographies with
support for DFS channels you need to get proper regulatory
certification for an intended radiator that supports DFS properly. If
you get this certification technically you cannot even expose a knob
to users to disable DFS when operating on a DFS channel.
Post by Christian Lamparter
so, why do you want a useless compile-time for *this option* *now*?
What I want to do is enable an option which lets distributors disable
DFS if they don't want to even deal with the question of whether or
not a card had DFS support enabled through driver support.
Post by Christian Lamparter
Is this something about politics/laws I don't know about [I'm just
curious, because I don't really buy "testing" here, since Zefir
obviously has a working prototype
No, we haven't passed certified testing with this code yet.
Post by Christian Lamparter
and Atheros has a working and certificated codebase as well
Exactly, and that is the code we are enabling Neratec with to be able
to upstream into the kernel for, but the code being referenced uses a
different 802.11 stack, different base driver, etc.
Post by Christian Lamparter
which he can access and base his work on. So I don't think its that unstable and needs added
ugliness.]
Its the same with 802.11s, its new code and people may want to disable
this crap to not deal with it in code path or even consider the
support for it.
Post by Christian Lamparter
Post by Luis R. Rodriguez
This is what I'm talking about. But yes, userspace also submits
itself to the same criteria.
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
No, DFS is set for certain channels on wireless-regdb/CRDA, I just
posted DFS master region support for wireless-regdb and CRDA. Apart
from this we then need driver support. To get DFS you need all of
these + hostapd part. Each one has its own set of components and does
deserve its own set of tests and review.
"ath9k [every driver], mac80211, cfg80211 and hostapd need extra
DFS IFDEFS?".
I still have yet to see patches for cfg80211 / mac80211 for DFS. What
I'm saying is we have an kconfig option on the 802.11 stack to allow
us to disable DFS support, and the driver respective component depend
on it.
Post by Christian Lamparter
In fact, ifdefs make it harder to do reviews, because
sometimes you just forget the IFDEF/ELSIF/ELSE context of the code.
I hate ifdefs, and if you read my e-mail carefully what I was
suggesting was to do this properly by building all the code if the
kconfig option is enabled therefore eliminating all ifdef junk from
the code and only leaving it for header files.
Post by Christian Lamparter
And regression testing can be done by "git bisect". In fact, isn't
this what git bisect is for?
There is a difference between regression testing and finding the
culprit of an issue. git bisect is used to find the culprit of an
issue. However if you want to ensure code does not regress you need to
ensure to run a suite of tests on code after a delta is applied. Only
if you find an issue do you then use git bisect.

I want proper test infrastructure set up before I even consider
enabling any DFS code upstream for ath9k.

Luis
Luis R. Rodriguez
2011-10-06 18:41:20 UTC
Permalink
On Thu, Oct 6, 2011 at 11:36 AM, Luis R. Rodriguez
Post by Luis R. Rodriguez
On Thu, Oct 6, 2011 at 9:49 AM, Christian Lamparter
Post by Christian Lamparter
Is this something about politics/laws I don't know about [I'm just
curious, because I don't really buy "testing" here, since Zefir
obviously has a working prototype
No, we haven't passed certified testing with this code yet.
And to be clear, no there is no political issues here or concerns. The
law however is strict about DFS, extremely strict and I want to make
sure upstream code respect it well and we get the best DFS test
infrastructure out there to test this to ensure we get to the point to
always have DFS properly working upstream.

Luis
Zefir Kurtisi
2011-10-06 20:32:34 UTC
Permalink
Post by Luis R. Rodriguez
On Thu, Oct 6, 2011 at 9:49 AM, Christian Lamparter
Post by Christian Lamparter
Post by Luis R. Rodriguez
You do have a good point, but I disagree that you do not need to test
/ regress test hardware / driver code for DFS.
Actually, you are sort of contradicting yourself here.
Take a look at your "wireless: add DFS master support" patch
series. I don't see any IFDEFs to select between the old
and the new "way" even though you know full well that there's
some black magic going on.
Well its true, but the regdb and CRDA stuff is can have the DFS master
region support, its just the mapping, not a technical implementation.
IMHO DFS support should be a kconfig on both the 802.11 stack and
driver part, and the driver part depend on the 802.11 stack option.
The concept for the management part that will be located in mac80211 and
hostapd is still WIP at TI, so we can just speculate about its
implementation for now.

If one consider object size irrelevant (working in the embedded world, I
tend to not fully agree), it is enough to disable the DFS capability in
the driver, since DFS requires a full support chain in all three layers
to enable operation in DFS bands.
Post by Luis R. Rodriguez
Post by Christian Lamparter
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/78455
"Here's a puzzle though... If we change this series to use the other
pad byte that was available, the first pad byte, instead of the last
one, we loose backward compatibility support and I cannot figure out
why."
That's an implementation weirdness with python pack on generating the
binary output, not a DFS issue per se.
Post by Christian Lamparter
[Note: This is just an recent enough example. I do think I could come
up with a better one, e.g.: why didn't athXk have a compile-time
switch to disable ANI when it was introduced because it can(and has?)
caused some regressions as well?]
No ANI is *required*, without it the cards are useless. ANI is also
properly tested and validated by our systems team. Have you tried
disabling ANI? When we introduced a revamp of the new ANI though we
did enable users to use the new ANI for older generation cards, the
module parameter is still there.
DFS is a different beast. Testing DFS cannot be compared to testing
ANI, DFS has a slew of different tests you need to run against, and
then *if* you do want to sell cards in certain geographies with
support for DFS channels you need to get proper regulatory
certification for an intended radiator that supports DFS properly. If
you get this certification technically you cannot even expose a knob
to users to disable DFS when operating on a DFS channel.
Post by Christian Lamparter
so, why do you want a useless compile-time for *this option* *now*?
What I want to do is enable an option which lets distributors disable
DFS if they don't want to even deal with the question of whether or
not a card had DFS support enabled through driver support.
Post by Christian Lamparter
Is this something about politics/laws I don't know about [I'm just
curious, because I don't really buy "testing" here, since Zefir
obviously has a working prototype
No, we haven't passed certified testing with this code yet.
Post by Christian Lamparter
and Atheros has a working and certificated codebase as well
Exactly, and that is the code we are enabling Neratec with to be able
to upstream into the kernel for, but the code being referenced uses a
different 802.11 stack, different base driver, etc.
I see, I should have officially announce the wiki page where we want to
collect our joint DFS development information [1] to include all those
valuable thoughts.

For clarification: the existing referenced implementation is not
portable to ath9k/mac80211. It follows a monolithic approach where the
DFS detector takes over full processing control after pattern match and
handles all required management functionality from within. Refactoring
those processing path to distribute it between driver, mac80211 and
hostapd is more complicated than a full redesign. FYI, Adrian is
following this approach for FreeBSD.

For the detector prototype: yes, I worked on some pattern matching
algorithms and have some working prototype. With the HW reliably
providing pulse events it's no rocket science to design a radar detector
that can pass certification (I tested it for ETSI), while it is to
design one that allows you to use DFS channels (i.e. low false detections).


[1] http://linuxwireless.org/en/developers/DFS/Development
Post by Luis R. Rodriguez
Post by Christian Lamparter
which he can access and base his work on. So I don't think its that unstable and needs added
ugliness.]
Its the same with 802.11s, its new code and people may want to disable
this crap to not deal with it in code path or even consider the
support for it.
Post by Christian Lamparter
Post by Luis R. Rodriguez
This is what I'm talking about. But yes, userspace also submits
itself to the same criteria.
Post by Christian Lamparter
Post by Luis R. Rodriguez
You may also want to simply disable DFS if you do not want to
deal with the regulatory test implications of having it enabled.
AFAIK you can't "simply" disable the DFS requirement: hostapd
(hw_features.c), [cfg80211] (checks if tx on secondary channel
is possible) and mac80211 (tx.c) all have checks. Indeed, the
easiest way is to modify crda's database. So there's no need
for an extra compile-time option.
No, DFS is set for certain channels on wireless-regdb/CRDA, I just
posted DFS master region support for wireless-regdb and CRDA. Apart
from this we then need driver support. To get DFS you need all of
these + hostapd part. Each one has its own set of components and does
deserve its own set of tests and review.
"ath9k [every driver], mac80211, cfg80211 and hostapd need extra
DFS IFDEFS?".
I still have yet to see patches for cfg80211 / mac80211 for DFS. What
I'm saying is we have an kconfig option on the 802.11 stack to allow
us to disable DFS support, and the driver respective component depend
on it.
As said above, disabling a driver's capability through a Kconfig option
should be enough (one ifdef per driver).
Post by Luis R. Rodriguez
Post by Christian Lamparter
In fact, ifdefs make it harder to do reviews, because
sometimes you just forget the IFDEF/ELSIF/ELSE context of the code.
I hate ifdefs, and if you read my e-mail carefully what I was
suggesting was to do this properly by building all the code if the
kconfig option is enabled therefore eliminating all ifdef junk from
the code and only leaving it for header files.
Post by Christian Lamparter
And regression testing can be done by "git bisect". In fact, isn't
this what git bisect is for?
There is a difference between regression testing and finding the
culprit of an issue. git bisect is used to find the culprit of an
issue. However if you want to ensure code does not regress you need to
ensure to run a suite of tests on code after a delta is applied. Only
if you find an issue do you then use git bisect.
I want proper test infrastructure set up before I even consider
enabling any DFS code upstream for ath9k.
Since regulatory compliance and open source by principle form a
gray-zone combination [2], testing for sure is essential to keep it more
white than black ;)

[2] http://linuxwireless.org/en/developers/Regulatory/statement#Principles
Post by Luis R. Rodriguez
Luis
Zefir
Luis R. Rodriguez
2011-10-06 20:41:27 UTC
Permalink
Post by Zefir Kurtisi
As said above, disabling a driver's capability through a Kconfig option
should be enough (one ifdef per driver).
OK cool.
Post by Zefir Kurtisi
Since regulatory compliance and open source by principle form a
gray-zone combination [2], testing for sure is essential to keep it more
white than black ;)
[2] http://linuxwireless.org/en/developers/Regulatory/statement#Principles
I actually do not think its grey now at all, we in fact IMHO have the
best regulatory framework out there, while still ensuring freedoms.

Luis
Zefir Kurtisi
2011-10-06 21:08:43 UTC
Permalink
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
As said above, disabling a driver's capability through a Kconfig option
should be enough (one ifdef per driver).
OK cool.
Post by Zefir Kurtisi
Since regulatory compliance and open source by principle form a
gray-zone combination [2], testing for sure is essential to keep it more
white than black ;)
[2] http://linuxwireless.org/en/developers/Regulatory/statement#Principles
I actually do not think its grey now at all, we in fact IMHO have the
best regulatory framework out there, while still ensuring freedoms.
Luis
Sorry, of course it is, I was not specific enough.

I was just wondering if principle 3 generally would prevent us from
adding a Kconfig option to enable DFS for ath9k as long as it is not
certified (which is the only way to ensure to have a 'known compliant
usage') plus depending on mac80211 and hostapd.


Zefir
Luis R. Rodriguez
2011-10-06 21:12:12 UTC
Permalink
Post by Zefir Kurtisi
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
As said above, disabling a driver's capability through a Kconfig option
should be enough (one ifdef per driver).
OK cool.
Post by Zefir Kurtisi
Since regulatory compliance and open source by principle form a
gray-zone combination [2], testing for sure is essential to keep it more
white than black ;)
[2] http://linuxwireless.org/en/developers/Regulatory/statement#Principles
I actually do not think its grey now at all, we in fact IMHO have the
best regulatory framework out there, while still ensuring freedoms.
   Luis
Sorry, of course it is, I was not specific enough.
I was just wondering if principle 3 generally would prevent us from
adding a Kconfig option to enable DFS for ath9k as long as it is not
certified (which is the only way to ensure to have a 'known compliant
usage') plus depending on mac80211 and hostapd.
Ah yeah great point. I punted internally to see if there was a EEPROM
bit we can read that may tell us if a card is DFS certified. If that
is not available I think what we can do is leave the option but set
the default to disabled and simply state that only platforms that have
passed DFS certification should have it enabled.

Luis
Adrian Chadd
2011-10-07 03:06:32 UTC
Permalink
Just FYI, that's what I'm likely to do for FreeBSD 10 (as I just don't
have time to try and make all the required regulatory changes before
the upcoming 9.0 release.)

Ie:

* DFS station mode (net80211) is going to be compiled and enabled by default;
* DFS master mode (net80211) is going to be compiled and enabled by default;
* The drivers which support radar detection in firmware (currently
only if_mwl) will set the DFS net80211 flag (ie, for master mode radar
detection);
* ath won't ship with the DFS enable flag (for master mode);
* I'll modify the regulatory database code to include per-band DFS
information (DFS domain, CAC/NOL timeout, interference timeout, etc)
and some device information (eg whether it supports HT20/HT40/etc DFS
detection);
* I'll then flip on the DFS channel enforcement in the net80211 code
so disables master mode on channels requiring DFS.

The radar detection code and channel interference code will live in
the ath driver but the DFS machinery (CAC, NOL, CSA, etc) is in
net80211. That way other NICs (eg if_mwl Marvell NICs) with DFS radar
detection support can leverage this.

I'll then likely ship two DFS modules - dfs_null (no DFS support, just
a placeholder for the API) and whatever code is ported from the
reference driver. Maybe I'll also include the code from Neratec if
it's dual-licenced. But I won't include radar patterns by default -
I'll include those on a documentation page which explains the how and
why of regulatory domain stuff.

Once FreeBSD ships DFS radar detection code, I'll make sure it isn't
compiled by default and even if enabled, it won't advertise DFS
channel support unless a valid radar pattern and radar parameter
configuration is loaded in. That way users won't inadvertently enable
it without being compliant.

Finally, I'm hoping to get all of this documented as much as possible
so the community can pick this stuff up and run with it. I was hoping
someone would throw me a 5ghz SDR (software defined radio) so I could
prototype up some open source radar pulse generation code, just to
lower the entry barrier to all of this.

HTH,


Adrian
Luis R. Rodriguez
2011-10-07 07:54:03 UTC
Permalink
Post by Adrian Chadd
Just FYI, that's what I'm likely to do for FreeBSD 10 (as I just don't
have time to try and make all the required regulatory changes before
the upcoming 9.0 release.)
* DFS station mode (net80211) is going to be compiled and enabled by default;
* DFS master mode (net80211) is going to be compiled and enabled by default;
* The drivers which support radar detection in firmware (currently
only if_mwl) will set the DFS net80211 flag (ie, for master mode radar
detection);
* ath won't ship with the DFS enable flag (for master mode);
* I'll modify the regulatory database code to include per-band DFS
information (DFS domain, CAC/NOL timeout, interference timeout, etc)
and some device information (eg whether it supports HT20/HT40/etc DFS
detection);
* I'll then flip on the DFS channel enforcement in the net80211 code
so disables master mode on channels requiring DFS.
The radar detection code and channel interference code will live in
the ath driver but the DFS machinery (CAC, NOL, CSA, etc) is in
net80211. That way other NICs (eg if_mwl Marvell NICs) with DFS radar
detection support can leverage this.
I'll then likely ship two DFS modules - dfs_null (no DFS support, just
a placeholder for the API) and whatever code is ported from the
reference driver. Maybe I'll also include the code from Neratec if
it's dual-licenced. But I won't include radar patterns by default -
I'll include those on a documentation page which explains the how and
why of regulatory domain stuff.
Once FreeBSD ships DFS radar detection code, I'll make sure it isn't
compiled by default and even if enabled, it won't advertise DFS
channel support unless a valid radar pattern and radar parameter
configuration is loaded in. That way users won't inadvertently enable
it without being compliant.
Neat, best of luck! FWIW, I've been moving along on the regulatory
simulator, feel free do send me patches in ways you want that to move
if that seems reasonable to you as place to share code between OSes,
that was at least my own goal, to help share as much test framework
and code to let us then cherry pick for our own OSes in whatever way
we want.
Post by Adrian Chadd
Finally, I'm hoping to get all of this documented as much as possible
so the community can pick this stuff up and run with it. I was hoping
someone would throw me a 5ghz SDR (software defined radio) so I could
prototype up some open source radar pulse generation code, just to
lower the entry barrier to all of this.
You can use the Winlab Orbit GNU Radio SDRs, I think you can make a
reservation for them.

Luis
Zefir Kurtisi
2011-10-07 08:48:43 UTC
Permalink
Post by Adrian Chadd
Just FYI, that's what I'm likely to do for FreeBSD 10 (as I just don't
have time to try and make all the required regulatory changes before
the upcoming 9.0 release.)
* DFS station mode (net80211) is going to be compiled and enabled by default;
* DFS master mode (net80211) is going to be compiled and enabled by default;
* The drivers which support radar detection in firmware (currently
only if_mwl) will set the DFS net80211 flag (ie, for master mode radar
detection);
* ath won't ship with the DFS enable flag (for master mode);
* I'll modify the regulatory database code to include per-band DFS
information (DFS domain, CAC/NOL timeout, interference timeout, etc)
and some device information (eg whether it supports HT20/HT40/etc DFS
detection);
* I'll then flip on the DFS channel enforcement in the net80211 code
so disables master mode on channels requiring DFS.
The radar detection code and channel interference code will live in
the ath driver but the DFS machinery (CAC, NOL, CSA, etc) is in
net80211. That way other NICs (eg if_mwl Marvell NICs) with DFS radar
detection support can leverage this.
Quite some work ahead. Good luck on that!

So you are confident to get existing code re-factored and split into multiple layers?
Post by Adrian Chadd
I'll then likely ship two DFS modules - dfs_null (no DFS support, just
a placeholder for the API) and whatever code is ported from the
reference driver. Maybe I'll also include the code from Neratec if
it's dual-licenced. But I won't include radar patterns by default -
I'll include those on a documentation page which explains the how and
why of regulatory domain stuff.
Initially we planned to have common pattern detectors in mac80211 to handle pulses reported by all drivers, but so far only TI is working on DFS -- with pattern matching done in firmware. So, no need yet for common detectors, we might end up going the other way around, i.e. taking your port and integrate in into ath9k -- if licensing allowed us to do.
Post by Adrian Chadd
Once FreeBSD ships DFS radar detection code, I'll make sure it isn't
compiled by default and even if enabled, it won't advertise DFS
channel support unless a valid radar pattern and radar parameter
configuration is loaded in. That way users won't inadvertently enable
it without being compliant.
Finally, I'm hoping to get all of this documented as much as possible
so the community can pick this stuff up and run with it. I was hoping
someone would throw me a 5ghz SDR (software defined radio) so I could
prototype up some open source radar pulse generation code, just to
lower the entry barrier to all of this.
Yeah, an AR9003 card used as cheap radar generator is on top of my wish list for DFS testing without our always otherwise occupied vector signal generator. But fur obvious reasons we wont get the required documentation for such unofficial hacks.
Post by Adrian Chadd
HTH,
Adrian
Thanks
Zefir
Adrian Chadd
2011-10-07 11:43:15 UTC
Permalink
Post by Zefir Kurtisi
Quite some work ahead. Good luck on that!
Thanks!
Post by Zefir Kurtisi
So you are confident to get existing code re-factored and split into multiple layers?
Thankfully, it's already done and in net80211. :)




Adrian

Christian Lamparter
2011-10-03 19:24:53 UTC
Permalink
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
+ /**
+ * enable radar pulse detection
+ *
+ * TODO: do this only for DFS channels
+ */
+ ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+ ath9k_hw_setrxfilter(ah,
+ ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+ ath_dbg(common, ATH_DBG_DFS,
+ "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
Why would anyone want to disable DFS driver support?
I would say: drop the ifdefs altogether since DFS
is and will be "required".

Regards,
Chr
Zefir Kurtisi
2011-10-04 10:11:44 UTC
Permalink
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/main.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
"Unable to reset channel, reset status %d\n", r);
goto out;
}
+#ifdef CONFIG_ATH9K_DFS
+ /**
+ * enable radar pulse detection
+ *
+ * TODO: do this only for DFS channels
+ */
+ ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+ ath9k_hw_setrxfilter(ah,
+ ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+ ath_dbg(common, ATH_DBG_DFS,
+ "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
This possibly won't work, since setting up DFS registers is part of HW layer and not done in the dfs module. If you want to have DFS conditionally compilable, you can not spare this #ifdefs.

But I see that this would work in patch 6/6 when calling ath9k_dfs_process_phyerr() on ATH9K_PHYERR_RADAR. Will be considered.
Post by Luis R. Rodriguez
Luis
Thanks,
Zefir
Luis R. Rodriguez
2011-10-05 22:23:19 UTC
Permalink
Post by Zefir Kurtisi
Post by Luis R. Rodriguez
---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e8aeb98..5defebe 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -344,6 +344,18 @@ static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan,
                       "Unable to reset channel, reset status %d\n", r);
               goto out;
       }
+#ifdef CONFIG_ATH9K_DFS
+       /**
+        * enable radar pulse detection
+        *
+        * TODO: do this only for DFS channels
+        */
+       ah->private_ops.set_radar_params(ah, &ah->radar_conf);
+       ath9k_hw_setrxfilter(ah,
+                       ath9k_hw_getrxfilter(ah) | ATH9K_RX_FILTER_PHYRADAR);
+       ath_dbg(common, ATH_DBG_DFS,
+               "DFS enabled for channel %d\n", hchan->chan->center_freq);
+#endif
Please spare the #ifdef and just call something within dfs.c, then
dfs.h would wrap it to nothing if DFS is disabled.
This possibly won't work, since setting up DFS registers is part of HW layer and not
done in the dfs module. If you want to have DFS conditionally compilable, you can not spare this #ifdefs.
Its not about sparing the ifdefs completely but instead to place them
strategically to remove #ifdef sprinkling all over C code. You can
leave ifdefs on header files, and for C files leaves this as a
conditional build time option.

Luis
Zefir Kurtisi
2011-10-03 10:29:16 UTC
Permalink
Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath9k/Kconfig | 7 +++++++
drivers/net/wireless/ath/ath9k/Makefile | 2 ++
2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index d9c08c6..adddcca 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -58,6 +58,13 @@ config ATH9K_RATE_CONTROL
Say Y, if you want to use the ath9k specific rate control
module instead of minstrel_ht.

+config ATH9K_DFS
+ bool "Atheros ath9k DFS support"
+ depends on ATH9K
+ default y
+ ---help---
+ Say Y, if you want to include DFS support in ath9k.
+
config ATH9K_HTC
tristate "Atheros HTC based wireless cards support"
depends on USB && MAC80211
diff --git a/drivers/net/wireless/ath/ath9k/Makefile b/drivers/net/wireless/ath/ath9k/Makefile
index 05a6fad..8b133c2 100644
--- a/drivers/net/wireless/ath/ath9k/Makefile
+++ b/drivers/net/wireless/ath/ath9k/Makefile
@@ -34,6 +34,8 @@ ath9k_hw-y:= \
ar9003_eeprom.o \
ar9003_paprd.o

+ath9k_hw-$(CONFIG_ATH9K_DFS) += dfs.o
+
obj-$(CONFIG_ATH9K_HW) += ath9k_hw.o

obj-$(CONFIG_ATH9K_COMMON) += ath9k_common.o
--
1.7.4.1
Luis R. Rodriguez
2011-10-03 18:26:04 UTC
Permalink
---
 drivers/net/wireless/ath/ath9k/Kconfig  |    7 +++++++
 drivers/net/wireless/ath/ath9k/Makefile |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index d9c08c6..adddcca 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -58,6 +58,13 @@ config ATH9K_RATE_CONTROL
         Say Y, if you want to use the ath9k specific rate control
         module instead of minstrel_ht.
+config ATH9K_DFS
+       bool "Atheros ath9k DFS support"
+       depends on ATH9K
+       default y
At this point selecting y does nothing. Leave this patch out until
selecting "y" means something.

Default should be n, and in particular Atheros itself can only likely
commit to supporting DFS for AR9003 when it finds resources to do so
as well as properly test it, so DFS support kconfig should state this.
If someone wants to step up to completely support all bugs for older
families that is their prerogative but we cannot commit to it, due to
the regulatory considerations though unless this happens this cannot
and should not be enabled for older families in code.

Luis
Zefir Kurtisi
2011-10-04 09:55:12 UTC
Permalink
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/Kconfig | 7 +++++++
drivers/net/wireless/ath/ath9k/Makefile | 2 ++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index d9c08c6..adddcca 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -58,6 +58,13 @@ config ATH9K_RATE_CONTROL
Say Y, if you want to use the ath9k specific rate control
module instead of minstrel_ht.
+config ATH9K_DFS
+ bool "Atheros ath9k DFS support"
+ depends on ATH9K
+ default y
At this point selecting y does nothing. Leave this patch out until
selecting "y" means something.
What do you mean by 'nothing'? It allows you to select DFS as ath9k feature in your kernel configuration, or? Though, I agree that enabling it by default is not a good idea.
Post by Luis R. Rodriguez
Default should be n, and in particular Atheros itself can only likely
commit to supporting DFS for AR9003 when it finds resources to do so
as well as properly test it, so DFS support kconfig should state this.
If someone wants to step up to completely support all bugs for older
families that is their prerogative but we cannot commit to it, due to
the regulatory considerations though unless this happens this cannot
and should not be enabled for older families in code.
In fact, AR9003 is the platform we are interested in. Although it seems that older chipsets do also detect pulses with this patches (AR9280 does, IIRC), I agree to limit DFS support to AR9003 (and later). This should be easily possible by setting priv_ops->set_radar_params for AR9003 only. I'll remove it for AR5008 in my v2 RFC.
Post by Luis R. Rodriguez
Luis
Thanks,
Zefir
Felix Fietkau
2011-10-04 10:37:00 UTC
Permalink
Post by Zefir Kurtisi
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/Kconfig | 7 +++++++
drivers/net/wireless/ath/ath9k/Makefile | 2 ++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index d9c08c6..adddcca 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -58,6 +58,13 @@ config ATH9K_RATE_CONTROL
Say Y, if you want to use the ath9k specific rate control
module instead of minstrel_ht.
+config ATH9K_DFS
+ bool "Atheros ath9k DFS support"
+ depends on ATH9K
+ default y
At this point selecting y does nothing. Leave this patch out until
selecting "y" means something.
What do you mean by 'nothing'? It allows you to select DFS as ath9k feature in your kernel configuration, or? Though, I agree that enabling it by default is not a good idea.
Post by Luis R. Rodriguez
Default should be n, and in particular Atheros itself can only likely
commit to supporting DFS for AR9003 when it finds resources to do so
as well as properly test it, so DFS support kconfig should state this.
If someone wants to step up to completely support all bugs for older
families that is their prerogative but we cannot commit to it, due to
the regulatory considerations though unless this happens this cannot
and should not be enabled for older families in code.
In fact, AR9003 is the platform we are interested in. Although it
seems that older chipsets do also detect pulses with this patches
(AR9280 does, IIRC), I agree to limit DFS support to AR9003 (and later).
This should be easily possible by setting priv_ops->set_radar_params for
AR9003 only. I'll remove it for AR5008 in my v2 RFC.
Please don't remove support code for older hardware. I'm fine with
adding a SREV check that prevents it from being enabled by default on
older hardware, but eventually I will need at least AR9280 DFS support
for a few devices in OpenWrt.

- Felix
Adrian Chadd
2011-10-04 12:25:07 UTC
Permalink
.. ditto for FreeBSD.

I'm hoping we can both leverage the same shared radar classification
modules with little changes needed in code. I'd like this to work for
at least AR9280.


Adrian
Luis R. Rodriguez
2011-10-05 22:20:36 UTC
Permalink
Post by Zefir Kurtisi
 ---
 drivers/net/wireless/ath/ath9k/Kconfig  |    7 +++++++
 drivers/net/wireless/ath/ath9k/Makefile |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
 diff --git a/drivers/net/wireless/ath/ath9k/Kconfig
b/drivers/net/wireless/ath/ath9k/Kconfig
 index d9c08c6..adddcca 100644
 --- a/drivers/net/wireless/ath/ath9k/Kconfig
 +++ b/drivers/net/wireless/ath/ath9k/Kconfig
         Say Y, if you want to use the ath9k specific rate control
         module instead of minstrel_ht.
 +config ATH9K_DFS
 +       bool "Atheros ath9k DFS support"
 +       depends on ATH9K
 +       default y
 At this point selecting y does nothing. Leave this patch out until
 selecting "y" means something.
What do you mean by 'nothing'? It allows you to select DFS as ath9k
feature in your kernel configuration, or? Though, I agree that enabling it
by default is not a good idea.
 Default should be n, and in particular Atheros itself can only likely
 commit to supporting DFS for AR9003 when it finds resources to do so
 as well as properly test it, so DFS support kconfig should state this.
 If someone wants to step up to completely support all bugs for older
 families that is their prerogative but we cannot commit to it, due to
 the regulatory considerations though unless this happens this cannot
 and should not be enabled for older families in code.
In fact, AR9003 is the platform we are interested in. Although it
seems that older chipsets do also detect pulses with this patches
(AR9280 does, IIRC), I agree to limit DFS support to AR9003 (and later).
This should be easily possible by setting priv_ops->set_radar_params for
AR9003 only. I'll remove it for AR5008 in my v2 RFC.
Please don't remove support code for older hardware. I'm fine with adding a
SREV check that prevents it from being enabled by default on older hardware,
but eventually I will need at least AR9280 DFS support for a few devices in
OpenWrt.
That's fine, leaving it disabled is what I meant, and only once
something has been properly tested do we enable it. I personally only
want to spend my own energy on AR9003 and newer.

Luis
Zefir Kurtisi
2011-10-03 10:29:15 UTC
Permalink
This initial DFS module provides basic functionality to deal with radar
pulses reported by the DFS HW pattern detector.

It is based on Atheros proprietary driver sources with the core
functionality ported to ath9k to inspect pulse data and perform
plausibility checks to filter false pulses.

The numerous TODOs left include
* checks for chirping pulses
* differentiation between different chipsets
* support for configuring the DFS HW
* etc.

The output of this module are radar events ready to be feed to a
pattern detection module.

Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath9k/dfs.c | 192 ++++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath9k/dfs.h | 24 ++++
2 files changed, 216 insertions(+), 0 deletions(-)
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.c
create mode 100644 drivers/net/wireless/ath/ath9k/dfs.h

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c
new file mode 100644
index 0000000..1fc9596
--- /dev/null
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -0,0 +1,192 @@
+/*
+ * Copyright (c) 2008-2011 Atheros Communications Inc.
+ * Copyright (c) 2011 Neratec Solutions AG
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include "hw.h"
+#include "hw-ops.h"
+#include "ath9k.h"
+#include "dfs.h"
+
+
+/* pulse duration reported is scaled with 1000/800 us */
+#define AR93X_NSECS_PER_DUR 800
+static u32 dur_to_usecs(u32 dur)
+{
+ return (dur * AR93X_NSECS_PER_DUR + 500) / 1000;
+}
+
+/* internal struct to pass radar data */
+struct ath_radar_data {
+ u8 pulse_bw_info;
+ u8 rssi;
+ u8 ext_rssi;
+ u8 pulse_length_ext;
+ u8 pulse_length_pri;
+};
+
+/* TODO: move into or synchronize this with generic header
+ * as soon as IF is defined */
+struct dfs_radar_pulse {
+ u16 freq;
+ u64 ts;
+ u32 width;
+ u8 rssi;
+};
+
+#define PRI_CH_RADAR_FOUND 0x01
+#define EXT_CH_RADAR_FOUND 0x02
+static bool postprocess_radar_event(struct ath_softc *sc,
+ struct ath_radar_data *are, struct dfs_radar_pulse *drp)
+{
+ u8 rssi;
+ u16 dur;
+
+ ath_dbg(ath9k_hw_common(sc->sc_ah), ATH_DBG_DFS,
+ "pulse_bw_info=0x%x, pri,ext len/rssi=(%u/%u, %u/%u)\n",
+ are->pulse_bw_info,
+ are->pulse_length_pri, are->rssi,
+ are->pulse_length_ext, are->ext_rssi);
+
+ /* Only the last 2 bits of the BW info are relevant, they indicate
+ which channel the radar was detected in.*/
+ are->pulse_bw_info &= 0x03;
+
+ switch (are->pulse_bw_info) {
+ case 0:
+ /* Bogus bandwidth info received in descriptor,
+ so ignore this PHY error */
+ DFS_STAT_INC(sc, bwinfo_discards);
+ return false;
+ case PRI_CH_RADAR_FOUND:
+ /* radar in ctrl channel */
+ dur = are->pulse_length_pri;
+ DFS_STAT_INC(sc, pri_phy_errors);
+ /* cannot use ctrl channel RSSI
+ * if extension channel is stronger */
+ rssi = (are->ext_rssi >= (are->rssi + 3)) ? 0 : are->rssi;
+ break;
+ case EXT_CH_RADAR_FOUND:
+ /* radar in extension channel */
+ dur = are->pulse_length_ext;
+ DFS_STAT_INC(sc, ext_phy_errors);
+ /* cannot use extension channel RSSI
+ * if control channel is stronger */
+ rssi = (are->rssi >= (are->ext_rssi + 12)) ? 0 : are->ext_rssi;
+ break;
+ case (PRI_CH_RADAR_FOUND | EXT_CH_RADAR_FOUND):
+ /*
+ * Conducted testing, when pulse is on DC, both pri and ext
+ * durations are reported to be same
+ *
+ * Radiated testing, when pulse is on DC, different pri and
+ * ext durations are reported, so take the larger of the two
+ * */
+ if (are->pulse_length_ext >= are->pulse_length_pri)
+ dur = are->pulse_length_ext;
+ else
+ dur = are->pulse_length_pri;
+ DFS_STAT_INC(sc, dc_phy_errors);
+
+ /* when both are present use stronger one */
+ rssi = (are->rssi < are->ext_rssi) ? are->ext_rssi : are->rssi;
+ break;
+ }
+
+ if (rssi == 0) {
+ DFS_STAT_INC(sc, rssi_discards);
+ return false;
+ }
+
+ /*
+ * TODO: check chirping pulses
+ */
+
+ /* convert duration to usecs */
+ drp->width = dur_to_usecs(dur);
+ drp->rssi = rssi;
+
+ DFS_STAT_INC(sc, pulses_detected);
+ return true;
+}
+
+
+/*
+ * DFS: check PHY-error for radar pulse and feed the detector
+ */
+void ath9k_dfs_process_phyerr(struct ath_softc *sc, void *data,
+ struct ath_rx_status *rs, u64 mactime)
+{
+ struct ath_radar_data ard;
+ u16 datalen;
+ char *vdata_end;
+ struct dfs_radar_pulse drp;
+ struct ath_hw *ah = sc->sc_ah;
+ struct ath_common *common = ath9k_hw_common(ah);
+
+ if ((!(rs->rs_phyerr != ATH9K_PHYERR_RADAR)) &&
+ (!(rs->rs_phyerr != ATH9K_PHYERR_FALSE_RADAR_EXT))) {
+ ath_dbg(common, ATH_DBG_DFS,
+ "Error: rs_phyer=0x%x not a radar error\n",
+ rs->rs_phyerr);
+ return;
+ }
+
+ datalen = rs->rs_datalen;
+ if (datalen == 0) {
+ DFS_STAT_INC(sc, datalen_discards);
+ return;
+ }
+
+ ard.rssi = rs->rs_rssi_ctl0;
+ ard.ext_rssi = rs->rs_rssi_ext0;
+
+ /* hardware stores this as 8 bit signed value.
+ * we will cap it at 0 if it is a negative number
+ */
+ if (ard.rssi & 0x80)
+ ard.rssi = 0;
+ if (ard.ext_rssi & 0x80)
+ ard.ext_rssi = 0;
+
+ vdata_end = (char *)data + datalen;
+ ard.pulse_bw_info = vdata_end[-1];
+ ard.pulse_length_ext = vdata_end[-2];
+ ard.pulse_length_pri = vdata_end[-3];
+
+ ath_dbg(common, ATH_DBG_DFS,
+ "bw_info=%d, length_pri=%d, length_ext=%d, "
+ "rssi_pri=%d, rssi_ext=%d\n",
+ ard.pulse_bw_info, ard.pulse_length_pri, ard.pulse_length_ext,
+ ard.rssi, ard.ext_rssi);
+
+ drp.freq = ah->curchan->channel;
+ drp.ts = mactime;
+ if (postprocess_radar_event(sc, &ard, &drp)) {
+ static u64 last_ts;
+ ath_dbg(common, ATH_DBG_DFS,
+ "ath9k_dfs_process_phyerr: channel=%d, ts=%llu, "
+ "width=%d, rssi=%d, delta_ts=%llu\n",
+ drp.freq, drp.ts, drp.width, drp.rssi, drp.ts-last_ts);
+ last_ts = drp.ts;
+ /*
+ * TODO: forward pulse to pattern detector
+ *
+ * ieee80211_add_radar_pulse(drp.freq, drp.ts,
+ * drp.width, drp.rssi);
+ */
+ }
+}
+EXPORT_SYMBOL(ath9k_dfs_process_phyerr);
diff --git a/drivers/net/wireless/ath/ath9k/dfs.h b/drivers/net/wireless/ath/ath9k/dfs.h
new file mode 100644
index 0000000..4d95cad
--- /dev/null
+++ b/drivers/net/wireless/ath/ath9k/dfs.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2008-2011 Atheros Communications Inc.
+ * Copyright (c) 2011 Neratec Solutions AG
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#ifndef ATH9K_DFS_H
+#define ATH9K_DFS_H
+
+void ath9k_dfs_process_phyerr(struct ath_softc *sc, void *data,
+ struct ath_rx_status *rs, u64 mactime);
+
+#endif /* ATH9K_DFS_H */
--
1.7.4.1
Adrian Chadd
2011-10-03 11:57:32 UTC
Permalink
Just out of curiousity, is there any "fast clock" on or off with the AR93xx ?

(That's one of the gotchas I came across when porting DFS code to
FreeBSD for the AR9160/AR9280.)

Thanks,


Adrian
Zefir Kurtisi
2011-10-03 12:23:29 UTC
Permalink
Post by Adrian Chadd
Just out of curiousity, is there any "fast clock" on or off with the AR93xx ?
(That's one of the gotchas I came across when porting DFS code to
FreeBSD for the AR9160/AR9280.)
Thanks,
Adrian
There might be one, but I left it out (for now) for the sake of simplicity.

I anyhow doubt it has a practical relevance, since the pulse width is reported with usec granularity and therefore a scaling factor of 1/811 vs. 1/800 has no impact on the re-scaled value for widths up to 50 usecs.


Zefir
Adrian Chadd
2011-10-03 12:43:36 UTC
Permalink
Post by Zefir Kurtisi
There might be one, but I left it out (for now) for the sake of simplicity.
I anyhow doubt it has a practical relevance, since the pulse width is reported with usec granularity and therefore a scaling factor of 1/811 vs. 1/800 has no impact on the re-scaled value for widths up to 50 usecs.
I just remember this was a sticking point when porting the fusion code
to FreeBSD.
Without that particular fix, some of the radar patterns didn't
actually match reliably.

It's possible it doesn't matter as much for your classification code.
I'm just bringing it up to be complete. :)



Adrian
Zefir Kurtisi
2011-10-03 14:21:37 UTC
Permalink
Post by Adrian Chadd
Post by Zefir Kurtisi
There might be one, but I left it out (for now) for the sake of simplicity.
I anyhow doubt it has a practical relevance, since the pulse width is reported with usec granularity and therefore a scaling factor of 1/811 vs. 1/800 has no impact on the re-scaled value for widths up to 50 usecs.
I just remember this was a sticking point when porting the fusion code
to FreeBSD.
Without that particular fix, some of the radar patterns didn't
actually match reliably.
It's possible it doesn't matter as much for your classification code.
I'm just bringing it up to be complete. :)
Adrian
Thanks!

Most probably it'll end being added to the lengthy TODO list.


Zefir
Adrian Chadd
2011-10-03 14:23:35 UTC
Permalink
Post by Zefir Kurtisi
Most probably it'll end being added to the lengthy TODO list.
:) No worries. I've just added the freebsd version of this to
FreeBSD-HEAD. I'll add in the duration calculation function soon.

Now all we need is an ISC/GPL dual licenced packet classification
module. *wink* :)


Adrian
Zefir Kurtisi
2011-10-03 10:29:13 UTC
Permalink
Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath9k/debug.c | 51 ++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath9k/debug.h | 29 ++++++++++++++++++
2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index cdece82..6ad2266 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -89,6 +89,53 @@ static const struct file_operations fops_debug = {

#endif

+
+#ifdef CONFIG_ATH9K_DFS
+
+#define DFS_STAT(s, p) \
+ len += snprintf(buf + len, size - len, "%28s : %10u\n", s, \
+ sc->debug.stats.dfs_stats.p);
+
+static ssize_t read_file_dfs(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath_softc *sc = file->private_data;
+ char *buf;
+ unsigned int len = 0, size = 8000;
+ ssize_t retval = 0;
+
+ buf = kzalloc(size, GFP_KERNEL);
+ if (buf == NULL)
+ return -ENOMEM;
+
+ DFS_STAT("DFS pulses detected ", pulses_detected);
+ DFS_STAT("Datalen discards ", datalen_discards);
+ DFS_STAT("RSSI discards ", rssi_discards);
+ DFS_STAT("BW info discards ", bwinfo_discards);
+ DFS_STAT("Primary channel pulses ", pri_phy_errors);
+ DFS_STAT("Secondary channel pulses", ext_phy_errors);
+ DFS_STAT("Dual channel pulses ", dc_phy_errors);
+
+ if (len > size)
+ len = size;
+
+ retval = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+ kfree(buf);
+
+ return retval;
+}
+
+
+static const struct file_operations fops_dfs_stats = {
+ .read = read_file_dfs,
+ .open = ath9k_debugfs_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+#endif /* CONFIG_ATH9K_DFS */
+
+
+
#define DMA_BUF_LEN 1024

static ssize_t read_file_tx_chainmask(struct file *file, char __user *user_buf,
@@ -1385,6 +1432,10 @@ int ath9k_init_debug(struct ath_hw *ah)
debugfs_create_u32("chanbw", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
&sc->chan_bw);

+#ifdef CONFIG_ATH9K_DFS
+ debugfs_create_file("dfs_stats", S_IRUSR, sc->debug.debugfs_phy, sc,
+ &fops_dfs_stats);
+#endif
sc->debug.regidx = 0;
return 0;
}
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 4a04510..3a3c3b7 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -25,8 +25,10 @@ struct ath_buf;

#ifdef CONFIG_ATH9K_DEBUGFS
#define TX_STAT_INC(q, c) sc->debug.stats.txstats[q].c++
+#define DFS_STAT_INC(sc, c) (sc->debug.stats.dfs_stats.c++)
#else
#define TX_STAT_INC(q, c) do { } while (0)
+#define DFS_STAT_INC(sc, c) do { } while (0)
#endif

#ifdef CONFIG_ATH9K_DEBUGFS
@@ -171,10 +173,37 @@ struct ath_rx_stats {
u8 rs_antenna;
};

+#ifdef CONFIG_ATH9K_DFS
+/**
+ * struct ath_dfs_stats - DFS Statistics
+ *
+ * @pulses_detected: No. of pulses detected so far
+ * @datalen_discards: No. of pulses discarded due to invalid datalen
+ * @rssi_discards: No. of pulses discarded due to invalid RSSI
+ * @bwinfo_discards: No. of pulses discarded due to invalid BW info
+ * @pri_phy_errors: No. of pulses reported for primary channel
+ * @ext_phy_errors: No. of pulses reported for extension channel
+ * @dc_phy_errors: No. of pulses reported for primary + extension channel
+ */
+struct ath_dfs_stats {
+ u32 pulses_detected;
+ u32 datalen_discards;
+ u32 rssi_discards;
+ u32 bwinfo_discards;
+ u32 pri_phy_errors;
+ u32 ext_phy_errors;
+ u32 dc_phy_errors;
+};
+#endif
+
+
struct ath_stats {
struct ath_interrupt_stats istats;
struct ath_tx_stats txstats[ATH9K_NUM_TX_QUEUES];
struct ath_rx_stats rxstats;
+#ifdef CONFIG_ATH9K_DFS
+ struct ath_dfs_stats dfs_stats;
+#endif
};

struct ath9k_debug {
--
1.7.4.1
Luis R. Rodriguez
2011-10-03 18:14:54 UTC
Permalink
---
 drivers/net/wireless/ath/ath9k/debug.c |   51 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/debug.h |   29 ++++++++++++++++++
 2 files changed, 80 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index cdece82..6ad2266 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -89,6 +89,53 @@ static const struct file_operations fops_debug = {
 #endif
+
+#ifdef CONFIG_ATH9K_DFS
This kconfig entry doesn't exist yet, so no point in referring to it
yet, you can add a patch that adds this but describe it as work in
progress or something like that and later correct the text as you move
on.
+
+#define DFS_STAT(s, p) \
+       len += snprintf(buf + len, size - len, "%28s : %10u\n", s, \
+                       sc->debug.stats.dfs_stats.p);
+
Either rename DFS_STAT to ATH9K_DFS_STAT or undef DFS_STAT after its usage.
+static ssize_t read_file_dfs(struct file *file, char __user *user_buf,
+                            size_t count, loff_t *ppos)
+{
+       struct ath_softc *sc = file->private_data;
+       char *buf;
+       unsigned int len = 0, size = 8000;
+       ssize_t retval = 0;
+
+       buf = kzalloc(size, GFP_KERNEL);
+       if (buf == NULL)
+               return -ENOMEM;
+
+       DFS_STAT("DFS pulses detected     ", pulses_detected);
+       DFS_STAT("Datalen discards        ", datalen_discards);
+       DFS_STAT("RSSI discards           ", rssi_discards);
+       DFS_STAT("BW info discards        ", bwinfo_discards);
+       DFS_STAT("Primary channel pulses  ", pri_phy_errors);
+       DFS_STAT("Secondary channel pulses", ext_phy_errors);
+       DFS_STAT("Dual channel pulses     ", dc_phy_errors);
+
+       if (len > size)
+               len = size;
+
+       retval = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+       kfree(buf);
+
+       return retval;
+}
+
+
+static const struct file_operations fops_dfs_stats = {
+       .read = read_file_dfs,
+       .open = ath9k_debugfs_open,
+       .owner = THIS_MODULE,
+       .llseek = default_llseek,
+};
+#endif /* CONFIG_ATH9K_DFS */
I'd prefer to keep this apart into a debugfs_dfs.c but that's just me,
I would like to ensure to keep *all* DFS stat as easy to review as
possible and am not a fan of the ifdef sprinkle.
+
+
 #define DMA_BUF_LEN 1024
 static ssize_t read_file_tx_chainmask(struct file *file, char __user *user_buf,
@@ -1385,6 +1432,10 @@ int ath9k_init_debug(struct ath_hw *ah)
       debugfs_create_u32("chanbw", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
                          &sc->chan_bw);
+#ifdef CONFIG_ATH9K_DFS
+       debugfs_create_file("dfs_stats", S_IRUSR, sc->debug.debugfs_phy, sc,
+                           &fops_dfs_stats);
+#endif
If you stuff the code into a file here you'd just need a caller to
ath9k_debugfs_dfs_create() or something like that.
       sc->debug.regidx = 0;
       return 0;
 }
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 4a04510..3a3c3b7 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -25,8 +25,10 @@ struct ath_buf;
 #ifdef CONFIG_ATH9K_DEBUGFS
 #define TX_STAT_INC(q, c) sc->debug.stats.txstats[q].c++
+#define DFS_STAT_INC(sc, c) (sc->debug.stats.dfs_stats.c++)
 #else
 #define TX_STAT_INC(q, c) do { } while (0)
+#define DFS_STAT_INC(sc, c) do { } while (0)
Who's using this? If no one, then why add it? That is add it only when
its in code.
 #endif
 #ifdef CONFIG_ATH9K_DEBUGFS
@@ -171,10 +173,37 @@ struct ath_rx_stats {
       u8 rs_antenna;
 };
+#ifdef CONFIG_ATH9K_DFS
+/**
+ * struct ath_dfs_stats - DFS Statistics
+ *
+ */
+struct ath_dfs_stats {
+       u32 pulses_detected;
+       u32 datalen_discards;
+       u32 rssi_discards;
+       u32 bwinfo_discards;
+       u32 pri_phy_errors;
+       u32 ext_phy_errors;
+       u32 dc_phy_errors;
+};
+#endif
+
+
 struct ath_stats {
       struct ath_interrupt_stats istats;
       struct ath_tx_stats txstats[ATH9K_NUM_TX_QUEUES];
       struct ath_rx_stats rxstats;
+#ifdef CONFIG_ATH9K_DFS
+       struct ath_dfs_stats dfs_stats;
+#endif
If code used this set of stats this would give a compile error if
CONFIG_ATH9K_DEBUGFS is enabled but CONFIG_ATH9K_DFS was disabled as
the ifdef over the increment stat stuff is over CONFIG_ATH9K_DEBUGFS
and not CONFIG_ATH9K_DFS, but again, no one uses this code yet.

Luis
Zefir Kurtisi
2011-10-04 08:27:36 UTC
Permalink
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
---
drivers/net/wireless/ath/ath9k/debug.c | 51 ++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath9k/debug.h | 29 ++++++++++++++++++
2 files changed, 80 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index cdece82..6ad2266 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -89,6 +89,53 @@ static const struct file_operations fops_debug = {
#endif
+
+#ifdef CONFIG_ATH9K_DFS
This kconfig entry doesn't exist yet, so no point in referring to it
yet, you can add a patch that adds this but describe it as work in
progress or something like that and later correct the text as you move
on.
True, but the Kconfig entry is introduced in 4/6 of this series. I am not aware that the ordering of patches within a series is relevant, as long as it compiles after each additionally applied patch. But sure I can reorder, if that solves your concern.
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
+
+#define DFS_STAT(s, p) \
+ len += snprintf(buf + len, size - len, "%28s : %10u\n", s, \
+ sc->debug.stats.dfs_stats.p);
+
Either rename DFS_STAT to ATH9K_DFS_STAT or undef DFS_STAT after its usage.
Ok.
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
+static ssize_t read_file_dfs(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath_softc *sc = file->private_data;
+ char *buf;
+ unsigned int len = 0, size = 8000;
+ ssize_t retval = 0;
+
+ buf = kzalloc(size, GFP_KERNEL);
+ if (buf == NULL)
+ return -ENOMEM;
+
+ DFS_STAT("DFS pulses detected ", pulses_detected);
+ DFS_STAT("Datalen discards ", datalen_discards);
+ DFS_STAT("RSSI discards ", rssi_discards);
+ DFS_STAT("BW info discards ", bwinfo_discards);
+ DFS_STAT("Primary channel pulses ", pri_phy_errors);
+ DFS_STAT("Secondary channel pulses", ext_phy_errors);
+ DFS_STAT("Dual channel pulses ", dc_phy_errors);
+
+ if (len > size)
+ len = size;
+
+ retval = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+ kfree(buf);
+
+ return retval;
+}
+
+
+static const struct file_operations fops_dfs_stats = {
+ .read = read_file_dfs,
+ .open = ath9k_debugfs_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+#endif /* CONFIG_ATH9K_DFS */
I'd prefer to keep this apart into a debugfs_dfs.c but that's just me,
I would like to ensure to keep *all* DFS stat as easy to review as
possible and am not a fan of the ifdef sprinkle.
Ok.
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
+
+
#define DMA_BUF_LEN 1024
static ssize_t read_file_tx_chainmask(struct file *file, char __user *user_buf,
@@ -1385,6 +1432,10 @@ int ath9k_init_debug(struct ath_hw *ah)
debugfs_create_u32("chanbw", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
&sc->chan_bw);
+#ifdef CONFIG_ATH9K_DFS
+ debugfs_create_file("dfs_stats", S_IRUSR, sc->debug.debugfs_phy, sc,
+ &fops_dfs_stats);
+#endif
If you stuff the code into a file here you'd just need a caller to
ath9k_debugfs_dfs_create() or something like that.
Ok.
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
sc->debug.regidx = 0;
return 0;
}
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 4a04510..3a3c3b7 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -25,8 +25,10 @@ struct ath_buf;
#ifdef CONFIG_ATH9K_DEBUGFS
#define TX_STAT_INC(q, c) sc->debug.stats.txstats[q].c++
+#define DFS_STAT_INC(sc, c) (sc->debug.stats.dfs_stats.c++)
#else
#define TX_STAT_INC(q, c) do { } while (0)
+#define DFS_STAT_INC(sc, c) do { } while (0)
Who's using this? If no one, then why add it? That is add it only when
its in code.
It is used in 3/6, see reordering issue above.
Post by Luis R. Rodriguez
Post by Zefir Kurtisi
#endif
#ifdef CONFIG_ATH9K_DEBUGFS
@@ -171,10 +173,37 @@ struct ath_rx_stats {
u8 rs_antenna;
};
+#ifdef CONFIG_ATH9K_DFS
+/**
+ * struct ath_dfs_stats - DFS Statistics
+ *
+ */
+struct ath_dfs_stats {
+ u32 pulses_detected;
+ u32 datalen_discards;
+ u32 rssi_discards;
+ u32 bwinfo_discards;
+ u32 pri_phy_errors;
+ u32 ext_phy_errors;
+ u32 dc_phy_errors;
+};
+#endif
+
+
struct ath_stats {
struct ath_interrupt_stats istats;
struct ath_tx_stats txstats[ATH9K_NUM_TX_QUEUES];
struct ath_rx_stats rxstats;
+#ifdef CONFIG_ATH9K_DFS
+ struct ath_dfs_stats dfs_stats;
+#endif
If code used this set of stats this would give a compile error if
CONFIG_ATH9K_DEBUGFS is enabled but CONFIG_ATH9K_DFS was disabled as
the ifdef over the increment stat stuff is over CONFIG_ATH9K_DEBUGFS
and not CONFIG_ATH9K_DFS, but again, no one uses this code yet.
True, will be fixed.
Post by Luis R. Rodriguez
Luis
Thanks,
Zefir
Zefir Kurtisi
2011-10-03 10:29:18 UTC
Permalink
Note: calculation of mactime had to be shifted before
ath9k_rx_skb_preprocess() since it is used to time-stamp the
radar pulse.

Signed-off-by: Zefir Kurtisi <***@neratec.com>
---
drivers/net/wireless/ath/ath9k/recv.c | 22 +++++++++++++++++-----
1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 02c9f97..f5bb114 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -17,6 +17,10 @@
#include <linux/dma-mapping.h>
#include "ath9k.h"
#include "ar9003_mac.h"
+#ifdef CONFIG_ATH9K_DFS
+#include "dfs.h"
+#endif
+

#define SKB_CB_ATHBUF(__skb) (*((struct ath_buf **)__skb->cb))

@@ -1850,11 +1854,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
if (flush)
goto requeue_drop_frag;

- retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
- rxs, &decrypt_error);
- if (retval)
- goto requeue_drop_frag;
-
rxs->mactime = (tsf & ~0xffffffffULL) | rs.rs_tstamp;
if (rs.rs_tstamp > tsf_lower &&
unlikely(rs.rs_tstamp - tsf_lower > 0x10000000))
@@ -1864,6 +1863,19 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
unlikely(tsf_lower - rs.rs_tstamp > 0x10000000))
rxs->mactime += 0x100000000ULL;

+#ifdef CONFIG_ATH9K_DFS
+ if ((hdr != NULL) && ((rs.rs_status & ATH9K_RXERR_PHY) != 0) &&
+ (rs.rs_phyerr == ATH9K_PHYERR_RADAR)) {
+ /* DFS: feed radar pulse */
+ ath9k_dfs_process_phyerr(sc, hdr, &rs, rxs->mactime);
+ }
+#endif
+
+ retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
+ rxs, &decrypt_error);
+ if (retval)
+ goto requeue_drop_frag;
+
/* Ensure we always have an skb to requeue once we are done
* processing the current buffer's skb */
requeue_skb = ath_rxbuf_alloc(common, common->rx_bufsize, GFP_ATOMIC);
--
1.7.4.1
Luis R. Rodriguez
2011-10-03 18:30:24 UTC
Permalink
Post by Zefir Kurtisi
Note: calculation of mactime had to be shifted before
ath9k_rx_skb_preprocess() since it is used to time-stamp the
radar pulse.
Do this in a separate patch and provide a valid commit log here of
what the addition actually does. As I see it you can unify the last
set of patches together. The debugfs stuff can be kept apart though
but with the considerations I mentioned.

Luis
Loading...