Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Pavel Machek]

Hi!

> If a device can't support wakeup, its /sys/devices/.../power/wakeup output is
> empty, this is confusing, a user doesn't know if it supports wakeup feature
> unless he/she read the ralated source code, for this case, it is more
> reasonable to output "unsupported". Otherwise, no matter what value the user
> sets to /sys/devices/.../power/wakeup, the result is the same:  Invalid argument,
> so the user doesn't know why.
> 
> This patch changes empty output to "unsupported" in order that a user knows
> wakeup feature isn't supported by this device when he/she
> 'cat /sys/devices/.../power/wakeup', please consider to apply,
> thanks.

What about simply removing "wakuep" file if wakeup is not supported?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[David Brownell]

> > This patch changes empty output to "unsupported" in order that a user knows
> > wakeup feature isn't supported by this device when he/she
> > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > thanks.
>
> What about simply removing "wakuep" file if wakeup is not supported?

It may not *stay* unsupported, so I think changing it in either
of those permanent ways would be confusing/misleading.

For example, USB devices have multiple states ... minimally, an
unconfigured state, and a configured state.  Some have multiple
configurations.  Only configured states can be wakeup-capable.
So a given device might have three states, but support wakeup
except in one of them...

- Dave

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[David Brownell]

> This patch changes empty output to "unsupported" in order that a user knows
> wakeup feature isn't supported by this device when he/she
> 'cat /sys/devices/.../power/wakeup', please consider to apply, thanks.

I don't much like this patch.  Not just for the technical reasons
mentioned in my previous note ... but also because it doesn't update
the documention at the top, which clearly states that "\n" is
returned for "temporary or permanent inability to issue wakeup".
And then it gives the example I gave before ...

Though I suppose a patch that changes the *entire* userspace interface,
(which includes its documentation, and all out-of-tree users) would have
shown more clearly why it wasn't a good idea.  ;)


> --- a/drivers/base/power/sysfs.c	2008-01-04 16:50:54.000000000 +0800
> +++ b/drivers/base/power/sysfs.c	2008-01-04 17:14:42.000000000 +0800
> @@ -49,7 +49,7 @@ wake_show(struct device * dev, struct de
>  {
>  	return sprintf(buf, "%s\n", device_can_wakeup(dev)
>  		? (device_may_wakeup(dev) ? enabled : disabled)
> -		: "");
> +		: "unsupported");
>  }
>  
>  static ssize_t
>
>

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Alan Stern]

On Fri, 4 Jan 2008, David Brownell wrote:

> > > This patch changes empty output to "unsupported" in order that a user knows
> > > wakeup feature isn't supported by this device when he/she
> > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > thanks.
> >
> > What about simply removing "wakuep" file if wakeup is not supported?
> 
> It may not *stay* unsupported, so I think changing it in either
> of those permanent ways would be confusing/misleading.

How about changing it to say "unavailable"?  That doesn't imply 
permanence.

Alan Stern

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Olivier Galibert]

On Fri, Jan 04, 2008 at 11:38:29AM -0500, Alan Stern wrote:
> How about changing it to say "unavailable"?  That doesn't imply 
> permanence.

How about not changing a userland-visible interface gratuitously?

  OG.

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Oliver Neukum]

Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:
> On Fri, Jan 04, 2008 at 11:38:29AM -0500, Alan Stern wrote:
> > How about changing it to say "unavailable"?  That doesn't imply 
> > permanence.
> 
> How about not changing a userland-visible interface gratuitously?

Very good idea.

	Regards
		Oliver

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 12:48 +0100, Pavel Machek wrote:
> Hi!
> 
> > If a device can't support wakeup, its /sys/devices/.../power/wakeup output is
> > empty, this is confusing, a user doesn't know if it supports wakeup feature
> > unless he/she read the ralated source code, for this case, it is more
> > reasonable to output "unsupported". Otherwise, no matter what value the user
> > sets to /sys/devices/.../power/wakeup, the result is the same:  Invalid argument,
> > so the user doesn't know why.
> > 
> > This patch changes empty output to "unsupported" in order that a user knows
> > wakeup feature isn't supported by this device when he/she
> > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > thanks.
> 
> What about simply removing "wakuep" file if wakeup is not supported?
> 									Pavel
At the beginning, i did that as you said, but it can't work, some
power/wakeup will disappear although they can be disabled or enabled, so
it is only one feasible way to make it exist permanently.
> 

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
> > > This patch changes empty output to "unsupported" in order that a user knows
> > > wakeup feature isn't supported by this device when he/she
> > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > thanks.
> >
> > What about simply removing "wakuep" file if wakeup is not supported?
> 
> It may not *stay* unsupported, so I think changing it in either
> of those permanent ways would be confusing/misleading.
> 
> For example, USB devices have multiple states ... minimally, an
> unconfigured state, and a configured state.  Some have multiple
> configurations.  Only configured states can be wakeup-capable.
> So a given device might have three states, but support wakeup
> except in one of them...
If so, we can change "unsupported" to "unconfigurable" or "inoperable"
which can cover the states "unconfigured", "unsupported" and other
unknown states. :-).
> 
> - Dave
> 

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 08:31 -0800, David Brownell wrote:
> > This patch changes empty output to "unsupported" in order that a user knows
> > wakeup feature isn't supported by this device when he/she
> > 'cat /sys/devices/.../power/wakeup', please consider to apply, thanks.
> 
> I don't much like this patch.  Not just for the technical reasons
> mentioned in my previous note ... but also because it doesn't update
> the documention at the top, which clearly states that "\n" is
> returned for "temporary or permanent inability to issue wakeup".
> And then it gives the example I gave before ...
> 
> Though I suppose a patch that changes the *entire* userspace interface,
> (which includes its documentation, and all out-of-tree users) would have
> shown more clearly why it wasn't a good idea.  ;)
Really, "\n" should be changed to show that change.
Anyway, "\n" isn't a good indicator for that state. :-)
> 
> 
> > --- a/drivers/base/power/sysfs.c	2008-01-04 16:50:54.000000000 +0800
> > +++ b/drivers/base/power/sysfs.c	2008-01-04 17:14:42.000000000 +0800
> > @@ -49,7 +49,7 @@ wake_show(struct device * dev, struct de
> >  {
> >  	return sprintf(buf, "%s\n", device_can_wakeup(dev)
> >  		? (device_may_wakeup(dev) ? enabled : disabled)
> > -		: "");
> > +		: "unsupported");
> >  }
> >  
> >  static ssize_t
> >
> >

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 11:38 -0500, Alan Stern wrote:
> On Fri, 4 Jan 2008, David Brownell wrote:
> 
> > > > This patch changes empty output to "unsupported" in order that a user knows
> > > > wakeup feature isn't supported by this device when he/she
> > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > > thanks.
> > >
> > > What about simply removing "wakuep" file if wakeup is not supported?
> > 
> > It may not *stay* unsupported, so I think changing it in either
> > of those permanent ways would be confusing/misleading.
> 
> How about changing it to say "unavailable"?  That doesn't imply 
> permanence.
This should be an option.
> 
> Alan Stern
> 

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 17:52 +0100, Olivier Galibert wrote:
> On Fri, Jan 04, 2008 at 11:38:29AM -0500, Alan Stern wrote:
> > How about changing it to say "unavailable"?  That doesn't imply 
> > permanence.
> 
> How about not changing a userland-visible interface gratuitously?
"empty" can't tell a user the state of wakeup of a device, so it is
necessary to change it.
> 
>   OG.
> 

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Rafael J. Wysocki]

On Monday, 7 of January 2008, Yi Yang wrote:
> On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
> > > > This patch changes empty output to "unsupported" in order that a user knows
> > > > wakeup feature isn't supported by this device when he/she
> > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > > thanks.
> > >
> > > What about simply removing "wakuep" file if wakeup is not supported?
> > 
> > It may not *stay* unsupported, so I think changing it in either
> > of those permanent ways would be confusing/misleading.
> > 
> > For example, USB devices have multiple states ... minimally, an
> > unconfigured state, and a configured state.  Some have multiple
> > configurations.  Only configured states can be wakeup-capable.
> > So a given device might have three states, but support wakeup
> > except in one of them...
> If so, we can change "unsupported" to "unconfigurable" or "inoperable"
> which can cover the states "unconfigured", "unsupported" and other
> unknown states. :-).

The main problem with your patch is that is changes a documented behavior
visible by the user space.  That should be done very cautiously.

Thanks,
Rafael

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Fri, 2008-01-04 at 18:20 +0100, Oliver Neukum wrote:
> Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:
> > On Fri, Jan 04, 2008 at 11:38:29AM -0500, Alan Stern wrote:
> > > How about changing it to say "unavailable"?  That doesn't imply 
> > > permanence.
> > 
> > How about not changing a userland-visible interface gratuitously?
> 
Why not do if we can make it better?
> Very good idea.
> 
> 	Regards
> 		Oliver

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Yi Yang]

On Mon, 2008-01-07 at 02:57 +0100, Rafael J. Wysocki wrote:
> On Monday, 7 of January 2008, Yi Yang wrote:
> > On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
> > > > > This patch changes empty output to "unsupported" in order that a user knows
> > > > > wakeup feature isn't supported by this device when he/she
> > > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > > > thanks.
> > > >
> > > > What about simply removing "wakuep" file if wakeup is not supported?
> > > 
> > > It may not *stay* unsupported, so I think changing it in either
> > > of those permanent ways would be confusing/misleading.
> > > 
> > > For example, USB devices have multiple states ... minimally, an
> > > unconfigured state, and a configured state.  Some have multiple
> > > configurations.  Only configured states can be wakeup-capable.
> > > So a given device might have three states, but support wakeup
> > > except in one of them...
> > If so, we can change "unsupported" to "unconfigurable" or "inoperable"
> > which can cover the states "unconfigured", "unsupported" and other
> > unknown states. :-).
> 
> The main problem with your patch is that is changes a documented behavior
> visible by the user space.  That should be done very cautiously.
Which applications are using this interface? if they assume "\n" means
the unconfigurable or unavailable state, this is very fragile.
> 
> Thanks,
> Rafael

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[David Brownell]

On Sunday 06 January 2008, Yi Yang wrote:
> 
> > How about not changing a userland-visible interface gratuitously?
>
> "empty" can't tell a user the state of wakeup of a device, so it is
> necessary to change it.

Words don't say anything at all in isolation.  For example,
here the current tristate behavior has been documented for
several years now ... and that's the only thing that could
have conveyed any meaning whatever.

I can't agree that it's even slightly "necessary" to make
such a grutuitous and backwards-incompatible change.

Re: [PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

[Rafael J. Wysocki]

On Monday, 7 of January 2008, Yi Yang wrote:
> On Mon, 2008-01-07 at 02:57 +0100, Rafael J. Wysocki wrote:
> > On Monday, 7 of January 2008, Yi Yang wrote:
> > > On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
> > > > > > This patch changes empty output to "unsupported" in order that a user knows
> > > > > > wakeup feature isn't supported by this device when he/she
> > > > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
> > > > > > thanks.
> > > > >
> > > > > What about simply removing "wakuep" file if wakeup is not supported?
> > > > 
> > > > It may not *stay* unsupported, so I think changing it in either
> > > > of those permanent ways would be confusing/misleading.
> > > > 
> > > > For example, USB devices have multiple states ... minimally, an
> > > > unconfigured state, and a configured state.  Some have multiple
> > > > configurations.  Only configured states can be wakeup-capable.
> > > > So a given device might have three states, but support wakeup
> > > > except in one of them...
> > > If so, we can change "unsupported" to "unconfigurable" or "inoperable"
> > > which can cover the states "unconfigured", "unsupported" and other
> > > unknown states. :-).
> > 
> > The main problem with your patch is that is changes a documented behavior
> > visible by the user space.  That should be done very cautiously.
> Which applications are using this interface? if they assume "\n" means
> the unconfigurable or unavailable state, this is very fragile.

I can't give you examples right now, but since the behavior has been
documented for quite some time, it's safe to assume there are some unless
proven otherwise.

Also, fragile or not, it is the user space's right to rely on documented
behavior of the kernel.

Thanks,
Rafael

[PATCH 2.6.24-rc8] cpufreq: fix obvious condition statement error

[Yi Yang]

The function __cpufreq_set_policy in file drivers/cpufreq/cpufreq.c
has a very obvious error:

        if (policy->min > data->min && policy->min > policy->max) {
                ret = -EINVAL;
                goto error_out;
        }

This condtion statement is wrong because it returns -EINVAL only if
policy->min is greater than policy->max (in this case, 
"policy->min > data->min" is true for ever.). In fact, it should
return -EINVAL as well if policy->max is less than data->min.

The correct condition should be:

	if (policy->min > data->max || policy->max < data->min) {

The following test result testifies the above conclusion:

Before applying this patch:

[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies
2394000 1596000
[root@yangyi-dev /]# echo 1596000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
1596000
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
1596000
[root@yangyi-dev /]# echo "2000000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
-bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
1596000
[root@yangyi-dev /]# echo "0" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
1596000
[root@yangyi-dev /]# echo "1595000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
1596000
[root@yangyi-dev /]#


After applying this patch:

[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies
2394000 1596000
[root@yangyi-dev /]# echo 1596000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
1596000
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
1596000
[root@localhost /]# echo "2000000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
-bash: echo: write error: Invalid argument
[root@localhost /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
1596000
[root@localhost /]# echo "0" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
-bash: echo: write error: Invalid argument
[root@localhost /]# echo "1595000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
-bash: echo: write error: Invalid argument
[root@localhost /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
1596000
[root@localhost /]# echo "1596000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@localhost /]# echo "2394000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
[root@localhost /]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
2394000
[root@localhost /]


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 cpufreq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/cpufreq/cpufreq.c	2008-01-23 05:02:28.000000000 +0800
+++ b/drivers/cpufreq/cpufreq.c	2008-01-23 06:11:21.000000000 +0800
@@ -1608,7 +1608,7 @@ static int __cpufreq_set_policy(struct c
 	memcpy(&policy->cpuinfo, &data->cpuinfo,
 				sizeof(struct cpufreq_cpuinfo));
 
-	if (policy->min > data->min && policy->min > policy->max) {
+	if (policy->min > data->max || policy->max < data->min) {
 		ret = -EINVAL;
 		goto error_out;
 	}

[PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Yi Yang]

When one cpu is set to offline, the caller process will hang, according to
the trace data, the problem lies in the refcount error in cpufreq driver,
cpufreq_cpu_callback will wait for completion policy->kobj_unregister
which is nerver completed because a refcount error in function
__cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
calling kobject release method.

In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
will be unregistered, this problem didn't exist in 2.6.24 and earlier.

The root cause is kobject API switch, kobject_init_and_add and kobject_put
replace older kobject_register and kobject_unregister in 2.6.25-rc1,
compared to 2.6.24, kobject_unregister is deleted in function
__cpufreq_remove_dev but it isn't replaced with kobject_put.

This patch adds kobject_put to balance refcount. I noticed Greg suggests
it will fix a power-off issue to remove kobject_get statement block, but i
think that isn't the best way because those code block has existed very long
and it is helpful because the successive statements are invoking relevant
data.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/drivers/cpufreq/cpufreq.c	2008-02-15 04:41:29.000000000 +0800
+++ b/drivers/cpufreq/cpufreq.c	2008-02-15 06:56:56.000000000 +0800
@@ -1057,12 +1057,17 @@ static int __cpufreq_remove_dev (struct 
 
 	unlock_policy_rwsem_write(cpu);
 
+	/* it matches the previous kobject_get */
 	kobject_put(&data->kobj);
 
 	/* we need to make sure that the underlying kobj is actually
 	 * not referenced anymore by anybody before we proceed with
 	 * unloading.
 	 */
+
+	/* unregister data->kobj, it matches kobject_init_and_add */
+	kobject_put(&data->kobj);
+
 	dprintk("waiting for dropping of refcount\n");
 	wait_for_completion(&data->kobj_unregister);
 	dprintk("wait complete\n");

[PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Yi Yang]

When one cpu is set to offline, the caller process will hang, according to
the trace data, the problem lies in the refcount error in cpufreq driver,
cpufreq_cpu_callback will wait for completion policy->kobj_unregister
which is nerver completed because a refcount error in function
__cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
calling kobject release method.

In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
will be unregistered, this problem didn't exist in 2.6.24 and earlier.

The root cause is kobject API switch, kobject_init_and_add and kobject_put
replace older kobject_register and kobject_unregister in 2.6.25-rc1,
compared to 2.6.24, kobject_unregister is deleted in function
__cpufreq_remove_dev but it isn't replaced with kobject_put.

This patch adds kobject_put to balance refcount. I noticed Greg suggests
it will fix a power-off issue to remove kobject_get statement block, but i
think that isn't the best way because those code block has existed very long
and it is helpful because the successive statements are invoking relevant
data.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/drivers/cpufreq/cpufreq.c	2008-02-15 04:41:29.000000000 +0800
+++ b/drivers/cpufreq/cpufreq.c	2008-02-15 06:56:56.000000000 +0800
@@ -1057,12 +1057,17 @@ static int __cpufreq_remove_dev (struct 
 
 	unlock_policy_rwsem_write(cpu);
 
+	/* it matches the previous kobject_get */
 	kobject_put(&data->kobj);
 
 	/* we need to make sure that the underlying kobj is actually
 	 * not referenced anymore by anybody before we proceed with
 	 * unloading.
 	 */
+
+	/* unregister data->kobj, it matches kobject_init_and_add */
+	kobject_put(&data->kobj);
+
 	dprintk("waiting for dropping of refcount\n");
 	wait_for_completion(&data->kobj_unregister);
 	dprintk("wait complete\n");

Re: [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Alan Stern]

On Fri, 15 Feb 2008, Yi Yang wrote:

> This patch adds kobject_put to balance refcount. I noticed Greg suggests
> it will fix a power-off issue to remove kobject_get statement block, but i
> think that isn't the best way because those code block has existed very long
> and it is helpful because the successive statements are invoking relevant
> data.

Are you referring to this section of code (before the region affected 
by your patch)?

	if (!kobject_get(&data->kobj)) {
		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
		cpufreq_debug_enable_ratelimit();
		unlock_policy_rwsem_write(cpu);
		return -EFAULT;
	}

Greg is correct that the kobject_get() here is useless and should be
removed.  kobject_get() never returns NULL unless its argument is NULL.  
Since &data->kobj can never be NULL, the "if" test will never fail.  
Hence there's no point in making the test at all.

The fact that a section of code has existed for a long time doesn't 
mean that it is right.  :-)

Furthermore, there's no reason to do the kobject_get().  Holding 2 
references to a kobject is no better than holding just 1 reference.  
Assuming you know that the kobject is still registered, then you also 
know that there is already a reference to it.  So you have no reason to 
take an additional reference.

Alan Stern

Re: [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Greg KH]

On Fri, Feb 15, 2008 at 10:52:51AM -0500, Alan Stern wrote:
> On Fri, 15 Feb 2008, Yi Yang wrote:
> 
> > This patch adds kobject_put to balance refcount. I noticed Greg suggests
> > it will fix a power-off issue to remove kobject_get statement block, but i
> > think that isn't the best way because those code block has existed very long
> > and it is helpful because the successive statements are invoking relevant
> > data.
> 
> Are you referring to this section of code (before the region affected 
> by your patch)?
> 
> 	if (!kobject_get(&data->kobj)) {
> 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 		cpufreq_debug_enable_ratelimit();
> 		unlock_policy_rwsem_write(cpu);
> 		return -EFAULT;
> 	}
> 
> Greg is correct that the kobject_get() here is useless and should be
> removed.  kobject_get() never returns NULL unless its argument is NULL.  
> Since &data->kobj can never be NULL, the "if" test will never fail.  
> Hence there's no point in making the test at all.
> 
> The fact that a section of code has existed for a long time doesn't 
> mean that it is right.  :-)
> 
> Furthermore, there's no reason to do the kobject_get().  Holding 2 
> references to a kobject is no better than holding just 1 reference.  
> Assuming you know that the kobject is still registered, then you also 
> know that there is already a reference to it.  So you have no reason to 
> take an additional reference.

There's the additional problem that this second reference count is never
dropped, causing a bug :)

thanks,

greg k-h

Re: [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Greg KH]

On Fri, Feb 15, 2008 at 07:48:41AM +0800, Yi Yang wrote:
> When one cpu is set to offline, the caller process will hang, according to
> the trace data, the problem lies in the refcount error in cpufreq driver,
> cpufreq_cpu_callback will wait for completion policy->kobj_unregister
> which is nerver completed because a refcount error in function
> __cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
> calling kobject release method.
> 
> In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
> will be unregistered, this problem didn't exist in 2.6.24 and earlier.
> 
> The root cause is kobject API switch, kobject_init_and_add and kobject_put
> replace older kobject_register and kobject_unregister in 2.6.25-rc1,
> compared to 2.6.24, kobject_unregister is deleted in function
> __cpufreq_remove_dev but it isn't replaced with kobject_put.
> 
> This patch adds kobject_put to balance refcount. I noticed Greg suggests
> it will fix a power-off issue to remove kobject_get statement block, but i
> think that isn't the best way because those code block has existed very long
> and it is helpful because the successive statements are invoking relevant
> data.
> 
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>

No, the additional kobject_get() needs to be removed.  I posted a patch
for this last night, and so did someone else earlier at:
		http://lkml.org/lkml/2008/2/8/342

this patch should not be added, I'll get the other one in instead.

thanks,

greg k-h

[PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Yi Yang]

cpuidle C-state sysfs node time and usage are very easy to overflow because
they are all of unsigned int type, time will overflow within about two hours,
usage will take longer time to overflow, but they are increasing for ever.

This patch will convert them to unsigned long long.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 drivers/cpuidle/cpuidle.c |    2 +-
 drivers/cpuidle/sysfs.c   |   10 ++++++++--
 include/linux/cpuidle.h   |    4 ++--

--- a/include/linux/cpuidle.h	2008-02-25 02:31:26.000000000 -0500
+++ b/include/linux/cpuidle.h	2008-02-25 04:30:24.000000000 -0500
@@ -38,8 +38,8 @@ struct cpuidle_state {
 	unsigned int	power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
 
-	unsigned int	usage;
-	unsigned int	time; /* in US */
+	unsigned long long	usage;
+	unsigned long long	time; /* in US */
 
 	int (*enter)	(struct cpuidle_device *dev,
 			 struct cpuidle_state *state);
--- a/drivers/cpuidle/cpuidle.c	2008-02-25 02:37:14.000000000 -0500
+++ b/drivers/cpuidle/cpuidle.c	2008-02-25 04:29:19.000000000 -0500
@@ -67,7 +67,7 @@ static void cpuidle_idle_call(void)
 	/* enter the state and update stats */
 	dev->last_residency = target_state->enter(dev, target_state);
 	dev->last_state = target_state;
-	target_state->time += dev->last_residency;
+	target_state->time += (unsigned long long)dev->last_residency;
 	target_state->usage++;
 
 	/* give the governor an opportunity to reflect on the outcome */
--- a/drivers/cpuidle/sysfs.c	2008-02-25 02:33:14.000000000 -0500
+++ b/drivers/cpuidle/sysfs.c	2008-02-25 03:10:50.000000000 -0500
@@ -218,6 +218,12 @@ static ssize_t show_state_##_name(struct
 	return sprintf(buf, "%u\n", state->_name);\
 }
 
+#define define_show_state_ull_function(_name) \
+static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
+{ \
+	return sprintf(buf, "%llu\n", state->_name);\
+}
+
 #define define_show_state_str_function(_name) \
 static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
 { \
@@ -228,8 +234,8 @@ static ssize_t show_state_##_name(struct
 
 define_show_state_function(exit_latency)
 define_show_state_function(power_usage)
-define_show_state_function(usage)
-define_show_state_function(time)
+define_show_state_ull_function(usage)
+define_show_state_ull_function(time)
 define_show_state_str_function(name)
 define_show_state_str_function(desc)
 

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Yi Yang]

On Mon, 2008-02-25 at 11:15 +0100, Ingo Molnar wrote:
> * Yi Yang <yi.y.yang@intel.com> wrote:
> 
> > cpuidle C-state sysfs node time and usage are very easy to overflow 
> > because they are all of unsigned int type, time will overflow within 
> > about two hours, usage will take longer time to overflow, but they are 
> > increasing for ever.
> > 
> > This patch will convert them to unsigned long long.
> 
> what happens if such an overflow happens - any particular regression or 
> other misbehavior, or just funny looking stats in sysfs?
They are just stats info in sysfs, cpuidle's behaviors don't depend on
them. I didn't notice any regression or misbehaviors.

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

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Ingo Molnar]

* Yi Yang <yi.y.yang@intel.com> wrote:

> cpuidle C-state sysfs node time and usage are very easy to overflow 
> because they are all of unsigned int type, time will overflow within 
> about two hours, usage will take longer time to overflow, but they are 
> increasing for ever.
> 
> This patch will convert them to unsigned long long.

what happens if such an overflow happens - any particular regression or 
other misbehavior, or just funny looking stats in sysfs?

	Ingo

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Len Brown]

applied

thanks,
-len

On Sunday 24 February 2008, Yi Yang wrote:
> cpuidle C-state sysfs node time and usage are very easy to overflow because
> they are all of unsigned int type, time will overflow within about two hours,
> usage will take longer time to overflow, but they are increasing for ever.
> 
> This patch will convert them to unsigned long long.
> 
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |    2 +-
>  drivers/cpuidle/sysfs.c   |   10 ++++++++--
>  include/linux/cpuidle.h   |    4 ++--
> 
> --- a/include/linux/cpuidle.h	2008-02-25 02:31:26.000000000 -0500
> +++ b/include/linux/cpuidle.h	2008-02-25 04:30:24.000000000 -0500
> @@ -38,8 +38,8 @@ struct cpuidle_state {
>  	unsigned int	power_usage; /* in mW */
>  	unsigned int	target_residency; /* in US */
>  
> -	unsigned int	usage;
> -	unsigned int	time; /* in US */
> +	unsigned long long	usage;
> +	unsigned long long	time; /* in US */
>  
>  	int (*enter)	(struct cpuidle_device *dev,
>  			 struct cpuidle_state *state);
> --- a/drivers/cpuidle/cpuidle.c	2008-02-25 02:37:14.000000000 -0500
> +++ b/drivers/cpuidle/cpuidle.c	2008-02-25 04:29:19.000000000 -0500
> @@ -67,7 +67,7 @@ static void cpuidle_idle_call(void)
>  	/* enter the state and update stats */
>  	dev->last_residency = target_state->enter(dev, target_state);
>  	dev->last_state = target_state;
> -	target_state->time += dev->last_residency;
> +	target_state->time += (unsigned long long)dev->last_residency;
>  	target_state->usage++;
>  
>  	/* give the governor an opportunity to reflect on the outcome */
> --- a/drivers/cpuidle/sysfs.c	2008-02-25 02:33:14.000000000 -0500
> +++ b/drivers/cpuidle/sysfs.c	2008-02-25 03:10:50.000000000 -0500
> @@ -218,6 +218,12 @@ static ssize_t show_state_##_name(struct
>  	return sprintf(buf, "%u\n", state->_name);\
>  }
>  
> +#define define_show_state_ull_function(_name) \
> +static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
> +{ \
> +	return sprintf(buf, "%llu\n", state->_name);\
> +}
> +
>  #define define_show_state_str_function(_name) \
>  static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
>  { \
> @@ -228,8 +234,8 @@ static ssize_t show_state_##_name(struct
>  
>  define_show_state_function(exit_latency)
>  define_show_state_function(power_usage)
> -define_show_state_function(usage)
> -define_show_state_function(time)
> +define_show_state_ull_function(usage)
> +define_show_state_ull_function(time)
>  define_show_state_str_function(name)
>  define_show_state_str_function(desc)
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>