[PATCH 00/03] wl1271: BA Initiator support

[PATCH 00/03] wl1271: BA Initiator support

[Shahar Levi]

Series that add 80211n BA initiator session support wl1271 driver. BA initiator 
session management included in FW independently.
BA receiver session not supported in that Series.

Dependencies:
- BA Initiator patches have a runtime dependency on
  commit "[PATCH v2] wl1271: 11n Support" series (patches is in linux-wireless
  mailing list)

Limitation:
- For now only TID0 supported (Beast Effort).

Thanks,
Shahar

Shahar Levi (3):
  wl1271: BA Initiator support, Add Definitions
  wl1271: BA Initiator support, BA functions
  wl1271: BA Initiator support, active BA ability

 drivers/net/wireless/wl12xx/wl1271_acx.c  |   51 +++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/wl1271_acx.h  |   20 +++++++++++
 drivers/net/wireless/wl12xx/wl1271_conf.h |    3 ++
 drivers/net/wireless/wl12xx/wl1271_main.c |   54 ++++++++++++++++++++++++++++-
 4 files changed, 127 insertions(+), 1 deletions(-)

[PATCH 01/03] wl1271: BA Initiator support, Add Definitions

[Shahar Levi]

New acx cmd wl1271_acx_ba_session_policy to set BA policy to the FW.
Macros to use with BA setting.

Signed-off-by: Shahar Levi <shahar_levi@ti.com>
---
 drivers/net/wireless/wl12xx/wl1271_acx.h  |   16 ++++++++++++++++
 drivers/net/wireless/wl12xx/wl1271_conf.h |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index b8da1bc..6c3c7c0 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -1055,6 +1055,22 @@ struct wl1271_acx_ht_information {
 	u8 padding[3];
 } __packed;
 
+/*
+ * BA sessen interface structure
+ */
+struct wl1271_acx_ba_session_policy {
+	struct acx_header header;
+	/* Mac address of: SA as receiver / RA as initiator */
+	u8 mac_address[ETH_ALEN];
+	u8 tid;			/* TID */
+	u8 policy;		/* Enable / Disable */
+	u16 win_size;		/* windows size in num of packet */
+	u16 inactivity_timeout;	/* as initiator inactivity timeout
+				 * in time units(TU) of 1024us.
+				 * as receiver reserved
+				 */
+} __packed;
+
 struct wl1271_acx_fw_tsf_information {
 	struct acx_header header;
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
index b3e608e..63a0a9a 100644
--- a/drivers/net/wireless/wl12xx/wl1271_conf.h
+++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
@@ -94,6 +94,9 @@ enum {
 #define HW_BG_RATES_MASK	0xffff
 #define HW_HT_RATES_OFFSET	16
 
+#define BA_RECEIVER_WIN_SIZE 8
+#define BA_INACTIVITY_TIMEOUT 10000
+
 enum {
 	CONF_SG_DISABLE = 0,
 	CONF_SG_PROTECTIVE,
-- 
1.6.0.4

[PATCH 02/03] wl1271: BA Initiator support, BA functions

[Shahar Levi]

New ops support ampdu_action as initiator returns EINVAL due to
the fact that BA initiator session manage in the FW independently.
acx function set BA policies.

Signed-off-by: Shahar Levi <shahar_levi@ti.com>
---
 drivers/net/wireless/wl12xx/wl1271_acx.c  |   55 +++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/wl1271_acx.h  |    4 ++
 drivers/net/wireless/wl12xx/wl1271_main.c |   42 ++++++++++++++++++++++
 3 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
index cc6b7d8..f82656e 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
@@ -1317,6 +1317,61 @@ out:
 	return ret;
 }
 
+/*
+ * wl1271_acx_set_ba_session
+ * configure BA session initiator\receiver parameters setting in the FW.
+ */
+int wl1271_acx_set_ba_session(struct wl1271 *wl,
+							  u16 id,
+							  u8 tid_index,
+							  u8 policy)
+{
+	struct wl1271_acx_ba_session_policy *acx;
+	int ret = 0;
+	/*
+	* Note, currently this value will be set to FFFFFFFFFFFF to indicate
+	* it is relevant for all peers.
+	*/
+	u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+
+	wl1271_debug(DEBUG_ACX, "acx ba session setting");
+
+	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
+	if (!acx) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(acx->mac_address, mac_address, ETH_ALEN);
+	acx->tid = tid_index;
+	acx->policy = policy;
+	acx->win_size = BA_RECEIVER_WIN_SIZE;
+
+	if (ACX_BA_SESSION_INITIATOR_POLICY == id)
+		acx->inactivity_timeout = BA_INACTIVITY_TIMEOUT;
+	else
+		if (ACX_BA_SESSION_RESPONDER_POLICY == id)
+			acx->inactivity_timeout = 0;
+		else {
+			wl1271_error("Incorrect acx command id=%x\n", id);
+			ret = -EINVAL;
+			goto out;
+		}
+
+	ret = wl1271_cmd_configure(wl,
+				   id,
+				   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/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index 6c3c7c0..f417624 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -1205,6 +1205,10 @@ 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,
+							  u16 id,
+							  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/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index af092f4..53c03e5 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1128,6 +1128,22 @@ static void wl1271_configure_filters(struct wl1271 *wl, unsigned int filters)
 	}
 }
 
+/*
+ * wl1271_set_ba_policies
+ * Set the BA session policies to the FW.
+ */
+static void wl1271_set_ba_policies(struct wl1271 *wl)
+{
+	u8	tid_index;
+
+	/* 802.11n BA session setting */
+	for (tid_index = 0; tid_index < CONF_TX_MAX_TID_COUNT; ++tid_index)
+			wl1271_acx_set_ba_session(wl,
+				ACX_BA_SESSION_INITIATOR_POLICY,
+				tid_index,
+				true);
+}
+
 static int wl1271_dummy_join(struct wl1271 *wl)
 {
 	int ret = 0;
@@ -2056,6 +2072,32 @@ 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)
+{
+		int ret;
+
+		switch (action) {
+		case IEEE80211_AMPDU_RX_START:
+		case IEEE80211_AMPDU_RX_STOP:
+
+		/* The BA initiator session management in FW independently */
+		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;
+		}
+
+		return ret;
+}
+
 /* can't be const, mac80211 writes to this */
 static struct ieee80211_rate wl1271_rates[] = {
 	{ .bitrate = 10,
-- 
1.6.0.4

[PATCH 03/03] wl1271: BA Initiator support, active BA ability

[Shahar Levi]

Add calling to BA policies after join and connect new ops.

Signed-off-by: Shahar Levi <shahar_levi@ti.com>
---
 drivers/net/wireless/wl12xx/wl1271_acx.c  |   10 +++-------
 drivers/net/wireless/wl12xx/wl1271_main.c |   14 ++++++++++++--
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
index f82656e..18ac48f 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
@@ -1328,11 +1328,6 @@ int wl1271_acx_set_ba_session(struct wl1271 *wl,
 {
 	struct wl1271_acx_ba_session_policy *acx;
 	int ret = 0;
-	/*
-	* Note, currently this value will be set to FFFFFFFFFFFF to indicate
-	* it is relevant for all peers.
-	*/
-	u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 
 	wl1271_debug(DEBUG_ACX, "acx ba session setting");
 
@@ -1342,14 +1337,14 @@ int wl1271_acx_set_ba_session(struct wl1271 *wl,
 		goto out;
 	}
 
-	memcpy(acx->mac_address, mac_address, ETH_ALEN);
+	memcpy(acx->mac_address, wl->bssid, ETH_ALEN);
 	acx->tid = tid_index;
 	acx->policy = policy;
 	acx->win_size = BA_RECEIVER_WIN_SIZE;
 
 	if (ACX_BA_SESSION_INITIATOR_POLICY == id)
 		acx->inactivity_timeout = BA_INACTIVITY_TIMEOUT;
-	else
+	else {
 		if (ACX_BA_SESSION_RESPONDER_POLICY == id)
 			acx->inactivity_timeout = 0;
 		else {
@@ -1357,6 +1352,7 @@ int wl1271_acx_set_ba_session(struct wl1271 *wl,
 			ret = -EINVAL;
 			goto out;
 		}
+	}
 
 	ret = wl1271_cmd_configure(wl,
 				   id,
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 53c03e5..b4e3b02 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1725,6 +1725,7 @@ 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);
+	bool set_ba = false;
 	bool do_join = false;
 	bool set_assoc = false;
 	int ret;
@@ -1950,7 +1951,9 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 							     true);
 			ret = wl1271_acx_set_ht_information(wl,
 						bss_conf->ht_operation_mode);
-			}
+
+			set_ba = true;
+		}
 		else
 			if (changed & BSS_CHANGED_ASSOC)
 				ret = wl1271_acx_set_ht_capabilities(wl,
@@ -1977,6 +1980,9 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 			wl1271_warning("cmd join failed %d", ret);
 			goto out_sleep;
 		}
+
+		if (set_ba)
+			wl1271_set_ba_policies(wl);
 	}
 
 out_sleep:
@@ -2080,10 +2086,13 @@ int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
 		int ret;
 
 		switch (action) {
+		/* Falling break here on purpose until we add BA receiver support */
 		case IEEE80211_AMPDU_RX_START:
 		case IEEE80211_AMPDU_RX_STOP:
 
-		/* The BA initiator session management in FW independently */
+		/* 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:
@@ -2351,6 +2360,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.6.0.4

Re: [PATCH 01/03] wl1271: BA Initiator support, Add Definitions

[Luciano Coelho]

On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
> New acx cmd wl1271_acx_ba_session_policy to set BA policy to the FW.
> Macros to use with BA setting.
> 
> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
> ---

You can merge this patch with the next patch, no need to make the
changes first in the header files and then in the c files, since they go
very much hand in hand.


>  drivers/net/wireless/wl12xx/wl1271_acx.h  |   16 ++++++++++++++++
>  drivers/net/wireless/wl12xx/wl1271_conf.h |    3 +++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
> index b8da1bc..6c3c7c0 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
> @@ -1055,6 +1055,22 @@ struct wl1271_acx_ht_information {
>  	u8 padding[3];
>  } __packed;
>  
> +/*
> + * BA sessen interface structure
> + */

BA session.  Actually you can get rid of this comment, as it is obvious
from the name of the structure that this is about BA sessions.

> +struct wl1271_acx_ba_session_policy {
> +	struct acx_header header;
> +	/* Mac address of: SA as receiver / RA as initiator */
> +	u8 mac_address[ETH_ALEN];

Is this really SA for receiver and RA for initiator? Not SA and DA? Or
TA and RA?


> +	u8 tid;			/* TID */

Comment here is unnecessary.

> +	u8 policy;		/* Enable / Disable */

Could we change policy to enable here? Then the comment can go away too,
because the name of the element will be clear enough already.


> +	u16 win_size;		/* windows size in num of packet */

"number of packets"


> +	u16 inactivity_timeout;	/* as initiator inactivity timeout
> +				 * in time units(TU) of 1024us.
> +				 * as receiver reserved
> +				 */

The comment style is wrong.


> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
> index b3e608e..63a0a9a 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
> @@ -94,6 +94,9 @@ enum {
>  #define HW_BG_RATES_MASK	0xffff
>  #define HW_HT_RATES_OFFSET	16
>  
> +#define BA_RECEIVER_WIN_SIZE 8
> +#define BA_INACTIVITY_TIMEOUT 10000

This should be added as a real configuration, like the other stuff in
the wl1271_conf.h file, which is then used in wl1271_main.c
default_conf.  Please define a struct conf_ba with win_size and
inactivity_timeout elements and set the actual values in the main file.


-- 
Cheers,
Luca.

Re: [PATCH 02/03] wl1271: BA Initiator support, BA functions

[Luciano Coelho]

On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
> New ops support ampdu_action as initiator returns EINVAL due to
> the fact that BA initiator session manage in the FW independently.
> acx function set BA policies.

Please rephrase this as it is hard to understand what the patch really
is about.


> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
> ---

Some more, mostly style comments.  Having mostly style-related comments
means that your implementation is really good and I appreciate it! It
just need a bit of polishing and parfumerie ;)

This patch can also be merged with the next one.  This doesn't add any
real functionality and there are things added here (for example the
mac_addr = FFFFFFFFFFFF is not used at all) that are changed in the next
patch.  This just confuses things.


> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
> index cc6b7d8..f82656e 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_acx.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
> @@ -1317,6 +1317,61 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * wl1271_acx_set_ba_session

Remove this line.  And then the comment can be in one line.


> + * configure BA session initiator\receiver parameters setting in the FW.
> + */
> +int wl1271_acx_set_ba_session(struct wl1271 *wl,
> +							  u16 id,
> +							  u8 tid_index,
> +							  u8 policy)

Fix the indentation and maybe it all fits in one line?


> +{
> +	struct wl1271_acx_ba_session_policy *acx;
> +	int ret = 0;
> +	/*
> +	* Note, currently this value will be set to FFFFFFFFFFFF to indicate
> +	* it is relevant for all peers.
> +	*/
> +	u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};

No need to do this here, since this code is not called and, when you
call it (ie. in patch 03/03), you remove this FF'ing anyway. ;)


> +	if (ACX_BA_SESSION_INITIATOR_POLICY == id)

Please use natural order (ie. id == ACX_BA_SESSION_INITIATOR_POLICY).


> +		acx->inactivity_timeout = BA_INACTIVITY_TIMEOUT;
> +	else
> +		if (ACX_BA_SESSION_RESPONDER_POLICY == id)
> +			acx->inactivity_timeout = 0;
> +		else {
> +			wl1271_error("Incorrect acx command id=%x\n", id);
> +			ret = -EINVAL;
> +			goto out;
> +		}

Use this structure for the if else statements:

if (...) {
	...
} else if (...) {
	...
} else {
	...
}

Or a switch-case.


> +
> +	ret = wl1271_cmd_configure(wl,
> +				   id,
> +				   acx,
> +				   sizeof(*acx));

Fits in one line.


> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
> index 6c3c7c0..f417624 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
> @@ -1205,6 +1205,10 @@ 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,
> +							  u16 id,
> +							  u8 tid_index,
> +							  u8 policy);

Indentation.


> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
> index af092f4..53c03e5 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
> @@ -1128,6 +1128,22 @@ static void wl1271_configure_filters(struct wl1271 *wl, unsigned int filters)
>  	}
>  }
>  
> +/*
> + * wl1271_set_ba_policies

Same as earlier: remove this line.  And then the comment can be in one
line.


> + * Set the BA session policies to the FW.
> + */
> +static void wl1271_set_ba_policies(struct wl1271 *wl)
> +{
> +	u8	tid_index;

No need for the TAB between u8 and tid_index.


> @@ -2056,6 +2072,32 @@ 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)
> +{
> +		int ret;
> +
> +		switch (action) {
> +		case IEEE80211_AMPDU_RX_START:
> +		case IEEE80211_AMPDU_RX_STOP:
> +
> +		/* The BA initiator session management in FW independently */
> +		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;
> +		}
> +
> +		return ret;
> +}

This function doesn't do anything, really.  Except for returning -EINVAL
in TX cases.  Is it really needed?


-- 
Cheers,
Luca.

Re: [PATCH 03/03] wl1271: BA Initiator support, active BA ability

[Luciano Coelho]

On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
> Add calling to BA policies after join and connect new ops.
> 
> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
> ---

I'll wait until you merge these three patches before reviewing this one,
as it seems to be mostly fixing stuff from the previous patch.


-- 
Cheers,
Luca.

Re: [PATCH 01/03] wl1271: BA Initiator support, Add Definitions

[Shahar Levi]

Thanks for the review.
All will be fix on v2.
Two comments inline.
regards,
Shahar

Luciano Coelho wrote:
> On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
>> New acx cmd wl1271_acx_ba_session_policy to set BA policy to the FW.
>> Macros to use with BA setting.
>>
>> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> ---
> 
> You can merge this patch with the next patch, no need to make the
> changes first in the header files and then in the c files, since they go
> very much hand in hand.
> 
> 
>>  drivers/net/wireless/wl12xx/wl1271_acx.h  |   16 ++++++++++++++++
>>  drivers/net/wireless/wl12xx/wl1271_conf.h |    3 +++
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> index b8da1bc..6c3c7c0 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> @@ -1055,6 +1055,22 @@ struct wl1271_acx_ht_information {
>>  	u8 padding[3];
>>  } __packed;
>>  
>> +/*
>> + * BA sessen interface structure
>> + */
> 
> BA session.  Actually you can get rid of this comment, as it is obvious
> from the name of the structure that this is about BA sessions.
> 
>> +struct wl1271_acx_ba_session_policy {
>> +	struct acx_header header;
>> +	/* Mac address of: SA as receiver / RA as initiator */
>> +	u8 mac_address[ETH_ALEN];
> 
> Is this really SA for receiver and RA for initiator? Not SA and DA? Or
> TA and RA?
yes, as initiator it is the receiver address, as receiver it is source 
address. I change the comment to:
Mac address of the peer: SA as receiver / RA as initiator

> 
> 
>> +	u8 tid;			/* TID */
> 
> Comment here is unnecessary.
> 
>> +	u8 policy;		/* Enable / Disable */
> 
> Could we change policy to enable here? Then the comment can go away too,
> because the name of the element will be clear enough already.
> 
> 
>> +	u16 win_size;		/* windows size in num of packet */
> 
> "number of packets"
> 
> 
>> +	u16 inactivity_timeout;	/* as initiator inactivity timeout
>> +				 * in time units(TU) of 1024us.
>> +				 * as receiver reserved
>> +				 */
> 
> The comment style is wrong.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> index b3e608e..63a0a9a 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> @@ -94,6 +94,9 @@ enum {
>>  #define HW_BG_RATES_MASK	0xffff
>>  #define HW_HT_RATES_OFFSET	16
>>  
>> +#define BA_RECEIVER_WIN_SIZE 8
move to wl1271_acx.h, not configurable.

>> +#define BA_INACTIVITY_TIMEOUT 10000
> 
> This should be added as a real configuration, like the other stuff in
> the wl1271_conf.h file, which is then used in wl1271_main.c
> default_conf.  Please define a struct conf_ba with win_size and
> inactivity_timeout elements and set the actual values in the main file.
> 
> 

Re: [PATCH 02/03] wl1271: BA Initiator support, BA functions

[Shahar Levi]

Thanks for the review. most will be fix on v2.
Two comments inline.
regards,
Shahar

Luciano Coelho wrote:
> On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
>> New ops support ampdu_action as initiator returns EINVAL due to
>> the fact that BA initiator session manage in the FW independently.
>> acx function set BA policies.
> 
> Please rephrase this as it is hard to understand what the patch really
> is about.
> 
> 
>> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> ---
> 
> Some more, mostly style comments.  Having mostly style-related comments
> means that your implementation is really good and I appreciate it! It
> just need a bit of polishing and parfumerie ;)
> 
> This patch can also be merged with the next one.  This doesn't add any
> real functionality and there are things added here (for example the
> mac_addr = FFFFFFFFFFFF is not used at all) that are changed in the next
> patch.  This just confuses things.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
>> index cc6b7d8..f82656e 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
>> @@ -1317,6 +1317,61 @@ out:
>>  	return ret;
>>  }
>>  
>> +/*
>> + * wl1271_acx_set_ba_session
> 
> Remove this line.  And then the comment can be in one line.
> 
> 
>> + * configure BA session initiator\receiver parameters setting in the FW.
>> + */
>> +int wl1271_acx_set_ba_session(struct wl1271 *wl,
>> +							  u16 id,
>> +							  u8 tid_index,
>> +							  u8 policy)
> 
> Fix the indentation and maybe it all fits in one line?
one character is go beyond.

> 
> 
>> +{
>> +	struct wl1271_acx_ba_session_policy *acx;
>> +	int ret = 0;
>> +	/*
>> +	* Note, currently this value will be set to FFFFFFFFFFFF to indicate
>> +	* it is relevant for all peers.
>> +	*/
>> +	u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> 
> No need to do this here, since this code is not called and, when you
> call it (ie. in patch 03/03), you remove this FF'ing anyway. ;)
> 
> 
>> +	if (ACX_BA_SESSION_INITIATOR_POLICY == id)
> 
> Please use natural order (ie. id == ACX_BA_SESSION_INITIATOR_POLICY).
> 
> 
>> +		acx->inactivity_timeout = BA_INACTIVITY_TIMEOUT;
>> +	else
>> +		if (ACX_BA_SESSION_RESPONDER_POLICY == id)
>> +			acx->inactivity_timeout = 0;
>> +		else {
>> +			wl1271_error("Incorrect acx command id=%x\n", id);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
> 
> Use this structure for the if else statements:
> 
> if (...) {
> 	...
> } else if (...) {
> 	...
> } else {
> 	...
> }
> 
> Or a switch-case.
> 
> 
>> +
>> +	ret = wl1271_cmd_configure(wl,
>> +				   id,
>> +				   acx,
>> +				   sizeof(*acx));
> 
> Fits in one line.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> index 6c3c7c0..f417624 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> @@ -1205,6 +1205,10 @@ 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,
>> +							  u16 id,
>> +							  u8 tid_index,
>> +							  u8 policy);
> 
> Indentation.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
>> index af092f4..53c03e5 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
>> @@ -1128,6 +1128,22 @@ static void wl1271_configure_filters(struct wl1271 *wl, unsigned int filters)
>>  	}
>>  }
>>  
>> +/*
>> + * wl1271_set_ba_policies
> 
> Same as earlier: remove this line.  And then the comment can be in one
> line.
> 
> 
>> + * Set the BA session policies to the FW.
>> + */
>> +static void wl1271_set_ba_policies(struct wl1271 *wl)
>> +{
>> +	u8	tid_index;
> 
> No need for the TAB between u8 and tid_index.
> 
> 
>> @@ -2056,6 +2072,32 @@ 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)
>> +{
>> +		int ret;
>> +
>> +		switch (action) {
>> +		case IEEE80211_AMPDU_RX_START:
>> +		case IEEE80211_AMPDU_RX_STOP:
>> +
>> +		/* The BA initiator session management in FW independently */
>> +		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;
>> +		}
>> +
>> +		return ret;
>> +}
> 
> This function doesn't do anything, really.  Except for returning -EINVAL
> in TX cases.  Is it really needed?
you right, for TX we didn't need it. i will remove it.