This adds support to configure the dynamic ps timeout via nl80211. The
relevant attribute is added to NL80211_CMD_SET_POWER_SAVE.
Signed-off-by: Juuso Oikarinen <[email protected]>
---
include/linux/nl80211.h | 2 ++
net/wireless/nl80211.c | 16 +++++++++++++++-
2 files changed, 17 insertions(+), 1 deletions(-)
diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
index 2ea3ede..c5f12e7 100644
--- a/include/linux/nl80211.h
+++ b/include/linux/nl80211.h
@@ -864,6 +864,8 @@ enum nl80211_attrs {
NL80211_ATTR_LOCAL_STATE_CHANGE,
+ NL80211_ATTR_PS_TIMEOUT,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index df5505b..709bda3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -151,6 +151,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
[NL80211_ATTR_PS_STATE] = { .type = NLA_U32 },
[NL80211_ATTR_CQM] = { .type = NLA_NESTED, },
[NL80211_ATTR_LOCAL_STATE_CHANGE] = { .type = NLA_FLAG },
+ [NL80211_ATTR_PS_TIMEOUT] = { .type = NLA_U32 },
};
/* policy for the attributes */
@@ -4682,6 +4683,7 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
struct net_device *dev;
u8 ps_state;
bool state;
+ int timeout = -1;
int err;
if (!info->attrs[NL80211_ATTR_PS_STATE]) {
@@ -4696,6 +4698,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
goto out;
}
+ if (info->attrs[NL80211_ATTR_PS_TIMEOUT]) {
+ timeout = nla_get_u32(info->attrs[NL80211_ATTR_PS_TIMEOUT]);
+ if (timeout < 0) {
+ err = -EINVAL;
+ goto out;
+ }
+ }
+
rtnl_lock();
err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
@@ -4710,11 +4720,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
}
state = (ps_state == NL80211_PS_ENABLED) ? true : false;
+ if (timeout < 0)
+ timeout = wdev->ps_timeout;
- if (state == wdev->ps)
+ if (state == wdev->ps && timeout == wdev->ps_timeout)
goto unlock_rdev;
wdev->ps = state;
+ wdev->ps_timeout = timeout;
if (rdev->ops->set_power_mgmt(wdev->wiphy, dev, wdev->ps,
wdev->ps_timeout))
@@ -4772,6 +4785,7 @@ static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
ps_state = NL80211_PS_DISABLED;
NLA_PUT_U32(msg, NL80211_ATTR_PS_STATE, ps_state);
+ NLA_PUT_U32(msg, NL80211_ATTR_PS_TIMEOUT, wdev->ps_timeout);
genlmsg_end(msg, hdr);
err = genlmsg_reply(msg, info);
--
1.6.3.3
On Tue, 2010-04-13 at 10:31 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> >> But we do not want to tie our hands to a simple timeout value because
> >> that only applies to the very crude dynamic ps implementation we have
> >> currently. What if, next year, we have an uber cool new power save
> >> implementation in mac80211 which doesn't use the timeout value at all,
> >> what do we do with the nl80211 timeout parameter then? Deprecate it?
> >
> > I agree that letting user-space configure a raw timeout is rather crude.
> > But in my view the problem really is that we don't have that uber-cool
> > power save feature of the future yet, and hence cannot fully take its
> > needs into account. So apparently, until we do, we have to live without
> > even the crude one?
> >
> > Or is it better to deprecate a semi-complex interface instead of a crude
> > one, if even that does not meet all requirements of whats to come?
>
> But nl80211 is a user space interface and there are special rules for
> that. Basically we need to support it a long time to avoid breaking
> existing user space applications. If we add new features to nl80211
> which we will deprecate later on, it will soon become a mess and
> difficult to maintain. That would create more work for the wireless
> community and I would like to minimise the extra work, naturally :)
>
> If this would affect only a kernel internal API (like mac80211 driver
> API), I would have no objections at all. That's always easy to change.
>
> >> I think power save control should happen outside nl80211. The problem
> >> here is that we want to inform kernel about events (or states) in user
> >> space (display turned off, user activity, voip call etc.). We need a
> >> different interface for that, nl80211 is not suitable for this. Also
> >> other subsystems than wireless would definitely benefit from this kind
> >> of information. The interface might already exist (pm qos?) or we have
> >> to create a new one, I haven't studied this that much.
> >
> > I looked into this slightly. I can see, that the mac80211 has support
> > for a latency-wish from user-space, using pm_qos. This seems to affect
> > only the dtim-period somehow (by the way, at least the wl1251 driver has
> > an obvious bug about this - if the AP dtim is > 1 and user-space
> > configures the latency properly, you can make the wl1251 miss broadcast
> > traffic in PSM.)
>
> Thanks, I wrote down that wl1251 issue and I'll take a look at it
> later on.
>
> >> I would recommend to see if the pm qos interface can be used to hint
> >> mac80211 dynamic ps enough so that you would get similar end result as
> >> with the WE power timeout value. It should be possible, but I haven't
> >> checked the code myself.
> >
> > The problem here is that the latency at least cannot be used in any
> > simple and/or general way I can think of to control the dynamic PS
> > timeout.
>
> Can't you do something like this:
>
> pm_qos >= 1000 -> timeout 0 ms (aka. full power save)
> pm_qos <= 100 -> timeout 100 ms
> pm_qos <= 50 -> timeout 300 ms
>
> That is, just some arbitrary numbers but which affect dynamic ps
> timeout. I haven't thought about the numbers at all, but they actually
> don't matter because it's easy to change them inside mac80211.
Theoretically I could, but I'm pretty sure whatever values I would
choose would be unacceptable by others, as they would be tuned for a
specific use.
Also, although AFAIK barely anyone uses the DTIM interval which is
determined based on th pm_qos, adding arbitrary rules like this to the
side risks breaking something for someone.
-Juuso
> > So we would still need to add some crude PS level stuff anyway,
> > unless someone comes up with that uber-cool solution of tomorrow
> > already today.
> >
> > I see there is fierce resistance, so I'll drop looking into this here.
> > As far as I can see, the result of this is that anyone using linux
> > wireless in their hand-held devices either cannot switch to nl80211
> > today or have to use nl80211 and wext simultaneously.
>
> If yoy need this right now, using wext for setting the timeout is the
> fastest way. But it's ugly :/
>
Ugly it is!
-Juuso
Johannes Berg <[email protected]> writes:
> On Mon, 2010-04-12 at 11:11 +0300, Juuso Oikarinen wrote:
>> This adds support to configure the dynamic ps timeout via nl80211. The
>> relevant attribute is added to NL80211_CMD_SET_POWER_SAVE.
>
> I'm not sure we even want this.
When I added power save support to nl80211, I deliberately left out
setting the timeout. In the future there will be more advanced methods
to control power save and the timeout value would be then meaningless.
--
Kalle Valo
On Mon, 2010-04-12 at 11:06 +0200, ext Johannes Berg wrote:
> On Mon, 2010-04-12 at 11:56 +0300, Juuso Oikarinen wrote:
>
> > We could obviously do that. This patch does not prevent adding
> > functionality ;)
>
> Well yeah but why let userspace make arbitrary decisions? :)
>
> > For desktops, obviously reduced latency is desirable, while increased
> > power consumption is not so much of an issue. For hand-held devices the
> > situation is often the opposite: in many situations we might want to
> > sacrifice some latency just to stretch the battery life that last inch
> > longer, possibly depending on the type of traffic we know we have going
> > on.
>
> I don't think this contradicts each other. And we can also factor in the
> network_latency pm_qos value. Keep in mind that the gain from the
> timeout goes down as it increases past the beacon interval.
You're right obviously. But what we're looking for here are timeout
values less than the beacon/DTIM interval.
There are triggers for reducing the timeout, other than those mentioned
by you above. A hand-held might for example attempt to save power based
on the amount of user activity on the device in general. That's
something only user space would know about, but still is a very
important factor in deciding the dynamic ps timeout value.
Also AFAIK currently in nl80211 there is no way go to "full power-save"
i.e. disable dynamic power-save, which is something user-space might
want to do to conserve power if the hand-held is not being used.
Are you more hesitant on the concept of configuring the dynamic ps
behavior from user-space or the granularity of control here? In the
nl80211 interface the literal timeout value could be replaced by
something to indicate the level of power-saving needed, for example.
> In some sense I think it would be smarter to implement a gradual
> powersave policy where the device first still wakes up for every beacon
> and only later goes down to waking up for DTIM only (which may or may
> not be the same ...)
Yes, this is a good idea, although I still feel it does not alleviate
the need for what I tried to explain above.
-Juuso
> johannes
>
Juuso Oikarinen <[email protected]> writes:
>> AFAIK the pm_qos values are not set in stone in any. Applications just
>> request something and kernel can do whatever it wants, even ignore it.
>> So I don't see any harm if we change how mac80211 uses pm_qos values.
>> And most probably this will change many times in the future.
>
> Ok, you convinced me. I will send a RFC patch with the above, and will
> sub-sequentially be flamed to death ;)
Nah, you are so used to flaming already that you don't feel a thing :)
> Be sure to look into that bug in wl1251 at some point, so I don't break
> it with this, if by chance I get this to go through ;)
I will.
--
Kalle Valo
On Mon, 2010-04-12 at 22:57 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> > There are triggers for reducing the timeout, other than those mentioned
> > by you above. A hand-held might for example attempt to save power based
> > on the amount of user activity on the device in general.
>
> Yes, this makes sense. We definitely want user space to inform kernel
> about different requirements.
>
> > That's something only user space would know about, but still is a
> > very important factor in deciding the dynamic ps timeout value.
>
> But we do not want to tie our hands to a simple timeout value because
> that only applies to the very crude dynamic ps implementation we have
> currently. What if, next year, we have an uber cool new power save
> implementation in mac80211 which doesn't use the timeout value at all,
> what do we do with the nl80211 timeout parameter then? Deprecate it?
>
> Also please remember we have also other cfg80211 drivers than
> mac80211. Finding a common ps interface for all of them is next to
> impossible. Some of them might support ps timeout, but some of them
> will not.
>
> > Also AFAIK currently in nl80211 there is no way go to "full power-save"
> > i.e. disable dynamic power-save, which is something user-space might
> > want to do to conserve power if the hand-held is not being used.
>
> With a more advanced power save implementation in mac80211 we should
> be able to this automatically, for example, based on information from
> PM qos interface, amount of data transfered etc.
>
> > Are you more hesitant on the concept of configuring the dynamic ps
> > behavior from user-space or the granularity of control here?
>
> For me it's the former.
>
> > In the nl80211 interface the literal timeout value could be replaced
> > by something to indicate the level of power-saving needed, for
> > example.
>
> Something like levels 1-5? This was proposed some time (years?) ago, I
> didn't like it then and I didn't like it now. It's just too vague.
>
> I think power save control should happen outside nl80211. The problem
> here is that we want to inform kernel about events (or states) in user
> space (display turned off, user activity, voip call etc.). We need a
> different interface for that, nl80211 is not suitable for this. Also
> other subsystems than wireless would definitely benefit from this kind
> of information. The interface might already exist (pm qos?) or we have
> to create a new one, I haven't studied this that much.
>
> >> In some sense I think it would be smarter to implement a gradual
> >> powersave policy where the device first still wakes up for every
> >> beacon and only later goes down to waking up for DTIM only (which
> >> may or may not be the same ...)
> >
> > Yes, this is a good idea, although I still feel it does not alleviate
> > the need for what I tried to explain above.
>
> The need is definitely valid, I don't deny that. It just needs to be
> implemented differently compared to what old Wireless Extensions did.
>
> I would recommend to see if the pm qos interface can be used to hint
> mac80211 dynamic ps enough so that you would get similar end result as
> with the WE power timeout value. It should be possible, but I haven't
> checked the code myself.
>
Okay, agreed. I will get back to the drawing board with this.
-Juuso
On Tue, Apr 13, 2010 at 2:10 AM, Juuso Oikarinen
<[email protected]> wrote:
> On Tue, 2010-04-13 at 11:07 +0200, ext Kalle Valo wrote:
>> Juuso Oikarinen <[email protected]> writes:
>>
>> >> > The problem here is that the latency at least cannot be used in any
>> >> > simple and/or general way I can think of to control the dynamic PS
>> >> > timeout.
>> >>
>> >> Can't you do something like this:
>> >>
>> >> pm_qos >= 1000 -> timeout 0 ms (aka. full power save)
>> >> pm_qos <= 100 -> timeout 100 ms
>> >> pm_qos <= 50 -> timeout 300 ms
>> >>
>> >> That is, just some arbitrary numbers but which affect dynamic ps
>> >> timeout. I haven't thought about the numbers at all, but they actually
>> >> don't matter because it's easy to change them inside mac80211.
>> >
>> > Theoretically I could, but I'm pretty sure whatever values I would
>> > choose would be unacceptable by others, as they would be tuned for a
>> > specific use.
>>
>> I don't see a problem with that. The current use of pm_qos is very
>> limited, I think it's all positive if we start using it more.
>>
>> > Also, although AFAIK barely anyone uses the DTIM interval which is
>> > determined based on th pm_qos, adding arbitrary rules like this to the
>> > side risks breaking something for someone.
>>
>> AFAIK the pm_qos values are not set in stone in any. Applications just
>> request something and kernel can do whatever it wants, even ignore it.
>> So I don't see any harm if we change how mac80211 uses pm_qos values.
>> And most probably this will change many times in the future.
>>
>
> Ok, you convinced me. I will send a RFC patch with the above, and will
> sub-sequentially be flamed to death ;)
>
> Be sure to look into that bug in wl1251 at some point, so I don't break
> it with this, if by chance I get this to go through ;)
If you do change this however, please document it. It took me a while
to review this before, it'd be nice to keep this documentation in
synch and updated as soon as your stuff gets merged.
http://wireless.kernel.org/en/developers/Documentation/pm-qos
Luis
Hi,
I looked into this slightly more, below some more thoughts.
On Mon, 2010-04-12 at 22:57 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> > There are triggers for reducing the timeout, other than those mentioned
> > by you above. A hand-held might for example attempt to save power based
> > on the amount of user activity on the device in general.
>
> Yes, this makes sense. We definitely want user space to inform kernel
> about different requirements.
>
> > That's something only user space would know about, but still is a
> > very important factor in deciding the dynamic ps timeout value.
>
> But we do not want to tie our hands to a simple timeout value because
> that only applies to the very crude dynamic ps implementation we have
> currently. What if, next year, we have an uber cool new power save
> implementation in mac80211 which doesn't use the timeout value at all,
> what do we do with the nl80211 timeout parameter then? Deprecate it?
I agree that letting user-space configure a raw timeout is rather crude.
But in my view the problem really is that we don't have that uber-cool
power save feature of the future yet, and hence cannot fully take its
needs into account. So apparently, until we do, we have to live without
even the crude one?
Or is it better to deprecate a semi-complex interface instead of a crude
one, if even that does not meet all requirements of whats to come?
> Also please remember we have also other cfg80211 drivers than
> mac80211. Finding a common ps interface for all of them is next to
> impossible. Some of them might support ps timeout, but some of them
> will not.
>
> > Also AFAIK currently in nl80211 there is no way go to "full power-save"
> > i.e. disable dynamic power-save, which is something user-space might
> > want to do to conserve power if the hand-held is not being used.
>
> With a more advanced power save implementation in mac80211 we should
> be able to this automatically, for example, based on information from
> PM qos interface, amount of data transfered etc.
>
> > Are you more hesitant on the concept of configuring the dynamic ps
> > behavior from user-space or the granularity of control here?
>
> For me it's the former.
>
> > In the nl80211 interface the literal timeout value could be replaced
> > by something to indicate the level of power-saving needed, for
> > example.
>
> Something like levels 1-5? This was proposed some time (years?) ago, I
> didn't like it then and I didn't like it now. It's just too vague.
>
> I think power save control should happen outside nl80211. The problem
> here is that we want to inform kernel about events (or states) in user
> space (display turned off, user activity, voip call etc.). We need a
> different interface for that, nl80211 is not suitable for this. Also
> other subsystems than wireless would definitely benefit from this kind
> of information. The interface might already exist (pm qos?) or we have
> to create a new one, I haven't studied this that much.
I looked into this slightly. I can see, that the mac80211 has support
for a latency-wish from user-space, using pm_qos. This seems to affect
only the dtim-period somehow (by the way, at least the wl1251 driver has
an obvious bug about this - if the AP dtim is > 1 and user-space
configures the latency properly, you can make the wl1251 miss broadcast
traffic in PSM.)
> >> In some sense I think it would be smarter to implement a gradual
> >> powersave policy where the device first still wakes up for every
> >> beacon and only later goes down to waking up for DTIM only (which
> >> may or may not be the same ...)
> >
> > Yes, this is a good idea, although I still feel it does not alleviate
> > the need for what I tried to explain above.
>
> The need is definitely valid, I don't deny that. It just needs to be
> implemented differently compared to what old Wireless Extensions did.
>
> I would recommend to see if the pm qos interface can be used to hint
> mac80211 dynamic ps enough so that you would get similar end result as
> with the WE power timeout value. It should be possible, but I haven't
> checked the code myself.
>
The problem here is that the latency at least cannot be used in any
simple and/or general way I can think of to control the dynamic PS
timeout. So we would still need to add some crude PS level stuff anyway,
unless someone comes up with that uber-cool solution of tomorrow already
today.
I see there is fierce resistance, so I'll drop looking into this here.
As far as I can see, the result of this is that anyone using linux
wireless in their hand-held devices either cannot switch to nl80211
today or have to use nl80211 and wext simultaneously.
-Juuso
Juuso Oikarinen <[email protected]> writes:
> There are triggers for reducing the timeout, other than those mentioned
> by you above. A hand-held might for example attempt to save power based
> on the amount of user activity on the device in general.
Yes, this makes sense. We definitely want user space to inform kernel
about different requirements.
> That's something only user space would know about, but still is a
> very important factor in deciding the dynamic ps timeout value.
But we do not want to tie our hands to a simple timeout value because
that only applies to the very crude dynamic ps implementation we have
currently. What if, next year, we have an uber cool new power save
implementation in mac80211 which doesn't use the timeout value at all,
what do we do with the nl80211 timeout parameter then? Deprecate it?
Also please remember we have also other cfg80211 drivers than
mac80211. Finding a common ps interface for all of them is next to
impossible. Some of them might support ps timeout, but some of them
will not.
> Also AFAIK currently in nl80211 there is no way go to "full power-save"
> i.e. disable dynamic power-save, which is something user-space might
> want to do to conserve power if the hand-held is not being used.
With a more advanced power save implementation in mac80211 we should
be able to this automatically, for example, based on information from
PM qos interface, amount of data transfered etc.
> Are you more hesitant on the concept of configuring the dynamic ps
> behavior from user-space or the granularity of control here?
For me it's the former.
> In the nl80211 interface the literal timeout value could be replaced
> by something to indicate the level of power-saving needed, for
> example.
Something like levels 1-5? This was proposed some time (years?) ago, I
didn't like it then and I didn't like it now. It's just too vague.
I think power save control should happen outside nl80211. The problem
here is that we want to inform kernel about events (or states) in user
space (display turned off, user activity, voip call etc.). We need a
different interface for that, nl80211 is not suitable for this. Also
other subsystems than wireless would definitely benefit from this kind
of information. The interface might already exist (pm qos?) or we have
to create a new one, I haven't studied this that much.
>> In some sense I think it would be smarter to implement a gradual
>> powersave policy where the device first still wakes up for every
>> beacon and only later goes down to waking up for DTIM only (which
>> may or may not be the same ...)
>
> Yes, this is a good idea, although I still feel it does not alleviate
> the need for what I tried to explain above.
The need is definitely valid, I don't deny that. It just needs to be
implemented differently compared to what old Wireless Extensions did.
I would recommend to see if the pm qos interface can be used to hint
mac80211 dynamic ps enough so that you would get similar end result as
with the WE power timeout value. It should be possible, but I haven't
checked the code myself.
--
Kalle Valo
Juuso Oikarinen <[email protected]> writes:
>> > The problem here is that the latency at least cannot be used in any
>> > simple and/or general way I can think of to control the dynamic PS
>> > timeout.
>>
>> Can't you do something like this:
>>
>> pm_qos >= 1000 -> timeout 0 ms (aka. full power save)
>> pm_qos <= 100 -> timeout 100 ms
>> pm_qos <= 50 -> timeout 300 ms
>>
>> That is, just some arbitrary numbers but which affect dynamic ps
>> timeout. I haven't thought about the numbers at all, but they actually
>> don't matter because it's easy to change them inside mac80211.
>
> Theoretically I could, but I'm pretty sure whatever values I would
> choose would be unacceptable by others, as they would be tuned for a
> specific use.
I don't see a problem with that. The current use of pm_qos is very
limited, I think it's all positive if we start using it more.
> Also, although AFAIK barely anyone uses the DTIM interval which is
> determined based on th pm_qos, adding arbitrary rules like this to the
> side risks breaking something for someone.
AFAIK the pm_qos values are not set in stone in any. Applications just
request something and kernel can do whatever it wants, even ignore it.
So I don't see any harm if we change how mac80211 uses pm_qos values.
And most probably this will change many times in the future.
--
Kalle Valo
Juuso Oikarinen <[email protected]> writes:
>> But we do not want to tie our hands to a simple timeout value because
>> that only applies to the very crude dynamic ps implementation we have
>> currently. What if, next year, we have an uber cool new power save
>> implementation in mac80211 which doesn't use the timeout value at all,
>> what do we do with the nl80211 timeout parameter then? Deprecate it?
>
> I agree that letting user-space configure a raw timeout is rather crude.
> But in my view the problem really is that we don't have that uber-cool
> power save feature of the future yet, and hence cannot fully take its
> needs into account. So apparently, until we do, we have to live without
> even the crude one?
>
> Or is it better to deprecate a semi-complex interface instead of a crude
> one, if even that does not meet all requirements of whats to come?
But nl80211 is a user space interface and there are special rules for
that. Basically we need to support it a long time to avoid breaking
existing user space applications. If we add new features to nl80211
which we will deprecate later on, it will soon become a mess and
difficult to maintain. That would create more work for the wireless
community and I would like to minimise the extra work, naturally :)
If this would affect only a kernel internal API (like mac80211 driver
API), I would have no objections at all. That's always easy to change.
>> I think power save control should happen outside nl80211. The problem
>> here is that we want to inform kernel about events (or states) in user
>> space (display turned off, user activity, voip call etc.). We need a
>> different interface for that, nl80211 is not suitable for this. Also
>> other subsystems than wireless would definitely benefit from this kind
>> of information. The interface might already exist (pm qos?) or we have
>> to create a new one, I haven't studied this that much.
>
> I looked into this slightly. I can see, that the mac80211 has support
> for a latency-wish from user-space, using pm_qos. This seems to affect
> only the dtim-period somehow (by the way, at least the wl1251 driver has
> an obvious bug about this - if the AP dtim is > 1 and user-space
> configures the latency properly, you can make the wl1251 miss broadcast
> traffic in PSM.)
Thanks, I wrote down that wl1251 issue and I'll take a look at it
later on.
>> I would recommend to see if the pm qos interface can be used to hint
>> mac80211 dynamic ps enough so that you would get similar end result as
>> with the WE power timeout value. It should be possible, but I haven't
>> checked the code myself.
>
> The problem here is that the latency at least cannot be used in any
> simple and/or general way I can think of to control the dynamic PS
> timeout.
Can't you do something like this:
pm_qos >= 1000 -> timeout 0 ms (aka. full power save)
pm_qos <= 100 -> timeout 100 ms
pm_qos <= 50 -> timeout 300 ms
That is, just some arbitrary numbers but which affect dynamic ps
timeout. I haven't thought about the numbers at all, but they actually
don't matter because it's easy to change them inside mac80211.
> So we would still need to add some crude PS level stuff anyway,
> unless someone comes up with that uber-cool solution of tomorrow
> already today.
>
> I see there is fierce resistance, so I'll drop looking into this here.
> As far as I can see, the result of this is that anyone using linux
> wireless in their hand-held devices either cannot switch to nl80211
> today or have to use nl80211 and wext simultaneously.
If yoy need this right now, using wext for setting the timeout is the
fastest way. But it's ugly :/
--
Kalle Valo
On Mon, 2010-04-12 at 11:11 +0300, Juuso Oikarinen wrote:
> This adds support to configure the dynamic ps timeout via nl80211. The
> relevant attribute is added to NL80211_CMD_SET_POWER_SAVE.
I'm not sure we even want this. Can't we calculate an appropriate value
based on the beacon interval etc. instead?
johannes
> Signed-off-by: Juuso Oikarinen <[email protected]>
> ---
> include/linux/nl80211.h | 2 ++
> net/wireless/nl80211.c | 16 +++++++++++++++-
> 2 files changed, 17 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
> index 2ea3ede..c5f12e7 100644
> --- a/include/linux/nl80211.h
> +++ b/include/linux/nl80211.h
> @@ -864,6 +864,8 @@ enum nl80211_attrs {
>
> NL80211_ATTR_LOCAL_STATE_CHANGE,
>
> + NL80211_ATTR_PS_TIMEOUT,
> +
> /* add attributes here, update the policy in nl80211.c */
>
> __NL80211_ATTR_AFTER_LAST,
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index df5505b..709bda3 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -151,6 +151,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
> [NL80211_ATTR_PS_STATE] = { .type = NLA_U32 },
> [NL80211_ATTR_CQM] = { .type = NLA_NESTED, },
> [NL80211_ATTR_LOCAL_STATE_CHANGE] = { .type = NLA_FLAG },
> + [NL80211_ATTR_PS_TIMEOUT] = { .type = NLA_U32 },
> };
>
> /* policy for the attributes */
> @@ -4682,6 +4683,7 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> struct net_device *dev;
> u8 ps_state;
> bool state;
> + int timeout = -1;
> int err;
>
> if (!info->attrs[NL80211_ATTR_PS_STATE]) {
> @@ -4696,6 +4698,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> goto out;
> }
>
> + if (info->attrs[NL80211_ATTR_PS_TIMEOUT]) {
> + timeout = nla_get_u32(info->attrs[NL80211_ATTR_PS_TIMEOUT]);
> + if (timeout < 0) {
> + err = -EINVAL;
> + goto out;
> + }
> + }
> +
> rtnl_lock();
>
> err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
> @@ -4710,11 +4720,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> }
>
> state = (ps_state == NL80211_PS_ENABLED) ? true : false;
> + if (timeout < 0)
> + timeout = wdev->ps_timeout;
>
> - if (state == wdev->ps)
> + if (state == wdev->ps && timeout == wdev->ps_timeout)
> goto unlock_rdev;
>
> wdev->ps = state;
> + wdev->ps_timeout = timeout;
>
> if (rdev->ops->set_power_mgmt(wdev->wiphy, dev, wdev->ps,
> wdev->ps_timeout))
> @@ -4772,6 +4785,7 @@ static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
> ps_state = NL80211_PS_DISABLED;
>
> NLA_PUT_U32(msg, NL80211_ATTR_PS_STATE, ps_state);
> + NLA_PUT_U32(msg, NL80211_ATTR_PS_TIMEOUT, wdev->ps_timeout);
>
> genlmsg_end(msg, hdr);
> err = genlmsg_reply(msg, info);
On Mon, 2010-04-12 at 11:56 +0300, Juuso Oikarinen wrote:
> We could obviously do that. This patch does not prevent adding
> functionality ;)
Well yeah but why let userspace make arbitrary decisions? :)
> For desktops, obviously reduced latency is desirable, while increased
> power consumption is not so much of an issue. For hand-held devices the
> situation is often the opposite: in many situations we might want to
> sacrifice some latency just to stretch the battery life that last inch
> longer, possibly depending on the type of traffic we know we have going
> on.
I don't think this contradicts each other. And we can also factor in the
network_latency pm_qos value. Keep in mind that the gain from the
timeout goes down as it increases past the beacon interval.
In some sense I think it would be smarter to implement a gradual
powersave policy where the device first still wakes up for every beacon
and only later goes down to waking up for DTIM only (which may or may
not be the same ...)
johannes
On Tue, 2010-04-13 at 11:07 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> >> > The problem here is that the latency at least cannot be used in any
> >> > simple and/or general way I can think of to control the dynamic PS
> >> > timeout.
> >>
> >> Can't you do something like this:
> >>
> >> pm_qos >= 1000 -> timeout 0 ms (aka. full power save)
> >> pm_qos <= 100 -> timeout 100 ms
> >> pm_qos <= 50 -> timeout 300 ms
> >>
> >> That is, just some arbitrary numbers but which affect dynamic ps
> >> timeout. I haven't thought about the numbers at all, but they actually
> >> don't matter because it's easy to change them inside mac80211.
> >
> > Theoretically I could, but I'm pretty sure whatever values I would
> > choose would be unacceptable by others, as they would be tuned for a
> > specific use.
>
> I don't see a problem with that. The current use of pm_qos is very
> limited, I think it's all positive if we start using it more.
>
> > Also, although AFAIK barely anyone uses the DTIM interval which is
> > determined based on th pm_qos, adding arbitrary rules like this to the
> > side risks breaking something for someone.
>
> AFAIK the pm_qos values are not set in stone in any. Applications just
> request something and kernel can do whatever it wants, even ignore it.
> So I don't see any harm if we change how mac80211 uses pm_qos values.
> And most probably this will change many times in the future.
>
Ok, you convinced me. I will send a RFC patch with the above, and will
sub-sequentially be flamed to death ;)
Be sure to look into that bug in wl1251 at some point, so I don't break
it with this, if by chance I get this to go through ;)
-Juuso
On Mon, 2010-04-12 at 10:32 +0200, ext Johannes Berg wrote:
> On Mon, 2010-04-12 at 11:11 +0300, Juuso Oikarinen wrote:
> > This adds support to configure the dynamic ps timeout via nl80211. The
> > relevant attribute is added to NL80211_CMD_SET_POWER_SAVE.
>
> I'm not sure we even want this. Can't we calculate an appropriate value
> based on the beacon interval etc. instead?
We could obviously do that. This patch does not prevent adding
functionality ;)
The problem in relying solely on that kind of algorithm just is that
reaching a value suiting all types of device in all situations would be
hard.
For desktops, obviously reduced latency is desirable, while increased
power consumption is not so much of an issue. For hand-held devices the
situation is often the opposite: in many situations we might want to
sacrifice some latency just to stretch the battery life that last inch
longer, possibly depending on the type of traffic we know we have going
on.
Therefore I feel we need to give user-space the opportunity to configure
this timeout, one way or other.
-Juuso
> johannes
>
> > Signed-off-by: Juuso Oikarinen <[email protected]>
> > ---
> > include/linux/nl80211.h | 2 ++
> > net/wireless/nl80211.c | 16 +++++++++++++++-
> > 2 files changed, 17 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
> > index 2ea3ede..c5f12e7 100644
> > --- a/include/linux/nl80211.h
> > +++ b/include/linux/nl80211.h
> > @@ -864,6 +864,8 @@ enum nl80211_attrs {
> >
> > NL80211_ATTR_LOCAL_STATE_CHANGE,
> >
> > + NL80211_ATTR_PS_TIMEOUT,
> > +
> > /* add attributes here, update the policy in nl80211.c */
> >
> > __NL80211_ATTR_AFTER_LAST,
> > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > index df5505b..709bda3 100644
> > --- a/net/wireless/nl80211.c
> > +++ b/net/wireless/nl80211.c
> > @@ -151,6 +151,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
> > [NL80211_ATTR_PS_STATE] = { .type = NLA_U32 },
> > [NL80211_ATTR_CQM] = { .type = NLA_NESTED, },
> > [NL80211_ATTR_LOCAL_STATE_CHANGE] = { .type = NLA_FLAG },
> > + [NL80211_ATTR_PS_TIMEOUT] = { .type = NLA_U32 },
> > };
> >
> > /* policy for the attributes */
> > @@ -4682,6 +4683,7 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> > struct net_device *dev;
> > u8 ps_state;
> > bool state;
> > + int timeout = -1;
> > int err;
> >
> > if (!info->attrs[NL80211_ATTR_PS_STATE]) {
> > @@ -4696,6 +4698,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> > goto out;
> > }
> >
> > + if (info->attrs[NL80211_ATTR_PS_TIMEOUT]) {
> > + timeout = nla_get_u32(info->attrs[NL80211_ATTR_PS_TIMEOUT]);
> > + if (timeout < 0) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> > + }
> > +
> > rtnl_lock();
> >
> > err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
> > @@ -4710,11 +4720,14 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
> > }
> >
> > state = (ps_state == NL80211_PS_ENABLED) ? true : false;
> > + if (timeout < 0)
> > + timeout = wdev->ps_timeout;
> >
> > - if (state == wdev->ps)
> > + if (state == wdev->ps && timeout == wdev->ps_timeout)
> > goto unlock_rdev;
> >
> > wdev->ps = state;
> > + wdev->ps_timeout = timeout;
> >
> > if (rdev->ops->set_power_mgmt(wdev->wiphy, dev, wdev->ps,
> > wdev->ps_timeout))
> > @@ -4772,6 +4785,7 @@ static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
> > ps_state = NL80211_PS_DISABLED;
> >
> > NLA_PUT_U32(msg, NL80211_ATTR_PS_STATE, ps_state);
> > + NLA_PUT_U32(msg, NL80211_ATTR_PS_TIMEOUT, wdev->ps_timeout);
> >
> > genlmsg_end(msg, hdr);
> > err = genlmsg_reply(msg, info);
>
>