[PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[Andrew Lutomirski]

Runtime adjustment of no_sleep_autoadjust seems fine, both looking at
the code and in practice.  This makes it easier to test.

Signed-off-by: Andy Lutomirski <luto@mit.edu>
---
I've been running this patch (and twiddling the setting) for a couple
months now, and it seems to work fine.  I think it's a bit late for
2.6.32, even though it's pretty much impossible for this to cause any
regressions, but it would be nice to see it go in for 2.6.33 until
no_sleep_autoadjust goes away.

diff --git a/drivers/net/wireless/iwlwifi/iwl-power.c
b/drivers/net/wireless/iwlwifi/iwl-power.c
index 60be976..4eba1ab 100644
--- a/drivers/net/wireless/iwlwifi/iwl-power.c
+++ b/drivers/net/wireless/iwlwifi/iwl-power.c
@@ -54,7 +54,7 @@
  * adjusting ...
  */
 bool no_sleep_autoadjust = true;
-module_param(no_sleep_autoadjust, bool, S_IRUGO);
+module_param(no_sleep_autoadjust, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(no_sleep_autoadjust,
 		 "don't automatically adjust sleep level "
 		 "according to maximum network latency");

Re: [PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[reinette chatre]

On Sun, 2009-11-08 at 11:30 -0800, Andrew Lutomirski wrote:
> Runtime adjustment of no_sleep_autoadjust seems fine, both looking at
> the code and in practice.  This makes it easier to test.
> 
> Signed-off-by: Andy Lutomirski <luto@mit.edu>
> ---
> I've been running this patch (and twiddling the setting) for a couple
> months now, and it seems to work fine.  I think it's a bit late for
> 2.6.32, even though it's pretty much impossible for this to cause any
> regressions, but it would be nice to see it go in for 2.6.33 until
> no_sleep_autoadjust goes away.
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-power.c
> b/drivers/net/wireless/iwlwifi/iwl-power.c
> index 60be976..4eba1ab 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-power.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-power.c
> @@ -54,7 +54,7 @@
>   * adjusting ...
>   */
>  bool no_sleep_autoadjust = true;
> -module_param(no_sleep_autoadjust, bool, S_IRUGO);
> +module_param(no_sleep_autoadjust, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(no_sleep_autoadjust,
>  		 "don't automatically adjust sleep level "
>  		 "according to maximum network latency");

I think this change is a bit deceiving since making this writable does
not result in what you write to it at runtime being communicated to the
device.

Reinette

Re: [PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[Andrew Lutomirski]

On Mon, Nov 9, 2009 at 12:19 PM, reinette chatre
<reinette.chatre@intel.com> wrote:
> On Sun, 2009-11-08 at 11:30 -0800, Andrew Lutomirski wrote:
>> Runtime adjustment of no_sleep_autoadjust seems fine, both looking at
>> the code and in practice.  This makes it easier to test.
>>
>> Signed-off-by: Andy Lutomirski <luto@mit.edu>
>> ---
>> I've been running this patch (and twiddling the setting) for a couple
>> months now, and it seems to work fine.  I think it's a bit late for
>> 2.6.32, even though it's pretty much impossible for this to cause any
>> regressions, but it would be nice to see it go in for 2.6.33 until
>> no_sleep_autoadjust goes away.
>>
>> diff --git a/drivers/net/wireless/iwlwifi/iwl-power.c
>> b/drivers/net/wireless/iwlwifi/iwl-power.c
>> index 60be976..4eba1ab 100644
>> --- a/drivers/net/wireless/iwlwifi/iwl-power.c
>> +++ b/drivers/net/wireless/iwlwifi/iwl-power.c
>> @@ -54,7 +54,7 @@
>>   * adjusting ...
>>   */
>>  bool no_sleep_autoadjust = true;
>> -module_param(no_sleep_autoadjust, bool, S_IRUGO);
>> +module_param(no_sleep_autoadjust, bool, S_IRUGO | S_IWUSR);
>>  MODULE_PARM_DESC(no_sleep_autoadjust,
>>                "don't automatically adjust sleep level "
>>                "according to maximum network latency");
>
> I think this change is a bit deceiving since making this writable does
> not result in what you write to it at runtime being communicated to the
> device.

Hmm.  It looks like sleep_level_override in debugfs can do exactly what I want.

Is there any reason that no_sleep_autoadjust doesn't just make
sleep_level_override default to 1?

--Andy

Re: [PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[reinette chatre]

Hi Andrew,

On Mon, 2009-11-09 at 10:07 -0800, Andrew Lutomirski wrote:
> Hmm.  It looks like sleep_level_override in debugfs can do exactly what I want.

You have not really stated what you are trying to do here so it is hard
to tell. sleep_level_override lets you hardcode the power saving index.

> 
> Is there any reason that no_sleep_autoadjust doesn't just make
> sleep_level_override default to 1?

That does seem what it is doing in iwl_power_update_mode().

Reinette

Re: [PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[Andrew Lutomirski]

On Mon, Nov 9, 2009 at 5:21 PM, reinette chatre
<reinette.chatre@intel.com> wrote:
> Hi Andrew,
>
> On Mon, 2009-11-09 at 10:07 -0800, Andrew Lutomirski wrote:
>> Hmm.  It looks like sleep_level_override in debugfs can do exactly what I want.
>
> You have not really stated what you are trying to do here so it is hard
> to tell. sleep_level_override lets you hardcode the power saving index.
>
>>
>> Is there any reason that no_sleep_autoadjust doesn't just make
>> sleep_level_override default to 1?
>
> That does seem what it is doing in iwl_power_update_mode().

I want to save power.  no_sleep_autoadjust=N and iwconfig wlan0 power
on does a good job of that.  I assume that no_sleep_autoadjust
defaults to Y because it's still not reliable (and I seem to have seen
a few networks where it causes occasional problems).  So I'd like to
be able to toggle it at runtime, file bug reports when it fails, but
still be able to use the networks it fails on :)

--Andy

Re: [PATCH] iwlcore: Allow runtime configuration of no_sleep_autoadjust

[reinette chatre]

Hi Andrew,

On Mon, 2009-11-09 at 14:47 -0800, Andrew Lutomirski wrote:
> On Mon, Nov 9, 2009 at 5:21 PM, reinette chatre
> <reinette.chatre@intel.com> wrote:
> > Hi Andrew,
> >
> > On Mon, 2009-11-09 at 10:07 -0800, Andrew Lutomirski wrote:
> >> Hmm.  It looks like sleep_level_override in debugfs can do exactly what I want.
> >
> > You have not really stated what you are trying to do here so it is hard
> > to tell. sleep_level_override lets you hardcode the power saving index.
> >
> >>
> >> Is there any reason that no_sleep_autoadjust doesn't just make
> >> sleep_level_override default to 1?
> >
> > That does seem what it is doing in iwl_power_update_mode().
> 
> I want to save power.  no_sleep_autoadjust=N and iwconfig wlan0 power
> on does a good job of that.  I assume that no_sleep_autoadjust
> defaults to Y because it's still not reliable (and I seem to have seen
> a few networks where it causes occasional problems).  So I'd like to
> be able to toggle it at runtime, file bug reports when it fails, but
> still be able to use the networks it fails on :)

Perhaps you could add a new debugfs file to modify no_sleep_autoadjust.
This will enable you to write the new power command to the device after
the setting has changed, similar to what is done in
iwl_dbgfs_sleep_level_override_write. This will ensure that your setting
takes effect at the time you make the change.

Reinette