[PATCH 2.6.31] iwlwifi: Reenable power_level

[PATCH 2.6.31] iwlwifi: Reenable power_level

[Andrew Lutomirski]

Right now, enabling power saving on iwlwifi is impossible, because
mac80211 won't tell iwlwifi that power saving is on (since
IEEE80211_HW_SUPPORTS_PS is not set) and iwlwifi will ignore the
user's power_level setting until mac80211 asks for power saving.
Setting this flag allows the user to manually enable power saving if
desired.

Signed-off-by: Andy Lutomirski <luto@mit.edu>

---

This fixes what looks to me like a regression: power_level used to
work but now fails silently.  In 2.6.32 I think this code is going
away, but this patch seems like a safe stopgap measure.

iwlwifi mostly ignores mac80211's power settings, but I think
advertising IEEE80211_HW_SUPPORTS_PS should be safe, even this late in
the 2.6.31 cycle because:

1. This patch won't have any effect until the user requests power
saving through wext or nl80211.
2. Even if that happens, the user still has to set power_level in
sysfs for anything to change.
3. As far as I can tell, power_level in sysfs wasn't any safer in
2.6.29 or 2.6.30 than it will be with this patch.
4. This fixes a regression.

The power_level sysfs entry is still rather odd in that setting and
getting don't behave in quite the way that the user would expect
(there should probably be power_level_requested and actual_power_level
as separate entries), but power_level looks like it's going away soon,
so this isn't worth fixing now.

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c
b/drivers/net/wireless/iwlwifi/iwl-core.c
index 18b135f..be6f1e0 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -1325,7 +1325,8 @@ int iwl_setup_mac(struct iwl_priv *priv)
 	hw->flags = IEEE80211_HW_SIGNAL_DBM |
 		    IEEE80211_HW_NOISE_DBM |
 		    IEEE80211_HW_AMPDU_AGGREGATION |
-		    IEEE80211_HW_SPECTRUM_MGMT;
+		    IEEE80211_HW_SPECTRUM_MGMT |
+		    IEEE80211_HW_SUPPORTS_PS;
 	hw->wiphy->interface_modes =
 		BIT(NL80211_IFTYPE_STATION) |
 		BIT(NL80211_IFTYPE_ADHOC);

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[reinette chatre]

Hi Andy,

On Wed, 2009-09-02 at 07:53 -0700, Andrew Lutomirski wrote:
> Right now, enabling power saving on iwlwifi is impossible, because
> mac80211 won't tell iwlwifi that power saving is on (since
> IEEE80211_HW_SUPPORTS_PS is not set) and iwlwifi will ignore the
> user's power_level setting until mac80211 asks for power saving.
> Setting this flag allows the user to manually enable power saving if
> desired.
> 
> Signed-off-by: Andy Lutomirski <luto@mit.edu>
> 
> ---

nack.

http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=2053 and
http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=2051 are two
examples of what happens if power save support is enabled. In addition
to this we are also seeing issues with 4965 that are described in the
commit that disabled powersave in 2.6.31 in the first place
(286d94906587901851906a5e2ddc09bc1a7ba1d9).


> This fixes what looks to me like a regression: power_level used to
> work but now fails silently.  In 2.6.32 I think this code is going
> away, but this patch seems like a safe stopgap measure.

2.6.32 will have power save support disabled also.

Reinette

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[Andrew Lutomirski]

On Wed, Sep 2, 2009 at 11:56 AM, reinette
chatre<reinette.chatre@intel.com> wrote:
> Hi Andy,
>
> On Wed, 2009-09-02 at 07:53 -0700, Andrew Lutomirski wrote:
>> Right now, enabling power saving on iwlwifi is impossible, because
>> mac80211 won't tell iwlwifi that power saving is on (since
>> IEEE80211_HW_SUPPORTS_PS is not set) and iwlwifi will ignore the
>> user's power_level setting until mac80211 asks for power saving.
>> Setting this flag allows the user to manually enable power saving if
>> desired.
>>
>> Signed-off-by: Andy Lutomirski <luto@mit.edu>
>>
>> ---
>
> nack.
>
> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=2053 and
> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=2051 are two
> examples of what happens if power save support is enabled. In addition
> to this we are also seeing issues with 4965 that are described in the
> commit that disabled powersave in 2.6.31 in the first place
> (286d94906587901851906a5e2ddc09bc1a7ba1d9).
>

Fair enough.  Would you accept a patch to remove power_level from sysfs instead?

>
>> This fixes what looks to me like a regression: power_level used to
>> work but now fails silently.  In 2.6.32 I think this code is going
>> away, but this patch seems like a safe stopgap measure.
>
> 2.6.32 will have power save support disabled also.

Any ETA for getting this fixed for real?  I like my battery life :)

--Andy

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

On Wed, 2009-09-02 at 08:56 -0700, reinette chatre wrote:

> 2.6.32 will have power save support disabled also.

Only by default -- there you can actually enable it as a user with
"iwconfig wlan0 power timeout 100ms" to turn it on -- unless that patch
didn't go in?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[reinette chatre]

Hi Andrew,

On Wed, 2009-09-02 at 09:41 -0700, Andrew Lutomirski wrote:

> Fair enough.  Would you accept a patch to remove power_level from sysfs instead?

No. This file is used when users want to manually reduce the power used
by device. This is the file you want to change if you want to extend
your battery life.

> >> This fixes what looks to me like a regression: power_level used to
> >> work but now fails silently.  In 2.6.32 I think this code is going
> >> away, but this patch seems like a safe stopgap measure.
> >
> > 2.6.32 will have power save support disabled also.
> 
> Any ETA for getting this fixed for real?  I like my battery life :)

We are working on it.

Reinette

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[reinette chatre]

On Wed, 2009-09-02 at 09:48 -0700, Johannes Berg wrote:
> On Wed, 2009-09-02 at 08:56 -0700, reinette chatre wrote:
> 
> > 2.6.32 will have power save support disabled also.
> 
> Only by default -- there you can actually enable it as a user with
> "iwconfig wlan0 power timeout 100ms" to turn it on -- unless that patch
> didn't go in?

yes - sorry, I should have added that it is disabled by default but
users can enable it using iwconfig.

Reinette

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Wed, 2009-09-02 at 09:54 -0700, reinette chatre wrote:
> Hi Andrew,
> 
> On Wed, 2009-09-02 at 09:41 -0700, Andrew Lutomirski wrote:
> 
> > Fair enough.  Would you accept a patch to remove power_level from sysfs instead?
> 
> No. This file is used when users want to manually reduce the power used
> by device. This is the file you want to change if you want to extend
> your battery life.

Actually, we have already removed that file for 2.6.32 I think? So it
may be fair to remove it for .31 since there you can't use it anyway.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[reinette chatre]

Hi Johannes,

On Wed, 2009-09-02 at 10:03 -0700, Johannes Berg wrote:
> On Wed, 2009-09-02 at 09:54 -0700, reinette chatre wrote:
> > On Wed, 2009-09-02 at 09:41 -0700, Andrew Lutomirski wrote:
> > > Fair enough.  Would you accept a patch to remove power_level from sysfs instead?
> > No. This file is used when users want to manually reduce the power used
> > by device. This is the file you want to change if you want to extend
> > your battery life.
> Actually, we have already removed that file for 2.6.32 I think? 

The file itself was removed, but from what I understand the same
functionality can now be obtained from sleep_level_override. Users who
previously used power_level can thus now use sleep_level_override, but
with sleep_level_override not existing in 2.6.31 they will have no
alternative if we remove power_level.

> So it
> may be fair to remove it for .31 since there you can't use it anyway.

You can still use it to manually set power index used by device to
reduce power usage.

Reinette

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

Hi Reinette,

> The file itself was removed, but from what I understand the same
> functionality can now be obtained from sleep_level_override. 

Indeed.

> Users who
> previously used power_level can thus now use sleep_level_override, but
> with sleep_level_override not existing in 2.6.31 they will have no
> alternative if we remove power_level.
> 
> > So it
> > may be fair to remove it for .31 since there you can't use it anyway.
> 
> You can still use it to manually set power index used by device to
> reduce power usage.

I was under the impression that Andrew said this actually didn't work.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[Andrew Lutomirski]

On Wed, Sep 2, 2009 at 1:30 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> Hi Reinette,
>
>> The file itself was removed, but from what I understand the same
>> functionality can now be obtained from sleep_level_override.
>
> Indeed.
>
>> Users who
>> previously used power_level can thus now use sleep_level_override, but
>> with sleep_level_override not existing in 2.6.31 they will have no
>> alternative if we remove power_level.
>>
>> > So it
>> > may be fair to remove it for .31 since there you can't use it anyway.
>>
>> You can still use it to manually set power index used by device to
>> reduce power usage.
>
> I was under the impression that Andrew said this actually didn't work.

Exactly.  My patch fixes that, although there might be a better way to do that.

--Andy

>
> johannes
>

Re: [PATCH 2.6.31] iwlwifi: Reenable power_level

[reinette chatre]

On Wed, 2009-09-02 at 10:38 -0700, Andrew Lutomirski wrote:
> On Wed, Sep 2, 2009 at 1:30 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> >
> > I was under the impression that Andrew said this actually didn't work.
> 
> Exactly.  My patch fixes that, although there might be a better way to do that.

Seems to be in iwl_power_update_mode() the mode is forced to CAM if
power save disabled, and power save is set to disabled if mac considers
it disabled (see setting of power_disabled in iwl_mac_config()).

One way around this is to always allow user to set power mode, even if
it is disabled from mac perspective. Since the power mode is updated in
many more spots (not always triggered by user) I do not know the full
implications of this change. Considering the unknown of this change and
the problems we have encountered when enabling power save I would rather
not merge such a patch without significant testing. I think it is too
late for 2.6.31.

The same problem should not exist in 2.6.32.

Reinette