2011-08-25 09:51:11

by Arik Nemtsov

[permalink] [raw]
Subject: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

When this flag is set, Tx aggregations will not be initiated even
if the hardware supports A-MPDU aggregation in general. This flag
is required for devices supporting (partial) software Rx-aggregation.

Signed-off-by: Arik Nemtsov <[email protected]>
---
include/net/mac80211.h | 4 ++++
net/mac80211/agg-tx.c | 3 ++-
net/mac80211/debugfs.c | 2 ++
net/mac80211/tx.c | 3 ++-
4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2f01d84..b21a5bb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1094,6 +1094,9 @@ enum sta_notify_cmd {
* stations based on the PM bit of incoming frames.
* Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
* the PS mode of connected stations.
+ *
+ * @IEEE80211_HW_TX_AMPDU_IN_HW_ONLY: The device handles TX aggregation
+ * strictly in HW. Packets should not be aggregated in software.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
@@ -1119,6 +1122,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_CQM_RSSI = 1<<20,
IEEE80211_HW_SUPPORTS_PER_STA_GTK = 1<<21,
IEEE80211_HW_AP_LINK_PS = 1<<22,
+ IEEE80211_HW_TX_AMPDU_IN_HW_ONLY = 1<<23,
};

/**
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index b7075f3..be59aef 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -364,7 +364,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
return -EINVAL;

if ((tid >= STA_TID_NUM) ||
- !(local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION))
+ !(local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) ||
+ (local->hw.flags & IEEE80211_HW_TX_AMPDU_IN_HW_ONLY))
return -EINVAL;

#ifdef CONFIG_MAC80211_HT_DEBUG
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 186e02f..6f1515d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -355,6 +355,8 @@ static ssize_t hwflags_read(struct file *file, char __user *user_buf,
sf += snprintf(buf + sf, mxln - sf, "SUPPORTS_PER_STA_GTK\n");
if (local->hw.flags & IEEE80211_HW_AP_LINK_PS)
sf += snprintf(buf + sf, mxln - sf, "AP_LINK_PS\n");
+ if (local->hw.flags & IEEE80211_HW_TX_AMPDU_IN_HW_ONLY)
+ sf += snprintf(buf + sf, mxln - sf, "HW_TX_AGGREGATION\n");

rv = simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
kfree(buf);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 69fd494..a0af2f3 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1232,7 +1232,8 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata,
tx->sta = sta_info_get(sdata, hdr->addr1);

if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
- (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
+ (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) &&
+ !(local->hw.flags & IEEE80211_HW_TX_AMPDU_IN_HW_ONLY)) {
struct tid_ampdu_tx *tid_tx;

qc = ieee80211_get_qos_ctl(hdr);
--
1.7.4.1



2011-08-29 06:21:10

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

On Thu, 2011-08-25 at 12:51 +0300, Arik Nemtsov wrote:
> When this flag is set, Tx aggregations will not be initiated even
> if the hardware supports A-MPDU aggregation in general. This flag
> is required for devices supporting (partial) software Rx-aggregation.
>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
> include/net/mac80211.h | 4 ++++
> net/mac80211/agg-tx.c | 3 ++-
> net/mac80211/debugfs.c | 2 ++
> net/mac80211/tx.c | 3 ++-
> 4 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 2f01d84..b21a5bb 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1094,6 +1094,9 @@ enum sta_notify_cmd {
> * stations based on the PM bit of incoming frames.
> * Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
> * the PS mode of connected stations.
> + *
> + * @IEEE80211_HW_TX_AMPDU_IN_HW_ONLY: The device handles TX aggregation
> + * strictly in HW. Packets should not be aggregated in software.

In addition to the discussion with Helmut -- this is also misleading
since mac80211 doesn't, right now, aggregate anything in software -- it
just handles session setup (which is what you don't want).

johannes


2011-08-25 09:51:19

by Arik Nemtsov

[permalink] [raw]
Subject: [PATCH 2/2] wl12xx: set mac80211 flags for aggregation support

We set the flag for A-MPDU support and also indicate that Tx aggregation
is performed in HW. This will prevent mac80211 from aggregating Tx
packets in software.

This patch depends on
"mac80211: add flag to indicate HW only Tx-agg support"

Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/main.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 7027a7e..fdeea4d 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -4478,7 +4478,9 @@ int wl1271_init_ieee80211(struct wl1271 *wl)
IEEE80211_HW_SUPPORTS_CQM_RSSI |
IEEE80211_HW_REPORTS_TX_ACK_STATUS |
IEEE80211_HW_SPECTRUM_MGMT |
- IEEE80211_HW_AP_LINK_PS;
+ IEEE80211_HW_AP_LINK_PS |
+ IEEE80211_HW_AMPDU_AGGREGATION |
+ IEEE80211_HW_TX_AMPDU_IN_HW_ONLY;

wl->hw->wiphy->cipher_suites = cipher_suites;
wl->hw->wiphy->n_cipher_suites = ARRAY_SIZE(cipher_suites);
--
1.7.4.1


2011-08-25 12:54:27

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

On Thu, Aug 25, 2011 at 2:38 PM, Arik Nemtsov <[email protected]> wrote:
> Tx aggregations are done entirely in HW.

Then your patch makes sense :)

Might be worth to note that in the commit message though.

Thanks,
Helmut

2011-08-25 09:59:54

by Luciano Coelho

[permalink] [raw]
Subject: Re: [PATCH 2/2] wl12xx: set mac80211 flags for aggregation support

On Thu, 2011-08-25 at 12:51 +0300, Arik Nemtsov wrote:
> We set the flag for A-MPDU support and also indicate that Tx aggregation
> is performed in HW. This will prevent mac80211 from aggregating Tx
> packets in software.
>
> This patch depends on
> "mac80211: add flag to indicate HW only Tx-agg support"

Please send patches with this kind of dependency separately. One for
mac80211 (which John will apply) and one for wl12xx, which I will apply
once the dependency is solved.

This avoids confusion as to who should apply what.

--
Cheers,
Luca.


2011-08-25 12:18:55

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

Hi,

On Thu, Aug 25, 2011 at 11:51 AM, Arik Nemtsov <[email protected]> wrote:
> When this flag is set, Tx aggregations will not be initiated even
> if the hardware supports A-MPDU aggregation in general. This flag
> is required for devices supporting (partial) software Rx-aggregation.

Is the hardware unable to do TX aggregation?
Or is it just not implemented?
Or is it entirely done in hw?

Instead of cluttering mac80211 with hw flags you could also just disallow tx
aggregation sessions in the ampdu_action callback in the driver.

Helmut

2011-08-25 12:38:34

by Arik Nemtsov

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

On Thu, Aug 25, 2011 at 15:18, Helmut Schaa <[email protected]> wrote:
> Hi,
>
> On Thu, Aug 25, 2011 at 11:51 AM, Arik Nemtsov <[email protected]> wrote:
>> When this flag is set, Tx aggregations will not be initiated even
>> if the hardware supports A-MPDU aggregation in general. This flag
>> is required for devices supporting (partial) software Rx-aggregation.
>
> Is the hardware unable to do TX aggregation?
> Or is it just not implemented?
> Or is it entirely done in hw?

Tx aggregations are done entirely in HW.

>
> Instead of cluttering mac80211 with hw flags you could also just disallow tx
> aggregation sessions in the ampdu_action callback in the driver.

Well the driver is doing some pretty heavy lifting before even calling
the ampdu_action callback. Namely, ieee80211_start_tx_ba_session()
doesn't call it. AFAICT, the return from the callback is only used as
an indication that Tx BA session are temporary unavailable, and
mac80211 should try again. This is not the case here.

Regards,
Arik

2011-08-29 19:43:42

by Arik Nemtsov

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

On Mon, Aug 29, 2011 at 09:21, Johannes Berg <[email protected]> wrote:
> On Thu, 2011-08-25 at 12:51 +0300, Arik Nemtsov wrote:
>> When this flag is set, Tx aggregations will not be initiated even
>> if the hardware supports A-MPDU aggregation in general. This flag
>> is required for devices supporting (partial) software Rx-aggregation.
>>
>> Signed-off-by: Arik Nemtsov <[email protected]>
>> ---
>> ?include/net/mac80211.h | ? ?4 ++++
>> ?net/mac80211/agg-tx.c ?| ? ?3 ++-
>> ?net/mac80211/debugfs.c | ? ?2 ++
>> ?net/mac80211/tx.c ? ? ?| ? ?3 ++-
>> ?4 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index 2f01d84..b21a5bb 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -1094,6 +1094,9 @@ enum sta_notify_cmd {
>> ? * ? stations based on the PM bit of incoming frames.
>> ? * ? Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
>> ? * ? the PS mode of connected stations.
>> + *
>> + * @IEEE80211_HW_TX_AMPDU_IN_HW_ONLY: The device handles TX aggregation
>> + * ? strictly in HW. Packets should not be aggregated in software.
>
> In addition to the discussion with Helmut -- this is also misleading
> since mac80211 doesn't, right now, aggregate anything in software -- it
> just handles session setup (which is what you don't want).

Sure. I'll rename it to something like
IEEE80211_HW_TX_AMPDU_SETUP_IN_HW (and change the description).

Arik

2011-08-25 13:09:38

by Arik Nemtsov

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: add flag to indicate HW only Tx-agg support

On Thu, Aug 25, 2011 at 15:54, Helmut Schaa <[email protected]> wrote:
> On Thu, Aug 25, 2011 at 2:38 PM, Arik Nemtsov <[email protected]> wrote:
>> Tx aggregations are done entirely in HW.
>
> Then your patch makes sense :)

Thanks.

>
> Might be worth to note that in the commit message though.

I'm waiting for more comments for now, but I agree the commit message
should be improved.

Arik