[PATCH 0/2] wl12xx: BA Initiator & receiver support

[PATCH 0/2] wl12xx: BA Initiator & receiver support

[Shahar Levi]

That series gather two patches:
[PATCH v5] wl12xx: BA initiator support
[PATCH v3] wl12xx: BA receiver support

Add 80211n BA initiator & receiver session support wl1271 driver.
BA initiator session management included in FW independently.
Include supported FW version auto detection mechanism.

Next step is to implement HW flag that in case of wl12xx driver indicates
  mac80211 to use BA win size of 8.

Shahar Levi (2):
  wl12xx: BA initiator support
  wl12xx: BA receiver support

 drivers/net/wireless/wl12xx/acx.c    |   86 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/acx.h    |   62 +++++++++++++++++++++++-
 drivers/net/wireless/wl12xx/boot.c   |   41 +++++++++++++++-
 drivers/net/wireless/wl12xx/conf.h   |    7 +++
 drivers/net/wireless/wl12xx/init.c   |   50 ++++++++++++++++++++
 drivers/net/wireless/wl12xx/main.c   |   73 +++++++++++++++++++++++++++--
 drivers/net/wireless/wl12xx/wl12xx.h |   15 ++++++-
 7 files changed, 323 insertions(+), 11 deletions(-)

[PATCH v5 1/2] wl12xx: BA initiator support

[Shahar Levi]

Add 80211n BA initiator session support wl1271 driver.
Include BA supported FW version auto detection mechanism.
BA initiator session management included in FW independently.

Signed-off-by: Shahar Levi <shahar_levi@ti.com>
---
Limitation:
- For now only one BA per direction is supported

Changes from v1:
- Remove wl1271_op_ampdu_action()
- set CONF_BA_INACTIVITY_TIMEOUT as configurable value
- Clean to Linux code style.

Changes from v2:
- Two new ACX commands ACX_BA_SESSION_POLICY_CFG & ACX_BA_SESSION_RX_SETUP
- New struct wl1271_acx_ba_session_policy
- Calling wl1271_set_ba_policies() once in init
- Use ieee80211_back_parties
- Rebase to latest

Changes from v3:
- New FW file name due to new FW API

Changes from v4:
- Reverse change of "New FW file name due to new FW API"
- Add supported FW version auto detection mechanism (Parsing fw version once and
  use it to validate BA support)

 drivers/net/wireless/wl12xx/acx.c    |   50 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/acx.h    |   41 ++++++++++++++++++++++++++-
 drivers/net/wireless/wl12xx/boot.c   |   41 +++++++++++++++++++++++++--
 drivers/net/wireless/wl12xx/conf.h   |    7 +++++
 drivers/net/wireless/wl12xx/init.c   |   50 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/main.c   |   14 ++++++---
 drivers/net/wireless/wl12xx/wl12xx.h |   15 +++++++++-
 7 files changed, 207 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index cc4068d..54fd68d 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -1309,6 +1309,56 @@ out:
 	return ret;
 }
 
+/* Configure BA session initiator\receiver parameters setting in the FW. */
+int wl1271_acx_set_ba_session(struct wl1271 *wl,
+			      enum ieee80211_back_parties direction,
+			      u8 tid_index, u8 policy)
+{
+	struct wl1271_acx_ba_session_policy *acx;
+	int ret;
+
+	wl1271_debug(DEBUG_ACX, "acx ba session setting");
+
+	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+	if (!acx) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* ANY role */
+	acx->role_id = 0xff;
+	acx->tid = tid_index;
+	acx->enable = policy;
+	acx->win_size = BA_WIN_SIZE;
+	acx->ba_direction = direction;
+
+	switch (direction) {
+	case WLAN_BACK_INITIATOR:
+		acx->inactivity_timeout = wl->conf.ht.inactivity_timeout;
+		break;
+	case WLAN_BACK_RECIPIENT:
+		acx->inactivity_timeout = 0;
+		break;
+	default:
+		wl1271_error("Incorrect acx command id=%x\n", direction);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = wl1271_cmd_configure(wl,
+				   ACX_BA_SESSION_POLICY_CFG,
+				   acx,
+				   sizeof(*acx));
+	if (ret < 0) {
+		wl1271_warning("acx ba session setting failed: %d", ret);
+		goto out;
+	}
+
+out:
+	kfree(acx);
+	return ret;
+}
+
 int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime)
 {
 	struct wl1271_acx_fw_tsf_information *tsf_info;
diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
index 9cbc3f4..df48468 100644
--- a/drivers/net/wireless/wl12xx/acx.h
+++ b/drivers/net/wireless/wl12xx/acx.h
@@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
 	u8 padding[3];
 } __packed;
 
+#define BA_WIN_SIZE 8
+
+struct wl1271_acx_ba_session_policy {
+	struct acx_header header;
+	/*
+	 * Specifies role Id, Range 0-7, 0xFF means ANY role.
+	 * Future use. For now this field is irrelevant
+	 */
+	u8 role_id;
+	/*
+	 * Specifies Link Id, Range 0-31, 0xFF means ANY  Link Id.
+	 * Not applicable if Role Id is set to ANY.
+	 */
+	u8 link_id;
+
+	u8 tid;
+
+	u8 enable;
+
+	/* Windows size in number of packets */
+	u16 win_size;
+
+	/*
+	 * As initiator inactivity timeout in time units(TU) of 1024us.
+	 * As receiver reserved
+	 */
+	u16 inactivity_timeout;
+
+	/* Initiator = 1/Receiver = 0 */
+	u8 ba_direction;
+
+	u8 padding[3];
+} __packed;
+
 struct wl1271_acx_fw_tsf_information {
 	struct acx_header header;
 
@@ -1113,8 +1147,8 @@ enum {
 	ACX_RSSI_SNR_WEIGHTS        = 0x0052,
 	ACX_KEEP_ALIVE_MODE         = 0x0053,
 	ACX_SET_KEEP_ALIVE_CONFIG   = 0x0054,
-	ACX_BA_SESSION_RESPONDER_POLICY = 0x0055,
-	ACX_BA_SESSION_INITIATOR_POLICY = 0x0056,
+	ACX_BA_SESSION_POLICY_CFG   = 0x0055,
+	ACX_BA_SESSION_RX_SETUP     = 0x0056,
 	ACX_PEER_HT_CAP             = 0x0057,
 	ACX_HT_BSS_OPERATION        = 0x0058,
 	ACX_COEX_ACTIVITY           = 0x0059,
@@ -1185,6 +1219,9 @@ int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
 				    bool allow_ht_operation);
 int wl1271_acx_set_ht_information(struct wl1271 *wl,
 				   u16 ht_operation_mode);
+int wl1271_acx_set_ba_session(struct wl1271 *wl,
+			      enum ieee80211_back_parties direction,
+			      u8 tid_index, u8 policy);
 int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime);
 
 #endif /* __WL1271_ACX_H__ */
diff --git a/drivers/net/wireless/wl12xx/boot.c b/drivers/net/wireless/wl12xx/boot.c
index 4a9f929..cd42e12 100644
--- a/drivers/net/wireless/wl12xx/boot.c
+++ b/drivers/net/wireless/wl12xx/boot.c
@@ -101,6 +101,39 @@ static void wl1271_boot_set_ecpu_ctrl(struct wl1271 *wl, u32 flag)
 	wl1271_write32(wl, ACX_REG_ECPU_CONTROL, cpu_ctrl);
 }
 
+static void wl1271_save_fw_ver(struct wl1271 *wl)
+{
+	char fw_ver_str[ETHTOOL_BUSINFO_LEN];
+	char *fw_ver_point;
+	int ret, i;
+
+	/* copy the fw version to temp str */
+	strncpy(fw_ver_str, wl->chip.fw_ver_str, sizeof(fw_ver_str));
+
+	for (i = (WL12XX_NUM_FW_VER - 1); i > 0; --i) {
+		/* find the last '.' */
+		fw_ver_point = strrchr(fw_ver_str, '.');
+
+		/* read version number */
+		ret = strict_strtoul(fw_ver_point+1, 10, &(wl->chip.fw_ver[i]));
+		if (ret < 0) {
+			wl1271_warning("fw version incorrect value");
+			memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
+			return;
+		}
+
+		/* clean '.' */
+		*fw_ver_point = '\0';
+	}
+
+	ret = strict_strtoul(fw_ver_point-1, 10, &(wl->chip.fw_ver[0]));
+	if (ret < 0) {
+		wl1271_warning("fw version incorrect value");
+		memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
+		return;
+	}
+}
+
 static void wl1271_boot_fw_version(struct wl1271 *wl)
 {
 	struct wl1271_static_data static_data;
@@ -108,11 +141,13 @@ static void wl1271_boot_fw_version(struct wl1271 *wl)
 	wl1271_read(wl, wl->cmd_box_addr, &static_data, sizeof(static_data),
 		    false);
 
-	strncpy(wl->chip.fw_ver, static_data.fw_version,
-		sizeof(wl->chip.fw_ver));
+	strncpy(wl->chip.fw_ver_str, static_data.fw_version,
+		sizeof(wl->chip.fw_ver_str));
 
 	/* make sure the string is NULL-terminated */
-	wl->chip.fw_ver[sizeof(wl->chip.fw_ver) - 1] = '\0';
+	wl->chip.fw_ver_str[sizeof(wl->chip.fw_ver_str) - 1] = '\0';
+
+	wl1271_save_fw_ver(wl);
 }
 
 static int wl1271_boot_upload_firmware_chunk(struct wl1271 *wl, void *buf,
diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
index a16b361..41df771 100644
--- a/drivers/net/wireless/wl12xx/conf.h
+++ b/drivers/net/wireless/wl12xx/conf.h
@@ -1090,6 +1090,12 @@ struct conf_rf_settings {
 	u8 tx_per_channel_power_compensation_5[CONF_TX_PWR_COMPENSATION_LEN_5];
 };
 
+#define CONF_BA_INACTIVITY_TIMEOUT 10000
+
+struct conf_ht_setting {
+	u16 inactivity_timeout;
+};
+
 struct conf_drv_settings {
 	struct conf_sg_settings sg;
 	struct conf_rx_settings rx;
@@ -1100,6 +1106,7 @@ struct conf_drv_settings {
 	struct conf_roam_trigger_settings roam_trigger;
 	struct conf_scan_settings scan;
 	struct conf_rf_settings rf;
+	struct conf_ht_setting ht;
 };
 
 #endif
diff --git a/drivers/net/wireless/wl12xx/init.c b/drivers/net/wireless/wl12xx/init.c
index 785a530..5482b85 100644
--- a/drivers/net/wireless/wl12xx/init.c
+++ b/drivers/net/wireless/wl12xx/init.c
@@ -213,6 +213,51 @@ static int wl1271_init_beacon_broadcast(struct wl1271 *wl)
 	return 0;
 }
 
+static void wl1271_check_ba_support(struct wl1271 *wl)
+{
+
+	/* validate FW cose ver x.x.x.50-60.x */
+	if ((wl->chip.fw_ver[3] >= WL12XX_BA_SUPPORT_FW_COST_VER2_START) &&
+	    (wl->chip.fw_ver[3] < WL12XX_BA_SUPPORT_FW_COST_VER2_END)) {
+		wl->ba_support = true;
+		return;
+	}
+
+	/* validate FW sub ver x.x.x.>= 33.339 */
+	if ((wl->chip.fw_ver[4] == WL12XX_BA_SUPPORT_FW_SUB_VER) &&
+	    (wl->chip.fw_ver[4] >= WL12XX_BA_SUPPORT_FW_COST_VER1)) {
+		wl->ba_support = true;
+		return;
+	}
+
+	wl->ba_support = false;
+}
+
+static int wl1271_set_ba_policies(struct wl1271 *wl)
+{
+	u8 tid_index;
+	u8 ret = 0;
+
+	/* Reset the BA RX indicators */
+	wl->ba_allowed = true;
+	wl->ba_rx_bitmap = 0;
+
+	/* validate that FW support BA */
+	wl1271_check_ba_support(wl);
+
+	if (wl->ba_support)
+		/* 802.11n initiator BA session setting */
+		for (tid_index = 0; tid_index < CONF_TX_MAX_TID_COUNT;
+		      ++tid_index) {
+			ret = wl1271_acx_set_ba_session(wl, WLAN_BACK_INITIATOR,
+							tid_index, true);
+			if (ret < 0)
+				break;
+		}
+
+	return ret;
+}
+
 int wl1271_hw_init(struct wl1271 *wl)
 {
 	struct conf_tx_ac_category *conf_ac;
@@ -364,6 +409,11 @@ int wl1271_hw_init(struct wl1271 *wl)
 	if (ret < 0)
 		goto out_free_memmap;
 
+	/* Configure initiator BA sessions policies */
+	ret = wl1271_set_ba_policies(wl);
+	if (ret < 0)
+		goto out_free_memmap;
+
 	return 0;
 
  out_free_memmap:
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 062247e..c44462d 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -233,7 +233,7 @@ static struct conf_drv_settings default_conf = {
 		.avg_weight_rssi_beacon       = 20,
 		.avg_weight_rssi_data         = 10,
 		.avg_weight_snr_beacon        = 20,
-		.avg_weight_snr_data          = 10
+		.avg_weight_snr_data          = 10,
 	},
 	.scan = {
 		.min_dwell_time_active        = 7500,
@@ -252,6 +252,9 @@ static struct conf_drv_settings default_conf = {
 			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		},
 	},
+	.ht = {
+		.inactivity_timeout = CONF_BA_INACTIVITY_TIMEOUT,
+	},
 };
 
 static void __wl1271_op_remove_interface(struct wl1271 *wl);
@@ -827,7 +830,7 @@ int wl1271_plt_start(struct wl1271 *wl)
 
 		wl->state = WL1271_STATE_PLT;
 		wl1271_notice("firmware booted in PLT mode (%s)",
-			      wl->chip.fw_ver);
+			      wl->chip.fw_ver_str);
 		goto out;
 
 irq_disable:
@@ -1061,11 +1064,11 @@ power_off:
 
 	wl->vif = vif;
 	wl->state = WL1271_STATE_ON;
-	wl1271_info("firmware booted (%s)", wl->chip.fw_ver);
+	wl1271_info("firmware booted (%s)", wl->chip.fw_ver_str);
 
 	/* update hw/fw version info in wiphy struct */
 	wiphy->hw_version = wl->chip.id;
-	strncpy(wiphy->fw_version, wl->chip.fw_ver,
+	strncpy(wiphy->fw_version, wl->chip.fw_ver_str,
 		sizeof(wiphy->fw_version));
 
 	/*
@@ -2090,7 +2093,8 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 			wl1271_warning("Set ht cap true failed %d", ret);
 			goto out_sleep;
 		}
-			ret = wl1271_acx_set_ht_information(wl,
+
+		ret = wl1271_acx_set_ht_information(wl,
 				bss_conf->ht_operation_mode);
 		if (ret < 0) {
 			wl1271_warning("Set ht information failed %d", ret);
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index 01711fe..7b34393 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -38,6 +38,11 @@
 #define DRIVER_NAME "wl1271"
 #define DRIVER_PREFIX DRIVER_NAME ": "
 
+#define WL12XX_BA_SUPPORT_FW_SUB_VER           339
+#define WL12XX_BA_SUPPORT_FW_COST_VER1          33
+#define WL12XX_BA_SUPPORT_FW_COST_VER2_START    50
+#define WL12XX_BA_SUPPORT_FW_COST_VER2_END      60
+
 enum {
 	DEBUG_NONE	= 0,
 	DEBUG_IRQ	= BIT(0),
@@ -161,10 +166,13 @@ struct wl1271_partition_set {
 
 struct wl1271;
 
+#define WL12XX_NUM_FW_VER 5
+
 /* FIXME: I'm not sure about this structure name */
 struct wl1271_chip {
 	u32 id;
-	char fw_ver[21];
+	char fw_ver_str[ETHTOOL_BUSINFO_LEN];
+	unsigned long fw_ver[WL12XX_NUM_FW_VER];
 };
 
 struct wl1271_stats {
@@ -399,6 +407,11 @@ struct wl1271 {
 
 	/* Most recently reported noise in dBm */
 	s8 noise;
+
+	/* RX BA constraint value */
+	bool ba_support;
+	u8 ba_allowed;
+	u8 ba_rx_bitmap;
 };
 
 int wl1271_plt_start(struct wl1271 *wl);
-- 
1.7.0.4

[PATCH v3 2/2] wl12xx: BA receiver support

[Shahar Levi]

Add new ampdu_action ops to support receiver BA.
The BA initiator session management in FW independently.

Signed-off-by: Shahar Levi <shahar_levi@ti.com>
---
Dependencies:
- BA Initiator patches have a runtime dependency on commit
   "[RFC v5] wl1271: BA Initiator support"

Changes from v1:
- Add mutex and (wl->state == WL1271_STATE_OFF) protection
- "BIT(tid)" instead of "BIT(0) << tid"
- Move all event code move to the next patch

Changes from v2:
- Add supported FW version auto detection mechanism

 drivers/net/wireless/wl12xx/acx.c  |   36 ++++++++++++++++++++++
 drivers/net/wireless/wl12xx/acx.h  |   21 +++++++++++++
 drivers/net/wireless/wl12xx/main.c |   59 ++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index 54fd68d..f33ab50 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -1359,6 +1359,42 @@ out:
 	return ret;
 }
 
+/* setup BA session receiver setting in the FW. */
+int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
+				       u16 *ssn, u8 policy)
+{
+	struct wl1271_acx_ba_receiver_setup *acx;
+	int ret;
+
+	wl1271_debug(DEBUG_ACX, "acx ba receiver session setting");
+
+	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+	if (!acx) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Single link for now */
+	acx->link_id = 1;
+	acx->tid = tid_index;
+	acx->enable = policy;
+	acx->win_size = 0;
+
+	if (policy)
+		acx->ssn = *ssn;
+
+	ret = wl1271_cmd_configure(wl, ACX_BA_SESSION_RX_SETUP, acx,
+				   sizeof(*acx));
+	if (ret < 0) {
+		wl1271_warning("acx ba receiver session failed: %d", ret);
+		goto out;
+	}
+
+out:
+	kfree(acx);
+	return ret;
+}
+
 int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime)
 {
 	struct wl1271_acx_fw_tsf_information *tsf_info;
diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
index df48468..8fc98b7 100644
--- a/drivers/net/wireless/wl12xx/acx.h
+++ b/drivers/net/wireless/wl12xx/acx.h
@@ -1085,6 +1085,25 @@ struct wl1271_acx_ba_session_policy {
 	u8 padding[3];
 } __packed;
 
+struct wl1271_acx_ba_receiver_setup {
+	struct acx_header header;
+
+	/* Specifies Link Id, Range 0-31, 0xFF means ANY  Link Id */
+	u8 link_id;
+
+	u8 tid;
+
+	u8 enable;
+
+	u8 padding[1];
+
+	/* Windows size in number of packets */
+	u16 win_size;
+
+	/* BA session starting sequence number.  RANGE 0-FFF */
+	u16 ssn;
+} __packed;
+
 struct wl1271_acx_fw_tsf_information {
 	struct acx_header header;
 
@@ -1222,6 +1241,8 @@ int wl1271_acx_set_ht_information(struct wl1271 *wl,
 int wl1271_acx_set_ba_session(struct wl1271 *wl,
 			      enum ieee80211_back_parties direction,
 			      u8 tid_index, u8 policy);
+int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
+				       u16 *ssn, u8 policy);
 int wl1271_acx_tsf_info(struct wl1271 *wl, u64 *mactime);
 
 #endif /* __WL1271_ACX_H__ */
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index c44462d..04b69ab 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -2251,6 +2251,64 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
 	return 0;
 }
 
+int wl1271_op_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+			   enum ieee80211_ampdu_mlme_action action,
+			   struct ieee80211_sta *sta, u16 tid, u16 *ssn)
+{
+	struct wl1271 *wl = hw->priv;
+	int ret;
+
+	mutex_lock(&wl->mutex);
+
+	if (unlikely(wl->state == WL1271_STATE_OFF)) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
+	ret = wl1271_ps_elp_wakeup(wl, false);
+	if (ret < 0)
+		goto out;
+
+	switch (action) {
+	case IEEE80211_AMPDU_RX_START:
+		if ((wl->ba_allowed) && (wl->ba_support)) {
+			ret = wl1271_acx_set_ba_receiver_session(wl, tid, ssn,
+								 true);
+			if (!ret)
+				wl->ba_rx_bitmap |= BIT(tid);
+		} else
+			ret = -EPERM;
+		break;
+
+	case IEEE80211_AMPDU_RX_STOP:
+		ret = wl1271_acx_set_ba_receiver_session(wl, tid, ssn, false);
+		if (!ret)
+			wl->ba_rx_bitmap &= ~BIT(tid);
+		break;
+
+	/*
+	 * The BA initiator session management in FW independently.
+	 * Falling break here on purpose for all TX APDU commands.
+	 */
+	case IEEE80211_AMPDU_TX_START:
+	case IEEE80211_AMPDU_TX_STOP:
+	case IEEE80211_AMPDU_TX_OPERATIONAL:
+		ret = -EINVAL;
+		break;
+
+	default:
+		wl1271_error("Incorrect ampdu action id=%x\n", action);
+		ret = -ENODEV;
+	}
+
+	wl1271_ps_elp_sleep(wl);
+
+out:
+	mutex_unlock(&wl->mutex);
+
+	return ret;
+}
+
 /* can't be const, mac80211 writes to this */
 static struct ieee80211_rate wl1271_rates[] = {
 	{ .bitrate = 10,
@@ -2497,6 +2555,7 @@ static const struct ieee80211_ops wl1271_ops = {
 	.conf_tx = wl1271_op_conf_tx,
 	.get_tsf = wl1271_op_get_tsf,
 	.get_survey = wl1271_op_get_survey,
+	.ampdu_action = wl1271_op_ampdu_action,
 	CFG80211_TESTMODE_CMD(wl1271_tm_cmd)
 };
 
-- 
1.7.0.4

Re: [PATCH v5 1/2] wl12xx: BA initiator support

[Luciano Coelho]

On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
> Add 80211n BA initiator session support wl1271 driver.
> Include BA supported FW version auto detection mechanism.
> BA initiator session management included in FW independently.
> 
> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
> ---

Some comments...


> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
> index cc4068d..54fd68d 100644
> --- a/drivers/net/wireless/wl12xx/acx.c
> +++ b/drivers/net/wireless/wl12xx/acx.c
> @@ -1309,6 +1309,56 @@ out:
>         return ret;
>  }
> 
> +/* Configure BA session initiator\receiver parameters setting in the FW. */

Please use forward slash here instead of backslash, ie. use
"initiator/receiver".


> diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
> index 9cbc3f4..df48468 100644
> --- a/drivers/net/wireless/wl12xx/acx.h
> +++ b/drivers/net/wireless/wl12xx/acx.h
> @@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
>         u8 padding[3];
>  } __packed;
> 
> +#define BA_WIN_SIZE 8

Should this be DEFAULT_BA_WIN_SIZE?


> diff --git a/drivers/net/wireless/wl12xx/boot.c b/drivers/net/wireless/wl12xx/boot.c
> index 4a9f929..cd42e12 100644
> --- a/drivers/net/wireless/wl12xx/boot.c
> +++ b/drivers/net/wireless/wl12xx/boot.c
> @@ -101,6 +101,39 @@ static void wl1271_boot_set_ecpu_ctrl(struct wl1271 *wl, u32 flag)
>         wl1271_write32(wl, ACX_REG_ECPU_CONTROL, cpu_ctrl);
>  }
> 
> +static void wl1271_save_fw_ver(struct wl1271 *wl)

This function name is not very informative.  Why not call it
wl1271_parse_fw_ver() instead?


> +{
> +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];

This is weird, but it seem to be what is used in cfg80211 (as Shahar
pointed out on IRC).  IMHO it should be ETHTOOL_FWVERS_LEN instead, both
here and in cfg80211.

In any case, this is a bit confusing here, because we don't use the
fw_version in the wiphy struct (we probably should).  Let's keep it like
this for now and maybe later we can change.

Also, I don't see why you need a local copy here.

> +       char *fw_ver_point;
> +       int ret, i;
> +
> +       /* copy the fw version to temp str */
> +       strncpy(fw_ver_str, wl->chip.fw_ver_str, sizeof(fw_ver_str));
> +
> +       for (i = (WL12XX_NUM_FW_VER - 1); i > 0; --i) {
> +               /* find the last '.' */
> +               fw_ver_point = strrchr(fw_ver_str, '.');
> +
> +               /* read version number */
> +               ret = strict_strtoul(fw_ver_point+1, 10, &(wl->chip.fw_ver[i]));
> +               if (ret < 0) {
> +                       wl1271_warning("fw version incorrect value");
> +                       memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
> +                       return;
> +               }
> +
> +               /* clean '.' */
> +               *fw_ver_point = '\0';
> +       }
> +
> +       ret = strict_strtoul(fw_ver_point-1, 10, &(wl->chip.fw_ver[0]));
> +       if (ret < 0) {
> +               wl1271_warning("fw version incorrect value");
> +               memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
> +               return;
> +       }
> +}

Instead of all this manual parsing, why don't you use sscanf? I think
the following could do the work very nicely:

	ret = sscanf(wl->chip.fw_ver_str + WL12XX_FW_VER_OFFSET,
		     "%lu.%lu.%lu.%lu.%lu", &wl->chip.fw_ver[0],
		     &wl->chip.fw_ver[1], &wl->chip.fw_ver[2]
		     &wl->chip.fw_ver[3], &wl->chip.fw_ver[4]);

Wouldn't something like this be much simpler? (or you could use %u if
you agree on using unsigned int, see below)


>  static int wl1271_boot_upload_firmware_chunk(struct wl1271 *wl, void *buf,
> diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
> index a16b361..41df771 100644
> --- a/drivers/net/wireless/wl12xx/conf.h
> +++ b/drivers/net/wireless/wl12xx/conf.h
> @@ -1090,6 +1090,12 @@ struct conf_rf_settings {
>         u8 tx_per_channel_power_compensation_5[CONF_TX_PWR_COMPENSATION_LEN_5];
>  };
> 
> +#define CONF_BA_INACTIVITY_TIMEOUT 10000

If this is a CONFigurable value, it should be in conf.h and in the
configuration parameters in main.c, shouldn't it?


> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 062247e..c44462d 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -233,7 +233,7 @@ static struct conf_drv_settings default_conf = {
>                 .avg_weight_rssi_beacon       = 20,
>                 .avg_weight_rssi_data         = 10,
>                 .avg_weight_snr_beacon        = 20,
> -               .avg_weight_snr_data          = 10
> +               .avg_weight_snr_data          = 10,
>         },
>         .scan = {
>                 .min_dwell_time_active        = 7500,
> @@ -252,6 +252,9 @@ static struct conf_drv_settings default_conf = {
>                         0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>                 },
>         },
> +       .ht = {
> +               .inactivity_timeout = CONF_BA_INACTIVITY_TIMEOUT,
> +       },

Ah, I see.  You are using that macro here, but I guess you could use the
value directly, since this is all configuration stuff, so no need to use
defines, unless the values mean something specific.  Here it is just a
measure of time, so a number can be used directly (like in the
min_dwell_time_active).


> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
> index 01711fe..7b34393 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
> @@ -38,6 +38,11 @@
>  #define DRIVER_NAME "wl1271"
>  #define DRIVER_PREFIX DRIVER_NAME ": "
> 
> +#define WL12XX_BA_SUPPORT_FW_SUB_VER           339
> +#define WL12XX_BA_SUPPORT_FW_COST_VER1          33
> +#define WL12XX_BA_SUPPORT_FW_COST_VER2_START    50
> +#define WL12XX_BA_SUPPORT_FW_COST_VER2_END      60

This defines are very confusing.  Can you explain a bit? What about
"COST" version 0 (like 6.0.0.0.343), will that branch never support BA?
Where do the 50 to 60 come from? And what is 33? This kind of magic
should be explained.


> @@ -161,10 +166,13 @@ struct wl1271_partition_set {
> 
>  struct wl1271;
> 
> +#define WL12XX_NUM_FW_VER 5
> +

WL12XX_FW_VER_OFFSET sounds better to me.  And it shouldn't it be 4,
which is the "Rev " prefix?


>  /* FIXME: I'm not sure about this structure name */
>  struct wl1271_chip {
>         u32 id;
> -       char fw_ver[21];
> +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];
> +       unsigned long fw_ver[WL12XX_NUM_FW_VER];

Why not unsigned int? (and then use %u.%u... as I mentioned earlier).


-- 
Cheers,
Luca.

Re: [PATCH v3 2/2] wl12xx: BA receiver support

[Luciano Coelho]

On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
> Add new ampdu_action ops to support receiver BA.
> The BA initiator session management in FW independently.
> 
> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
> ---

Some comments.



> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
> index 54fd68d..f33ab50 100644
> --- a/drivers/net/wireless/wl12xx/acx.c
> +++ b/drivers/net/wireless/wl12xx/acx.c
> @@ -1359,6 +1359,42 @@ out:
>  	return ret;
>  }
>  
> +/* setup BA session receiver setting in the FW. */
> +int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
> +				       u16 *ssn, u8 policy)

You don't modify ssn here, so why pass it as a pointer? Use u16 directly
here instead.

Actually it's even worse.  As stated in mac80211.h, ssn can be NULL
here, so you would be accessing a NULL pointer in that case.

I see that you check "policy", which indicates whether ssn is valid or
not, but why not make it cleaner by checking if ssn is NULL and setting
it to zero before passing instead?


> +       acx->enable = policy;

Also, because of this, it would make more sense if policy was a boolean
and was called "enable" instead.


> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index c44462d..04b69ab 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -2251,6 +2251,64 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
>  	return 0;
>  }
>  
> +int wl1271_op_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +			   enum ieee80211_ampdu_mlme_action action,
> +			   struct ieee80211_sta *sta, u16 tid, u16 *ssn)
> +{
> +	struct wl1271 *wl = hw->priv;
> +	int ret;
> +
> +	mutex_lock(&wl->mutex);
> +
> +	if (unlikely(wl->state == WL1271_STATE_OFF)) {
> +		ret = -EAGAIN;
> +		goto out;
> +	}
> +
> +	ret = wl1271_ps_elp_wakeup(wl, false);
> +	if (ret < 0)
> +		goto out;
> +
> +	switch (action) {
> +	case IEEE80211_AMPDU_RX_START:
> +		if ((wl->ba_allowed) && (wl->ba_support)) {

I'm a bit confused.  What is ba_allowed, actually? In the previous patch
you always call wl1271_set_ba_policies() which always sets it to true.
So it seems quite useless.


> +			ret = wl1271_acx_set_ba_receiver_session(wl, tid, ssn,
> +								 true);
> +			if (!ret)
> +				wl->ba_rx_bitmap |= BIT(tid);
> +		} else
> +			ret = -EPERM;

The indentation is wrong here.  And -EPERM is definitely not the right
return code for this.  It should probably -ENOTSUPP or something like
that.

> +	/*
> +	 * The BA initiator session management in FW independently.
> +	 * Falling break here on purpose for all TX APDU commands.
> +	 */
> +	case IEEE80211_AMPDU_TX_START:
> +	case IEEE80211_AMPDU_TX_STOP:
> +	case IEEE80211_AMPDU_TX_OPERATIONAL:
> +		ret = -EINVAL;
> +		break;

-EINVAL? Actually, should mac80211 even call us with these actions? I
don't remember now, did you implement the way to prevent mac80211 from
trying to to AMPDU_TX by itself?


> +
> +	default:
> +		wl1271_error("Incorrect ampdu action id=%x\n", action);
> +		ret = -ENODEV;

-ENODEV? All these error codes look quite random to me.  Can you explain
your choices?
 

-- 
Cheers,
Luca.

Re: [PATCH v5 1/2] wl12xx: BA initiator support

[Levi, Shahar]

On Tue, Jan 11, 2011 at 2:13 AM, Levi, Shahar <shahar_levi@ti.com> wrote:
>
> On Mon, Jan 10, 2011 at 5:00 PM, Luciano Coelho <coelho@ti.com> wrote:
>>
>> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
>> > Add 80211n BA initiator session support wl1271 driver.
>> > Include BA supported FW version auto detection mechanism.
>> > BA initiator session management included in FW independently.
>> >
>> > Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> > ---
>>
>> Some comments...
>
thanks for your review.

>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
>> > index cc4068d..54fd68d 100644
>> > --- a/drivers/net/wireless/wl12xx/acx.c
>> > +++ b/drivers/net/wireless/wl12xx/acx.c
>> > @@ -1309,6 +1309,56 @@ out:
>> >         return ret;
>> >  }
>> >
>> > +/* Configure BA session initiator\receiver parameters setting in the FW. */
>>
>> Please use forward slash here instead of backslash, ie. use
>> "initiator/receiver".
>
np, will be fix.

>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
>> > index 9cbc3f4..df48468 100644
>> > --- a/drivers/net/wireless/wl12xx/acx.h
>> > +++ b/drivers/net/wireless/wl12xx/acx.h
>> > @@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
>> >         u8 padding[3];
>> >  } __packed;
>> >
>> > +#define BA_WIN_SIZE 8
>>
>> Should this be DEFAULT_BA_WIN_SIZE?
>
No, the FW support win size of 8. it is not configurable.

>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/boot.c b/drivers/net/wireless/wl12xx/boot.c
>> > index 4a9f929..cd42e12 100644
>> > --- a/drivers/net/wireless/wl12xx/boot.c
>> > +++ b/drivers/net/wireless/wl12xx/boot.c
>> > @@ -101,6 +101,39 @@ static void wl1271_boot_set_ecpu_ctrl(struct wl1271 *wl, u32 flag)
>> >         wl1271_write32(wl, ACX_REG_ECPU_CONTROL, cpu_ctrl);
>> >  }
>> >
>> > +static void wl1271_save_fw_ver(struct wl1271 *wl)
>>
>> This function name is not very informative.  Why not call it
>> wl1271_parse_fw_ver() instead?
>
np, will be fix

>>
>>
>> > +{
>> > +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];
>>
>> This is weird, but it seem to be what is used in cfg80211 (as Shahar
>> pointed out on IRC).  IMHO it should be ETHTOOL_FWVERS_LEN instead, both
>> here and in cfg80211.
>>
>> In any case, this is a bit confusing here, because we don't use the
>> fw_version in the wiphy struct (we probably should).  Let's keep it like
>> this for now and maybe later we can change.
>>
>> Also, I don't see why you need a local copy here.
>
i use local copy in order to remove '.' (*fw_ver_point = '\0') without
destroyed wl->chip.fw_ver_str.

>>
>> > +       char *fw_ver_point;
>> > +       int ret, i;
>> > +
>> > +       /* copy the fw version to temp str */
>> > +       strncpy(fw_ver_str, wl->chip.fw_ver_str, sizeof(fw_ver_str));
>> > +
>> > +       for (i = (WL12XX_NUM_FW_VER - 1); i > 0; --i) {
>> > +               /* find the last '.' */
>> > +               fw_ver_point = strrchr(fw_ver_str, '.');
>> > +
>> > +               /* read version number */
>> > +               ret = strict_strtoul(fw_ver_point+1, 10, &(wl->chip.fw_ver[i]));
>> > +               if (ret < 0) {
>> > +                       wl1271_warning("fw version incorrect value");
>> > +                       memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
>> > +                       return;
>> > +               }
>> > +
>> > +               /* clean '.' */
>> > +               *fw_ver_point = '\0';
>> > +       }
>> > +
>> > +       ret = strict_strtoul(fw_ver_point-1, 10, &(wl->chip.fw_ver[0]));
>> > +       if (ret < 0) {
>> > +               wl1271_warning("fw version incorrect value");
>> > +               memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
>> > +               return;
>> > +       }
>> > +}
>>
>> Instead of all this manual parsing, why don't you use sscanf? I think
>> the following could do the work very nicely:
>>
>>        ret = sscanf(wl->chip.fw_ver_str + WL12XX_FW_VER_OFFSET,
>>                     "%lu.%lu.%lu.%lu.%lu", &wl->chip.fw_ver[0],
>>                     &wl->chip.fw_ver[1], &wl->chip.fw_ver[2]
>>                     &wl->chip.fw_ver[3], &wl->chip.fw_ver[4]);
>>
>> Wouldn't something like this be much simpler? (or you could use %u if
>> you agree on using unsigned int, see below)
>
good point, i will try and update.

>>
>>
>> >  static int wl1271_boot_upload_firmware_chunk(struct wl1271 *wl, void *buf,
>> > diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
>> > index a16b361..41df771 100644
>> > --- a/drivers/net/wireless/wl12xx/conf.h
>> > +++ b/drivers/net/wireless/wl12xx/conf.h
>> > @@ -1090,6 +1090,12 @@ struct conf_rf_settings {
>> >         u8 tx_per_channel_power_compensation_5[CONF_TX_PWR_COMPENSATION_LEN_5];
>> >  };
>> >
>> > +#define CONF_BA_INACTIVITY_TIMEOUT 10000
>>
>> If this is a CONFigurable value, it should be in conf.h and in the
>> configuration parameters in main.c, shouldn't it?
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
>> > index 062247e..c44462d 100644
>> > --- a/drivers/net/wireless/wl12xx/main.c
>> > +++ b/drivers/net/wireless/wl12xx/main.c
>> > @@ -233,7 +233,7 @@ static struct conf_drv_settings default_conf = {
>> >                 .avg_weight_rssi_beacon       = 20,
>> >                 .avg_weight_rssi_data         = 10,
>> >                 .avg_weight_snr_beacon        = 20,
>> > -               .avg_weight_snr_data          = 10
>> > +               .avg_weight_snr_data          = 10,
>> >         },
>> >         .scan = {
>> >                 .min_dwell_time_active        = 7500,
>> > @@ -252,6 +252,9 @@ static struct conf_drv_settings default_conf = {
>> >                         0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> >                 },
>> >         },
>> > +       .ht = {
>> > +               .inactivity_timeout = CONF_BA_INACTIVITY_TIMEOUT,
>> > +       },
>>
>> Ah, I see.  You are using that macro here, but I guess you could use the
>> value directly, since this is all configuration stuff, so no need to use
>> defines, unless the values mean something specific.  Here it is just a
>> measure of time, so a number can be used directly (like in the
>> min_dwell_time_active).
>
np, will be fix

>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
>> > index 01711fe..7b34393 100644
>> > --- a/drivers/net/wireless/wl12xx/wl12xx.h
>> > +++ b/drivers/net/wireless/wl12xx/wl12xx.h
>> > @@ -38,6 +38,11 @@
>> >  #define DRIVER_NAME "wl1271"
>> >  #define DRIVER_PREFIX DRIVER_NAME ": "
>> >
>> > +#define WL12XX_BA_SUPPORT_FW_SUB_VER           339
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER1          33
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER2_START    50
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER2_END      60
>>
>> This defines are very confusing.  Can you explain a bit?
yes i will add comments.

>>
>> What about
>> "COST" version 0 (like 6.0.0.0.343), will that branch never support BA?Where do the 50 to 60 come from?
>
no, all open source version will be tag from 50 to 60.

>>
>> And what is 33? This kind of magic
>>
>> should be explained.
>>
>>
>> > @@ -161,10 +166,13 @@ struct wl1271_partition_set {
>> >
>> >  struct wl1271;
>> >
>> > +#define WL12XX_NUM_FW_VER 5
>> > +
>>
>> WL12XX_FW_VER_OFFSET sounds better to me.
>>
>> And it shouldn't it be 4,
>> which is the "Rev " prefix?
>
the macro represent the number of numbers in the version. it is not offset.

>>
>>
>> >  /* FIXME: I'm not sure about this structure name */
>> >  struct wl1271_chip {
>> >         u32 id;
>> > -       char fw_ver[21];
>> > +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];
>> > +       unsigned long fw_ver[WL12XX_NUM_FW_VER];
>>
>> Why not unsigned int? (and then use %u.%u... as I mentioned earlier).
>
i will try and update.

>>
>> --
>> Cheers,
>> Luca.
>>
Shahar

Re: [PATCH v3 2/2] wl12xx: BA receiver support

[Levi, Shahar]

On Mon, Jan 10, 2011 at 5:44 PM, Luciano Coelho <coelho@ti.com> wrote:
> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
>> Add new ampdu_action ops to support receiver BA.
>> The BA initiator session management in FW independently.
>>
>> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> ---
>
> Some comments.
Thanks for your review.

>
>
>
>> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
>> index 54fd68d..f33ab50 100644
>> --- a/drivers/net/wireless/wl12xx/acx.c
>> +++ b/drivers/net/wireless/wl12xx/acx.c
>> @@ -1359,6 +1359,42 @@ out:
>>       return ret;
>>  }
>>
>> +/* setup BA session receiver setting in the FW. */
>> +int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
>> +                                    u16 *ssn, u8 policy)
>
> You don't modify ssn here, so why pass it as a pointer? Use u16 directly
> here instead.
>
> Actually it's even worse.  As stated in mac80211.h, ssn can be NULL
> here, so you would be accessing a NULL pointer in that case.
>
> I see that you check "policy", which indicates whether ssn is valid or
> not, but why not make it cleaner by checking if ssn is NULL and setting
> it to zero before passing instead?
good chtch. it was left from previous implantation. will be fix.

>
>
>> +       acx->enable = policy;
>
> Also, because of this, it would make more sense if policy was a boolean
> and was called "enable" instead.
np, will be fix

>
>
>> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
>> index c44462d..04b69ab 100644
>> --- a/drivers/net/wireless/wl12xx/main.c
>> +++ b/drivers/net/wireless/wl12xx/main.c
>> @@ -2251,6 +2251,64 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
>>       return 0;
>>  }
>>
>> +int wl1271_op_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> +                        enum ieee80211_ampdu_mlme_action action,
>> +                        struct ieee80211_sta *sta, u16 tid, u16 *ssn)
>> +{
>> +     struct wl1271 *wl = hw->priv;
>> +     int ret;
>> +
>> +     mutex_lock(&wl->mutex);
>> +
>> +     if (unlikely(wl->state == WL1271_STATE_OFF)) {
>> +             ret = -EAGAIN;
>> +             goto out;
>> +     }
>> +
>> +     ret = wl1271_ps_elp_wakeup(wl, false);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     switch (action) {
>> +     case IEEE80211_AMPDU_RX_START:
>> +             if ((wl->ba_allowed) && (wl->ba_support)) {
>
> I'm a bit confused.  What is ba_allowed, actually? In the previous patch
> you always call wl1271_set_ba_policies() which always sets it to true.
> So it seems quite useless.
ba_allowed belong to next path "Stop BA session event from device". i
will move it.

>
>
>> +                     ret = wl1271_acx_set_ba_receiver_session(wl, tid, ssn,
>> +                                                              true);
>> +                     if (!ret)
>> +                             wl->ba_rx_bitmap |= BIT(tid);
>> +             } else
>> +                     ret = -EPERM;
>
> The indentation is wrong here.  And -EPERM is definitely not the right
> return code for this.  It should probably -ENOTSUPP or something like
> that.
any error will be good. i go on "Operation not permitted". will be fix.

>
>> +     /*
>> +      * The BA initiator session management in FW independently.
>> +      * Falling break here on purpose for all TX APDU commands.
>> +      */
>> +     case IEEE80211_AMPDU_TX_START:
>> +     case IEEE80211_AMPDU_TX_STOP:
>> +     case IEEE80211_AMPDU_TX_OPERATIONAL:
>> +             ret = -EINVAL;
>> +             break;
>
> -EINVAL? Actually, should mac80211 even call us with these actions? I
> don't remember now, did you implement the way to prevent mac80211 from
> trying to to AMPDU_TX by itself?
the mac souldn't call those due to the fact the rate management is in the FW.

>
>
>> +
>> +     default:
>> +             wl1271_error("Incorrect ampdu action id=%x\n", action);
>> +             ret = -ENODEV;
>
> -ENODEV? All these error codes look quite random to me.  Can you explain
> your choices?
amm, i am trying to remeber why i use that one without success. i will
switch it to EINVAL.

>
>
> --
> Cheers,
> Luca.
Shahar

Re: [PATCH v5 1/2] wl12xx: BA initiator support

[Luciano Coelho]

On Tue, 2011-01-11 at 01:18 +0100, Levi, Shahar wrote:
> On Tue, Jan 11, 2011 at 2:13 AM, Levi, Shahar <shahar_levi@ti.com> wrote:
> >
> > On Mon, Jan 10, 2011 at 5:00 PM, Luciano Coelho <coelho@ti.com> wrote:
> >>
> >> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
> >> > diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
> >> > index 9cbc3f4..df48468 100644
> >> > --- a/drivers/net/wireless/wl12xx/acx.h
> >> > +++ b/drivers/net/wireless/wl12xx/acx.h
> >> > @@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
> >> >         u8 padding[3];
> >> >  } __packed;
> >> >
> >> > +#define BA_WIN_SIZE 8
> >>
> >> Should this be DEFAULT_BA_WIN_SIZE?
> >
> No, the FW support win size of 8. it is not configurable.

If only 8 is supported, why do we even have to pass it to the firmware
in the ACX_BA_SESSION_POLICY_CFG command? I think that, even though this
cannot be really changed, it should be part of the conf structure.


> >> > +{
> >> > +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];
> >>
> >> This is weird, but it seem to be what is used in cfg80211 (as Shahar
> >> pointed out on IRC).  IMHO it should be ETHTOOL_FWVERS_LEN instead, both
> >> here and in cfg80211.
> >>
> >> In any case, this is a bit confusing here, because we don't use the
> >> fw_version in the wiphy struct (we probably should).  Let's keep it like
> >> this for now and maybe later we can change.
> >>
> >> Also, I don't see why you need a local copy here.
> >
> i use local copy in order to remove '.' (*fw_ver_point = '\0') without
> destroyed wl->chip.fw_ver_str.

Ah, I see, but if you use sscanf, as I suggested, this won't be needed
anymore.


> >> > @@ -161,10 +166,13 @@ struct wl1271_partition_set {
> >> >
> >> >  struct wl1271;
> >> >
> >> > +#define WL12XX_NUM_FW_VER 5
> >> > +
> >>
> >> WL12XX_FW_VER_OFFSET sounds better to me.
> >>
> >> And it shouldn't it be 4,
> >> which is the "Rev " prefix?
> >
> the macro represent the number of numbers in the version. it is not offset.

Right, I guess I didn't follow your algorithm in details, since using
sscanf would be much easier.


-- 
Cheers,
Luca.

wl12xx compat wireless rcu_read_lock issue

[DE CESCO, Jonathan]

Hi,

When trying to test wl12xx driver from compat-wireless-2010-12-13 (with a wl1273 device), I notice a crash when initializing the wlan interface.

Find below the patch that seems to solve the issue:

diff --git a/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c b/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
index 1fc5a36..480f44b 100644
--- a/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
+++ b/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
@@ -1858,11 +1858,17 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 {
            enum wl1271_cmd_ps_mode mode;
            struct wl1271 *wl = hw->priv;
-           struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid);
+          rcu_read_lock();
+          struct ieee80211_sta *sta;
            bool do_join = false;
            bool set_assoc = false;
            int ret;

+          sta = ieee80211_find_sta(vif, bss_conf->bssid);
+          if (!sta) {
+                      rcu_read_unlock();
+                      return;
+          }

            wl1271_debug(DEBUG_MAC80211, "mac80211 bss info changed");
+     	rcu_read_unlock();
            mutex_lock(&wl->mutex);

--

Is this a known bug that has already been encountered? I am using the compat wireless flavor of the mac80211 framework and driver so I don't really know if you can come across this issue with standard kernel.

Thanks,

John

Re: wl12xx compat wireless rcu_read_lock issue

[Luciano Coelho]

On Tue, 2011-01-11 at 11:22 +0100, DE CESCO, Jonathan wrote:
> Hi,
> 
> When trying to test wl12xx driver from compat-wireless-2010-12-13 (with a wl1273 device), I notice a crash when initializing the wlan interface.
> 
> Find below the patch that seems to solve the issue:
> 
> diff --git a/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c b/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
> index 1fc5a36..480f44b 100644
> --- a/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
> +++ b/compat-wireless-2010-12-13/drivers/net/wireless/wl12xx/main.c
> @@ -1858,11 +1858,17 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
>  {
>             enum wl1271_cmd_ps_mode mode;
>             struct wl1271 *wl = hw->priv;
> -           struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid);
> +          rcu_read_lock();
> +          struct ieee80211_sta *sta;
>             bool do_join = false;
>             bool set_assoc = false;
>             int ret;
> 
> +          sta = ieee80211_find_sta(vif, bss_conf->bssid);
> +          if (!sta) {
> +                      rcu_read_unlock();
> +                      return;
> +          }
> 
>             wl1271_debug(DEBUG_MAC80211, "mac80211 bss info changed");
> +     	rcu_read_unlock();
>             mutex_lock(&wl->mutex);
> 
> --
> 
> Is this a known bug that has already been encountered? I am using the compat wireless flavor of the mac80211 framework and driver so I don't really know if you can come across this issue with standard kernel.

I hadn't seen it before, but then again, I just integrated the AP
patches which added this code.  The code has been tested quite well
before, but indeed this is broken.

We need to lock the RCU before calling ieee80211_find_sta() and also
whenever we access the resulting pointer.

In your patch, the if (!sta) part is wrong, because we still do lots of
things in this function even if sta is NULL.  So that if can be removed.

Also, because the RCU lock needs to be held when the returned pointer is
accessed, we should hold it until the end of the function.

>From mac80211 documentation:

/**
 * ieee80211_find_sta - find a station
 *
 * @vif: virtual interface to look for station on
 * @addr: station's address
 *
 * This function must be called under RCU lock and the
 * resulting pointer is only valid under RCU lock as well.
 */

Thanks for reporting this!

-- 
Cheers,
Luca.

Re: wl12xx compat wireless rcu_read_lock issue

[Luciano Coelho]

On Tue, 2011-01-11 at 17:03 +0100, Arik Nemtsov wrote:
> On Tue, Jan 11, 2011 at 13:28, Luciano Coelho <coelho@ti.com> wrote:
> > I hadn't seen it before, but then again, I just integrated the AP
> > patches which added this code.  The code has been tested quite well
> > before, but indeed this is broken.

[...]

> Actually this change is part of the HT patch series (in patch
> 18357850b694ba3fa29363c7d86ccd8783f4a065) so it was there quite some
> time.
> But I did port over without noticing the bug :)

Right, I didn't check very carefully when the bug was introduced and it
doesn't matter much anyway.  But it's a bug and needs to be fixed.  Does
any volunteers to send a patch or should I do it myself? (feeling
lazy :P)


-- 
Cheers,
Luca.

Re: [PATCH v5 1/2] wl12xx: BA initiator support

[Levi, Shahar]

On Tue, Jan 11, 2011 at 10:04 AM, Luciano Coelho <coelho@ti.com> wrote:
> On Tue, 2011-01-11 at 01:18 +0100, Levi, Shahar wrote:
>> On Tue, Jan 11, 2011 at 2:13 AM, Levi, Shahar <shahar_levi@ti.com> wrote:
>> >
>> > On Mon, Jan 10, 2011 at 5:00 PM, Luciano Coelho <coelho@ti.com> wrote:
>> >>
>> >> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
>> >> > diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
>> >> > index 9cbc3f4..df48468 100644
>> >> > --- a/drivers/net/wireless/wl12xx/acx.h
>> >> > +++ b/drivers/net/wireless/wl12xx/acx.h
>> >> > @@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
>> >> >         u8 padding[3];
>> >> >  } __packed;
>> >> >
>> >> > +#define BA_WIN_SIZE 8
>> >>
>> >> Should this be DEFAULT_BA_WIN_SIZE?
>> >
>> No, the FW support win size of 8. it is not configurable.
>
> If only 8 is supported, why do we even have to pass it to the firmware
> in the ACX_BA_SESSION_POLICY_CFG command? I think that, even though this
> cannot be really changed, it should be part of the conf structure.
i did some more investigation on that, the WLAN_BACK_INITIATOR win
size could be up to 64 pacets; the WLAN_BACK_RECIPIENT could be up to
8.
i will fix by call the macro RX_BA_WIN_SIZE 8 and add in conf
structure tx_ba_win_size

>
>
>> >> > +{
>> >> > +       char fw_ver_str[ETHTOOL_BUSINFO_LEN];
>> >>
>> >> This is weird, but it seem to be what is used in cfg80211 (as Shahar
>> >> pointed out on IRC).  IMHO it should be ETHTOOL_FWVERS_LEN instead, both
>> >> here and in cfg80211.
>> >>
>> >> In any case, this is a bit confusing here, because we don't use the
>> >> fw_version in the wiphy struct (we probably should).  Let's keep it like
>> >> this for now and maybe later we can change.
>> >>
>> >> Also, I don't see why you need a local copy here.
>> >
>> i use local copy in order to remove '.' (*fw_ver_point = '\0') without
>> destroyed wl->chip.fw_ver_str.
>
> Ah, I see, but if you use sscanf, as I suggested, this won't be needed
> anymore.
>
>
>> >> > @@ -161,10 +166,13 @@ struct wl1271_partition_set {
>> >> >
>> >> >  struct wl1271;
>> >> >
>> >> > +#define WL12XX_NUM_FW_VER 5
>> >> > +
>> >>
>> >> WL12XX_FW_VER_OFFSET sounds better to me.
>> >>
>> >> And it shouldn't it be 4,
>> >> which is the "Rev " prefix?
>> >
>> the macro represent the number of numbers in the version. it is not offset.
>
> Right, I guess I didn't follow your algorithm in details, since using
> sscanf would be much easier.
>
>
> --
> Cheers,
> Luca.
>
>
np, i will try to use sscanf and update.

Thanks.

Re: [PATCH v3 2/2] wl12xx: BA receiver support

[Levi, Shahar]

On Tue, Jan 11, 2011 at 2:54 AM, Levi, Shahar <shahar_levi@ti.com> wrote:
>>> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
>>> index 54fd68d..f33ab50 100644
>>> --- a/drivers/net/wireless/wl12xx/acx.c
>>> +++ b/drivers/net/wireless/wl12xx/acx.c
>>> @@ -1359,6 +1359,42 @@ out:
>>>       return ret;
>>>  }
>>>
>>> +/* setup BA session receiver setting in the FW. */
>>> +int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
>>> +                                    u16 *ssn, u8 policy)
>>
>> You don't modify ssn here, so why pass it as a pointer? Use u16 directly
>> here instead.
>>
>> Actually it's even worse.  As stated in mac80211.h, ssn can be NULL
>> here, so you would be accessing a NULL pointer in that case.
>>
>> I see that you check "policy", which indicates whether ssn is valid or
>> not, but why not make it cleaner by checking if ssn is NULL and setting
>> it to zero before passing instead?
> good chtch. it was left from previous implantation. will be fix.
There is two options from you suggestion:
a) move ssn directly and then check policy if it is valid
b) keep ssn as pointer and check it to validate it is valid.

I will use a option.
Thanks,
Shahar