[PATCH v2 0/7] wl12xx: add some psm/suspend features

[PATCH v2 0/7] wl12xx: add some psm/suspend features

[Eliad Peller]

Add some psm / suspend features:
* Add the ability to force psm (by default, the fw uses dynamic ps)
* Use differnet wake up conditions on suspend
* Add pattern support to wowlan triggers

(depends on the "wl12xx: update fw api" patchset)

v2: the rx filters patches were squashed and rewritten (thanks Eyal!)

Eyal Shapira (7):
  wl12xx: Set different wake up conditions in case of suspend
  wl12xx: add suspend_listen_interval debugfs file
  wl12xx: add forced_ps mode
  wl12xx: add forced_ps debugfs file
  wl12xx: add RX data filter ACX commands
  wl12xx: add RX data filters management functions
  wl12xx: support wowlan wakeup patterns

 drivers/net/wireless/wl12xx/acx.c     |  115 +++++++++++++-
 drivers/net/wireless/wl12xx/acx.h     |   34 ++++-
 drivers/net/wireless/wl12xx/conf.h    |   19 +++
 drivers/net/wireless/wl12xx/debugfs.c |  131 ++++++++++++++++-
 drivers/net/wireless/wl12xx/main.c    |  272 +++++++++++++++++++++++++++++++--
 drivers/net/wireless/wl12xx/ps.c      |   12 +-
 drivers/net/wireless/wl12xx/rx.c      |   61 ++++++++
 drivers/net/wireless/wl12xx/rx.h      |    8 +-
 drivers/net/wireless/wl12xx/wl12xx.h  |   38 +++++-
 9 files changed, 662 insertions(+), 28 deletions(-)

-- 
1.7.6.401.g6a319

[PATCH v2 1/7] wl12xx: Set different wake up conditions in case of suspend

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

Added ability to set different wake up conditions for suspend/resume.
Set default values to wake up every 3 DTIMs while suspended
and every 1 DTIM while resumed

Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/acx.c  |   10 ++++---
 drivers/net/wireless/wl12xx/acx.h  |    3 +-
 drivers/net/wireless/wl12xx/conf.h |   13 +++++++++
 drivers/net/wireless/wl12xx/main.c |   51 +++++++++++++++++++++++++++++++++--
 drivers/net/wireless/wl12xx/ps.c   |    4 ++-
 5 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index af2c312..bc96db0 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -34,12 +34,14 @@
 #include "reg.h"
 #include "ps.h"
 
-int wl1271_acx_wake_up_conditions(struct wl1271 *wl, struct wl12xx_vif *wlvif)
+int wl1271_acx_wake_up_conditions(struct wl1271 *wl, struct wl12xx_vif *wlvif,
+				  u8 wake_up_event, u8 listen_interval)
 {
 	struct acx_wake_up_condition *wake_up;
 	int ret;
 
-	wl1271_debug(DEBUG_ACX, "acx wake up conditions");
+	wl1271_debug(DEBUG_ACX, "acx wake up conditions (wake_up_event %d listen_interval %d)",
+		     wake_up_event, listen_interval);
 
 	wake_up = kzalloc(sizeof(*wake_up), GFP_KERNEL);
 	if (!wake_up) {
@@ -48,8 +50,8 @@ int wl1271_acx_wake_up_conditions(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 	}
 
 	wake_up->role_id = wlvif->role_id;
-	wake_up->wake_up_event = wl->conf.conn.wake_up_event;
-	wake_up->listen_interval = wl->conf.conn.listen_interval;
+	wake_up->wake_up_event = wake_up_event;
+	wake_up->listen_interval = listen_interval;
 
 	ret = wl1271_cmd_configure(wl, ACX_WAKE_UP_CONDITIONS,
 				   wake_up, sizeof(*wake_up));
diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
index 0749df5..a28fc04 100644
--- a/drivers/net/wireless/wl12xx/acx.h
+++ b/drivers/net/wireless/wl12xx/acx.h
@@ -1226,7 +1226,8 @@ enum {
 
 
 int wl1271_acx_wake_up_conditions(struct wl1271 *wl,
-				  struct wl12xx_vif *wlvif);
+				  struct wl12xx_vif *wlvif,
+				  u8 wake_up_event, u8 listen_interval);
 int wl1271_acx_sleep_auth(struct wl1271 *wl, u8 sleep_auth);
 int wl1271_acx_tx_power(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 			int power);
diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
index 10e5e3d..d97aad6 100644
--- a/drivers/net/wireless/wl12xx/conf.h
+++ b/drivers/net/wireless/wl12xx/conf.h
@@ -810,6 +810,19 @@ struct conf_conn_settings {
 	u8 listen_interval;
 
 	/*
+	 * Firmware wakeup conditions during suspend
+	 * Range: CONF_WAKE_UP_EVENT_*
+	 */
+	u8 suspend_wake_up_event;
+
+	/*
+	 * Listen interval during suspend.
+	 * Currently will be in DTIMs (1-10)
+	 *
+	 */
+	u8 suspend_listen_interval;
+
+	/*
 	 * Enable or disable the beacon filtering.
 	 *
 	 * Range: CONF_BCN_FILT_MODE_*
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 2bb1c24..29a00fc 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -218,6 +218,8 @@ static struct conf_drv_settings default_conf = {
 	.conn = {
 		.wake_up_event               = CONF_WAKE_UP_EVENT_DTIM,
 		.listen_interval             = 1,
+		.suspend_wake_up_event       = CONF_WAKE_UP_EVENT_N_DTIM,
+		.suspend_listen_interval     = 3,
 		.bcn_filt_mode               = CONF_BCN_FILT_MODE_ENABLED,
 		.bcn_filt_ie_count           = 2,
 		.bcn_filt_ie = {
@@ -1561,6 +1563,35 @@ static struct notifier_block wl1271_dev_notifier = {
 };
 
 #ifdef CONFIG_PM
+static int wl1271_configure_suspend_sta(struct wl1271 *wl,
+					struct wl12xx_vif *wlvif)
+{
+	int ret = 0;
+
+	mutex_lock(&wl->mutex);
+
+	if (!test_bit(WLVIF_FLAG_STA_ASSOCIATED, &wlvif->flags))
+		goto out_unlock;
+
+	ret = wl1271_ps_elp_wakeup(wl);
+	if (ret < 0)
+		goto out_unlock;
+
+	ret = wl1271_acx_wake_up_conditions(wl, wlvif,
+				    wl->conf.conn.suspend_wake_up_event,
+				    wl->conf.conn.suspend_listen_interval);
+
+	if (ret < 0)
+		wl1271_error("suspend: set wake up conditions failed: %d", ret);
+
+
+	wl1271_ps_elp_sleep(wl);
+
+out_unlock:
+	mutex_unlock(&wl->mutex);
+	return ret;
+
+}
 
 static int wl1271_configure_suspend_ap(struct wl1271 *wl,
 				       struct wl12xx_vif *wlvif)
@@ -1588,6 +1619,8 @@ out_unlock:
 static int wl1271_configure_suspend(struct wl1271 *wl,
 				    struct wl12xx_vif *wlvif)
 {
+	if (wlvif->bss_type == BSS_TYPE_STA_BSS)
+		return wl1271_configure_suspend_sta(wl, wlvif);
 	if (wlvif->bss_type == BSS_TYPE_AP_BSS)
 		return wl1271_configure_suspend_ap(wl, wlvif);
 	return 0;
@@ -1596,10 +1629,11 @@ static int wl1271_configure_suspend(struct wl1271 *wl,
 static void wl1271_configure_resume(struct wl1271 *wl,
 				    struct wl12xx_vif *wlvif)
 {
-	int ret;
+	int ret = 0;
 	bool is_ap = wlvif->bss_type == BSS_TYPE_AP_BSS;
+	bool is_sta = wlvif->bss_type == BSS_TYPE_STA_BSS;
 
-	if (!is_ap)
+	if ((!is_ap) && (!is_sta))
 		return;
 
 	mutex_lock(&wl->mutex);
@@ -1607,7 +1641,18 @@ static void wl1271_configure_resume(struct wl1271 *wl,
 	if (ret < 0)
 		goto out;
 
-	wl1271_acx_beacon_filter_opt(wl, wlvif, false);
+	if (is_sta) {
+		ret = wl1271_acx_wake_up_conditions(wl, wlvif,
+				    wl->conf.conn.wake_up_event,
+				    wl->conf.conn.listen_interval);
+
+		if (ret < 0)
+			wl1271_error("resume: wake up conditions failed: %d",
+				     ret);
+
+	} else if (is_ap) {
+		ret = wl1271_acx_beacon_filter_opt(wl, wlvif, false);
+	}
 
 	wl1271_ps_elp_sleep(wl);
 out:
diff --git a/drivers/net/wireless/wl12xx/ps.c b/drivers/net/wireless/wl12xx/ps.c
index e209e29..d197922 100644
--- a/drivers/net/wireless/wl12xx/ps.c
+++ b/drivers/net/wireless/wl12xx/ps.c
@@ -170,7 +170,9 @@ int wl1271_ps_set_mode(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 		wl1271_debug(DEBUG_PSM, "entering psm (mode=%d,timeout=%u)",
 			     mode, timeout);
 
-		ret = wl1271_acx_wake_up_conditions(wl, wlvif);
+		ret = wl1271_acx_wake_up_conditions(wl, wlvif,
+					    wl->conf.conn.wake_up_event,
+					    wl->conf.conn.listen_interval);
 		if (ret < 0) {
 			wl1271_error("couldn't set wake up conditions");
 			return ret;
-- 
1.7.6.401.g6a319

[PATCH v2 2/7] wl12xx: add suspend_listen_interval debugfs file

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

Add read/write to suspend_dtim_interval file which
controls the number of DTIM periods between wakeups
while the host is suspended.
The value while the host is resumed is controlled
by the file dtim_interval which existed previously.

Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/debugfs.c |   59 +++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/debugfs.c b/drivers/net/wireless/wl12xx/debugfs.c
index 4dcb3f7..15353fa 100644
--- a/drivers/net/wireless/wl12xx/debugfs.c
+++ b/drivers/net/wireless/wl12xx/debugfs.c
@@ -625,6 +625,64 @@ static const struct file_operations dtim_interval_ops = {
 	.llseek = default_llseek,
 };
 
+
+
+static ssize_t suspend_dtim_interval_read(struct file *file,
+					  char __user *user_buf,
+					  size_t count, loff_t *ppos)
+{
+	struct wl1271 *wl = file->private_data;
+	u8 value;
+
+	if (wl->conf.conn.suspend_wake_up_event == CONF_WAKE_UP_EVENT_DTIM ||
+	    wl->conf.conn.suspend_wake_up_event == CONF_WAKE_UP_EVENT_N_DTIM)
+		value = wl->conf.conn.suspend_listen_interval;
+	else
+		value = 0;
+
+	return wl1271_format_buffer(user_buf, count, ppos, "%d\n", value);
+}
+
+static ssize_t suspend_dtim_interval_write(struct file *file,
+					   const char __user *user_buf,
+					   size_t count, loff_t *ppos)
+{
+	struct wl1271 *wl = file->private_data;
+	unsigned long value;
+	int ret;
+
+	ret = kstrtoul_from_user(user_buf, count, 10, &value);
+	if (ret < 0) {
+		wl1271_warning("illegal value for suspend_dtim_interval");
+		return -EINVAL;
+	}
+
+	if (value < 1 || value > 10) {
+		wl1271_warning("suspend_dtim value is not in valid range");
+		return -ERANGE;
+	}
+
+	mutex_lock(&wl->mutex);
+
+	wl->conf.conn.suspend_listen_interval = value;
+	/* for some reason there are different event types for 1 and >1 */
+	if (value == 1)
+		wl->conf.conn.suspend_wake_up_event = CONF_WAKE_UP_EVENT_DTIM;
+	else
+		wl->conf.conn.suspend_wake_up_event = CONF_WAKE_UP_EVENT_N_DTIM;
+
+	mutex_unlock(&wl->mutex);
+	return count;
+}
+
+
+static const struct file_operations suspend_dtim_interval_ops = {
+	.read = suspend_dtim_interval_read,
+	.write = suspend_dtim_interval_write,
+	.open = wl1271_open_file_generic,
+	.llseek = default_llseek,
+};
+
 static ssize_t beacon_interval_read(struct file *file, char __user *user_buf,
 				    size_t count, loff_t *ppos)
 {
@@ -949,6 +1007,7 @@ static int wl1271_debugfs_add_files(struct wl1271 *wl,
 	DEBUGFS_ADD(driver_state, rootdir);
 	DEBUGFS_ADD(vifs_state, rootdir);
 	DEBUGFS_ADD(dtim_interval, rootdir);
+	DEBUGFS_ADD(suspend_dtim_interval, rootdir);
 	DEBUGFS_ADD(beacon_interval, rootdir);
 	DEBUGFS_ADD(beacon_filtering, rootdir);
 	DEBUGFS_ADD(dynamic_ps_timeout, rootdir);
-- 
1.7.6.401.g6a319

[PATCH v2 3/7] wl12xx: add forced_ps mode

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

For certain WiFi certification tests forcing PS
is necessary. Since DPS is now enabled in the FW
and this can't be achieved by using netlatency
this required a new config option.

Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/conf.h    |    6 ++++++
 drivers/net/wireless/wl12xx/debugfs.c |    2 +-
 drivers/net/wireless/wl12xx/main.c    |   25 +++++++++++++++++++------
 drivers/net/wireless/wl12xx/ps.c      |    8 ++++----
 drivers/net/wireless/wl12xx/wl12xx.h  |    2 +-
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
index d97aad6..f29fbfd 100644
--- a/drivers/net/wireless/wl12xx/conf.h
+++ b/drivers/net/wireless/wl12xx/conf.h
@@ -934,6 +934,12 @@ struct conf_conn_settings {
 	u16 dynamic_ps_timeout;
 
 	/*
+	 * Specifies whether dynamic PS should be disabled and PSM forced.
+	 * This is required for certain WiFi certification tests.
+	 */
+	u8 forced_ps;
+
+	/*
 	 *
 	 * Specifies the interval of the connection keep-alive null-func
 	 * frame in ms.
diff --git a/drivers/net/wireless/wl12xx/debugfs.c b/drivers/net/wireless/wl12xx/debugfs.c
index 15353fa..02da445 100644
--- a/drivers/net/wireless/wl12xx/debugfs.c
+++ b/drivers/net/wireless/wl12xx/debugfs.c
@@ -358,7 +358,7 @@ static ssize_t dynamic_ps_timeout_write(struct file *file,
 	 */
 
 	wl12xx_for_each_wlvif_sta(wl, wlvif) {
-		if (test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags))
+		if (test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags))
 			wl1271_ps_set_mode(wl, wlvif, STATION_AUTO_PS_MODE);
 	}
 
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 29a00fc..f2960df 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -244,6 +244,7 @@ static struct conf_drv_settings default_conf = {
 		.psm_exit_retries            = 16,
 		.psm_entry_nullfunc_retries  = 3,
 		.dynamic_ps_timeout          = 100,
+		.forced_ps                   = false,
 		.keep_alive_interval         = 55000,
 		.max_listen_interval         = 20,
 	},
@@ -2481,17 +2482,29 @@ static int wl12xx_config_vif(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 
 		if ((conf->flags & IEEE80211_CONF_PS) &&
 		    test_bit(WLVIF_FLAG_STA_ASSOCIATED, &wlvif->flags) &&
-		    !test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags)) {
+		    !test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags)) {
 
-			wl1271_debug(DEBUG_PSM, "auto ps enabled");
+			int ps_mode;
+			char *ps_mode_str;
+
+			if (wl->conf.conn.forced_ps) {
+				ps_mode = STATION_POWER_SAVE_MODE;
+				ps_mode_str = "forced";
+			} else {
+				ps_mode = STATION_AUTO_PS_MODE;
+				ps_mode_str = "auto";
+			}
+
+			wl1271_debug(DEBUG_PSM, "%s ps enabled", ps_mode_str);
+
+			ret = wl1271_ps_set_mode(wl, wlvif, ps_mode);
 
-			ret = wl1271_ps_set_mode(wl, wlvif,
-						 STATION_AUTO_PS_MODE);
 			if (ret < 0)
-				wl1271_warning("enter auto ps failed %d", ret);
+				wl1271_warning("enter %s ps failed %d",
+					       ps_mode_str, ret);
 
 		} else if (!(conf->flags & IEEE80211_CONF_PS) &&
-			   test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags)) {
+			   test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags)) {
 
 			wl1271_debug(DEBUG_PSM, "auto ps disabled");
 
diff --git a/drivers/net/wireless/wl12xx/ps.c b/drivers/net/wireless/wl12xx/ps.c
index d197922..fa993c2 100644
--- a/drivers/net/wireless/wl12xx/ps.c
+++ b/drivers/net/wireless/wl12xx/ps.c
@@ -56,7 +56,7 @@ void wl1271_elp_work(struct work_struct *work)
 		if (wlvif->bss_type == BSS_TYPE_AP_BSS)
 			goto out;
 
-		if (!test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags) &&
+		if (!test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags) &&
 		    test_bit(WLVIF_FLAG_IN_USE, &wlvif->flags))
 			goto out;
 	}
@@ -84,7 +84,7 @@ void wl1271_ps_elp_sleep(struct wl1271 *wl)
 		if (wlvif->bss_type == BSS_TYPE_AP_BSS)
 			return;
 
-		if (!test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags) &&
+		if (!test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags) &&
 		    test_bit(WLVIF_FLAG_IN_USE, &wlvif->flags))
 			return;
 	}
@@ -182,7 +182,7 @@ int wl1271_ps_set_mode(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 		if (ret < 0)
 			return ret;
 
-		set_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags);
+		set_bit(WLVIF_FLAG_IN_PS, &wlvif->flags);
 
 		/* enable beacon early termination. Not relevant for 5GHz */
 		if (wlvif->band == IEEE80211_BAND_2GHZ) {
@@ -205,7 +205,7 @@ int wl1271_ps_set_mode(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 		if (ret < 0)
 			return ret;
 
-		clear_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags);
+		clear_bit(WLVIF_FLAG_IN_PS, &wlvif->flags);
 		break;
 	case STATION_POWER_SAVE_MODE:
 	default:
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index 9cc7051..1463341 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -254,7 +254,7 @@ enum wl12xx_vif_flags {
 	WLVIF_FLAG_STA_ASSOCIATED,
 	WLVIF_FLAG_IBSS_JOINED,
 	WLVIF_FLAG_AP_STARTED,
-	WLVIF_FLAG_IN_AUTO_PS,
+	WLVIF_FLAG_IN_PS,
 	WLVIF_FLAG_STA_STATE_SENT,
 	WLVIF_FLAG_RX_STREAMING_STARTED,
 	WLVIF_FLAG_PSPOLL_FAILURE,
-- 
1.7.6.401.g6a319

Re: [PATCH v2 3/7] wl12xx: add forced_ps mode

[Luciano Coelho]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> From: Eyal Shapira <eyal@wizery.com>
> 
> For certain WiFi certification tests forcing PS
> is necessary. Since DPS is now enabled in the FW
> and this can't be achieved by using netlatency
> this required a new config option.
> 
> Signed-off-by: Eyal Shapira <eyal@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

[...]

> diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
> index d97aad6..f29fbfd 100644
> --- a/drivers/net/wireless/wl12xx/conf.h
> +++ b/drivers/net/wireless/wl12xx/conf.h
> @@ -934,6 +934,12 @@ struct conf_conn_settings {
>  	u16 dynamic_ps_timeout;
>  
>  	/*
> +	 * Specifies whether dynamic PS should be disabled and PSM forced.
> +	 * This is required for certain WiFi certification tests.
> +	 */
> +	u8 forced_ps;
> +
> +	/*

We are kind of abusing the conf struct.  Originally it contained the
stuff that was coming from the INI file, now we're putting everything
there.  It's okay for now, but just a reminder that we need to clean all
this up at some point (soon!).


> diff --git a/drivers/net/wireless/wl12xx/debugfs.c b/drivers/net/wireless/wl12xx/debugfs.c
> index 15353fa..02da445 100644
> --- a/drivers/net/wireless/wl12xx/debugfs.c
> +++ b/drivers/net/wireless/wl12xx/debugfs.c
> @@ -358,7 +358,7 @@ static ssize_t dynamic_ps_timeout_write(struct file *file,
>          */
>  
>         wl12xx_for_each_wlvif_sta(wl, wlvif) {
> -               if (test_bit(WLVIF_FLAG_IN_AUTO_PS, &wlvif->flags))
> +               if (test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags))
>                         wl1271_ps_set_mode(wl, wlvif, STATION_AUTO_PS_MODE);
>         }

Don't you want to change this file so that we can dynamically change from auto-PS to forced-PS?

> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 29a00fc..f2960df 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -244,6 +244,7 @@ static struct conf_drv_settings default_conf = {
>                 .psm_exit_retries            = 16,
>                 .psm_entry_nullfunc_retries  = 3,
>                 .dynamic_ps_timeout          = 100,
> +               .forced_ps                   = false,
>                 .keep_alive_interval         = 55000,
>                 .max_listen_interval         = 20,
>         },

This is hardcoded and would require the driver to be recompiled in order
to enable this feature.  Is that even allowed during certification (ie.
use two different binaries for different testcases)?


-- 
Cheers,
Luca.

[PATCH v2 4/7] wl12xx: add forced_ps debugfs file

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

Added control over forced_ps option through debugfs.
This can be either 1 or 0.

Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/debugfs.c |   70 +++++++++++++++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/debugfs.c b/drivers/net/wireless/wl12xx/debugfs.c
index 02da445..1c26238 100644
--- a/drivers/net/wireless/wl12xx/debugfs.c
+++ b/drivers/net/wireless/wl12xx/debugfs.c
@@ -376,6 +376,75 @@ static const struct file_operations dynamic_ps_timeout_ops = {
 	.llseek = default_llseek,
 };
 
+static ssize_t forced_ps_read(struct file *file, char __user *user_buf,
+			  size_t count, loff_t *ppos)
+{
+	struct wl1271 *wl = file->private_data;
+
+	return wl1271_format_buffer(user_buf, count,
+				    ppos, "%d\n",
+				    wl->conf.conn.forced_ps);
+}
+
+static ssize_t forced_ps_write(struct file *file,
+				    const char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct wl1271 *wl = file->private_data;
+	struct wl12xx_vif *wlvif;
+	unsigned long value;
+	int ret, ps_mode;
+
+	ret = kstrtoul_from_user(user_buf, count, 10, &value);
+	if (ret < 0) {
+		wl1271_warning("illegal value in forced_ps");
+		return -EINVAL;
+	}
+
+	if (value != 1 && value != 0) {
+		wl1271_warning("forced_ps should be either 0 or 1");
+		return -ERANGE;
+	}
+
+	mutex_lock(&wl->mutex);
+
+	if (wl->conf.conn.forced_ps == value)
+		goto out;
+
+	wl->conf.conn.forced_ps = value;
+
+	if (wl->state == WL1271_STATE_OFF)
+		goto out;
+
+	ret = wl1271_ps_elp_wakeup(wl);
+	if (ret < 0)
+		goto out;
+
+	/* In case we're already in PSM, trigger it again to switch mode
+	 * immediately without waiting for re-association
+	 */
+
+	ps_mode = value ? STATION_POWER_SAVE_MODE : STATION_AUTO_PS_MODE;
+
+	wl12xx_for_each_wlvif_sta(wl, wlvif) {
+		if (test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags))
+			wl1271_ps_set_mode(wl, wlvif, ps_mode);
+	}
+
+	wl1271_ps_elp_sleep(wl);
+
+out:
+	mutex_unlock(&wl->mutex);
+	return count;
+}
+
+static const struct file_operations forced_ps_ops = {
+	.read = forced_ps_read,
+	.write = forced_ps_write,
+	.open = wl1271_open_file_generic,
+	.llseek = default_llseek,
+};
+
 static ssize_t driver_state_read(struct file *file, char __user *user_buf,
 				 size_t count, loff_t *ppos)
 {
@@ -1011,6 +1080,7 @@ static int wl1271_debugfs_add_files(struct wl1271 *wl,
 	DEBUGFS_ADD(beacon_interval, rootdir);
 	DEBUGFS_ADD(beacon_filtering, rootdir);
 	DEBUGFS_ADD(dynamic_ps_timeout, rootdir);
+	DEBUGFS_ADD(forced_ps, rootdir);
 
 	streaming = debugfs_create_dir("rx_streaming", rootdir);
 	if (!streaming || IS_ERR(streaming))
-- 
1.7.6.401.g6a319

Re: [PATCH v2 4/7] wl12xx: add forced_ps debugfs file

[Luciano Coelho]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> From: Eyal Shapira <eyal@wizery.com>
> 
> Added control over forced_ps option through debugfs.
> This can be either 1 or 0.
> 
> Signed-off-by: Eyal Shapira <eyal@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

Ah, now I see it. :)

This should be squashed with the previous one.  And I think it should
probably be the same file (ie. "dynamic_ps_timeout") that should include
this feature.  Maybe passing -1 to enable forced PS?

-- 
Cheers,
Luca.

Re: [PATCH v2 4/7] wl12xx: add forced_ps debugfs file

[Luciano Coelho]

On Thu, 2012-02-02 at 09:44 +0200, Luciano Coelho wrote: 
> On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> > From: Eyal Shapira <eyal@wizery.com>
> > 
> > Added control over forced_ps option through debugfs.
> > This can be either 1 or 0.
> > 
> > Signed-off-by: Eyal Shapira <eyal@wizery.com>
> > Signed-off-by: Eliad Peller <eliad@wizery.com>
> > ---
> 
> Ah, now I see it. :)
> 
> This should be squashed with the previous one.

Changed my mind, no need to squash.


-- 
Cheers,
Luca.

[PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

(based on Pontus' patch)

Added commands for setting a specific filter
and controlling the behaviour RX data filters
implemented by the FW.

Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
Signed-off-by: Ido Reis <idor@ti.com>
Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/acx.c    |  105 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/acx.h    |   31 ++++++++++-
 drivers/net/wireless/wl12xx/wl12xx.h |   33 +++++++++++
 3 files changed, 168 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index bc96db0..668d337 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -1740,3 +1740,108 @@ out:
 	return ret;
 
 }
+
+int wl1271_acx_toggle_rx_data_filter(struct wl1271 *wl, bool enable,
+				     u8 default_action)
+{
+	struct acx_rx_data_filter_state *acx;
+	int ret;
+
+	wl1271_debug(DEBUG_ACX, "acx toggle rx data filter en: %d act: %d",
+		     enable, default_action);
+
+	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+	if (!acx) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	acx->enable = enable ? 1 : 0;
+	acx->default_action = default_action;
+
+	ret = wl1271_cmd_configure(wl, ACX_ENABLE_RX_DATA_FILTER, acx,
+				   sizeof(*acx));
+	if (ret < 0) {
+		wl1271_warning("toggling rx data filter failed: %d", ret);
+		goto out;
+	}
+
+out:
+	kfree(acx);
+	return ret;
+}
+
+int wl1271_acx_set_rx_data_filter(struct wl1271 *wl, u8 index, bool enable,
+				  struct wl12xx_rx_data_filter *filter)
+{
+	struct acx_rx_data_filter_cfg *acx;
+	int fields_size = 0;
+	int acx_size;
+	int ret;
+
+	if (enable && !filter) {
+		wl1271_warning("acx_set_rx_data_filter: enable but no filter");
+		return -EINVAL;
+	}
+
+	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
+		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
+			       index);
+		return -EINVAL;
+	}
+
+	if (filter) {
+		if (filter->action < FILTER_DROP ||
+		    filter->action > FILTER_FW_HANDLE) {
+			wl1271_warning("invalid filter action (%d)",
+				       filter->action);
+			return -EINVAL;
+		}
+
+		if (filter->num_fields != 1 &&
+		    filter->num_fields != 2) {
+			wl1271_warning("invalid filter num_fields (%d)",
+				       filter->num_fields);
+			return -EINVAL;
+		}
+	}
+
+	wl1271_debug(DEBUG_ACX, "acx set rx data filter idx: %d, enable: %d",
+		     index, enable);
+
+	if (enable) {
+		fields_size = filter->fields_size;
+
+		wl1271_debug(DEBUG_ACX, "act: %d num_fields: %d field_size: %d",
+		      filter->action, filter->num_fields, fields_size);
+	}
+
+	acx_size = roundup(sizeof(*acx) + fields_size, 4);
+	acx = kzalloc(acx_size, GFP_KERNEL);
+
+	if (!acx)
+		return -ENOMEM;
+
+	acx->enable = enable ? 1 : 0;
+	acx->index = index;
+
+	if (enable) {
+		acx->num_fields = filter->num_fields;
+		acx->action = filter->action;
+
+		memcpy(acx->fields, filter->fields, filter->fields_size);
+	}
+
+	wl1271_dump(DEBUG_ACX, "RX_FILTER: ", acx, acx_size);
+
+	ret = wl1271_cmd_configure(wl, ACX_SET_RX_DATA_FILTER, acx,
+				   acx_size);
+	if (ret < 0) {
+		wl1271_warning("setting rx data filter failed: %d", ret);
+		goto out;
+	}
+
+out:
+	kfree(acx);
+	return ret;
+}
diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
index a28fc04..9d0d30b 100644
--- a/drivers/net/wireless/wl12xx/acx.h
+++ b/drivers/net/wireless/wl12xx/acx.h
@@ -1152,6 +1152,32 @@ struct wl12xx_acx_config_hangover {
 	u8 padding[2];
 } __packed;
 
+
+struct acx_rx_data_filter_state {
+	struct acx_header header;
+	u8 enable;
+
+	/* action of type FILTER_XXX */
+	u8 default_action;
+	u8 pad[2];
+} __packed;
+
+
+struct acx_rx_data_filter_cfg {
+	struct acx_header header;
+
+	u8 enable;
+
+	/* range 0 - MAX_DATA_FILTERS */
+	u8 index;
+
+	u8 action;
+
+	u8 num_fields;
+
+	struct wl12xx_rx_data_filter_field fields[0];
+} __packed;
+
 enum {
 	ACX_WAKE_UP_CONDITIONS           = 0x0000,
 	ACX_MEM_CFG                      = 0x0001,
@@ -1310,5 +1336,8 @@ int wl1271_acx_set_inconnection_sta(struct wl1271 *wl, u8 *addr);
 int wl1271_acx_fm_coex(struct wl1271 *wl);
 int wl12xx_acx_set_rate_mgmt_params(struct wl1271 *wl);
 int wl12xx_acx_config_hangover(struct wl1271 *wl);
-
+int wl1271_acx_toggle_rx_data_filter(struct wl1271 *wl, bool enable,
+				     u8 default_action);
+int wl1271_acx_set_rx_data_filter(struct wl1271 *wl, u8 index, bool enable,
+				  struct wl12xx_rx_data_filter *filter);
 #endif /* __WL1271_ACX_H__ */
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index 1463341..c18ad0a 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -277,6 +277,39 @@ struct wl1271_link {
 	u8 ba_bitmap;
 };
 
+#define WL1271_MAX_RX_DATA_FILTERS 4
+#define WL1271_RX_DATA_FILTER_MAX_FIELD_PATTERNS 8
+
+/* FW MAX FILTER SIZE is 98 bytes. The MAX_PATTERN_SIZE is imposed
+ * after taking into account the mask bytes and other structs members
+ */
+#define WL1271_RX_DATA_FILTER_MAX_PATTERN_SIZE 43
+#define WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE 14
+
+#define WL1271_RX_DATA_FILTER_FLAG_MASK                BIT(0)
+#define WL1271_RX_DATA_FILTER_FLAG_IP_HEADER           0
+#define WL1271_RX_DATA_FILTER_FLAG_ETHERNET_HEADER     BIT(1)
+
+enum rx_data_filter_action {
+	FILTER_DROP = 0,
+	FILTER_SIGNAL = 1,
+	FILTER_FW_HANDLE = 2
+};
+
+struct wl12xx_rx_data_filter_field {
+	__le16 offset;
+	u8 len;
+	u8 flags;
+	u8 pattern[0];
+} __packed;
+
+struct wl12xx_rx_data_filter {
+	u8 action;
+	int num_fields;
+	int fields_size;
+	struct wl12xx_rx_data_filter_field fields[0];
+} __packed;
+
 struct wl1271 {
 	struct ieee80211_hw *hw;
 	bool mac80211_registered;
-- 
1.7.6.401.g6a319

Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Luciano Coelho]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> From: Eyal Shapira <eyal@wizery.com>
> 
> (based on Pontus' patch)
> 
> Added commands for setting a specific filter
> and controlling the behaviour RX data filters
> implemented by the FW.
> 
> Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
> Signed-off-by: Ido Reis <idor@ti.com>
> Signed-off-by: Eyal Shapira <eyal@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

Why is Ido's sign-off here? (just curious)


> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
> index bc96db0..668d337 100644
> --- a/drivers/net/wireless/wl12xx/acx.c
> +++ b/drivers/net/wireless/wl12xx/acx.c
> @@ -1740,3 +1740,108 @@ out:
>  	return ret;
>  
>  }
> +
> +int wl1271_acx_toggle_rx_data_filter(struct wl1271 *wl, bool enable,
> +				     u8 default_action)

Instead of "toggle" we usually have "enable" in the function name.  This
is not very important, obviously, but if we get a v3 of this patch, you
could change it for a little more consistency. ;)


> {
> +	struct acx_rx_data_filter_state *acx;
> +	int ret;
> +
> +	wl1271_debug(DEBUG_ACX, "acx toggle rx data filter en: %d act: %d",
> +		     enable, default_action);
> +
> +	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
> +	if (!acx) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	acx->enable = enable ? 1 : 0;

Do we really need this? We usually trust that enable will be either true
(1) or false (0).


> +int wl1271_acx_set_rx_data_filter(struct wl1271 *wl, u8 index, bool enable,
> +				  struct wl12xx_rx_data_filter *filter)
> +{
> +	struct acx_rx_data_filter_cfg *acx;
> +	int fields_size = 0;
> +	int acx_size;
> +	int ret;
> +
> +	if (enable && !filter) {
> +		wl1271_warning("acx_set_rx_data_filter: enable but no filter");
> +		return -EINVAL;
> +	}
> +
> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
> +			       index);
> +		return -EINVAL;
> +	}

Should we use BUG_ON instead? This is only used internally in the
driver, so if it get here, it's a bug.  And if the filters come from
userspace, we should validate them before continuing anyway.


> +	if (filter) {
> +		if (filter->action < FILTER_DROP ||
> +		    filter->action > FILTER_FW_HANDLE) {
> +			wl1271_warning("invalid filter action (%d)",
> +				       filter->action);
> +			return -EINVAL;
> +		}
> +
> +		if (filter->num_fields != 1 &&
> +		    filter->num_fields != 2) {
> +			wl1271_warning("invalid filter num_fields (%d)",
> +				       filter->num_fields);
> +			return -EINVAL;
> +		}

Same for these two.



> +	acx_size = roundup(sizeof(*acx) + fields_size, 4);

ALIGN()?


> +	acx = kzalloc(acx_size, GFP_KERNEL);
> +
> +	if (!acx)
> +		return -ENOMEM;
> +
> +	acx->enable = enable ? 1 : 0;

Same as above.


> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
> index 1463341..c18ad0a 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
> @@ -277,6 +277,39 @@ struct wl1271_link {
>  	u8 ba_bitmap;
>  };
>  
> +#define WL1271_MAX_RX_DATA_FILTERS 4
> +#define WL1271_RX_DATA_FILTER_MAX_FIELD_PATTERNS 8

This is too long for a macro?


> +/* FW MAX FILTER SIZE is 98 bytes. The MAX_PATTERN_SIZE is imposed
> + * after taking into account the mask bytes and other structs members
> + */

This is slightly off according to the coding-style. ;)

Also s/structs/struct/


> +#define WL1271_RX_DATA_FILTER_MAX_PATTERN_SIZE 43
> +#define WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE 14
> +
> +#define WL1271_RX_DATA_FILTER_FLAG_MASK                BIT(0)
> +#define WL1271_RX_DATA_FILTER_FLAG_IP_HEADER           0
> +#define WL1271_RX_DATA_FILTER_FLAG_ETHERNET_HEADER     BIT(1)

It would also be nice to find some smaller names for these.


> +struct wl12xx_rx_data_filter_field {
> +	__le16 offset;
> +	u8 len;
> +	u8 flags;
> +	u8 pattern[0];
> +} __packed;
> +
> +struct wl12xx_rx_data_filter {
> +	u8 action;
> +	int num_fields;
> +	int fields_size;
> +	struct wl12xx_rx_data_filter_field fields[0];
> +} __packed;

No need for __packed here.  These are internal structs and not part of
the FW API.


-- 
Cheers,
Luca.

Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Kalle Valo]

Luciano Coelho <coelho@ti.com> writes:

>> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
>> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
>> +			       index);
>> +		return -EINVAL;
>> +	}
>
> Should we use BUG_ON instead? This is only used internally in the
> driver, so if it get here, it's a bug.  And if the filters come from
> userspace, we should validate them before continuing anyway.

BUG_ON() is evil and wireless drivers should really not use it,
WARN_ON_ONCE() and return with an error is much more user friendly.

-- 
Kalle Valo

Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Luciano Coelho]

On Mon, 2012-02-06 at 16:07 +0200, Kalle Valo wrote: 
> Luciano Coelho <coelho@ti.com> writes:
> 
> >> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
> >> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
> >> +			       index);
> >> +		return -EINVAL;
> >> +	}
> >
> > Should we use BUG_ON instead? This is only used internally in the
> > driver, so if it get here, it's a bug.  And if the filters come from
> > userspace, we should validate them before continuing anyway.
> 
> BUG_ON() is evil and wireless drivers should really not use it,
> WARN_ON_ONCE() and return with an error is much more user friendly.

Yeah, BUG_ON() is evil, but sometimes it can be good to have.  I don't
agree with "wireless drivers should really not use it".  There are 181
occurrences in wireless-testing/master right now[1].  I'm not saying
this measure is either accurate or an excuse to use it where we
shouldn't, but it shows that it really has widespread usage in wireless
drivers.

My point was that this can only happen very early, when the driver is
being initialized.  But this is not exactly this case, it only happens
when we suspend or resume, so you're right here.

Maybe in this case we should check with a BUILD_BUG_ON somehow.

And actually, I nacked this part of the patchset, they definitely need
to be reworked.  Eliad already sent v2 of the first 4, leaving 5/6/7
out.

In any case, thanks a lot for your comment, Kalle! :)


[1]
luca@cumari:~/kernel/wl12xx$ find drivers/net/wireless/ -name '*.[ch]' -exec grep -e '\<BUG_ON\>' {} \;|wc -l
181


-- 
Cheers,
Luca.

Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Kalle Valo]

Luciano Coelho <coelho@ti.com> writes:

> On Mon, 2012-02-06 at 16:07 +0200, Kalle Valo wrote: 
>> Luciano Coelho <coelho@ti.com> writes:
>> 
>> >> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
>> >> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
>> >> +			       index);
>> >> +		return -EINVAL;
>> >> +	}
>> >
>> > Should we use BUG_ON instead? This is only used internally in the
>> > driver, so if it get here, it's a bug.  And if the filters come from
>> > userspace, we should validate them before continuing anyway.
>> 
>> BUG_ON() is evil and wireless drivers should really not use it,
>> WARN_ON_ONCE() and return with an error is much more user friendly.
>
> Yeah, BUG_ON() is evil, but sometimes it can be good to have.

What good does BUG_ON() bring compared to WARN_ON_ONCE()? For example,
does BUG_ON() even get logged to disk? Most likely not, so
WARN_ON_ONCE() is much easier to report by the users.

> I don't agree with "wireless drivers should really not use it". There
> are 181 occurrences in wireless-testing/master right now[1].

Then there are 181 misuses of BUG_ON() ;)

> I'm not saying this measure is either accurate or an excuse to use it
> where we shouldn't, but it shows that it really has widespread usage
> in wireless drivers.

Yes, and that's why I have started a war against misuse of BUG_ON() :)

-- 
Kalle Valo

Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Luciano Coelho]

On Tue, 2012-02-07 at 18:05 +0200, Kalle Valo wrote: 
> Luciano Coelho <coelho@ti.com> writes:
> 
> > On Mon, 2012-02-06 at 16:07 +0200, Kalle Valo wrote: 
> >> Luciano Coelho <coelho@ti.com> writes:
> >> 
> >> >> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
> >> >> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
> >> >> +			       index);
> >> >> +		return -EINVAL;
> >> >> +	}
> >> >
> >> > Should we use BUG_ON instead? This is only used internally in the
> >> > driver, so if it get here, it's a bug.  And if the filters come from
> >> > userspace, we should validate them before continuing anyway.
> >> 
> >> BUG_ON() is evil and wireless drivers should really not use it,
> >> WARN_ON_ONCE() and return with an error is much more user friendly.
> >
> > Yeah, BUG_ON() is evil, but sometimes it can be good to have.
> 
> What good does BUG_ON() bring compared to WARN_ON_ONCE()? For example,
> does BUG_ON() even get logged to disk? Most likely not, so
> WARN_ON_ONCE() is much easier to report by the users.
> 
> > I don't agree with "wireless drivers should really not use it". There
> > are 181 occurrences in wireless-testing/master right now[1].
> 
> Then there are 181 misuses of BUG_ON() ;)

:D


> > I'm not saying this measure is either accurate or an excuse to use it
> > where we shouldn't, but it shows that it really has widespread usage
> > in wireless drivers.
> 
> Yes, and that's why I have started a war against misuse of BUG_ON() :)

Okay, I will definitely not be in the frontline against you in this
war. :)


-- 
Cheers,
Luca.

[PATCH v2 6/7] wl12xx: add RX data filters management functions

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

(based on Pontus' patch)

More prep work for supporting RX data filters
in FW. These functions use a driver saved state
of the enabled filters to prevent actions (enable/disable)
which don't match the FW status.

Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
Signed-off-by: Ido Reis <idor@ti.com>
Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/rx.c     |   61 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/rx.h     |    8 ++++-
 drivers/net/wireless/wl12xx/wl12xx.h |    3 ++
 3 files changed, 71 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/rx.c b/drivers/net/wireless/wl12xx/rx.c
index 4fbd2a7..d025db6 100644
--- a/drivers/net/wireless/wl12xx/rx.c
+++ b/drivers/net/wireless/wl12xx/rx.c
@@ -282,3 +282,64 @@ void wl12xx_rx(struct wl1271 *wl, struct wl12xx_fw_status *status)
 
 	wl12xx_rearm_rx_streaming(wl, active_hlids);
 }
+
+/*
+ * Global on / off for RX packet filtering in firmware
+ */
+int wl1271_rx_data_filtering_enable(struct wl1271 *wl, bool enable,
+				    enum rx_data_filter_action policy)
+{
+	int ret;
+
+	if (policy < FILTER_DROP || policy > FILTER_FW_HANDLE) {
+		wl1271_warning("filter policy value is not in valid range");
+		return -ERANGE;
+	}
+
+	if (enable < 0 || enable > 1) {
+		wl1271_warning("filter enable value is not in valid range");
+		return -ERANGE;
+	}
+
+	ret = wl1271_acx_toggle_rx_data_filter(wl, enable, policy);
+
+	return ret;
+}
+
+int wl1271_rx_data_filter_enable(struct wl1271 *wl,
+				 int index,
+				 bool enable,
+				 struct wl12xx_rx_data_filter *filter)
+{
+	int ret;
+
+	if (wl->rx_data_filters_status[index] == enable) {
+		wl1271_debug(DEBUG_ACX, "Request to enable an already "
+			     "enabled rx filter %d", index);
+		return 0;
+	}
+
+	ret = wl1271_acx_set_rx_data_filter(wl, index, enable, filter);
+
+	if (ret) {
+		wl1271_error("Failed to %s rx data filter %d (err=%d)",
+			     enable ? "enable" : "disable", index, ret);
+		return ret;
+	}
+
+	wl->rx_data_filters_status[index] = enable;
+
+	return 0;
+}
+
+/* Unset any active filters */
+void wl1271_rx_data_filters_clear_all(struct wl1271 *wl)
+{
+	int i;
+
+	for (i = 0; i < WL1271_MAX_RX_DATA_FILTERS; i++) {
+		if (!wl->rx_data_filters_status[i])
+			continue;
+		wl1271_rx_data_filter_enable(wl, i, 0, NULL);
+	}
+}
diff --git a/drivers/net/wireless/wl12xx/rx.h b/drivers/net/wireless/wl12xx/rx.h
index 86ba6b1..b4db4014 100644
--- a/drivers/net/wireless/wl12xx/rx.h
+++ b/drivers/net/wireless/wl12xx/rx.h
@@ -128,5 +128,11 @@ struct wl1271_rx_descriptor {
 
 void wl12xx_rx(struct wl1271 *wl, struct wl12xx_fw_status *status);
 u8 wl1271_rate_to_idx(int rate, enum ieee80211_band band);
-
+int wl1271_rx_data_filtering_enable(struct wl1271 *wl, bool enable,
+				    enum rx_data_filter_action policy);
+int wl1271_rx_data_filter_enable(struct wl1271 *wl,
+				 int index,
+				 bool enable,
+				 struct wl12xx_rx_data_filter *filter);
+void wl1271_rx_data_filters_clear_all(struct wl1271 *wl);
 #endif
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index c18ad0a..720ea82 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -509,6 +509,9 @@ struct wl1271 {
 
 	/* last wlvif we transmitted from */
 	struct wl12xx_vif *last_wlvif;
+
+	/* RX Data filter rule status - enabled/disabled */
+	bool rx_data_filters_status[WL1271_MAX_RX_DATA_FILTERS];
 };
 
 struct wl1271_station {
-- 
1.7.6.401.g6a319

Re: [PATCH v2 6/7] wl12xx: add RX data filters management functions

[Luciano Coelho]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> From: Eyal Shapira <eyal@wizery.com>
> 
> (based on Pontus' patch)
> 
> More prep work for supporting RX data filters
> in FW. These functions use a driver saved state
> of the enabled filters to prevent actions (enable/disable)
> which don't match the FW status.
> 
> Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
> Signed-off-by: Ido Reis <idor@ti.com>
> Signed-off-by: Eyal Shapira <eyal@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

[...]


> diff --git a/drivers/net/wireless/wl12xx/rx.c b/drivers/net/wireless/wl12xx/rx.c
> index 4fbd2a7..d025db6 100644
> --- a/drivers/net/wireless/wl12xx/rx.c
> +++ b/drivers/net/wireless/wl12xx/rx.c
> @@ -282,3 +282,64 @@ void wl12xx_rx(struct wl1271 *wl, struct wl12xx_fw_status *status)
>  
>  	wl12xx_rearm_rx_streaming(wl, active_hlids);
>  }
> +
> +/*
> + * Global on / off for RX packet filtering in firmware
> + */
> +int wl1271_rx_data_filtering_enable(struct wl1271 *wl, bool enable,
> +				    enum rx_data_filter_action policy)
> +{
> +	int ret;
> +
> +	if (policy < FILTER_DROP || policy > FILTER_FW_HANDLE) {
> +		wl1271_warning("filter policy value is not in valid range");
> +		return -ERANGE;
> +	}

Again, this is only called internally, so we either use a BUG_ON or we
don't check the values here at all.


> +	if (enable < 0 || enable > 1) {
> +		wl1271_warning("filter enable value is not in valid range");
> +		return -ERANGE;
> +	}

No need to check the validity of the bool here.


> +	ret = wl1271_acx_toggle_rx_data_filter(wl, enable, policy);

If you want to be really sure the boolean is really a boolean, you
should use !!enable here, though I don't think it's necessary.


> +int wl1271_rx_data_filter_enable(struct wl1271 *wl,
> +				 int index,
> +				 bool enable,
> +				 struct wl12xx_rx_data_filter *filter)
> +{
> +	int ret;
> +
> +	if (wl->rx_data_filters_status[index] == enable) {
> +		wl1271_debug(DEBUG_ACX, "Request to enable an already "
> +			     "enabled rx filter %d", index);
> +		return 0;
> +	}

Maybe DEBUG_RX? Or something else? It's definitely not DEBUG_ACX,
though.


> +void wl1271_rx_data_filters_clear_all(struct wl1271 *wl);
>  #endif
> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
> index c18ad0a..720ea82 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
> @@ -509,6 +509,9 @@ struct wl1271 {
>  
>  	/* last wlvif we transmitted from */
>  	struct wl12xx_vif *last_wlvif;
> +
> +	/* RX Data filter rule status - enabled/disabled */
> +	bool rx_data_filters_status[WL1271_MAX_RX_DATA_FILTERS];

Maybe s/_status/_enabled/? Also I think for all the code we should
s/rx_data_filter/rx_filter/ so we can reduce the length of the symbols.

-- 
Cheers,
Luca.

[PATCH v2 7/7] wl12xx: support wowlan wakeup patterns

[Eliad Peller]

From: Eyal Shapira <eyal@wizery.com>

(based on Pontus' patch)

Use FW RX data filters to support cfg80211 wowlan wakeup patterns.
This enables to wake up the host from suspend following detection
of certain configurable patters within an incoming packet.
Up to 4 patterns are supported. Once the host is resumed
any configured RX data filter is cleared.
A single pattern can match several bytes sequences with different
offsets within a packet.

Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
Signed-off-by: Ido Reis <idor@ti.com>
Signed-off-by: Eyal Shapira <eyal@wizery.com>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |  200 ++++++++++++++++++++++++++++++++++--
 1 files changed, 193 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index f2960df..27c064b 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1564,8 +1564,180 @@ static struct notifier_block wl1271_dev_notifier = {
 };
 
 #ifdef CONFIG_PM
+int wl1271_validate_wowlan_pattern(struct cfg80211_wowlan_trig_pkt_pattern *p)
+{
+	if (p->pattern_len > WL1271_RX_DATA_FILTER_MAX_PATTERN_SIZE) {
+		wl1271_warning("WoWLAN pattern too big");
+		return -E2BIG;
+	}
+
+	if (!p->mask) {
+		wl1271_warning("No mask in WoWLAN pattern");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int wl1271_build_rx_filter_field(struct wl12xx_rx_data_filter_field *field,
+				 u8 *pattern, u8 len, u8 offset,
+				 u8 *bitmask, u8 flags)
+{
+	u8 *mask;
+	int i;
+
+	field->flags = flags | WL1271_RX_DATA_FILTER_FLAG_MASK;
+
+	/* Not using the capability to set offset within an RX filter field.
+	 * The offset param is used to access pattern and bitmask.
+	 */
+	field->offset = 0;
+	field->len = len;
+	memcpy(field->pattern, &pattern[offset], len);
+
+	/* Translate the WowLAN bitmask (in bits) to the FW RX filter field
+	   mask which is in bytes */
+
+	mask = field->pattern + len;
+
+	for (i = offset; i < (offset+len); i++) {
+		if (test_bit(i, (unsigned long *)bitmask))
+			*mask = 0xFF;
+
+		mask++;
+	}
+
+	return sizeof(*field) + len + (bitmask ? len : 0);
+}
+
+/* Allocates an RX filter returned through f
+   which needs to be freed using kfree() */
+int wl1271_convert_wowlan_pattern_to_rx_filter(
+	struct cfg80211_wowlan_trig_pkt_pattern *p,
+	struct wl12xx_rx_data_filter **f)
+{
+	int filter_size, num_fields, fields_size;
+	int first_field_size;
+	int ret = 0;
+	struct wl12xx_rx_data_filter_field *field;
+	struct wl12xx_rx_data_filter *filter;
+
+	/* If pattern is longer then the ETHERNET header we split it into
+	 * 2 fields in the rx filter as you can't have a single
+	 * field across ETH header boundary. The first field will filter
+	 * anything in the ETH header and the 2nd one from the IP header.
+	 * Each field will contain pattern bytes and mask bytes
+	 */
+	if (p->pattern_len > WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE)
+		num_fields = 2;
+	else
+		num_fields = 1;
+
+	fields_size = (sizeof(*field) * num_fields) + (2 * p->pattern_len);
+	filter_size = sizeof(*filter) + fields_size;
+
+	filter = kzalloc(filter_size, GFP_KERNEL);
+	if (!filter) {
+		wl1271_warning("Failed to alloc rx filter");
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	field = &filter->fields[0];
+	first_field_size = wl1271_build_rx_filter_field(field,
+			p->pattern,
+			min(p->pattern_len,
+			    WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE),
+			0,
+			p->mask,
+			WL1271_RX_DATA_FILTER_FLAG_ETHERNET_HEADER);
+
+	field = (struct wl12xx_rx_data_filter_field *)
+		  (((u8 *)filter->fields) + first_field_size);
+
+	if (num_fields > 1) {
+		wl1271_build_rx_filter_field(field,
+		     p->pattern,
+		     p->pattern_len - WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE,
+		     WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE,
+		     p->mask,
+		     WL1271_RX_DATA_FILTER_FLAG_IP_HEADER);
+	}
+
+	filter->action = FILTER_SIGNAL;
+	filter->num_fields = num_fields;
+	filter->fields_size = fields_size;
+
+	*f = filter;
+	return 0;
+
+err:
+	kfree(filter);
+	*f = NULL;
+
+	return ret;
+}
+
+static int wl1271_configure_wowlan(struct wl1271 *wl,
+				   struct cfg80211_wowlan *wow)
+{
+	int i, ret;
+
+	if (!wow) {
+		wl1271_rx_data_filtering_enable(wl, 0, FILTER_SIGNAL);
+		return 0;
+	}
+
+	WARN_ON(wow->n_patterns > WL1271_MAX_RX_DATA_FILTERS);
+	if (wow->any || !wow->n_patterns)
+		return 0;
+
+	wl1271_rx_data_filters_clear_all(wl);
+
+	/* Translate WoWLAN patterns into filters */
+	for (i = 0; i < wow->n_patterns; i++) {
+		struct cfg80211_wowlan_trig_pkt_pattern *p;
+		struct wl12xx_rx_data_filter *filter = NULL;
+
+		p = &wow->patterns[i];
+
+		ret = wl1271_validate_wowlan_pattern(p);
+		if (ret) {
+			wl1271_warning("validate_wowlan_pattern "
+				       "failed (%d)", ret);
+			goto out;
+		}
+
+		ret = wl1271_convert_wowlan_pattern_to_rx_filter(p, &filter);
+		if (ret) {
+			wl1271_warning("convert_wowlan_pattern_to_rx_filter "
+				       "failed (%d)", ret);
+			goto out;
+		}
+
+		ret = wl1271_rx_data_filter_enable(wl, i, 1, filter);
+
+		kfree(filter);
+		if (ret) {
+			wl1271_warning("rx_data_filter_enable "
+				       " failed (%d)", ret);
+			goto out;
+		}
+	}
+
+	ret = wl1271_rx_data_filtering_enable(wl, 1, FILTER_DROP);
+	if (ret) {
+		wl1271_warning("rx_data_filtering_enable failed (%d)", ret);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
 static int wl1271_configure_suspend_sta(struct wl1271 *wl,
-					struct wl12xx_vif *wlvif)
+					struct wl12xx_vif *wlvif,
+					struct cfg80211_wowlan *wow)
 {
 	int ret = 0;
 
@@ -1578,6 +1750,10 @@ static int wl1271_configure_suspend_sta(struct wl1271 *wl,
 	if (ret < 0)
 		goto out_unlock;
 
+	ret = wl1271_configure_wowlan(wl, wow);
+	if (ret < 0)
+		wl1271_error("suspend: Could not configure WoWLAN: %d", ret);
+
 	ret = wl1271_acx_wake_up_conditions(wl, wlvif,
 				    wl->conf.conn.suspend_wake_up_event,
 				    wl->conf.conn.suspend_listen_interval);
@@ -1618,10 +1794,11 @@ out_unlock:
 }
 
 static int wl1271_configure_suspend(struct wl1271 *wl,
-				    struct wl12xx_vif *wlvif)
+				    struct wl12xx_vif *wlvif,
+					struct cfg80211_wowlan *wow)
 {
 	if (wlvif->bss_type == BSS_TYPE_STA_BSS)
-		return wl1271_configure_suspend_sta(wl, wlvif);
+		return wl1271_configure_suspend_sta(wl, wlvif, wow);
 	if (wlvif->bss_type == BSS_TYPE_AP_BSS)
 		return wl1271_configure_suspend_ap(wl, wlvif);
 	return 0;
@@ -1643,6 +1820,9 @@ static void wl1271_configure_resume(struct wl1271 *wl,
 		goto out;
 
 	if (is_sta) {
+		/* Remove WoWLAN filtering */
+		wl1271_configure_wowlan(wl, NULL);
+
 		ret = wl1271_acx_wake_up_conditions(wl, wlvif,
 				    wl->conf.conn.wake_up_event,
 				    wl->conf.conn.listen_interval);
@@ -1668,11 +1848,11 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
 	int ret;
 
 	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
-	WARN_ON(!wow || !wow->any);
+	WARN_ON(!wow);
 
 	wl->wow_enabled = true;
 	wl12xx_for_each_wlvif(wl, wlvif) {
-		ret = wl1271_configure_suspend(wl, wlvif);
+		ret = wl1271_configure_suspend(wl, wlvif, wow);
 		if (ret < 0) {
 			wl1271_warning("couldn't prepare device to suspend");
 			return ret;
@@ -1734,7 +1914,7 @@ static int wl1271_op_resume(struct ieee80211_hw *hw)
 
 	return 0;
 }
-#endif
+#endif /* CONFIG_PM */
 
 static int wl1271_op_start(struct ieee80211_hw *hw)
 {
@@ -5172,8 +5352,14 @@ static int __devinit wl12xx_probe(struct platform_device *pdev)
 	if (!ret) {
 		wl->irq_wake_enabled = true;
 		device_init_wakeup(wl->dev, 1);
-		if (pdata->pwr_in_suspend)
+		if (pdata->pwr_in_suspend) {
 			hw->wiphy->wowlan.flags = WIPHY_WOWLAN_ANY;
+			hw->wiphy->wowlan.n_patterns =
+				WL1271_MAX_RX_DATA_FILTERS;
+			hw->wiphy->wowlan.pattern_min_len = 1;
+			hw->wiphy->wowlan.pattern_max_len =
+				WL1271_RX_DATA_FILTER_MAX_PATTERN_SIZE;
+		}
 
 	}
 	disable_irq(wl->irq);
-- 
1.7.6.401.g6a319

Re: [PATCH v2 0/7] wl12xx: add some psm/suspend features

[Luciano Coelho]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> Add some psm / suspend features:
> * Add the ability to force psm (by default, the fw uses dynamic ps)
> * Use differnet wake up conditions on suspend
> * Add pattern support to wowlan triggers
> 
> (depends on the "wl12xx: update fw api" patchset)
> 
> v2: the rx filters patches were squashed and rewritten (thanks Eyal!)
> 
> Eyal Shapira (7):
>   wl12xx: Set different wake up conditions in case of suspend
>   wl12xx: add suspend_listen_interval debugfs file
>   wl12xx: add forced_ps mode
>   wl12xx: add forced_ps debugfs file
>   wl12xx: add RX data filter ACX commands
>   wl12xx: add RX data filters management functions
>   wl12xx: support wowlan wakeup patterns
> 
>  drivers/net/wireless/wl12xx/acx.c     |  115 +++++++++++++-
>  drivers/net/wireless/wl12xx/acx.h     |   34 ++++-
>  drivers/net/wireless/wl12xx/conf.h    |   19 +++
>  drivers/net/wireless/wl12xx/debugfs.c |  131 ++++++++++++++++-
>  drivers/net/wireless/wl12xx/main.c    |  272 +++++++++++++++++++++++++++++++--
>  drivers/net/wireless/wl12xx/ps.c      |   12 +-
>  drivers/net/wireless/wl12xx/rx.c      |   61 ++++++++
>  drivers/net/wireless/wl12xx/rx.h      |    8 +-
>  drivers/net/wireless/wl12xx/wl12xx.h  |   38 +++++-
>  9 files changed, 662 insertions(+), 28 deletions(-)

Eliad, could you split this patchset in two? I can take the first part,
about forced PS when you send v2.

The WoWLAN part needs to be reworked.  Let's split it out so that it
doesn't hold the forced PS back.


-- 
Cheers,
Luca.

Re: [PATCH v2 0/7] wl12xx: add some psm/suspend features

[Eliad Peller]

On Thu, Feb 2, 2012 at 11:31 AM, Luciano Coelho <coelho@ti.com> wrote:
> On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote:
>> Add some psm / suspend features:
>> * Add the ability to force psm (by default, the fw uses dynamic ps)
>> * Use differnet wake up conditions on suspend
>> * Add pattern support to wowlan triggers
>>
>> (depends on the "wl12xx: update fw api" patchset)
>>
>> v2: the rx filters patches were squashed and rewritten (thanks Eyal!)
>>
>> Eyal Shapira (7):
>>   wl12xx: Set different wake up conditions in case of suspend
>>   wl12xx: add suspend_listen_interval debugfs file
>>   wl12xx: add forced_ps mode
>>   wl12xx: add forced_ps debugfs file
>>   wl12xx: add RX data filter ACX commands
>>   wl12xx: add RX data filters management functions
>>   wl12xx: support wowlan wakeup patterns
>>
>
> Eliad, could you split this patchset in two? I can take the first part,
> about forced PS when you send v2.
>
> The WoWLAN part needs to be reworked.  Let's split it out so that it
> doesn't hold the forced PS back.
>
sure.
i'll send it soon.

Eliad.