ath9k: DFS pattern detection

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/debug.c | ? 51 ++++++++++++++++++++++++++++++++ ?drivers/net/wireless/ath/ath9k/debug.h | ? 29 ++++++++++++++++++ ?2 files changed, 80 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/debug.c +0 -0

index cdece82..6ad2266 100644
@@ -89,6 +89,53 @@ static const struct file_operations fops_debug = {
 

?#endif

drivers/net/wireless/ath/ath9k/debug.c +27 -0

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

drivers/net/wireless/ath/ath9k/debug.c +0 -0

 ? ? ? ?sc->debug.regidx = 0;
 ? ? ? ?return 0;
 ?}

drivers/net/wireless/ath/ath9k/debug.h +0 -0

index 4a04510..3a3c3b7 100644
@@ -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

drivers/net/wireless/ath/ath9k/debug.h +21 -0

@@ -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;
+};

Quoted 15 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/debug.c |   51 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/debug.h |   29 ++++++++++++++++++
 2 files changed, 80 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/debug.c +0 -0

index cdece82..6ad2266 100644
@@ -89,6 +89,53 @@ static const struct file_operations fops_debug = {
 
 #endif
 

drivers/net/wireless/ath/ath9k/debug.c static const struct file_operations fops_debug = { · line 93

Quoted 2 lines

drivers/net/wireless/ath/ath9k/debug.c +2 -0

+
+#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.

drivers/net/wireless/ath/ath9k/debug.c static const struct file_operations fops_debug = { · line 98

Quoted 5 lines

drivers/net/wireless/ath/ath9k/debug.c +5 -0

+
+#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.

Quoted 33 lines

drivers/net/wireless/ath/ath9k/debug.c +33 -0

+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,

drivers/net/wireless/ath/ath9k/debug.c static const struct file_operations fops_debug = { · line 135

Quoted 4 lines

drivers/net/wireless/ath/ath9k/debug.c +4 -0

+       .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.

Quoted 9 lines

drivers/net/wireless/ath/ath9k/debug.c +2 -0

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

drivers/net/wireless/ath/ath9k/debug.c int ath9k_init_debug(struct ath_hw *ah) · line 1438

Quoted 4 lines

drivers/net/wireless/ath/ath9k/debug.c +4 -0

+#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.

Quoted 9 lines

drivers/net/wireless/ath/ath9k/debug.c +0 -0

       sc->debug.regidx = 0;
       return 0;
 }

drivers/net/wireless/ath/ath9k/debug.h +0 -0

index 4a04510..3a3c3b7 100644
@@ -25,8 +25,10 @@ struct ath_buf;
 

drivers/net/wireless/ath/ath9k/debug.h struct ath_buf; · line 31

Quoted 6 lines

drivers/net/wireless/ath/ath9k/debug.h +2 -0

 #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.

Quoted 35 lines

drivers/net/wireless/ath/ath9k/debug.h +24 -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;

drivers/net/wireless/ath/ath9k/debug.h struct ath_rx_stats { · line 206

Quoted 3 lines

drivers/net/wireless/ath/ath9k/debug.h +3 -0

+#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

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath.h | ? 34 ++++++++++++++++++---------------- ?1 files changed, 18 insertions(+), 16 deletions(-)

drivers/net/wireless/ath/ath.h +0 -0

index 6d56532..34d4da1 100644
@@ -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

drivers/net/wireless/ath/ath.h +0 -0

@@ -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

Quoted 51 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath.h |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

drivers/net/wireless/ath/ath.h +14 -16

index 6d56532..34d4da1 100644
@@ -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),

drivers/net/wireless/ath/ath.h ath_printk(const char *level, struct ath_common *common, const char *fmt, ...); · line 241

Quoted 5 lines

drivers/net/wireless/ath/ath.h +4 -0

+       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

Luis

Quoted 52 lines · Luis R. Rodriguez

Quoted 51 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath.h | ? 34 ++++++++++++++++++---------------- ?1 files changed, 18 insertions(+), 16 deletions(-)

drivers/net/wireless/ath/ath.h +0 -0

index 6d56532..34d4da1 100644
@@ -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

drivers/net/wireless/ath/ath.h +0 -0

@@ -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),

Quoted 8 lines

Quoted 5 lines

+ ? ? ? 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).

Quoted 2 lines

? Luis

Thanks,
Zefir
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

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.

Quoted 3 lines · 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

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

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/Kconfig ?| ? ?7 +++++++ ?drivers/net/wireless/ath/ath9k/Makefile | ? ?2 ++ ?2 files changed, 9 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/Kconfig +0 -0

index d9c08c6..adddcca 100644
@@ -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

Quoted 15 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/Kconfig  |    7 +++++++
 drivers/net/wireless/ath/ath9k/Makefile |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/Kconfig +0 -0

index d9c08c6..adddcca 100644
@@ -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.
 

drivers/net/wireless/ath/ath9k/Kconfig config ATH9K_RATE_CONTROL · line 64

Quoted 4 lines

drivers/net/wireless/ath/ath9k/Kconfig +4 -0

+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

Quoted 16 lines · Luis R. Rodriguez

Quoted 15 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi<[email protected]>

---

  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.

Quoted 8 lines

Quoted 4 lines

+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.

Quoted 8 lines

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.

Quoted 41 lines · Zefir Kurtisi

Quoted 19 lines · Luis R. Rodriguez

?On Mon, Oct 3, 2011 at 3:29 AM, Zefir Kurtisi<[email protected]>

Quoted 16 lines · ?wrote:

?Signed-off-by: Zefir Kurtisi<[email protected]> ?---
?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.

Quoted 8 lines

Quoted 4 lines

?+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.

Quoted 8 lines

?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.

Quoted 5 lines

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.

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/main.c | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +2 -0

index e8aeb98..5defebe 100644
@@ -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
+ ? ? ? /**

Quoted 21 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +7 -0

index e8aeb98..5defebe 100644
@@ -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);

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 358

Quoted 5 lines

drivers/net/wireless/ath/ath9k/main.c +5 -0

+       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.

Quoted 22 lines · Luis R. Rodriguez

Quoted 21 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/main.c | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +7 -0

index e8aeb98..5defebe 100644
@@ -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);

Quoted 8 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 358

Quoted 5 lines

drivers/net/wireless/ath/ath9k/main.c +5 -0

+ ? ? ? 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".

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · Luis R. Rodriguez

Quoted 11 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +0 -0

index e8aeb98..5defebe 100644

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 4 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

@@ -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.

Quoted 3 lines

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.

Quoted 26 lines · Luis R. Rodriguez

Quoted 23 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 12 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
@@ -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".

Quoted 1 line

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.

Quoted 2 lines

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

.. 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.

Quoted 2 lines

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.

Quoted 5 lines · Luis R. Rodriguez

Quoted 2 lines

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.

Quoted 27 lines · Christian Lamparter

Quoted 26 lines · Luis R. Rodriguez

Quoted 23 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 12 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
@@ -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".

Quoted 7 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 2 lines

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.

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.

Quoted 1 line · 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.

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

Quoted 6 lines · Christian Lamparter

Quoted 1 line · 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.

Quoted 10 lines

[ 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. :)

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.

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.

Quoted 41 lines · Zefir Kurtisi

Quoted 27 lines · Christian Lamparter

Quoted 26 lines · Luis R. Rodriguez

Quoted 23 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 12 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
@@ -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".

Quoted 7 lines

Quoted 1 line

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

Quoted 2 lines

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?

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 27 lines · Christian Lamparter

Quoted 26 lines · Luis R. Rodriguez

Quoted 23 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/main.c | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 12 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
@@ -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".

Quoted 7 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 26 lines · Luis R. Rodriguez

Quoted 23 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/main.c | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 12 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
@@ -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".

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 25 lines · Christian Lamparter

Quoted 24 lines · Luis R. Rodriguez

Quoted 17 lines · Christian Lamparter

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 347

Quoted 6 lines

drivers/net/wireless/ath/ath9k/main.c +1 -0

index e8aeb98..5defebe 100644
[...]
+#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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines · 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.

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

Quoted 10 lines

This is what I'm talking about. But yes, userspace also submits itself to the same criteria.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 3 lines · 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.

Quoted 10 lines · Christian Lamparter

Quoted 2 lines · 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.

Quoted 6 lines

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."

That's an implementation weirdness with python pack on generating the binary output, not a DFS issue per se.

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 16 lines

Quoted 15 lines

This is what I'm talking about. But yes, userspace also submits itself to the same criteria.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

This "deserve its own set of tests and review". Does it translate in:
"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.

Quoted 2 lines

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.

Quoted 2 lines

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

As said above, disabling a driver's capability through a Kconfig option should be enough (one ifdef per driver).

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

Quoted 2 lines · Zefir Kurtisi

As said above, disabling a driver's capability through a Kconfig option should be enough (one ifdef per driver).

OK cool.

Quoted 4 lines

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

Quoted 1 line

[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

Quoted 15 lines · Luis R. Rodriguez

Quoted 2 lines · On Thu, Oct 6, 2011 at 1:32 PM, Zefir Kurtisi<[email protected]> ?wrote:

As said above, disabling a driver's capability through a Kconfig option should be enough (one ifdef per driver).

OK cool.

Quoted 5 lines

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.

Quoted 1 line

? ?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.

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.

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

Quite some work ahead. Good luck on that!

So you are confident to get existing code re-factored and split into multiple layers?

Quoted 21 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
 drivers/net/wireless/ath/ath9k/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +7 -0

index e8aeb98..5defebe 100644
@@ -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);

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 358

Quoted 5 lines

drivers/net/wireless/ath/ath9k/main.c +5 -0

+       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

Quoted 22 lines · Luis R. Rodriguez

Quoted 21 lines · Zefir Kurtisi

Signed-off-by Zefir Kurtisi <[email protected]>

---
?drivers/net/wireless/ath/ath9k/main.c | ? 12 ++++++++++++ ?1 files changed, 12 insertions(+), 0 deletions(-)

drivers/net/wireless/ath/ath9k/main.c +7 -0

index e8aeb98..5defebe 100644
@@ -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);

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan, · line 358

Quoted 5 lines

drivers/net/wireless/ath/ath9k/main.c +5 -0

+ ? ? ? 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.

Note: calculation of mactime had to be shifted before ath9k_rx_skb_preprocess() since it is used to time-stamp the radar pulse.