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

Hi!

Quoted 11 lines

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

Signature · 2 lines

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

David Brownell

2008-01-04 16:09 UTC #3 lore raw

Quoted 6 lines

Quoted 4 lines
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

David Brownell

2008-01-04 16:09 UTC #4 lore raw

In reply to Pavel Machek

Quoted 6 lines

Quoted 4 lines
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.

- Dave

Alan Stern

2008-01-04 16:38 UTC #5 lore raw

Quoted 9 lines · David Brownell

Quoted 6 lines
Quoted 4 lines
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

2008-01-04 16:38 UTC #6 lore raw

Quoted 9 lines · David Brownell

Quoted 6 lines
Quoted 4 lines
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

Olivier Galibert

2008-01-04 16:52 UTC #7 lore raw

Quoted 2 lines · Alan Stern

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

How about not changing a userland-visible interface gratuitously?

OG.

Olivier Galibert

2008-01-04 16:52 UTC #8 lore raw

In reply to Oliver Neukum

Quoted 2 lines · Alan Stern

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

How about not changing a userland-visible interface gratuitously?

OG.

Oliver Neukum

2008-01-04 17:20 UTC #9 lore raw

Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:

Quoted 5 lines · Alan Stern

Quoted 2 lines
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

Yi Yang

author

2008-01-07 02:00 UTC #10 lore raw

Quoted 7 lines · Oliver Neukum

Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:
Quoted 5 lines · Alan Stern
Quoted 2 lines
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?

Quoted 4 lines

Very good idea.
Regards
Oliver

Yi Yang

author

2008-01-07 02:08 UTC #11 lore raw

Quoted 7 lines · Oliver Neukum

Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:
Quoted 5 lines · Alan Stern
Quoted 2 lines
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?

Quoted 4 lines

Very good idea.
Regards
Oliver

Yi Yang

author

2008-01-07 01:55 UTC #12 lore raw

In reply to Olivier Galibert

Quoted 5 lines · Olivier Galibert

Quoted 2 lines · Alan Stern
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.

Quoted 3 lines

OG.

Yi Yang

author

2008-01-07 02:01 UTC #13 lore raw

In reply to Olivier Galibert

Quoted 5 lines · Olivier Galibert

Quoted 2 lines · Alan Stern
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.

Quoted 3 lines

OG.

David Brownell

2008-01-07 03:49 UTC #14 lore raw

Quoted 5 lines · Yi Yang

Quoted 2 lines
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.

David Brownell

2008-01-07 03:49 UTC #15 lore raw

Quoted 5 lines · Yi Yang

Quoted 2 lines
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.

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

Yi Yang

author

2008-01-07 01:52 UTC #16 lore raw

Quoted 7 lines · Alan Stern

Quoted 5 lines · David Brownell
Quoted 5 lines
Quoted 4 lines
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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 3 lines

Alan Stern

Yi Yang

author

2008-01-07 01:59 UTC #17 lore raw

Quoted 7 lines · Alan Stern

Quoted 5 lines · David Brownell
Quoted 5 lines
Quoted 4 lines
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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 3 lines

Alan Stern

Yi Yang

author

2008-01-07 01:37 UTC #18 lore raw

Quoted 7 lines · David Brownell

Quoted 6 lines
Quoted 4 lines
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?

Quoted 8 lines

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. :-).

Quoted 3 lines

- Dave

Yi Yang

author

2008-01-07 01:46 UTC #19 lore raw

In reply to Rafael J. Wysocki

Quoted 7 lines · David Brownell

Quoted 6 lines
Quoted 4 lines
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?

Quoted 8 lines

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. :-).

Quoted 3 lines

- Dave

Rafael J. Wysocki

2008-01-07 01:55 UTC #20 lore raw

Quoted 11 lines · Yi Yang

Quoted 10 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 5 lines
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

Yi Yang

author

2008-01-07 02:05 UTC #21 lore raw

Quoted 17 lines · Rafael J. Wysocki

Quoted 16 lines · Yi Yang
Quoted 15 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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...

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

Thanks,
Rafael

Yi Yang

author

2008-01-07 02:21 UTC #22 lore raw

Quoted 17 lines · Rafael J. Wysocki

Quoted 16 lines · Yi Yang
Quoted 15 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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...

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

Thanks,
Rafael

Rafael J. Wysocki

2008-01-07 16:43 UTC #23 lore raw

Quoted 18 lines · Yi Yang

Quoted 17 lines · Rafael J. Wysocki
Quoted 16 lines · Yi Yang
Quoted 15 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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...

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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

Rafael J. Wysocki

2008-01-07 16:46 UTC #24 lore raw

Quoted 18 lines · Yi Yang

Quoted 17 lines · Rafael J. Wysocki
Quoted 16 lines · Yi Yang
Quoted 15 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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...

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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

Rafael J. Wysocki

2008-01-07 01:57 UTC #25 lore raw

Quoted 11 lines · Yi Yang

Quoted 10 lines · David Brownell
Quoted 6 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 5 lines
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

Yi Yang

author

2008-01-07 01:21 UTC #26 lore raw

In reply to Pavel Machek

Quoted 9 lines · Pavel Machek

Hi!
Quoted 7 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 1 line

Yi Yang

author

2008-01-07 01:27 UTC #27 lore raw

In reply to Pavel Machek

Quoted 9 lines · Pavel Machek

Hi!
Quoted 7 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 1 line

David Brownell

2008-01-04 16:31 UTC #28 lore raw

Quoted 3 lines

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

Quoted 13 lines

drivers/base/power/sysfs.c +1 -1

@@ -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

David Brownell

2008-01-04 16:31 UTC #29 lore raw

Quoted 3 lines

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.

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

Quoted 13 lines

drivers/base/power/sysfs.c +1 -1

@@ -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

Yi Yang

author

2008-01-07 01:51 UTC #30 lore raw

Quoted 10 lines · David Brownell

Quoted 3 lines
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 ...

Quoted 3 lines

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. :-)

Quoted 15 lines

drivers/base/power/sysfs.c +1 -1

@@ -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

Yi Yang

author

2008-01-07 01:56 UTC #31 lore raw

Quoted 10 lines · David Brownell

Quoted 3 lines
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 ...

Quoted 3 lines

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. :-)

Quoted 15 lines

drivers/base/power/sysfs.c +1 -1

@@ -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

Yi Yang

author

2008-01-22 23:05 UTC #32 lore raw

PATCH 1/1 cpufreq: fix obvious condition statement error

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 <[email protected]>

---
 cpufreq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq.c +1 -1

@@ -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;
 	}

Yi Yang

author

2008-01-29 08:27 UTC #33 lore raw

PATCH 1/1 x86: add sysfs interface for cpuid module

Current cpuid module will create a char device for every logical cpu, when a user cats /dev/cpu/*/cpuid, he/she will enter a limitless loop, the root cause is that cpuid module doesn't decide wether a cpuid level is valid, it just uses an offset to denote cpuid level and take it to cpuid instruction, cpuid instruction will ignore it and return some data specific to cpu model, cpuid doesn't an error return value because it is void type. So cpuid module will execute cpuid continuously and return data although most of data make no sense.

This patch tries to add a sysfs interface for cpuid, users can see all the available cpuid levels, specify a specific level and get cpuid corresponding to this cpuid level.

For every logical cpu, this patch will create a cpuid directory under /sys/devices/system/cpu/cpu*/, there are three entries under cpuid:

avail_levels cur_level cur_cpuid

A user can get all the available cpuid levels from avail_levels, he/she can set one available cpuid level to cur_level, then he/she can get cpuid from cur_cpuid, cur_cpuid corresponds to cur_level.

This patch uses sysfs to avoid limitless loop and provide more flexible interface for cpuid, please consider to merge to -mm tree in order to test.

Here are some example output:
[root@yangyi-dev /]# ls /sys/devices/system/cpu/cpu0/cpuid avail_levels cur_cpuid cur_level
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpuid/avail_levels 0x00000000
0x00000001
0x00000002
0x00000003
0x00000004
0x00000005
0x00000006
0x00000007
0x00000008
0x00000009
0x0000000a
0x80000000
0x80000001
0x80000002
0x80000003
0x80000004
0x80000005
0x80000006
0x80000007
0x80000008
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpuid/cur_level 0x00000000
[root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpuid/cur_cpuid 0x0000000a 0x756e6547 0x6c65746e 0x49656e69
[root@yangyi-dev /]# echo 0x80000007 > /sys/devices/system/cpu/cpu0/cpuid/cur_level [root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpuid/cur_cpuid 0x00000000 0x00000000 0x00000000 0x00000000
[root@yangyi-dev /]# echo 0x80000001 > /sys/devices/system/cpu/cpu0/cpuid/cur_level [root@yangyi-dev /]# cat /sys/devices/system/cpu/cpu0/cpuid/cur_cpuid 0x00000000 0x00000000 0x00000001 0x20100000
[root@yangyi-dev /]# ls /sys/devices/system/cpu/cpu1/cpuid avail_levels cur_cpuid cur_level
[root@yangyi-dev /]#

Signed-off-by Yi Yang <[email protected]>

---
 cpuid.c |  233 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 232 insertions(+), 1 deletion(-)

arch/x86/kernel/cpuid.c +232 -1

@@ -175,6 +175,228 @@ static struct notifier_block __cpuinitda
 	.notifier_call = cpuid_class_cpu_callback,
 };
 
+struct cpuid_info {
+	int cpu;
+	unsigned int cur_level;
+	unsigned int min_level;
+	unsigned int max_level;
+	unsigned int min_ext_level;
+	unsigned int max_ext_level;
+	struct kobject kobj;
+};
+
+struct cpuid_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct cpuid_info *, char *);
+	ssize_t (*store)(struct cpuid_info *, const char *, size_t count);
+};
+
+static struct cpuid_info cpuid_infos[NR_CPUS];
+
+static cpumask_t cpu_map = CPU_MASK_NONE;
+
+static ssize_t show_cur_level(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	return sprintf(buf, "0x%08x\n", cpuid_info_p->cur_level);
+}
+
+static ssize_t store_cur_level(struct cpuid_info *cpuid_info_p,
+				const char *buf, size_t count)
+{
+	char *p;
+	unsigned int val;
+	size_t len = 13;
+	char tmpbuf[16];
+
+	if ((count > len) || (count <= 0))
+		return -EINVAL;
+
+	len = count;
+	if (buf[count-1] == '\n')
+		 len--;
+
+	memcpy(tmpbuf, buf, len);
+	tmpbuf[len] = '\0';
+	val = simple_strtoul(tmpbuf, &p, 0);
+
+	if (*p != '\0')
+		return -EINVAL;
+	if (((val >= cpuid_info_p->min_level)
+		&& (val <= cpuid_info_p->max_level))
+		|| ((val >= cpuid_info_p->min_ext_level)
+		&& (val <= cpuid_info_p->max_ext_level))) {
+		cpuid_info_p->cur_level = val;
+		return count;
+	} else
+		return -EINVAL;
+}
+
+static ssize_t show_avail_levels(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	unsigned int level;
+	ssize_t len = 0;
+
+	level = cpuid_info_p->min_level;
+	while (level <= cpuid_info_p->max_level) {
+		len += sprintf(buf + len, "0x%08x\n", level);
+		level++;
+	}
+
+	level = cpuid_info_p->min_ext_level;
+	while (level <= cpuid_info_p->max_ext_level) {
+		len += sprintf(buf + len, "0x%08x\n", level);
+		level++;
+	}
+	return len;
+}
+
+static ssize_t show_cur_cpuid(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	u32 data[4];
+
+	do_cpuid(cpuid_info_p->cpu, cpuid_info_p->cur_level, data);
+	return sprintf(buf, "0x%08x 0x%08x 0x%08x 0x%08x\n",
+			data[0], data[1], data[2], data[3]);
+}
+
+static struct cpuid_attr cur_level =
+	__ATTR(cur_level, 0600, show_cur_level, store_cur_level);
+static struct cpuid_attr avail_levels =
+	__ATTR(avail_levels, 0400, show_avail_levels, NULL);
+static struct cpuid_attr cur_cpuid =
+	__ATTR(cur_cpuid, 0400, show_cur_cpuid, NULL);
+
+static struct attribute *default_attrs[] = {
+	&cur_level.attr,
+	&avail_levels.attr,
+	&cur_cpuid.attr,
+	NULL
+};
+#define to_cpuid_info(k) container_of(k, struct cpuid_info, kobj)
+#define to_cpuid_attr(a) container_of(a, struct cpuid_attr, attr)
+
+static ssize_t check_cpu(struct cpuid_info *cpuid_info_p)
+{
+	int cpu = cpuid_info_p->cpu;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+	if (cpu >= NR_CPUS || !cpu_online(cpu))
+		return -ENXIO;	/* No such CPU */
+	if (c->cpuid_level < 0)
+		return -EIO;	/* CPUID not supported */
+
+	return 0;
+}
+
+static ssize_t cpuid_show(struct kobject *kobj, struct attribute *attr,
+				char *buf)
+{
+	ssize_t ret;
+	struct cpuid_attr *fattr = to_cpuid_attr(attr);
+	struct cpuid_info *cpuid_info_p = to_cpuid_info(kobj);
+
+	ret = check_cpu(cpuid_info_p);
+	if (ret != 0)
+		return ret;
+
+	return fattr->show(cpuid_info_p, buf);
+}
+
+static ssize_t cpuid_store(struct kobject *kobj, struct attribute *attr,
+				const char *buf, size_t count)
+{
+	struct cpuid_attr *fattr = to_cpuid_attr(attr);
+	struct cpuid_info *cpuid_info_p = to_cpuid_info(kobj);
+	ssize_t ret;
+
+	ret = check_cpu(cpuid_info_p);
+	if (ret != 0)
+		return ret;
+
+	ret = fattr->store ? fattr->store(cpuid_info_p, buf, count) : 0;
+	return ret;
+}
+
+static void cpuid_sysfs_release(struct kobject *kobj)
+{
+}
+
+static struct sysfs_ops cpuid_sysfs_ops = {
+	.show   = cpuid_show,
+	.store  = cpuid_store,
+};
+
+static struct kobj_type ktype_cpuid = {
+	.sysfs_ops      = &cpuid_sysfs_ops,
+	.default_attrs  = default_attrs,
+	.release        = cpuid_sysfs_release,
+};
+
+static __cpuinit int create_cpuid_sysfs(int cpu)
+{
+	struct sys_device *cpu_sys_dev = NULL;
+	u32 data[4];
+	int retval;
+
+	cpu_sys_dev = get_cpu_sysdev(cpu);
+	if (cpu_sys_dev == NULL)
+		return -1;
+
+	cpuid_infos[cpu].kobj.parent = &cpu_sys_dev->kobj;
+	kobject_set_name(&(cpuid_infos[cpu].kobj), "%s", "cpuid");
+	cpuid_infos[cpu].kobj.ktype = &ktype_cpuid;
+
+	do_cpuid(cpu, 0, data);
+	cpuid_infos[cpu].cpu = cpu;
+	cpuid_infos[cpu].cur_level = 0;
+	cpuid_infos[cpu].min_level = 0;
+	cpuid_infos[cpu].max_level = data[0];
+
+	do_cpuid(cpu, 0x80000000, data);
+	cpuid_infos[cpu].min_ext_level = 0x80000000;
+	cpuid_infos[cpu].max_ext_level = data[0];
+
+	retval = kobject_register(&(cpuid_infos[cpu].kobj));
+	if (retval < 0) {
+		cpu_clear(cpu, cpu_map);
+		return retval;
+	}
+	cpu_set(cpu, cpu_map);
+	return 0;
+}
+
+static void remove_cpuid_sysfs(int cpu)
+{
+	if (cpu_isset(cpu, cpu_map)) {
+		cpu_clear(cpu, cpu_map);
+		kobject_unregister(&(cpuid_infos[cpu].kobj));
+		memset(&cpuid_infos[cpu], 0, sizeof(struct cpuid_info));
+	}
+}
+
+static int __cpuinit cpuid_sysfs_cpu_callback(struct notifier_block *nfb,
+					      unsigned long action,
+					      void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+
+	switch (action) {
+	case CPU_ONLINE:
+	case CPU_ONLINE_FROZEN:
+		create_cpuid_sysfs(cpu);
+		break;
+	case CPU_DEAD:
+	case CPU_DEAD_FROZEN:
+		remove_cpuid_sysfs(cpu);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __cpuinitdata cpuid_sysfs_cpu_notifier = {
+	.notifier_call = cpuid_sysfs_cpu_callback,
+};
+
 static int __init cpuid_init(void)
 {
 	int i, err = 0;

@@ -195,8 +417,13 @@ static int __init cpuid_init(void)
 		err = cpuid_device_create(i);
 		if (err != 0) 
 			goto out_class;
+
+		err = create_cpuid_sysfs(i);
+		if (err != 0)
+			goto out_class;
 	}
 	register_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	register_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
 
 	err = 0;
 	goto out;

@@ -205,6 +432,7 @@ out_class:
 	i = 0;
 	for_each_online_cpu(i) {
 		cpuid_device_destroy(i);
+		remove_cpuid_sysfs(i);
 	}
 	class_destroy(cpuid_class);
 out_chrdev:

@@ -217,11 +445,14 @@ static void __exit cpuid_exit(void)
 {
 	int cpu = 0;
 
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
 		cpuid_device_destroy(cpu);
+		remove_cpuid_sysfs(cpu);
+	}
 	class_destroy(cpuid_class);
 	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
 	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
 }
 
 module_init(cpuid_init);

Sam Ravnborg

2008-01-29 08:44 UTC #34 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

arch/x86/kernel/cpuid.c static struct notifier_block __cpuinitda · line 398

Quoted 4 lines

arch/x86/kernel/cpuid.c +4 -0

+
+static struct notifier_block __cpuinitdata cpuid_sysfs_cpu_notifier = {
+	.notifier_call = cpuid_sysfs_cpu_callback,
+};

Data is annotated _cpuintidata

but

Quoted 1 line

+

Data is annotated _cpuintidata

arch/x86/kernel/cpuid.c static void __exit cpuid_exit(void) · line 455

Quoted 13 lines

arch/x86/kernel/cpuid.c +4 -1

@@ -217,11 +445,14 @@ static void __exit cpuid_exit(void)
 {
 	int cpu = 0;
 
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
 		cpuid_device_destroy(cpu);
+		remove_cpuid_sysfs(cpu);
+	}
 	class_destroy(cpuid_class);
 	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
 	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);

used in an __exit function.

You should have seen a Section mismatch warning for this. The right fix is to annotate the cpuid_sysfs_cpu_notifier with __initdata_refok (soon to be named __refdata) Or even better to declare it const and use _refconst.

Sam

Yi Yang

author

2008-01-30 07:35 UTC #35 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 27 lines · Sam Ravnborg

arch/x86/kernel/cpuid.c static struct notifier_block __cpuinitda · line 398
Quoted 4 lines
arch/x86/kernel/cpuid.c +4 -0
+
+static struct notifier_block __cpuinitdata cpuid_sysfs_cpu_notifier = {
+	.notifier_call = cpuid_sysfs_cpu_callback,
+};
Data is annotated _cpuintidata
but
Quoted 1 line
+
Data is annotated _cpuintidata
arch/x86/kernel/cpuid.c static void __exit cpuid_exit(void) · line 455
Quoted 13 lines
arch/x86/kernel/cpuid.c +4 -1
@@ -217,11 +445,14 @@ static void __exit cpuid_exit(void)
 {
 	int cpu = 0;
 
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
 		cpuid_device_destroy(cpu);
+		remove_cpuid_sysfs(cpu);
+	}
 	class_destroy(cpuid_class);
 	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
 	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
used in an __exit function.

Quoted 4 lines

You should have seen a Section mismatch warning for this. The right fix is to annotate the cpuid_sysfs_cpu_notifier with __initdata_refok (soon to be named __refdata) Or even better to declare it const and use _refconst.

I think __cpuinitdata is different from __initdata, i have tested it by insmod, rmmod, echo 0/1 > /sys/devices/system/cpu/cpu1/online repeatly, it hasn't any issue.

Quoted 2 lines

Sam

Sam Ravnborg

2008-01-30 07:49 UTC #36 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 28 lines · Yi Yang

Quoted 27 lines · Sam Ravnborg
arch/x86/kernel/cpuid.c static struct notifier_block __cpuinitda · line 398
Quoted 4 lines
arch/x86/kernel/cpuid.c +4 -0
+
+static struct notifier_block __cpuinitdata cpuid_sysfs_cpu_notifier = {
+	.notifier_call = cpuid_sysfs_cpu_callback,
+};
Data is annotated _cpuintidata
but
Quoted 1 line
+
Data is annotated _cpuintidata
arch/x86/kernel/cpuid.c static void __exit cpuid_exit(void) · line 455
Quoted 13 lines
arch/x86/kernel/cpuid.c +4 -1
@@ -217,11 +445,14 @@ static void __exit cpuid_exit(void)
 {
 	int cpu = 0;
 
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
 		cpuid_device_destroy(cpu);
+		remove_cpuid_sysfs(cpu);
+	}
 	class_destroy(cpuid_class);
 	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
 	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
used in an __exit function.

Quoted 7 lines

Quoted 4 lines
You should have seen a Section mismatch warning for this. The right fix is to annotate the cpuid_sysfs_cpu_notifier with __initdata_refok (soon to be named __refdata) Or even better to declare it const and use _refconst.
I think __cpuinitdata is different from __initdata, i have tested it by insmod, rmmod, echo 0/1 > /sys/devices/system/cpu/cpu1/online repeatly, it hasn't any issue.

__cpuinit & _cpuinitdata have over time been used for different purposes:
a) To annotate code/data used in the init path and that in the non HOTPLUG_CPU case can be discarded after init.
b) To annotate code/data used in the 'core' HOTPLUG_CPU functionality that isonly in use if HOTPLUG_CPU='y'

The b) usage is questionable and the annotation of cpuid_sysfs_cpu_notifier beongs in the b) category.

The correct solution would be to factor out the 'core' HOTPLUG_CPU=y code to a set of separate files and used to usual mechanishm in the Makefile to select when to include this code in the kernel.

The improved section mismatch checks by modpost has just brought this issue to the attention and now you add code that does the wrong thing it is being discussed.

Sam

H. Peter Anvin

maintainer

2008-01-29 15:54 UTC #37 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

In reply to H. Peter Anvin

Quoted 16 lines · Yi Yang

Current cpuid module will create a char device for every logical cpu, when a user cats /dev/cpu/*/cpuid, he/she will enter a limitless loop, the root cause is that cpuid module doesn't decide wether a cpuid level is valid, it just uses an offset to denote cpuid level and take it to cpuid instruction, cpuid instruction will ignore it and return some data specific to cpu model, cpuid doesn't an error return value because it is void type. So cpuid module will execute cpuid continuously and return data although most of data make no sense.
This patch tries to add a sysfs interface for cpuid, users can see all the available cpuid levels, specify a specific level and get cpuid corresponding to this cpuid level.
For every logical cpu, this patch will create a cpuid directory under /sys/devices/system/cpu/cpu*/, there are three entries under cpuid:

Quoted 8 lines

avail_levels cur_level cur_cpuid
A user can get all the available cpuid levels from avail_levels, he/she can set one available cpuid level to cur_level, then he/she can get cpuid from cur_cpuid, cur_cpuid corresponds to cur_level.
This patch uses sysfs to avoid limitless loop and provide more flexible interface for cpuid, please consider to merge to -mm tree in order to test.

This is broken.

Triple broken.

It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)

It's broken, because the algorithm used to determine valid CPUID levels is incorrect; it fails to recognize any CPUID levels other than the main Intel and AMD ones, e.g. the Transmeta 0x8086xxxx (and sometimes more) and VIA 0xc000xxxx levels.

It's broken, because it is better for the userspace extractor to have this logic than to stuff it into the kernel, where it sits hogging unswappable memory at all times.

-hpa

Yi Yang

author

2008-01-30 03:04 UTC #38 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 36 lines · H. Peter Anvin

Quoted 16 lines · Yi Yang
Current cpuid module will create a char device for every logical cpu, when a user cats /dev/cpu/*/cpuid, he/she will enter a limitless loop, the root cause is that cpuid module doesn't decide wether a cpuid level is valid, it just uses an offset to denote cpuid level and take it to cpuid instruction, cpuid instruction will ignore it and return some data specific to cpu model, cpuid doesn't an error return value because it is void type. So cpuid module will execute cpuid continuously and return data although most of data make no sense.
This patch tries to add a sysfs interface for cpuid, users can see all the available cpuid levels, specify a specific level and get cpuid corresponding to this cpuid level.
For every logical cpu, this patch will create a cpuid directory under /sys/devices/system/cpu/cpu*/, there are three entries under cpuid:
Quoted 8 lines
avail_levels cur_level cur_cpuid
A user can get all the available cpuid levels from avail_levels, he/she can set one available cpuid level to cur_level, then he/she can get cpuid from cur_cpuid, cur_cpuid corresponds to cur_level.
This patch uses sysfs to avoid limitless loop and provide more flexible interface for cpuid, please consider to merge to -mm tree in order to test.
This is broken.
Triple broken.
It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)

Quoted 4 lines

It's broken, because the algorithm used to determine valid CPUID levels is incorrect; it fails to recognize any CPUID levels other than the main Intel and AMD ones, e.g. the Transmeta 0x8086xxxx (and sometimes more) and VIA 0xc000xxxx levels.

Thank you for pointing out these issues, i think we can let users input any cpuid level and output the corresponding cpuid, in this way we can avoid to consider cpu differences and left this to userspace. We can also consider all the x86 platforms to do cpuid for every one.

Quoted 4 lines

It's broken, because it is better for the userspace extractor to have this logic than to stuff it into the kernel, where it sits hogging unswappable memory at all times.

It seems not to be very appropriate to let user space consider hardware details. /proc/cpuinfo should be an example to justify this.

Is there any user application using /dev/cpu/*/cpuid? if no, i think it is feasible to provide an interface in the kernel.

I noticed an application cpu-z on Windows, maybe we can clone it on Linux.

Quoted 2 lines

-hpa

H. Peter Anvin

maintainer

2008-01-30 06:10 UTC #39 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 4 lines · Yi Yang

Quoted 4 lines
It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)
It's broken, because the algorithm used to determine valid CPUID levels is incorrect; it fails to recognize any CPUID levels other than the main Intel and AMD ones, e.g. the Transmeta 0x8086xxxx (and sometimes more) and VIA 0xc000xxxx levels.

Quoted 8 lines

Thank you for pointing out these issues, i think we can let users input any cpuid level and output the corresponding cpuid, in this way we can avoid to consider cpu differences and left this to userspace. We can also consider all the x86 platforms to do cpuid for every one.
Quoted 1 line
It's broken, because it is better for the userspace extractor to have this logic than to stuff it into the kernel, where it sits hogging unswappable memory at all times.
It seems not to be very appropriate to let user space consider hardware details. /proc/cpuinfo should be an example to justify this.

/proc/cpuinfo represents what the kernel needs to know, so it reflects the kernel's interpretation of CPUID. There is no reason to interpret things in the kernel that the kernel doesn't need.

Quoted 2 lines

Is there any user application using /dev/cpu/*/cpuid? if no, i think it is feasible to provide an interface in the kernel.

Yes. It's called x86info, I believe.

-hpa

Yi Yang

author

2008-01-30 07:08 UTC #40 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 12 lines · H. Peter Anvin

Quoted 8 lines · Yi Yang
Current cpuid module will create a char device for every logical cpu, when a user cats /dev/cpu/*/cpuid, he/she will enter a limitless loop, the root cause is that cpuid module doesn't decide wether a cpuid level is valid, it just uses an offset to denote cpuid level and take it to cpuid instruction, cpuid instruction will ignore it and return some data
This patch uses sysfs to avoid limitless loop and provide more flexible interface for cpuid, please consider to merge to -mm tree in order to test.
This is broken.

Quoted 7 lines

Triple broken.
It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)

level 4 doesn't result in repeating on Intel CPU, cpuid module sets file offset to level, so cat /dev/cpu/*/cpuid will run cpuid instruction continuously.

H. Peter Anvin

maintainer

2008-01-30 07:18 UTC #41 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 6 lines · Yi Yang

Quoted 2 lines
It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)
level 4 doesn't result in repeating on Intel CPU, cpuid module sets file offset to level, so cat /dev/cpu/*/cpuid will run cpuid instruction continuously.

The issue is that Intel suddenly made CPUID ECX-sensitive, which there is no way to represent.

As far as cat /dev/cpu/*/cpuid, that's a user error.

-hpa

Yi Yang

author

2008-01-30 07:24 UTC #42 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 8 lines · H. Peter Anvin

Quoted 6 lines · Yi Yang
Quoted 6 lines
It's broken, because it doesn't take into account the fact that Intel broke CPUID level 4 and made it "repeating" (neither did the cpuid char device, because it predated the Intel braindamage; I've had a patch for it privately for a while, but didn't push it upstream because paravirt broke it royally and I wanted the situation to settle down.)

Quoted 6 lines

Quoted 3 lines
level 4 doesn't result in repeating on Intel CPU, cpuid module sets file offset to level, so cat /dev/cpu/*/cpuid will run cpuid instruction continuously.
The issue is that Intel suddenly made CPUID ECX-sensitive, which there is no way to represent.

Function cpuid has reset ecx to 0 immediate before calling to __cpuid, so this shouldn't be a problem now.

in include/asm-x86/processor_32.h
/*
* Generic CPUID function
* clear %ecx since some cpus (Cyrix MII) do not set or clear %ecx
* resulting in stale register contents being returned. */
static inline void cpuid(unsigned int op,
unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx)

{
        *eax = op;
        *ecx = 0;
        __cpuid(eax, ebx, ecx, edx);
}

Quoted 5 lines

As far as cat /dev/cpu/*/cpuid, that's a user error.
-hpa

H. Peter Anvin

maintainer

2008-01-30 07:34 UTC #43 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 2 lines · Yi Yang

Function cpuid has reset ecx to 0 immediate before calling to __cpuid, so this shouldn't be a problem now.

Unless, of course, you want to get to the information for the higher CPUID levels.

The easiest way to fix that would be to use cpuid_count() and let /dev/cpu/*/cpuid take the %ecx value in the high half of the offset. That would have minimal impact on the interface.

-hpa

Yi Yang

author

2008-01-30 08:56 UTC #44 lore raw

RFC PATCH 1/1 x86: add sysfs interface for cpuid module, try 2

This patch is try 2, it should cover VIA/Cyrix and Transmeta Crusoe, please help to test it if anybody has VIA/Cyrix and Transmeta Crusoe. This patch just makes users get cpuid raw data more conveniently by /sys/devices/syste/cpu/cpu*/cpuid/* without using any userspace application.

This patch tries to add a sysfs interface for cpuid, users can see all the available cpuid levels, specify a specific level and get cpuid corresponding to this cpuid level.

For every logical cpu, this patch will create a cpuid directory under /sys/devices/system/cpu/cpu*/, there are three entries under cpuid:

avail_levels cur_level cur_cpuid

Signed-off-by Yi Yang <[email protected]>

---
 cpuid.c |  255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 254 insertions(+), 1 deletion(-)

arch/x86/kernel/cpuid.c +254 -1

@@ -175,6 +175,250 @@ static struct notifier_block __cpuinitda
 	.notifier_call = cpuid_class_cpu_callback,
 };
 
+struct cpuid_info {
+	int cpu;
+	unsigned int cur_level;
+	unsigned int min_level;
+	unsigned int max_level;
+	unsigned int min_ext_level;
+	unsigned int max_ext_level;
+	struct kobject kobj;
+};
+
+struct cpuid_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct cpuid_info *, char *);
+	ssize_t (*store)(struct cpuid_info *, const char *, size_t count);
+};
+
+static struct cpuid_info cpuid_infos[NR_CPUS];
+
+static cpumask_t cpu_map = CPU_MASK_NONE;
+
+#define A32(a, b, c, d) (((d) << 24) + ((c) << 16) + ((b) << 8) + (a))
+
+static int is_centaur(u32 *data)
+{
+	return data[1] == A32('C', 'e', 'n', 't') &&
+	       data[3] == A32('a', 'u', 'r', 'H') &&
+	       data[2] == A32('a', 'u', 'l', 's');
+}
+
+static int is_transmeta(u32 *data)
+{
+	return data[1] == A32('G', 'e', 'n', 'u') &&
+	       data[3] == A32('i', 'n', 'e', 'T') &&
+	       data[2] == A32('M', 'x', '8', '6');
+}
+
+static ssize_t show_cur_level(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	return sprintf(buf, "0x%08x\n", cpuid_info_p->cur_level);
+}
+
+static ssize_t store_cur_level(struct cpuid_info *cpuid_info_p,
+				const char *buf, size_t count)
+{
+	char *p;
+	unsigned int val;
+	size_t len = 13;
+	char tmpbuf[16];
+
+	if ((count > len) || (count <= 0))
+		return -EINVAL;
+
+	len = count;
+	if (buf[count-1] == '\n')
+		 len--;
+
+	memcpy(tmpbuf, buf, len);
+	tmpbuf[len] = '\0';
+	val = simple_strtoul(tmpbuf, &p, 0);
+
+	if (*p != '\0')
+		return -EINVAL;
+	if (((val >= cpuid_info_p->min_level)
+		&& (val <= cpuid_info_p->max_level))
+		|| ((val >= cpuid_info_p->min_ext_level)
+		&& (val <= cpuid_info_p->max_ext_level))) {
+		cpuid_info_p->cur_level = val;
+		return count;
+	} else
+		return -EINVAL;
+}
+
+static ssize_t show_avail_levels(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	unsigned int level;
+	ssize_t len = 0;
+
+	level = cpuid_info_p->min_level;
+	while (level <= cpuid_info_p->max_level) {
+		len += sprintf(buf + len, "0x%08x\n", level);
+		level++;
+	}
+
+	level = cpuid_info_p->min_ext_level;
+	while (level <= cpuid_info_p->max_ext_level) {
+		len += sprintf(buf + len, "0x%08x\n", level);
+		level++;
+	}
+	return len;
+}
+
+static ssize_t show_cur_cpuid(struct cpuid_info *cpuid_info_p, char *buf)
+{
+	u32 data[4];
+
+	do_cpuid(cpuid_info_p->cpu, cpuid_info_p->cur_level, data);
+	return sprintf(buf, "0x%08x 0x%08x 0x%08x 0x%08x\n",
+			data[0], data[1], data[2], data[3]);
+}
+
+static struct cpuid_attr cur_level =
+	__ATTR(cur_level, 0600, show_cur_level, store_cur_level);
+static struct cpuid_attr avail_levels =
+	__ATTR(avail_levels, 0400, show_avail_levels, NULL);
+static struct cpuid_attr cur_cpuid =
+	__ATTR(cur_cpuid, 0400, show_cur_cpuid, NULL);
+
+static struct attribute *default_attrs[] = {
+	&cur_level.attr,
+	&avail_levels.attr,
+	&cur_cpuid.attr,
+	NULL
+};
+#define to_cpuid_info(k) container_of(k, struct cpuid_info, kobj)
+#define to_cpuid_attr(a) container_of(a, struct cpuid_attr, attr)
+
+static ssize_t check_cpu(struct cpuid_info *cpuid_info_p)
+{
+	int cpu = cpuid_info_p->cpu;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+	if (cpu >= NR_CPUS || !cpu_online(cpu))
+		return -ENXIO;	/* No such CPU */
+	if (c->cpuid_level < 0)
+		return -EIO;	/* CPUID not supported */
+
+	return 0;
+}
+
+static ssize_t cpuid_show(struct kobject *kobj, struct attribute *attr,
+				char *buf)
+{
+	ssize_t ret;
+	struct cpuid_attr *fattr = to_cpuid_attr(attr);
+	struct cpuid_info *cpuid_info_p = to_cpuid_info(kobj);
+
+	ret = check_cpu(cpuid_info_p);
+	if (ret != 0)
+		return ret;
+
+	return fattr->show(cpuid_info_p, buf);
+}
+
+static ssize_t cpuid_store(struct kobject *kobj, struct attribute *attr,
+				const char *buf, size_t count)
+{
+	struct cpuid_attr *fattr = to_cpuid_attr(attr);
+	struct cpuid_info *cpuid_info_p = to_cpuid_info(kobj);
+	ssize_t ret;
+
+	ret = check_cpu(cpuid_info_p);
+	if (ret != 0)
+		return ret;
+
+	ret = fattr->store ? fattr->store(cpuid_info_p, buf, count) : 0;
+	return ret;
+}
+
+static void cpuid_sysfs_release(struct kobject *kobj)
+{
+}
+
+static struct sysfs_ops cpuid_sysfs_ops = {
+	.show   = cpuid_show,
+	.store  = cpuid_store,
+};
+
+static struct kobj_type ktype_cpuid = {
+	.sysfs_ops      = &cpuid_sysfs_ops,
+	.default_attrs  = default_attrs,
+	.release        = cpuid_sysfs_release,
+};
+
+static __init_refok int create_cpuid_sysfs(int cpu)
+{
+	struct sys_device *cpu_sys_dev = NULL;
+	u32 data[4];
+	int retval;
+	u32 ext_level = 0x80000000;
+
+	cpu_sys_dev = get_cpu_sysdev(cpu);
+	if (cpu_sys_dev == NULL)
+		return -1;
+
+	cpuid_infos[cpu].kobj.parent = &cpu_sys_dev->kobj;
+	kobject_set_name(&(cpuid_infos[cpu].kobj), "%s", "cpuid");
+	cpuid_infos[cpu].kobj.ktype = &ktype_cpuid;
+
+	do_cpuid(cpu, 0, data);
+	cpuid_infos[cpu].cpu = cpu;
+	cpuid_infos[cpu].cur_level = 0;
+	cpuid_infos[cpu].min_level = 0;
+	cpuid_infos[cpu].max_level = data[0];
+
+	if (is_centaur(data))
+		ext_level = 0xc0000000;
+	else if (is_transmeta(data))
+		ext_level = 0x80860000;
+
+	do_cpuid(cpu, ext_level, data);
+	cpuid_infos[cpu].min_ext_level = ext_level;
+	cpuid_infos[cpu].max_ext_level = data[0];
+
+	retval = kobject_register(&(cpuid_infos[cpu].kobj));
+	if (retval < 0) {
+		cpu_clear(cpu, cpu_map);
+		return retval;
+	}
+	cpu_set(cpu, cpu_map);
+	return 0;
+}
+
+static void remove_cpuid_sysfs(int cpu)
+{
+	if (cpu_isset(cpu, cpu_map)) {
+		cpu_clear(cpu, cpu_map);
+		kobject_unregister(&(cpuid_infos[cpu].kobj));
+		memset(&cpuid_infos[cpu], 0, sizeof(struct cpuid_info));
+	}
+}
+
+static int __init_refok cpuid_sysfs_cpu_callback(struct notifier_block *nfb,
+					      unsigned long action,
+					      void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+
+	switch (action) {
+	case CPU_ONLINE:
+	case CPU_ONLINE_FROZEN:
+		create_cpuid_sysfs(cpu);
+		break;
+	case CPU_DEAD:
+	case CPU_DEAD_FROZEN:
+		remove_cpuid_sysfs(cpu);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __initdata_refok cpuid_sysfs_cpu_notifier = {
+	.notifier_call = cpuid_sysfs_cpu_callback,
+};
+
 static int __init cpuid_init(void)
 {
 	int i, err = 0;

@@ -195,8 +439,13 @@ static int __init cpuid_init(void)
 		err = cpuid_device_create(i);
 		if (err != 0) 
 			goto out_class;
+
+		err = create_cpuid_sysfs(i);
+		if (err != 0)
+			goto out_class;
 	}
 	register_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	register_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
 
 	err = 0;
 	goto out;

@@ -205,6 +454,7 @@ out_class:
 	i = 0;
 	for_each_online_cpu(i) {
 		cpuid_device_destroy(i);
+		remove_cpuid_sysfs(i);
 	}
 	class_destroy(cpuid_class);
 out_chrdev:

@@ -217,11 +467,14 @@ static void __exit cpuid_exit(void)
 {
 	int cpu = 0;
 
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
 		cpuid_device_destroy(cpu);
+		remove_cpuid_sysfs(cpu);
+	}
 	class_destroy(cpuid_class);
 	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
 	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
+	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
 }
 
 module_init(cpuid_init);

H. Peter Anvin

maintainer

2008-01-30 17:36 UTC #45 lore raw

Re: [RFC PATCH 2.6.24] x86: add sysfs interface for cpuid module, try 2

Quoted 4 lines · Yi Yang

This patch is try 2, it should cover VIA/Cyrix and Transmeta Crusoe, please help to test it if anybody has VIA/Cyrix and Transmeta Crusoe. This patch just makes users get cpuid raw data more conveniently by /sys/devices/syste/cpu/cpu*/cpuid/* without using any userspace application.

Still completely useless garbage.

-hpa

Yi Yang

author

2008-01-31 10:34 UTC #46 lore raw

PATCH 1/1 Add new string functions real_strtoul and change kernel params to use them

Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/ull, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores successive invalid char and converts prefix part to a numeric although input is actually invalid.

This patch tries to fix the aforementioned issues and implements real_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:

write error: Invalid argument

Any modules which export numeric sysfs node can use real_strtox instead of simple_strtox to reject any invalid input. Please consider to merge to -mm tree in order to test.

Here are some test results:

Before applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#

After applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#

Signed-off-by Yi Yang <[email protected]>

---
 include/linux/kernel.h |    4 ++++
 kernel/params.c        |   20 ++++++++++----------
 lib/vsprintf.c         |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)

include/linux/kernel.h +4 -0

@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int real_strtoul(const char *, unsigned int, unsigned long *);
+extern int real_strtol(const char *, unsigned int, long *);
+extern int real_strtoull(const char *, unsigned int, unsigned long long *);
+extern int real_strtoll(const char *, unsigned int, long long *);
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)

lib/vsprintf.c +46 -0

52 lines not on this page · raw message

kernel/params.c +10 ~~-10~~

@@ -180,12 +180,12 @@ int parse_args(const char *name,
 #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
 	int param_set_##name(const char *val, struct kernel_param *kp)	\
 	{								\
-		char *endp;						\
 		tmptype l;						\
+		int ret;						\
 									\
 		if (!val) return -EINVAL;				\
-		l = strtolfn(val, &endp, 0);				\
-		if (endp == val || ((type)l != l))			\
+		ret = strtolfn(val, 0, &l);				\
+		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\

@@ -195,13 +195,13 @@ int parse_args(const char *name,
 		return sprintf(buffer, format, *((type *)kp->arg));	\
 	}
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, simple_strtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, simple_strtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, simple_strtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, simple_strtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(short, short, "%hi", long, real_strtol);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(int, int, "%i", long, real_strtol);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(long, long, "%li", long, real_strtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, real_strtoul);
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {

Randy Dunlap

2008-01-31 17:05 UTC #47 lore raw

Re: [PATCH 2.6.24] Add new string functions real_strtoul and change kernel params to use them

Quoted 9 lines · On Thu, 31 Jan 2008 09:18:22 +0800 Yi Yang wrote:

Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/ull, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores successive invalid char and converts prefix part to a numeric although input is actually invalid.

Quoted 3 lines

This patch tries to fix the aforementioned issues and implements real_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:

How about a prefix of safe_ or strict_ or something other than real_? real_ sounds too much like a real number function string parser.

Quoted 176 lines

write error: Invalid argument
Any modules which export numeric sysfs node can use real_strtox instead of simple_strtox to reject any invalid input. Please consider to merge to -mm tree in order to test.
Here are some test results:
Before applying this patch:
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#
After applying this patch:
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#
Signed-off-by Yi Yang <[email protected]>
---
 include/linux/kernel.h |    4 ++++
 kernel/params.c        |   20 ++++++++++----------
 lib/vsprintf.c         |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)
include/linux/kernel.h +4 -0
@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int real_strtoul(const char *, unsigned int, unsigned long *);
+extern int real_strtol(const char *, unsigned int, long *);
+extern int real_strtoull(const char *, unsigned int, unsigned long long *);
+extern int real_strtoll(const char *, unsigned int, long long *);
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)
lib/vsprintf.c +46 -0
@@ -126,6 +126,52 @@ long long simple_strtoll(const char *cp,
 	return simple_strtoull(cp,endp,base);
 }
 
+#define define_real_strtoux(type, valtype)				\
+int real_strtou##type(const char *cp, unsigned int base, valtype *res)	\
+{									\
+	char *tail;							\
+	valtype val;							\
+	size_t len;							\
+									\
+	*res = 0;							\
+	len = strlen(cp);						\
+	if (len == 0)							\
+		return -EINVAL;						\
+									\
+	val = simple_strtoul(cp, &tail, base);				\
+	if ((*tail == '\0') ||						\
+		(len == (size_t)(tail - cp) + 1) && (*tail == '\n')) {	\
+		*res = val;						\
+		return 0;						\
+	}								\
+									\
+	return -EINVAL;							\
+}									\
+
+#define define_real_strtox(type, valtype)				\
+int real_strto##type(const char *cp, unsigned int base, valtype *res)	\
+{									\
+	int ret;							\
+	if (*cp == '-') {						\
+		ret = real_strtou##type(cp+1, base, res);		\
+		if (ret != 0)						\
+			*res = -(*res);					\
+	} else								\
+		ret = real_strtou##type(cp+1, base, res);		\
+									\
+	return ret;							\
+}									\
+
+define_real_strtoux(l, unsigned long)
+define_real_strtox(l, long)
+define_real_strtoux(ll, unsigned long long)
+define_real_strtox(ll, long long)
+
+EXPORT_SYMBOL(real_strtoul);
+EXPORT_SYMBOL(real_strtol);
+EXPORT_SYMBOL(real_strtoll);
+EXPORT_SYMBOL(real_strtoull);
+
 static int skip_atoi(const char **s)
 {
 	int i=0;
kernel/params.c +10 ~~-11~~
@@ -180,12 +180,12 @@ int parse_args(const char *name,
 #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
 	int param_set_##name(const char *val, struct kernel_param *kp)	\
 	{								\
-		char *endp;						\
 		tmptype l;						\
+		int ret;						\
 									\
 		if (!val) return -EINVAL;				\
-		l = strtolfn(val, &endp, 0);				\
-		if (endp == val || ((type)l != l))			\
+		ret = strtolfn(val, 0, &l);				\
+		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\
@@ -195,13 +195,13 @@ int parse_args(const char *name,
 		return sprintf(buffer, format, *((type *)kp->arg));	\
 	}
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, simple_strtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, simple_strtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, simple_strtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, simple_strtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(short, short, "%hi", long, real_strtol);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(int, int, "%i", long, real_strtol);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, real_strtoul);
+STANDARD_PARAM_DEF(long, long, "%li", long, real_strtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, real_strtoul);
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {
 
 
--

Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

---
~Randy

Yi Yang

author

2008-02-01 01:42 UTC #48 lore raw

Re: [PATCH 2.6.24] Add new string functions real_strtoul and change kernel params to use them

Quoted 11 lines · Randy Dunlap

Quoted 9 lines · On Thu, 31 Jan 2008 09:18:22 +0800 Yi Yang wrote:
Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/ull, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores successive invalid char and converts prefix part to a numeric although input is actually invalid.

Quoted 7 lines

Quoted 3 lines
This patch tries to fix the aforementioned issues and implements real_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:
How about a prefix of safe_ or strict_ or something other than real_? real_ sounds too much like a real number function string parser.

I named it as strict_ at the beginning, but it results in some alignment issues checkpatch.pl will always warn, i don't know if warnings will make the patch out of the door.

In kernel/params.c, STANDARD_PARAM_DEF(a function definition macro) will be over 80 chars, is it correct coding style to split it to two lines?

Randy Dunlap

2008-02-01 02:48 UTC #49 lore raw

Re: [PATCH 2.6.24] Add new string functions real_strtoul and change kernel params to use them

Quoted 19 lines · On Fri, 01 Feb 2008 00:30:17 +0800 Yi Yang wrote:

Quoted 11 lines · Randy Dunlap
Quoted 9 lines · On Thu, 31 Jan 2008 09:18:22 +0800 Yi Yang wrote:
Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/ull, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores successive invalid char and converts prefix part to a numeric although input is actually invalid.
Quoted 7 lines
Quoted 3 lines
This patch tries to fix the aforementioned issues and implements real_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:
How about a prefix of safe_ or strict_ or something other than real_? real_ sounds too much like a real number function string parser.

Quoted 6 lines

I named it as strict_ at the beginning, but it results in some alignment issues checkpatch.pl will always warn, i don't know if warnings will make the patch out of the door.
In kernel/params.c, STANDARD_PARAM_DEF(a function definition macro) will be over 80 chars, is it correct coding style to split it to two lines?

Yes, if it can be done cleanly. Otherwise just leave it long (IMHO).

---
~Randy

Yi Yang

author

2008-02-01 07:23 UTC #50 lore raw

PATCH 1/1 Add new string functions strict_strto* and convert kernel params to use them, take 2

This patch is a resend, it changes previous name "real_" to "strict_" according to Randy Dunlap's feedback. Please consider to apply. thanks.

Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/uul, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores succesive invalid char and converts prefix part to a numeric although input is acctually invalid.

This patch tries to fix the aforementioned issues and implements strict_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:

write error: Invalid argument

Any modules which export numeric sysfs node can use strict_strtox instead of simple_strtox to reject any invalid input.

Here are some test results:

Before applying this patch:

After applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#

Signed-off-by Yi Yang <[email protected]>

---
 include/linux/kernel.h |    4 ++++
 kernel/params.c        |   20 ++++++++++----------
 lib/vsprintf.c         |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)

--- a/include/linux/kernel.h 2008-01-31 00:41:46.000000000 +0800
--- b/include/linux/kernel.h 2008-02-01 04:30:49.000000000 +0800
@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
extern long simple_strtol(const char *,char **,unsigned int); extern unsigned long long simple_strtoull(const char *,char **,unsigned int); extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int strict_strtoul(const char *, unsigned int, unsigned long *); +extern int strict_strtol(const char *, unsigned int, long *); +extern int strict_strtoull(const char *, unsigned int, unsigned long long *); +extern int strict_strtoll(const char *, unsigned int, long long *);
extern int sprintf(char * buf, const char * fmt, ...)
__attribute__ ((format (printf, 2, 3)));
extern int vsprintf(char *buf, const char *, va_list)
--- a/lib/vsprintf.c 2008-01-30 08:13:03.000000000 +0800
--- b/lib/vsprintf.c 2008-02-01 04:33:27.000000000 +0800
@@ -126,6 +126,52 @@ long long simple_strtoll(const char *cp,
return simple_strtoull(cp,endp,base);
}

+#define define_strict_strtoux(type, valtype) \
+int strict_strtou##type(const char *cp, unsigned int base, valtype *res)\
+{ \
+ char *tail; \
+ valtype val; \
+ size_t len; \
+ \
+ *res = 0; \
+ len = strlen(cp); \
+ if (len == 0) \
+ return -EINVAL; \
+ \
+ val = simple_strtoul(cp, &tail, base); \
+ if ((*tail == '\0') || \
+ (len == (size_t)(tail - cp) + 1) && (*tail == '\n')) { \
+ *res = val; \
+ return 0; \
+ } \
+ \
+ return -EINVAL; \
+} \
+
+#define define_strict_strtox(type, valtype) \
+int strict_strto##type(const char *cp, unsigned int base, valtype *res) \
+{ \
+ int ret; \
+ if (*cp == '-') { \
+ ret = strict_strtou##type(cp+1, base, res); \
+ if (ret != 0) \
+ *res = -(*res); \
+ } else \
+ ret = strict_strtou##type(cp+1, base, res); \
+ \
+ return ret; \
+} \
+
+define_strict_strtoux(l, unsigned long) +define_strict_strtox(l, long)
+define_strict_strtoux(ll, unsigned long long) +define_strict_strtox(ll, long long)
+
+EXPORT_SYMBOL(strict_strtoul);
+EXPORT_SYMBOL(strict_strtol);
+EXPORT_SYMBOL(strict_strtoll); +EXPORT_SYMBOL(strict_strtoull);
+
static int skip_atoi(const char **s)
{
int i=0;
--- a/kernel/params.c 2008-01-31 00:44:44.000000000 +0800
--- b/kernel/params.c 2008-02-01 04:32:12.000000000 +0800
@@ -180,12 +180,12 @@ int parse_args(const char *name,
#define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn) \
int param_set_##name(const char *val, struct kernel_param *kp) \
{ \
- char *endp; \
tmptype l; \
+ int ret; \
\
if (!val) return -EINVAL; \
- l = strtolfn(val, &endp, 0); \
- if (endp == val || ((type)l != l)) \
+ ret = strtolfn(val, 0, &l); \
+ if (ret == -EINVAL || ((type)l != l)) \
return -EINVAL; \
*((type *)kp->arg) = l; \
return 0; \
@@ -195,13 +195,13 @@ int parse_args(const char *name,
return sprintf(buffer, format, *((type *)kp->arg)); \
}

-STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, simple_strtoul); -STANDARD_PARAM_DEF(short, short, "%hi", long, simple_strtol); -STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, simple_strtoul); -STANDARD_PARAM_DEF(int, int, "%i", long, simple_strtol); -STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, simple_strtoul); -STANDARD_PARAM_DEF(long, long, "%li", long, simple_strtol); -STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, simple_strtoul); +STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); +STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); +STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol); +STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, strict_strtoul); +STANDARD_PARAM_DEF(long, long, "%li", long, strict_strtol); +STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);

int param_set_charp(const char *val, struct kernel_param *kp) {

Andrew Morton

maintainer

2008-02-01 07:38 UTC #51 lore raw

Re: [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 2

Quoted 64 lines · On Fri, 01 Feb 2008 06:11:43 +0800 Yi Yang <[email protected]> wrote:

This patch is a resend, it changes previous name "real_" to "strict_" according to Randy Dunlap's feedback. Please consider to apply. thanks.
Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/uul, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores succesive invalid char and converts prefix part to a numeric although input is acctually invalid.
This patch tries to fix the aforementioned issues and implements strict_strtox serial functions, kernel/params.c uses them to strictly validate input, so module params will reject such values as 0x1234xxxx and returns an error:
write error: Invalid argument
Any modules which export numeric sysfs node can use strict_strtox instead of simple_strtox to reject any invalid input.
Here are some test results:
Before applying this patch:
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#
After applying this patch:
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument

Quoted 9 lines

[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak 4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak 4096
[root@yangyi-dev /]#

Oh dear. So if someone has an existing script which does

	echo 0x1000g > /sys/module/e1000/parameters/copybreak

we just broke their setup.

This is what happens when we screw up kernel interfaces: we should remain bug-for-bug compatible with earlier releases. argh, we suck :(

I'm inclined to merge it anyway - everyone already knows we suck, and not many people will be affected and they suck too.

Andrew Morton

maintainer

2008-02-01 07:45 UTC #52 lore raw

Re: [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 2

Quoted 54 lines · On Fri, 01 Feb 2008 06:11:43 +0800 Yi Yang <[email protected]> wrote:

--- a/include/linux/kernel.h 2008-01-31 00:41:46.000000000 +0800
--- b/include/linux/kernel.h 2008-02-01 04:30:49.000000000 +0800

include/linux/kernel.h +39 -2

@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int strict_strtoul(const char *, unsigned int, unsigned long *);
+extern int strict_strtol(const char *, unsigned int, long *);
+extern int strict_strtoull(const char *, unsigned int, unsigned long long *);
+extern int strict_strtoll(const char *, unsigned int, long long *);
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)
--- a/lib/vsprintf.c	2008-01-30 08:13:03.000000000 +0800
--- b/lib/vsprintf.c	2008-02-01 04:33:27.000000000 +0800

@@ -126,6 +126,52 @@ long long simple_strtoll(const char *cp,
 	return simple_strtoull(cp,endp,base);
 }
 
+#define define_strict_strtoux(type, valtype)				\
+int strict_strtou##type(const char *cp, unsigned int base, valtype *res)\
+{									\
+	char *tail;							\
+	valtype val;							\
+	size_t len;							\
+									\
+	*res = 0;							\
+	len = strlen(cp);						\
+	if (len == 0)							\
+		return -EINVAL;						\
+									\
+	val = simple_strtoul(cp, &tail, base);				\
+	if ((*tail == '\0') ||						\
+		(len == (size_t)(tail - cp) + 1) && (*tail == '\n')) {	\
+		*res = val;						\
+		return 0;						\
+	}								\
+									\
+	return -EINVAL;							\
+}									\
+
+#define define_strict_strtox(type, valtype)				\
+int strict_strto##type(const char *cp, unsigned int base, valtype *res)	\
+{									\
+	int ret;							\
+	if (*cp == '-') {						\
+		ret = strict_strtou##type(cp+1, base, res);		\
+		if (ret != 0)						\
+			*res = -(*res);					\
+	} else								\
+		ret = strict_strtou##type(cp+1, base, res);		\
+									\
+	return ret;							\
+}									\

include/linux/kernel.h long long simple_strtoll(const char *cp, · line 173

Quoted 10 lines

include/linux/kernel.h +10 -0

+
+define_strict_strtoux(l, unsigned long)
+define_strict_strtox(l, long)
+define_strict_strtoux(ll, unsigned long long)
+define_strict_strtox(ll, long long)
+
+EXPORT_SYMBOL(strict_strtoul);
+EXPORT_SYMBOL(strict_strtol);
+EXPORT_SYMBOL(strict_strtoll);
+EXPORT_SYMBOL(strict_strtoull);

These are global, exported-to-modules library functions. Documentation is compulsory (as far as I'm concerned).

Could you please add a comment block in there which at least explains how they differ from the other strto* functions?

Andrew Morton

maintainer

2008-02-01 07:47 UTC #53 lore raw

Re: [PATCH 2.6.24] Add new string functions strict_strto* and convert kernel params to use them, take 2

Quoted 2 lines · On Fri, 01 Feb 2008 06:11:43 +0800 Yi Yang <[email protected]> wrote:

--- a/include/linux/kernel.h 2008-01-31 00:41:46.000000000 +0800
--- b/include/linux/kernel.h 2008-02-01 04:30:49.000000000 +0800

This isn't a patch. I wonder how that happened?

Yi Yang

author

2008-02-01 09:16 UTC #54 lore raw

PATCH 1/1 Add new string functions strict_strto* and convert kernel params to use them, take 3

Andrew, i'm really very very sorry for those mistakes, here is the latest, it adds documentation for every new function, please use it to replace the patch in -mm tree you added just now:

add-new-string-functions-strict_strto-and-convert-kernel-params-to-use-them.patch

Thank you, Andrew and Randy.

Currently, for every sysfs node, the callers will be responsible for implementing store operation, so many many callers are doing duplicate things to validate input, they have the same mistakes because they are calling simple_strtol/ul/ll/uul, especially for module params, they are just numeric, but you can echo such values as 0x1234xxx, 07777888 and 1234aaa, for these cases, module params store operation just ignores succesive invalid char and converts prefix part to a numeric although input is acctually invalid.

write error: Invalid argument

Any modules which export numeric sysfs node can use strict_strtox instead of simple_strtox to reject any invalid input. Please consider to merge to -mm tree in order to test.

Here are some test results:

Before applying this patch:

After applying this patch:

[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000g > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo 0x1000gggggggg > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# echo 0100008 > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# echo 010000aaaaa > /sys/module/e1000/parameters/copybreak -bash: echo: write error: Invalid argument
[root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]# echo -n 4096 > /sys/module/e1000/parameters/copybreak [root@yangyi-dev /]# cat /sys/module/e1000/parameters/copybreak
4096
[root@yangyi-dev /]#

Signed-off-by Yi Yang <[email protected]>

---
 include/linux/kernel.h |    4 +
 kernel/params.c        |   20 +++----
 lib/vsprintf.c         |  123 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 10 deletions(-)

include/linux/kernel.h +4 -0

@@ -141,6 +141,10 @@ extern unsigned long simple_strtoul(cons
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
+extern int strict_strtoul(const char *, unsigned int, unsigned long *);
+extern int strict_strtol(const char *, unsigned int, long *);
+extern int strict_strtoull(const char *, unsigned int, unsigned long long *);
+extern int strict_strtoll(const char *, unsigned int, long long *);
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)

lib/vsprintf.c +123 -0

129 lines not on this page · raw message

kernel/params.c +10 ~~-10~~

@@ -180,12 +180,12 @@ int parse_args(const char *name,
 #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
 	int param_set_##name(const char *val, struct kernel_param *kp)	\
 	{								\
-		char *endp;						\
 		tmptype l;						\
+		int ret;						\
 									\
 		if (!val) return -EINVAL;				\
-		l = strtolfn(val, &endp, 0);				\
-		if (endp == val || ((type)l != l))			\
+		ret = strtolfn(val, 0, &l);				\
+		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\

@@ -195,13 +195,13 @@ int parse_args(const char *name,
 		return sprintf(buffer, format, *((type *)kp->arg));	\
 	}
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, simple_strtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, simple_strtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, simple_strtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, simple_strtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, simple_strtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, strict_strtoul);
+STANDARD_PARAM_DEF(long, long, "%li", long, strict_strtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {

dean gaudet

2008-02-01 16:31 UTC #55 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

why do we need another kernel cpuid reading method when sched_setaffinity exists and cpuid is available in ring3?

-dean

H. Peter Anvin

maintainer

2008-02-01 17:57 UTC #56 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 1 line · dean gaudet

why do we need another kernel cpuid reading method when sched_setaffinity exists and cpuid is available in ring3?

Because /dev/cpu/*/cpuid:

a) predates sched_setaffinity by quite a few years
b) is already widely used
c) doesn't have issues with relative priorities (using sched_setaffinity can easily wait forever.)
d) can be accessed by unprivileged processes given appropriate device info.

-hpa

H. Peter Anvin

maintainer

2008-02-01 17:59 UTC #57 lore raw

Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

Quoted 12 lines · H. Peter Anvin

Quoted 1 line · dean gaudet
why do we need another kernel cpuid reading method when sched_setaffinity exists and cpuid is available in ring3?
Because /dev/cpu/*/cpuid:
a) predates sched_setaffinity by quite a few years
b) is already widely used
c) doesn't have issues with relative priorities (using sched_setaffinity can easily wait forever.)
d) can be accessed by unprivileged processes given appropriate device info.

(a) and (b) obviously doesn't apply to the proposed sysfs hack; nor does it give the administrator the control given by (d). The sysfs hack is pointless.

-hpa

Yi Yang

author

2008-02-14 23:44 UTC #58 lore raw

PATCH 1/1 cpufreq: fix cpufreq policy refcount imbalance

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 <[email protected]>

---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/cpufreq/cpufreq.c +5 -0

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

Yi Yang

author

2008-02-14 23:44 UTC #59 lore raw

PATCH 1/1 cpufreq: fix cpufreq policy refcount imbalance

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.

Signed-off-by Yi Yang <[email protected]>

---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/cpufreq/cpufreq.c +5 -0

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

Yi Yang

author

2008-02-14 23:48 UTC #60 lore raw

PATCH 1/1 cpufreq: fix cpufreq policy refcount imbalance

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.

Signed-off-by Yi Yang <[email protected]>

---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/cpufreq/cpufreq.c +5 -0

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

Alan Stern

2008-02-15 15:52 UTC #61 lore raw

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

Quoted 5 lines · Yi Yang

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

Greg KH

2008-02-15 18:24 UTC #62 lore raw

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

Quoted 23 lines · Alan Stern

Quoted 5 lines · Yi Yang
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.

Quoted 8 lines

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

Greg KH

2008-02-15 18:24 UTC #63 lore raw

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

Quoted 23 lines · Alan Stern

Quoted 5 lines · Yi Yang
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.

Quoted 8 lines

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

Alan Stern

2008-02-15 15:52 UTC #64 lore raw

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

Quoted 5 lines · Yi Yang

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

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

Alan Stern

Greg KH

2008-02-15 21:01 UTC #65 lore raw

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

Quoted 15 lines · 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.

Quoted 8 lines

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 <[email protected]>

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

Greg KH

2008-02-15 21:01 UTC #66 lore raw

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

Quoted 15 lines · 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.

Quoted 8 lines

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 <[email protected]>

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

Yi Yang

author

2008-02-25 00:46 UTC #67 lore raw

PATCH 1/1 cpuidle: fix cpuidle time and usage overflow

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 <[email protected]>

---
 drivers/cpuidle/cpuidle.c |    2 +-
 drivers/cpuidle/sysfs.c   |   10 ++++++++--
 include/linux/cpuidle.h   |    4 ++--

include/linux/cpuidle.h +2 -2

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

drivers/cpuidle/cpuidle.c +1 -1

@@ -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 */

drivers/cpuidle/sysfs.c +8 -2

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

Ingo Molnar

2008-02-25 10:15 UTC #68 lore raw

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

Quoted 6 lines · 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.

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

Ingo

Yi Yang

author

2008-02-25 01:10 UTC #69 lore raw

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

Quoted 11 lines · Ingo Molnar

Quoted 6 lines · 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.
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.

Quoted 6 lines

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

Yi Yang

author

2008-02-25 01:10 UTC #70 lore raw

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

In reply to Ingo Molnar

Quoted 11 lines · Ingo Molnar

Quoted 6 lines · 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.
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.

Quoted 6 lines

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

Ingo Molnar

2008-02-25 10:15 UTC #71 lore raw

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

Quoted 6 lines · 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.

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

Ingo

Len Brown

reviewer Applied

2008-03-26 04:46 UTC #72 lore raw

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

applied

thanks,
-len

Quoted 71 lines · Yi Yang

This patch will convert them to unsigned long long.

Signed-off-by Yi Yang <[email protected]>

---
 drivers/cpuidle/cpuidle.c |    2 +-
 drivers/cpuidle/sysfs.c   |   10 ++++++++--
 include/linux/cpuidle.h   |    4 ++--

include/linux/cpuidle.h +2 -2

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

drivers/cpuidle/cpuidle.c +1 -1

@@ -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 */

drivers/cpuidle/sysfs.c +8 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yi Yang

author

2008-02-25 00:46 UTC #73 lore raw

PATCH 1/1 cpuidle: fix cpuidle time and usage overflow

This patch will convert them to unsigned long long.

Signed-off-by Yi Yang <[email protected]>

---
 drivers/cpuidle/cpuidle.c |    2 +-
 drivers/cpuidle/sysfs.c   |   10 ++++++++--
 include/linux/cpuidle.h   |    4 ++--

include/linux/cpuidle.h +2 -2

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

drivers/cpuidle/cpuidle.c +1 -1

@@ -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 */

drivers/cpuidle/sysfs.c +8 -2

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

Yi Yang

author

2008-02-14 23:48 UTC #74 lore raw

PATCH 1/1 cpufreq: fix cpufreq policy refcount imbalance

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.

Signed-off-by Yi Yang <[email protected]>

---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/cpufreq/cpufreq.c +5 -0

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

Yi Yang

author

2008-01-22 23:05 UTC #75 lore raw

PATCH 1/1 cpufreq: fix obvious condition statement error