In some cases the driver might want to change the
default dynamic ps timeout (e.g. coex activity adds
latency to the rx/tx path, which might result in
redundant psm entrance).
Introduce a new ieee80211_set_dyn_ps_timeout() function
to let low-level drivers change the default timeout.
Signed-off-by: Eliad Peller <[email protected]>
---
include/net/mac80211.h | 14 ++++++++++++++
net/mac80211/ieee80211_i.h | 3 +++
net/mac80211/iface.c | 1 +
net/mac80211/main.c | 3 +++
net/mac80211/mlme.c | 31 +++++++++++++++++++++++++++++++
net/mac80211/pm.c | 1 +
6 files changed, 53 insertions(+), 0 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5e5029b..0db4ba8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3133,6 +3133,20 @@ void ieee80211_disable_dyn_ps(struct ieee80211_vif *vif);
void ieee80211_enable_dyn_ps(struct ieee80211_vif *vif);
/**
+ * ieee80211_set_dyn_ps_timeout - set the default dynamic ps timeout
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @timeout: timeout in ms, -1 for default value.
+ *
+ * This function lets the driver set its default dynamic ps timeout.
+ * This timeout will be evaluated only if the user didn't set his own
+ * timeout (explicitly or implicitly, by setting network_latency).
+ * Setting timeout to -1 will cause this timeout to be ignored, and the
+ * default timeout (100ms) will be used.
+ */
+void ieee80211_set_dyn_ps_timeout(struct ieee80211_vif *vif, int timeout);
+
+/**
* ieee80211_cqm_rssi_notify - inform a configured connection quality monitoring
* rssi threshold triggered
*
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c204cee..d960a16 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -969,6 +969,7 @@ struct ieee80211_local {
struct ieee80211_sub_if_data *ps_sdata;
struct work_struct dynamic_ps_enable_work;
struct work_struct dynamic_ps_disable_work;
+ struct work_struct dynamic_ps_reconfig_work;
struct timer_list dynamic_ps_timer;
struct notifier_block network_latency_notifier;
struct notifier_block ifa_notifier;
@@ -978,6 +979,7 @@ struct ieee80211_local {
* this will override whatever chosen by mac80211 internally.
*/
int dynamic_ps_forced_timeout;
+ int dynamic_ps_driver_timeout;
int dynamic_ps_user_timeout;
bool disable_dynamic_ps;
@@ -1281,6 +1283,7 @@ u32 ieee80211_mandatory_rates(struct ieee80211_local *local,
void ieee80211_dynamic_ps_enable_work(struct work_struct *work);
void ieee80211_dynamic_ps_disable_work(struct work_struct *work);
+void ieee80211_dynamic_ps_reconfig_work(struct work_struct *work);
void ieee80211_dynamic_ps_timer(unsigned long data);
void ieee80211_send_nullfunc(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index eaa80a3..cf1ecd6 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -442,6 +442,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
ieee80211_configure_filter(local);
+ cancel_work_sync(&local->dynamic_ps_reconfig_work);
del_timer_sync(&local->dynamic_ps_timer);
cancel_work_sync(&local->dynamic_ps_enable_work);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index a5809a1..7f9cc77 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -650,6 +650,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
ieee80211_dynamic_ps_enable_work);
INIT_WORK(&local->dynamic_ps_disable_work,
ieee80211_dynamic_ps_disable_work);
+ INIT_WORK(&local->dynamic_ps_reconfig_work,
+ ieee80211_dynamic_ps_reconfig_work);
setup_timer(&local->dynamic_ps_timer,
ieee80211_dynamic_ps_timer, (unsigned long) local);
@@ -905,6 +907,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
local->hw.conf.listen_interval = local->hw.max_listen_interval;
local->dynamic_ps_forced_timeout = -1;
+ local->dynamic_ps_driver_timeout = -1;
result = ieee80211_wep_init(local);
if (result < 0)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index fb2f0f9..4274e94 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -556,6 +556,21 @@ void ieee80211_disable_dyn_ps(struct ieee80211_vif *vif)
}
EXPORT_SYMBOL(ieee80211_disable_dyn_ps);
+void ieee80211_set_dyn_ps_timeout(struct ieee80211_vif *vif, int timeout)
+{
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+ struct ieee80211_local *local = sdata->local;
+
+ WARN_ON(sdata->vif.type != NL80211_IFTYPE_STATION ||
+ !(local->hw.flags & IEEE80211_HW_SUPPORTS_PS) ||
+ (local->hw.flags & IEEE80211_HW_SUPPORTS_DYNAMIC_PS));
+
+ local->dynamic_ps_driver_timeout = timeout;
+ ieee80211_queue_work(&local->hw,
+ &local->dynamic_ps_reconfig_work);
+}
+EXPORT_SYMBOL(ieee80211_set_dyn_ps_timeout);
+
/* powersave */
static void ieee80211_enable_ps(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata)
@@ -687,6 +702,8 @@ void ieee80211_recalc_ps(struct ieee80211_local *local, s32 latency)
if (latency > (1900 * USEC_PER_MSEC) &&
latency != (2000 * USEC_PER_SEC))
timeout = 0;
+ else if (local->dynamic_ps_driver_timeout >= 0)
+ timeout = local->dynamic_ps_driver_timeout;
else
timeout = 100;
}
@@ -814,6 +831,20 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
netif_tx_wake_all_queues(sdata->dev);
}
+void ieee80211_dynamic_ps_reconfig_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local,
+ dynamic_ps_reconfig_work);
+
+ WARN_ON(!(local->hw.flags & IEEE80211_HW_SUPPORTS_PS) ||
+ (local->hw.flags & IEEE80211_HW_SUPPORTS_DYNAMIC_PS));
+
+ mutex_lock(&local->iflist_mtx);
+ ieee80211_recalc_ps(local, -1);
+ mutex_unlock(&local->iflist_mtx);
+}
+
void ieee80211_dynamic_ps_timer(unsigned long data)
{
struct ieee80211_local *local = (void *) data;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index 6326d34..afb52a5 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -69,6 +69,7 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
* Note that this particular timer doesn't need to be
* restarted at resume.
*/
+ cancel_work_sync(&local->dynamic_ps_reconfig_work);
cancel_work_sync(&local->dynamic_ps_enable_work);
del_timer_sync(&local->dynamic_ps_timer);
--
1.7.6.401.g6a319
Ohad Ben-Cohen <[email protected]> writes:
> On Tue, Sep 20, 2011 at 5:49 PM, Kalle Valo <[email protected]> wrote:
>> Ohad Ben-Cohen <[email protected]> writes:
>> > A lot has happened since then :) The wl12xx firmware today is vastly
>> > different and much improved.
>>
>> Yeah, quite a turn. That's good :)
>
> And it's getting even better.. we have some news:
>
> One of the things we've been pushing for for some time is to get real
> IEEE80211_HW_SUPPORTS_DYNAMIC_PS support. Coincidentally we've been
> just notified that this is materializing for wl12xx as we speak, so
> we'll be shifting to IEEE80211_HW_SUPPORTS_DYNAMIC_PS very soon.
>
> In that case, I can happily say this whole discussion is moot. We
> agree not to agree, but at least we don't have to resolve it :)
Wow, that's great!
Having the dynps implementation in hardware makes perfect sense. That
way you get to do all sorts of optimisation without affecting the host
and will save some power as well.
Thanks for kicking some sense to your firmware engineers ;)
--
Kalle Valo
On Sun, 2011-09-18 at 15:03 +0300, Eliad Peller wrote:
> In some cases the driver might want to change the
> default dynamic ps timeout (e.g. coex activity adds
> latency to the rx/tx path, which might result in
> redundant psm entrance).
>
> Introduce a new ieee80211_set_dyn_ps_timeout() function
> to let low-level drivers change the default timeout.
>
> Signed-off-by: Eliad Peller <[email protected]>
> ---
Let's see what Johannes is going to say about this change in mac80211,
but IIRC this timeout used to exist with WEXT, but it was not
implemented in nl80211. We (at Nokia, probably Juuso) tried to
implement it a long time ago, but after some discussions with Johannes,
it was decided that this value wouldn't be settable from userspace at
least. I don't know if it was considered setting it from the driver
side, though.
--
Cheers,
Luca.
Ohad Ben-Cohen <[email protected]> writes:
> On Tue, Sep 20, 2011 at 11:37 AM, Kalle Valo <[email protected]> wrote:
>> One extreme is that wl12xx would set IEEE80211_HW_SUPPORTS_DYNAMIC_PS
>> and reimplement the dynps timer in the driver. That way the driver
>> would have full control how it works and not complicate the mac80211
>> implementation.
>
> That sounds like an awful code duplication just to prevent a small API
> extension.
Heh, that's always relative and actually for me it's the opposite.
What you consider small API update is for me a major change as it
allows the driver control dynps timer parameters, which we haven't
allowed earlier.
And for me adding that dynps timer to a driver isn't that much more
code, basically it's just a simple timer. And I doubt that you even
need to stop the queues when changing the ps state like mac80211 does.
I would not describe it as "an awful lot of code".
Having your own time would allow you to change to the BT coex
implementation of the week your firmware provides and not interferece
with mac80211 and other drivers.
> Yes, adding API complicates the code, but isn't the whole point of
> mac80211 is to prevent egregious code duplication ?
Sure, but there's always a limit for everything. mac80211 is growing
all the time and becoming more complex. I think the "All in" approach
won't work for too long and we need to start considering how to keep
mac80211 still maintainable. Like I have said earlier, the power save
implementation is a good example of this.
--
Kalle Valo
Ohad Ben-Cohen <[email protected]> writes:
> On Tue, Sep 20, 2011 at 11:34 AM, Kalle Valo <[email protected]> wrote:
>> IIRC Juuso added ieee80211_enable/disable_dyn_ps() to make it possible
>> use BT COEX with wl12xx.
>
> A lot has happened since then :) The wl12xx firmware today is vastly
> different and much improved.
Yeah, quite a turn. That's good :)
>> Now you are saying that you actually want the opposite?
>
> Yes. The firmware is now capable of doing coex without enforcing PSM.
> This leads to a much better WLAN TP while BT is on.
So we can remove the enable/disable dynps interface soon?
>> But nevertheless I'm not still convinced that doing all this in
>> mac80211 is the right thing. Especially that this seems to be very
>> wl12xx specific, right?
>
> Not really, why do you consider this wl12xx specific more than any
> other hw offloading or mac80211 API that is used by a single driver ?
Can we please avoid hand waving and stick to the matter at hand,
please?
The problem is that the current mac80211 powersave implementation is a
HUGE mess and nobody fully understands it nor wants to touch it. We
should be trying to simplify that instead of creating it more complex.
I actually believe that it's not possible to properly support all the
different power save hardware designs out there, instead I think that
some of the most eccentric implementations should not be supported by
mac80211 at all. For example, waking up for beacons or multicast
frames is a good example, we should just rip them out from mac80211
and let drivers deal that crap.
Now I try to understand what would be the best option for your
requirement. Are there any other drivers which needs this
ieee80211_set_dyn_ps_timeout() interface? If there is more than one
driver needing, implementing this to mac80211 would make more sense.
--
Kalle Valo
On Mon, Sep 19, 2011 at 7:38 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2011-09-19 at 19:30 +0300, Eliad Peller wrote:
>
>> AFAIU from the coex guys, the scenario is something like this:
>> upon coex activity, the fw might delay its rx and tx paths. this means
>> that the fw might get a frame within the 100ms of the dyn ps, but
>> delay its processing and pass it up to the driver only later.
>> this will cause redundant psm enter (after 100ms) and psm exit (after
>> the fw passed the packet).
>> i'm not sure about the exact effect during coex operation, but
>> eventually these psm enter/exit affect the throughput.
>>
>> another point here, is that during a specific period (during auto_mode
>> on), there might or might not be coex activity. thus, we can't just
>> disable dyn_ps, as it will hurt throughput (when there is no coex
>> activity).
>>
>> bottom line - i'm not sure about all the details, but according to
>> their tests - it does improve the throughput.
>> (i can try getting better details if you have additional questions)
>
> It'd be interesting to see if we can just treat this as a "minimum awake
> time", kinda like going back to the range I thought about earlier.
>
do you mean something like:
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4274e94..57695e3 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -702,11 +702,12 @@ void ieee80211_recalc_ps(struct ieee80211_local
*local, s32 latency)
if (latency > (1900 * USEC_PER_MSEC) &&
latency != (2000 * USEC_PER_SEC))
timeout = 0;
- else if (local->dynamic_ps_driver_timeout >= 0)
- timeout = local->dynamic_ps_driver_timeout;
else
timeout = 100;
}
+ if (timeout && local->dynamic_ps_driver_timeout > timeout)
+ timeout = local->dynamic_ps_driver_timeout;
+
local->dynamic_ps_user_timeout = timeout;
if (!local->disable_dynamic_ps)
conf->dynamic_ps_timeout =
?
i don't find it much different.
Eliad.
On Tue, Sep 20, 2011 at 3:13 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2011-09-20 at 15:05 +0300, Eliad Peller wrote:
>
>> > It'd be interesting to see if we can just treat this as a "minimum awake
>> > time", kinda like going back to the range I thought about earlier.
>> >
>> do you mean something like:
>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> index 4274e94..57695e3 100644
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -702,11 +702,12 @@ void ieee80211_recalc_ps(struct ieee80211_local
>> *local, s32 latency)
>> ? ? ? ? ? ? ? ? ? ? ? ? if (latency > (1900 * USEC_PER_MSEC) &&
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? latency != (2000 * USEC_PER_SEC))
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? timeout = 0;
>> - ? ? ? ? ? ? ? ? ? ? ? else if (local->dynamic_ps_driver_timeout >= 0)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? timeout = local->dynamic_ps_driver_timeout;
>> ? ? ? ? ? ? ? ? ? ? ? ? else
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? timeout = 100;
>> ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? if (timeout && local->dynamic_ps_driver_timeout > timeout)
>> + ? ? ? ? ? ? ? ? ? ? ? timeout = local->dynamic_ps_driver_timeout;
>> +
>> ? ? ? ? ? ? ? ? local->dynamic_ps_user_timeout = timeout;
>> ? ? ? ? ? ? ? ? if (!local->disable_dynamic_ps)
>> ? ? ? ? ? ? ? ? ? ? ? ? conf->dynamic_ps_timeout =
>>
>> ?
>>
>> i don't find it much different.
>
> I did mean something like that, yes, but I was thinking we actually
> calculate the timeout based on latency requirements :)
>
i think that f(max_latency) -> dynamic_ps_timeout is not well defined,
so that's probably why we have to use hardcoded values here.
with the current code, i think the original patch is a bit better, as
the driver (for whatever reason) will also be able to decrease the
timeout.
however, i don't mind for either of them.
hopefully, the whole ps stuff should be redesigned at some point :)
Eliad.
On Tue, Sep 20, 2011 at 11:37 AM, Kalle Valo <[email protected]> wrote:
> One extreme is that wl12xx would set IEEE80211_HW_SUPPORTS_DYNAMIC_PS
> and reimplement the dynps timer in the driver. That way the driver
> would have full control how it works and not complicate the mac80211
> implementation.
That sounds like an awful code duplication just to prevent a small API
extension.
Yes, adding API complicates the code, but isn't the whole point of
mac80211 is to prevent egregious code duplication ?
On Tue, Sep 20, 2011 at 9:36 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Tue, Sep 20, 2011 at 7:49 AM, Kalle Valo <[email protected]> wrote:
>> Ohad Ben-Cohen <[email protected]> writes:
>>
>>> On Tue, Sep 20, 2011 at 11:34 AM, Kalle Valo <[email protected]> wrote:
>>>> IIRC Juuso added ieee80211_enable/disable_dyn_ps() to make it possible
>>>> use BT COEX with wl12xx.
>>>
>>> A lot has happened since then :) The wl12xx firmware today is vastly
>>> different and much improved.
>>
>> Yeah, quite a turn. That's good :)
>>
>>>> Now you are saying that you actually want the opposite?
>>>
>>> Yes. The firmware is now capable of doing coex without enforcing PSM.
>>> This leads to a much better WLAN TP while BT is on.
>>
>> So we can remove the enable/disable dynps interface soon?
>
> Eliad, can you elaborate a little on the behaviour before that
> required the enable/disable dyn ps? You say now the firmware is
> capable of doing BT coex *without* enforcing PSM, can you elaborate on
> that?
>
As Ohad noted, we'll probably shift to
IEEE80211_HW_SUPPORTS_DYNAMIC_PS soon, so this patch is redundant.
anyway, just for the record - the fw supports 2 (mutually exclusive)
coex events:
1. "soft gemini sense" event - the fw indicates coex activity is about
to start. since psm is needed in order to let the BT control the
antenna, and psm is controlled from the host, we disable dyn_ps, so we
won't get out of psm after sending a packet.
2. "auto mode" event - the fw enters a period, in which it might (and
might not) start coex (i.e. the coex activity is transparent to the
driver during this period). during this period, the fw enters and
exits psm independently. there are sub-periods in which there are not
bt activity (e.g. silence during a2dp?). that's why we don't want to
disable dyn_ps - it will badly hurt throughput.
that's what i remember from what i've been told a few months ago. i
also found it weird back then, but it did increase the throughput.
> From what I read so far it seems the firmware needs to be fixed so
> that it can handle atomically going in and out of PSM when dealing
> with BT Coex, one option that comes to mind is dropping the frames it
> has and pending if it is doing BT coex work, which introduces the
> extra delay you mentioned. The drop would likely also help with rate
> control on the other end adjusting itself to environmental factors, in
> this case btcoex event.
>
i'm not familiar with the fw code.
as mentioned above, we're moving to dyn_ps in fw soon, so from the
driver side, there's one less thing to worry about :)
anyway, thanks for your suggestions,
Eliad.
On Tue, Sep 20, 2011 at 5:49 PM, Kalle Valo <[email protected]> wrote:
> Ohad Ben-Cohen <[email protected]> writes:
> > A lot has happened since then :) The wl12xx firmware today is vastly
> > different and much improved.
>
> Yeah, quite a turn. That's good :)
And it's getting even better.. we have some news:
One of the things we've been pushing for for some time is to get real
IEEE80211_HW_SUPPORTS_DYNAMIC_PS support. Coincidentally we've been
just notified that this is materializing for wl12xx as we speak, so
we'll be shifting to IEEE80211_HW_SUPPORTS_DYNAMIC_PS very soon.
In that case, I can happily say this whole discussion is moot. We
agree not to agree, but at least we don't have to resolve it :)
On Mon, 2011-09-19 at 08:09 +0300, Luciano Coelho wrote:
> On Sun, 2011-09-18 at 15:03 +0300, Eliad Peller wrote:
> > In some cases the driver might want to change the
> > default dynamic ps timeout (e.g. coex activity adds
> > latency to the rx/tx path, which might result in
> > redundant psm entrance).
> Let's see what Johannes is going to say about this change in mac80211,
> but IIRC this timeout used to exist with WEXT, but it was not
> implemented in nl80211. We (at Nokia, probably Juuso) tried to
> implement it a long time ago, but after some discussions with Johannes,
> it was decided that this value wouldn't be settable from userspace at
> least. I don't know if it was considered setting it from the driver
> side, though.
Yeah it's a good question ... but in this case there actually is some
actual reason for it -- I think it's probably used for BT coexist?
Obviously the user can't make an informed choice in that scenario, but
the device might actually be able to? My biggest objection back then was
that the user has no real reason to play with it, and the latency
properties of it are better done in other ways.
Which actually makes me think that having a completely fixed value from
the driver might not be a good thing? I mean, yes, we allow wext to
override it, but really we want this to be controlled by the latency
requirements I think. That's obviously something of a pipe dream right
now since nothing seems to ever use that, but ...
So anyway, do we really want this to be completely fixed by the driver?
Maybe a range would be better?
johannes
On Mon, 2011-09-19 at 19:30 +0300, Eliad Peller wrote:
> AFAIU from the coex guys, the scenario is something like this:
> upon coex activity, the fw might delay its rx and tx paths. this means
> that the fw might get a frame within the 100ms of the dyn ps, but
> delay its processing and pass it up to the driver only later.
> this will cause redundant psm enter (after 100ms) and psm exit (after
> the fw passed the packet).
> i'm not sure about the exact effect during coex operation, but
> eventually these psm enter/exit affect the throughput.
>
> another point here, is that during a specific period (during auto_mode
> on), there might or might not be coex activity. thus, we can't just
> disable dyn_ps, as it will hurt throughput (when there is no coex
> activity).
>
> bottom line - i'm not sure about all the details, but according to
> their tests - it does improve the throughput.
> (i can try getting better details if you have additional questions)
It'd be interesting to see if we can just treat this as a "minimum awake
time", kinda like going back to the range I thought about earlier.
johannes
On Mon, 2011-09-19 at 19:01 +0300, Eliad Peller wrote:
> > Yeah it's a good question ... but in this case there actually is some
> > actual reason for it -- I think it's probably used for BT coexist?
> > Obviously the user can't make an informed choice in that scenario, but
> > the device might actually be able to? My biggest objection back then was
> > that the user has no real reason to play with it, and the latency
> > properties of it are better done in other ways.
> >
> i agree.
> however, note that the network_latency can only set the dynamic ps
> on/off. it can't control the timeout.
Oh, I thought it also controlled the timeout -- I seem to remember at
least discussing that back when this got added...
johannes
On Tue, 2011-09-20 at 15:05 +0300, Eliad Peller wrote:
> > It'd be interesting to see if we can just treat this as a "minimum awake
> > time", kinda like going back to the range I thought about earlier.
> >
> do you mean something like:
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 4274e94..57695e3 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -702,11 +702,12 @@ void ieee80211_recalc_ps(struct ieee80211_local
> *local, s32 latency)
> if (latency > (1900 * USEC_PER_MSEC) &&
> latency != (2000 * USEC_PER_SEC))
> timeout = 0;
> - else if (local->dynamic_ps_driver_timeout >= 0)
> - timeout = local->dynamic_ps_driver_timeout;
> else
> timeout = 100;
> }
> + if (timeout && local->dynamic_ps_driver_timeout > timeout)
> + timeout = local->dynamic_ps_driver_timeout;
> +
> local->dynamic_ps_user_timeout = timeout;
> if (!local->disable_dynamic_ps)
> conf->dynamic_ps_timeout =
>
> ?
>
> i don't find it much different.
I did mean something like that, yes, but I was thinking we actually
calculate the timeout based on latency requirements :)
johannes
Johannes Berg <[email protected]> writes:
>> bottom line - i'm not sure about all the details, but according to
>> their tests - it does improve the throughput.
>> (i can try getting better details if you have additional questions)
>
> It'd be interesting to see if we can just treat this as a "minimum awake
> time", kinda like going back to the range I thought about earlier.
Yeah. Or maybe have a separate event to postpone dynps timer or
something like that (if firmware informs host whenever there's coex
traffic).
One extreme is that wl12xx would set IEEE80211_HW_SUPPORTS_DYNAMIC_PS
and reimplement the dynps timer in the driver. That way the driver
would have full control how it works and not complicate the mac80211
implementation.
--
Kalle Valo
On Tue, Sep 20, 2011 at 7:49 AM, Kalle Valo <[email protected]> wrote:
> Ohad Ben-Cohen <[email protected]> writes:
>
>> On Tue, Sep 20, 2011 at 11:34 AM, Kalle Valo <[email protected]> wrote:
>>> IIRC Juuso added ieee80211_enable/disable_dyn_ps() to make it possible
>>> use BT COEX with wl12xx.
>>
>> A lot has happened since then :) The wl12xx firmware today is vastly
>> different and much improved.
>
> Yeah, quite a turn. That's good :)
>
>>> Now you are saying that you actually want the opposite?
>>
>> Yes. The firmware is now capable of doing coex without enforcing PSM.
>> This leads to a much better WLAN TP while BT is on.
>
> So we can remove the enable/disable dynps interface soon?
Eliad, can you elaborate a little on the behaviour before that
required the enable/disable dyn ps? You say now the firmware is
capable of doing BT coex *without* enforcing PSM, can you elaborate on
that?
>From what I read so far it seems the firmware needs to be fixed so
that it can handle atomically going in and out of PSM when dealing
with BT Coex, one option that comes to mind is dropping the frames it
has and pending if it is doing BT coex work, which introduces the
extra delay you mentioned. The drop would likely also help with rate
control on the other end adjusting itself to environmental factors, in
this case btcoex event.
Luis
Eliad Peller <[email protected]> writes:
> AFAIU from the coex guys, the scenario is something like this: upon
> coex activity, the fw might delay its rx and tx paths. this means
> that the fw might get a frame within the 100ms of the dyn ps, but
> delay its processing and pass it up to the driver only later. this
> will cause redundant psm enter (after 100ms) and psm exit (after the
> fw passed the packet). i'm not sure about the exact effect during
> coex operation, but eventually these psm enter/exit affect the
> throughput.
>
> another point here, is that during a specific period (during auto_mode
> on), there might or might not be coex activity. thus, we can't just
> disable dyn_ps, as it will hurt throughput (when there is no coex
> activity).
IIRC Juuso added ieee80211_enable/disable_dyn_ps() to make it possible
use BT COEX with wl12xx. Now you are saying that you actually want the
opposite? I'm confused now.
But nevertheless I'm not still convinced that doing all this in
mac80211 is the right thing. Especially that this seems to be very
wl12xx specific, right?
> bottom line - i'm not sure about all the details, but according to
> their tests - it does improve the throughput.
Firmware engineers are notarious in making quick hacks to improve one
special case and not thinking about the big picture ;)
--
Kalle Valo
Luciano Coelho <[email protected]> writes:
> On Sun, 2011-09-18 at 15:03 +0300, Eliad Peller wrote:
>> In some cases the driver might want to change the
>> default dynamic ps timeout (e.g. coex activity adds
>> latency to the rx/tx path, which might result in
>> redundant psm entrance).
>>
>> Introduce a new ieee80211_set_dyn_ps_timeout() function
>> to let low-level drivers change the default timeout.
>>
>> Signed-off-by: Eliad Peller <[email protected]>
>> ---
>
>
> Let's see what Johannes is going to say about this change in mac80211,
> but IIRC this timeout used to exist with WEXT, but it was not
> implemented in nl80211. We (at Nokia, probably Juuso) tried to
> implement it a long time ago, but after some discussions with Johannes,
> it was decided that this value wouldn't be settable from userspace at
> least. I don't know if it was considered setting it from the driver
> side, though.
My first thought about this was: "This is so wrong." :)
So we now have the wext interface for setting the timeout, we also use
the PM QoS framework to set it and with this patch even from drivers
can set it. That's quite a mess.
What are these "some cases" referred above? I'm just worried that this
is just a workaround for an issue and adding the extra complexity is a
high cost just to workaround something. Please remember that mac80211
power save is a big problem already now.
--
Kalle Valo
On Mon, Sep 19, 2011 at 6:41 PM, Kalle Valo <[email protected]> wrote:
> Luciano Coelho <[email protected]> writes:
>
>> On Sun, 2011-09-18 at 15:03 +0300, Eliad Peller wrote:
>>> In some cases the driver might want to change the
>>> default dynamic ps timeout (e.g. coex activity adds
>>> latency to the rx/tx path, which might result in
>>> redundant psm entrance).
>>>
>>> Introduce a new ieee80211_set_dyn_ps_timeout() function
>>> to let low-level drivers change the default timeout.
>>>
>>> Signed-off-by: Eliad Peller <[email protected]>
>>> ---
>>
>>
>> Let's see what Johannes is going to say about this change in mac80211,
>> but IIRC this timeout used to exist with WEXT, but it was not
>> implemented in nl80211. ?We (at Nokia, probably Juuso) tried to
>> implement it a long time ago, but after some discussions with Johannes,
>> it was decided that this value wouldn't be settable from userspace at
>> least. ?I don't know if it was considered setting it from the driver
>> side, though.
>
> My first thought about this was: "This is so wrong." :)
>
yeah, mine too :)
> So we now have the wext interface for setting the timeout, we also use
> the PM QoS framework to set it and with this patch even from drivers
> can set it. That's quite a mess.
>
> What are these "some cases" referred above? I'm just worried that this
> is just a workaround for an issue and adding the extra complexity is a
> high cost just to workaround something. Please remember that mac80211
> power save is a big problem already now.
>
AFAIU from the coex guys, the scenario is something like this:
upon coex activity, the fw might delay its rx and tx paths. this means
that the fw might get a frame within the 100ms of the dyn ps, but
delay its processing and pass it up to the driver only later.
this will cause redundant psm enter (after 100ms) and psm exit (after
the fw passed the packet).
i'm not sure about the exact effect during coex operation, but
eventually these psm enter/exit affect the throughput.
another point here, is that during a specific period (during auto_mode
on), there might or might not be coex activity. thus, we can't just
disable dyn_ps, as it will hurt throughput (when there is no coex
activity).
bottom line - i'm not sure about all the details, but according to
their tests - it does improve the throughput.
(i can try getting better details if you have additional questions)
Eliad.
On Mon, Sep 19, 2011 at 3:05 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2011-09-19 at 08:09 +0300, Luciano Coelho wrote:
>> On Sun, 2011-09-18 at 15:03 +0300, Eliad Peller wrote:
>> > In some cases the driver might want to change the
>> > default dynamic ps timeout (e.g. coex activity adds
>> > latency to the rx/tx path, which might result in
>> > redundant psm entrance).
>
>> Let's see what Johannes is going to say about this change in mac80211,
>> but IIRC this timeout used to exist with WEXT, but it was not
>> implemented in nl80211. ?We (at Nokia, probably Juuso) tried to
>> implement it a long time ago, but after some discussions with Johannes,
>> it was decided that this value wouldn't be settable from userspace at
>> least. ?I don't know if it was considered setting it from the driver
>> side, though.
>
> Yeah it's a good question ... but in this case there actually is some
> actual reason for it -- I think it's probably used for BT coexist?
> Obviously the user can't make an informed choice in that scenario, but
> the device might actually be able to? My biggest objection back then was
> that the user has no real reason to play with it, and the latency
> properties of it are better done in other ways.
>
i agree.
however, note that the network_latency can only set the dynamic ps
on/off. it can't control the timeout.
> Which actually makes me think that having a completely fixed value from
> the driver might not be a good thing? I mean, yes, we allow wext to
> override it, but really we want this to be controlled by the latency
> requirements I think. That's obviously something of a pipe dream right
> now since nothing seems to ever use that, but ...
>
> So anyway, do we really want this to be completely fixed by the driver?
> Maybe a range would be better?
>
i guess it might be better. but as it doesn't have any real use, and
the code is over-complicated anyway, i think we can leave it for now
:)
Eliad.
On Tue, Sep 20, 2011 at 11:34 AM, Kalle Valo <[email protected]> wrote:
> IIRC Juuso added ieee80211_enable/disable_dyn_ps() to make it possible
> use BT COEX with wl12xx.
A lot has happened since then :) The wl12xx firmware today is vastly
different and much improved.
> Now you are saying that you actually want the opposite?
Yes. The firmware is now capable of doing coex without enforcing PSM.
This leads to a much better WLAN TP while BT is on.
> But nevertheless I'm not still convinced that doing all this in
> mac80211 is the right thing. Especially that this seems to be very
> wl12xx specific, right?
Not really, why do you consider this wl12xx specific more than any
other hw offloading or mac80211 API that is used by a single driver ?