[PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

Allow the low level driver to set U-APSD parameters when operating in
managed mode.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 include/net/mac80211.h |    9 +++++++++
 net/mac80211/main.c    |    8 ++++++++
 net/mac80211/mlme.c    |    4 ++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4d6e6c6..36c3318 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1288,6 +1288,13 @@ enum ieee80211_hw_flags {
  *
  * @offchannel_tx_hw_queue: HW queue ID to use for offchannel TX
  *	(if %IEEE80211_HW_QUEUE_CONTROL is set)
+ *
+ * @sta_uapsd_queues: Bitmask of enabled U-APSD queues when operating in
+ *	managed mode (%IEEE80211_WMM_IE_STA_QOSINFO_AC_VO & co). Defaults
+ *	to all four ACs.
+ *
+ * @sta_uapsd_max_sp_len: Maximum number of buffered frames AP can deliver in
+ *	a service period. Defaults to %IEEE80211_WMM_IE_STA_QOSINFO_SP_ALL
  */
 struct ieee80211_hw {
 	struct ieee80211_conf conf;
@@ -1309,6 +1316,8 @@ struct ieee80211_hw {
 	u8 max_rx_aggregation_subframes;
 	u8 max_tx_aggregation_subframes;
 	u8 offchannel_tx_hw_queue;
+	unsigned int sta_uapsd_queues;
+	unsigned int sta_uapsd_max_sp_len;
 };
 
 /**
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index b70f7f0..56e6c61 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -594,6 +594,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	local->hw.max_rx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF;
 	local->hw.max_tx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF;
 	local->hw.offchannel_tx_hw_queue = IEEE80211_INVAL_HW_QUEUE;
+	local->hw.sta_uapsd_queues = IEEE80211_DEFAULT_UAPSD_QUEUES;
+	local->hw.sta_uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
 	local->hw.conf.long_frame_max_tx_count = wiphy->retry_long;
 	local->hw.conf.short_frame_max_tx_count = wiphy->retry_short;
 	local->user_power_level = -1;
@@ -708,6 +710,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	if (hw->max_report_rates == 0)
 		hw->max_report_rates = hw->max_rates;
 
+	if (hw->sta_uapsd_queues & ~IEEE80211_WMM_IE_STA_QOSINFO_AC_MASK)
+		return -EINVAL;
+
+	if (hw->sta_uapsd_max_sp_len & ~IEEE80211_WMM_IE_STA_QOSINFO_SP_MASK)
+		return -EINVAL;
+
 	/*
 	 * generic code guarantees at least one band,
 	 * set this very early because much code assumes
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 2b5235e..da52587 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2974,8 +2974,8 @@ void ieee80211_sta_setup_sdata(struct ieee80211_sub_if_data *sdata)
 
 	ifmgd->flags = 0;
 	ifmgd->powersave = sdata->wdev.ps;
-	ifmgd->uapsd_queues = IEEE80211_DEFAULT_UAPSD_QUEUES;
-	ifmgd->uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
+	ifmgd->uapsd_queues = sdata->local->hw.sta_uapsd_queues;
+	ifmgd->uapsd_max_sp_len = sdata->local->hw.sta_uapsd_max_sp_len;
 
 	mutex_init(&ifmgd->mtx);
 
-- 
1.7.9.5

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Arik Nemtsov <arik@wizery.com> writes:

> Allow the low level driver to set U-APSD parameters when operating in
> managed mode.

Why? I assume there is a reason for this...

-- 
Kalle Valo

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

On Wed, May 23, 2012 at 1:44 PM, Kalle Valo <kvalo@adurom.com> wrote:
> Arik Nemtsov <arik@wizery.com> writes:
>
>> Allow the low level driver to set U-APSD parameters when operating in
>> managed mode.
>
> Why? I assume there is a reason for this...

Of course. We want to change this in wlcore. We've encountered
compatibility problems with some APs when we set ACs other than VO to
U-APSD mode.

Arik

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Arik Nemtsov <arik@wizery.com> writes:

> On Wed, May 23, 2012 at 1:44 PM, Kalle Valo <kvalo@adurom.com> wrote:
>> Arik Nemtsov <arik@wizery.com> writes:
>>
>>> Allow the low level driver to set U-APSD parameters when operating in
>>> managed mode.
>>
>> Why? I assume there is a reason for this...
>
> Of course.

Then you could write it down to the commit log instead of forcing others
to ask.

> We want to change this in wlcore. We've encountered compatibility
> problems with some APs when we set ACs other than VO to U-APSD mode.

Sounds like a hack to do this in the driver. Why can't mac80211 do it?

-- 
Kalle Valo

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

On Wed, May 23, 2012 at 1:59 PM, Kalle Valo <kvalo@adurom.com> wrote:
>> We want to change this in wlcore. We've encountered compatibility
>> problems with some APs when we set ACs other than VO to U-APSD mode.
>
> Sounds like a hack to do this in the driver. Why can't mac80211 do it?

Well it may be an issue specific to our HW. We didn't want to change
the default behavior of all other mac80211 drivers.

I'm not sure what you mean by "hack" here. We just enable the driver
to override a default value, which is used by mac80211.

Arik

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Arik Nemtsov <arik@wizery.com> writes:

> On Wed, May 23, 2012 at 1:59 PM, Kalle Valo <kvalo@adurom.com> wrote:
>>> We want to change this in wlcore. We've encountered compatibility
>>> problems with some APs when we set ACs other than VO to U-APSD mode.
>>
>> Sounds like a hack to do this in the driver. Why can't mac80211 do it?
>
> Well it may be an issue specific to our HW. We didn't want to change
> the default behavior of all other mac80211 drivers.

I would prefer to consider all drivers first. But as you are not even
describing the problem and which APs are broken it's difficult for
anyone comment. Being verbose in the commit log doesn't hurt, quite the
opposite.

> I'm not sure what you mean by "hack" here.

Wikipedia has a good description:

"a kludge (or often a "hack") is a solution to a problem, doing a task,
or fixing a system that is inefficient, inelegant, or even unfathomable,
but which nevertheless (more or less) works."

I'm sure the solution you propose works but it's not elegant.

> We just enable the driver to override a default value, which is used
> by mac80211.

You are moving logic from mac80211 to the driver but it should be the
opposite, the driver should be dumb and mac80211 should control
everything. Didn't we have a similar discussion last year when talking
about controlling dynamic power save?

I would have expected to see a user space interface for controlling
these parameters, but not a driver interface.

-- 
Kalle Valo

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

On Wed, May 23, 2012 at 5:22 PM, Kalle Valo <kvalo@adurom.com> wrote:
>> We just enable the driver to override a default value, which is used
>> by mac80211.
>
> You are moving logic from mac80211 to the driver but it should be the
> opposite, the driver should be dumb and mac80211 should control
> everything. Didn't we have a similar discussion last year when talking
> about controlling dynamic power save?

I disagree. Not all mac80211 drivers are like ath9k. The
FW/lower-driver can be just as smart in certain areas (BA sessions for
instance).
Also, let's stay practical here. This is not a large and complex
feature like dynamic power save.

It's interesting to hear Johannes' take on this.

Arik

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Arik Nemtsov <arik@wizery.com> writes:

> On Wed, May 23, 2012 at 5:22 PM, Kalle Valo <kvalo@adurom.com> wrote:
>>> We just enable the driver to override a default value, which is used
>>> by mac80211.
>>
>> You are moving logic from mac80211 to the driver but it should be the
>> opposite, the driver should be dumb and mac80211 should control
>> everything. Didn't we have a similar discussion last year when talking
>> about controlling dynamic power save?
>
> I disagree. Not all mac80211 drivers are like ath9k. The
> FW/lower-driver can be just as smart in certain areas (BA sessions for
> instance).

But this was a workaround for an IOP problem with certain APs, right?
How is firmware going to be smart in that case?

It would help to see what you are exactly planning to do on wl12xx with
this interface. Currently I'm just guessing what your plans really are.

> Also, let's stay practical here. This is not a large and complex
> feature like dynamic power save.

So when making small changes it's doesn't matter if the patch looks
wrong? That sounds like a bad idea. What do we when have 50 of those
small patches?

> It's interesting to hear Johannes' take on this.

I'm sure he will answer when he finds the time. But instead crying
Johannes for help you could address my concerns. You are basically
ignoring my comments right now.

-- 
Kalle Valo

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

On Thu, May 24, 2012 at 8:12 AM, Kalle Valo <kvalo@adurom.com> wrote:
> But this was a workaround for an IOP problem with certain APs, right?
> How is firmware going to be smart in that case?
>
> It would help to see what you are exactly planning to do on wl12xx with
> this interface. Currently I'm just guessing what your plans really are.

The plan is to enable only VO as U-APSD (as I wrote in a previous
email). We've had problems with some APs (can't remember the models)
that assume non-VO ACs always use legacy PS. This would lead to
problems - our driver would never wake up. In that sense the problem
is general.

But in wl12xx the FW handles network PS on its own (PS-poll etc.), so
it may just be a bug with our FW. So hard-coding this into mac80211
seems a bit of an overkill.

Maybe you can offer a better solution?

Arik

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Janusz Dziedzic]

2012/5/24 Arik Nemtsov <arik@wizery.com>:
> On Thu, May 24, 2012 at 8:12 AM, Kalle Valo <kvalo@adurom.com> wrote:
>> But this was a workaround for an IOP problem with certain APs, right?
>> How is firmware going to be smart in that case?
>>
>> It would help to see what you are exactly planning to do on wl12xx with
>> this interface. Currently I'm just guessing what your plans really are.
>
> The plan is to enable only VO as U-APSD (as I wrote in a previous
> email). We've had problems with some APs (can't remember the models)
> that assume non-VO ACs always use legacy PS. This would lead to
> problems - our driver would never wake up. In that sense the problem
> is general.
>
> But in wl12xx the FW handles network PS on its own (PS-poll etc.), so
> it may just be a bug with our FW. So hard-coding this into mac80211
> seems a bit of an overkill.
>
> Maybe you can offer a better solution?
>

I am not sure this is better.
We do this litte bit different.
So disable all ACs by default in mac80211.

Next we are using wpa_supplicant:
p2p_set client_apsd - and pass required params via nl80211 to mac80211.
So, we could decide which ACs set.
Reassoc from supplicat is required after that.


BR
Janusz

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@gmail.com> writes:

> 2012/5/24 Arik Nemtsov <arik@wizery.com>:
>
>> Maybe you can offer a better solution?
>>
>
> I am not sure this is better.
> We do this litte bit different.
> So disable all ACs by default in mac80211.
>
> Next we are using wpa_supplicant:
> p2p_set client_apsd - and pass required params via nl80211 to mac80211.
> So, we could decide which ACs set.
> Reassoc from supplicat is required after that.

I was also thinking of something similar:

Be concervative in mac80211 and only enable U-APSD for VO, just like
Arik was planning to do only for wl12xx. And then add a userspace
interface for making it possible to control this and enable/disable
classes individually.

Or do we already have a user space interface for that?

-- 
Kalle Valo

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Arik Nemtsov]

On Thu, May 24, 2012 at 9:39 AM, Kalle Valo <kvalo@adurom.com> wrote:
> Janusz Dziedzic <janusz.dziedzic@gmail.com> writes:
>
>> 2012/5/24 Arik Nemtsov <arik@wizery.com>:
>>
>>> Maybe you can offer a better solution?
>>>
>>
>> I am not sure this is better.
>> We do this litte bit different.
>> So disable all ACs by default in mac80211.
>>
>> Next we are using wpa_supplicant:
>> p2p_set client_apsd - and pass required params via nl80211 to mac80211.
>> So, we could decide which ACs set.
>> Reassoc from supplicat is required after that.
>
> I was also thinking of something similar:
>
> Be concervative in mac80211 and only enable U-APSD for VO, just like
> Arik was planning to do only for wl12xx. And then add a userspace
> interface for making it possible to control this and enable/disable
> classes individually.
>
> Or do we already have a user space interface for that?

Well I'm fine with hard-coding it in mac80211 to be VO only.

Are we sure there are real use-cases for changing this dynamically?
One would have to find out the AP is "bad" (how?), change the queues
and then re-associate.
That's why I this user-space is an overkill. But if the consensus here
is that user space is preferred, I can live with that too.

Janusz - It seems driver_nl80211.c doesn't really pass down the
"uapsd" parameter to kernel, and consequently mac80211 doesn't get it.
Are you working with a non-cfg80211 based driver? Or maybe you have an
internal patch to fix this?
Btw, it seems the "uapsd" param can be set via the regular wpa_s
control interface, not only in p2p mode. It's not hard to enable this
from user space as well (and maybe add it to the config file).

Arik

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Janusz Dziedzic]

2012/5/25 Arik Nemtsov <arik@wizery.com>:
> On Thu, May 24, 2012 at 9:39 AM, Kalle Valo <kvalo@adurom.com> wrote:
>> Janusz Dziedzic <janusz.dziedzic@gmail.com> writes:
>>
>>> 2012/5/24 Arik Nemtsov <arik@wizery.com>:
>>>
>>>> Maybe you can offer a better solution?
>>>>
>>>
>>> I am not sure this is better.
>>> We do this litte bit different.
>>> So disable all ACs by default in mac80211.
>>>
>>> Next we are using wpa_supplicant:
>>> p2p_set client_apsd - and pass required params via nl80211 to mac80211.
>>> So, we could decide which ACs set.
>>> Reassoc from supplicat is required after that.
>>
>> I was also thinking of something similar:
>>
>> Be concervative in mac80211 and only enable U-APSD for VO, just like
>> Arik was planning to do only for wl12xx. And then add a userspace
>> interface for making it possible to control this and enable/disable
>> classes individually.
>>
>> Or do we already have a user space interface for that?
>
> Well I'm fine with hard-coding it in mac80211 to be VO only.
>
> Are we sure there are real use-cases for changing this dynamically?
> One would have to find out the AP is "bad" (how?), change the queues
> and then re-associate.
> That's why I this user-space is an overkill. But if the consensus here
> is that user space is preferred, I can live with that too.
>
> Janusz - It seems driver_nl80211.c doesn't really pass down the
> "uapsd" parameter to kernel, and consequently mac80211 doesn't get it.
> Are you working with a non-cfg80211 based driver? Or maybe you have an
> internal patch to fix this?
> Btw, it seems the "uapsd" param can be set via the regular wpa_s
> control interface, not only in p2p mode. It's not hard to enable this
> from user space as well (and maybe add it to the config file).
>

Yes, we are using cfg80211/mac80211 and have a patch for that.
But this is for old-compat so I think this should be rewritten for new
one, while WMM and MAX_SP attr was added in meantime...

Question is if this is best interface for application that required UAPSD?
For Android it could be fine, but for linux I am not sure. Maybe iw
param should be added?



BR
Janusz

Re: [PATCH] mac80211: allow setting default U-APSD queues/max_sp_len for STA

[Kalle Valo]

Arik Nemtsov <arik@wizery.com> writes:

> On Thu, May 24, 2012 at 8:12 AM, Kalle Valo <kvalo@adurom.com> wrote:
>> But this was a workaround for an IOP problem with certain APs, right?
>> How is firmware going to be smart in that case?
>>
>> It would help to see what you are exactly planning to do on wl12xx with
>> this interface. Currently I'm just guessing what your plans really are.
>
> The plan is to enable only VO as U-APSD (as I wrote in a previous
> email).

You mean you will hardcode it in wl12xx and with all APs only VO has
U-APSD enabled?

> We've had problems with some APs (can't remember the models) that
> assume non-VO ACs always use legacy PS. This would lead to problems -
> our driver would never wake up. In that sense the problem is general.

Yeah, that looks like a general problem but of course it's difficult to
make any conclusions with this little data.

-- 
Kalle Valo