[PATCH] intel_idle: Set dev->power_specified

[PATCH] intel_idle: Set dev->power_specified

[Jean Delvare]

The intel_idle driver defines power consumption for all states, but
they can't be seen in sysfs because the driver doesn't set
dev->power_specified, while the idle code expects that.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Len Brown <lenb@kernel.org>
---
This is the simple fix. But in all honesty, I don't get the point of
dev->power_specified. It should be equally easy to check if the first
state's .power has a non-zero value, and at least this doesn't require
any cooperation from the driver. As it stands, I expect that future
drivers will have the same problem intel_idle had, i.e. they will
forget to set dev->power_specified and power consumption values
won't be visible in syfs.

Am I missing any obvious problem? If not, I'll be happy to provide an
alternative patch dropping dev->power_specified altogether.

 drivers/idle/intel_idle.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.36-rc7.orig/drivers/idle/intel_idle.c	2010-10-07 08:53:05.000000000 +0200
+++ linux-2.6.36-rc7/drivers/idle/intel_idle.c	2010-10-08 16:16:41.000000000 +0200
@@ -369,6 +369,7 @@ static int intel_idle_cpuidle_devices_in
 			dev->state_count += 1;
 		}
 
+		dev->power_specified = 1;
 		dev->cpu = i;
 		if (cpuidle_register_device(dev)) {
 			pr_debug(PREFIX "cpuidle_register_device %d failed!\n",


-- 
Jean Delvare

Re: [PATCH] intel_idle: Set dev->power_specified

[Jean Delvare]

Hi again Len,

On Fri, 8 Oct 2010 17:44:45 +0200, Jean Delvare wrote:
> The intel_idle driver defines power consumption for all states, but
> they can't be seen in sysfs because the driver doesn't set
> dev->power_specified, while the idle code expects that.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Len Brown <lenb@kernel.org>
> ---
> This is the simple fix. But in all honesty, I don't get the point of
> dev->power_specified. It should be equally easy to check if the first
> state's .power has a non-zero value, and at least this doesn't require
> any cooperation from the driver. As it stands, I expect that future
> drivers will have the same problem intel_idle had, i.e. they will
> forget to set dev->power_specified and power consumption values
> won't be visible in syfs.

Apparently I couldn't be more right: the acpi/processor_idle driver
also doesn't set dev->power_specified even if power consumption values
are available. Grepping the whole kernel source tree, it appears that
none of the cpuidle drivers sets dev->power_specified, so I'm not sure
if that code path was even tested (at least it works for me...)

Good news is that it means that getting rid of that flag should be
easy, if you agree with that approach.

> 
> Am I missing any obvious problem? If not, I'll be happy to provide an
> alternative patch dropping dev->power_specified altogether.
> 
>  drivers/idle/intel_idle.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- linux-2.6.36-rc7.orig/drivers/idle/intel_idle.c	2010-10-07 08:53:05.000000000 +0200
> +++ linux-2.6.36-rc7/drivers/idle/intel_idle.c	2010-10-08 16:16:41.000000000 +0200
> @@ -369,6 +369,7 @@ static int intel_idle_cpuidle_devices_in
>  			dev->state_count += 1;
>  		}
>  
> +		dev->power_specified = 1;
>  		dev->cpu = i;
>  		if (cpuidle_register_device(dev)) {
>  			pr_debug(PREFIX "cpuidle_register_device %d failed!\n",

-- 
Jean Delvare

Re: [PATCH] intel_idle: Set dev->power_specified

[Len Brown]

> On Fri, 8 Oct 2010 17:44:45 +0200, Jean Delvare wrote:
> > The intel_idle driver defines power consumption for all states, but
> > they can't be seen in sysfs because the driver doesn't set
> > dev->power_specified, while the idle code expects that.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Len Brown <lenb@kernel.org>
> > ---
> > This is the simple fix. But in all honesty, I don't get the point of
> > dev->power_specified. It should be equally easy to check if the first
> > state's .power has a non-zero value, and at least this doesn't require
> > any cooperation from the driver. As it stands, I expect that future
> > drivers will have the same problem intel_idle had, i.e. they will
> > forget to set dev->power_specified and power consumption values
> > won't be visible in syfs.
> 
> Apparently I couldn't be more right: the acpi/processor_idle driver
> also doesn't set dev->power_specified even if power consumption values
> are available. Grepping the whole kernel source tree, it appears that
> none of the cpuidle drivers sets dev->power_specified, so I'm not sure
> if that code path was even tested (at least it works for me...)
> 
> Good news is that it means that getting rid of that flag should be
> easy, if you agree with that approach.

Sorry I didn't notice your e-mail until today -- for this afternoon
I noticed the same problem you did.

The power_specified flag was added in 2.6.36-rc, and intel_idle,
acpi_idle, and sh_idle do not know about it and so they get these
crazy negative-shown-as-large-positive numbers in sysfs now.

But I was actually planning to delete all the power_usage numbers
in these drivers anyway, because they are bogus, and I sent
a patch to do that to intel_idle and acpi_idle this afternoon.

Unfortunately, the power_specified patch makes power_usage significant
for the first time, apparently because the out of tree ARM stuff
is doing that.  I'm not immediately excited about it...

thanks,
Len Brown, Intel Open Source Technology Center

> > 
> > Am I missing any obvious problem? If not, I'll be happy to provide an
> > alternative patch dropping dev->power_specified altogether.
> > 
> >  drivers/idle/intel_idle.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > --- linux-2.6.36-rc7.orig/drivers/idle/intel_idle.c	2010-10-07 08:53:05.000000000 +0200
> > +++ linux-2.6.36-rc7/drivers/idle/intel_idle.c	2010-10-08 16:16:41.000000000 +0200
> > @@ -369,6 +369,7 @@ static int intel_idle_cpuidle_devices_in
> >  			dev->state_count += 1;
> >  		}
> >  
> > +		dev->power_specified = 1;
> >  		dev->cpu = i;
> >  		if (cpuidle_register_device(dev)) {
> >  			pr_debug(PREFIX "cpuidle_register_device %d failed!\n",
> 
> -- 
> Jean Delvare
>