[2.6.31] iwlwifi: Reenable power_level
diff mbox

Message ID cb0375e10909020753o4d314f61k74dd35025f61666a@mail.gmail.com
State Not Applicable, archived
Headers show

Commit Message

Andrew Lutomirski Sept. 2, 2009, 2:53 p.m. UTC
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

reinette chatre Sept. 2, 2009, 3:56 p.m. UTC | #1
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lutomirski Sept. 2, 2009, 4:41 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Sept. 2, 2009, 4:48 p.m. UTC | #3
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
reinette chatre Sept. 2, 2009, 4:54 p.m. UTC | #4
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
reinette chatre Sept. 2, 2009, 4:57 p.m. UTC | #5
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Sept. 2, 2009, 5:03 p.m. UTC | #6
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
reinette chatre Sept. 2, 2009, 5:17 p.m. UTC | #7
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Sept. 2, 2009, 5:30 p.m. UTC | #8
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
Andrew Lutomirski Sept. 2, 2009, 5:38 p.m. UTC | #9
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
reinette chatre Sept. 2, 2009, 6:23 p.m. UTC | #10
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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);