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

This is a discussion on [linux-pm][PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device - Kernel ; 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". ...

+ Reply to Thread
Page 1 of 3 1 2 3 LastLast
Results 1 to 20 of 51

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

  1. [linux-pm][PATCH] base: Change power/wakeup output from "" to "unsupported" if wakeup feature isn't supported by a device

    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.

    Signed-off-by: Yi Yang
    ---
    sysfs.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

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

    static ssize_t


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

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

    Hi!

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


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

    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

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

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

    On Fri, 4 Jan 2008, David Brownell wrote:

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

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

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


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

    Alan Stern

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

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

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


    How about not changing a userland-visible interface gratuitously?

    OG.

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

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

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

    >
    > How about not changing a userland-visible interface gratuitously?


    Very good idea.

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

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

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

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

    At the beginning, i did that as you said, but it can't work, some
    power/wakeup will disappear although they can be disabled or enabled, so
    it is only one feasible way to make it exist permanently.
    >


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

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

    On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
    > > > This patch changes empty output to "unsupported" in order that a user knows
    > > > wakeup feature isn't supported by this device when he/she
    > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
    > > > thanks.

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

    >
    > It may not *stay* unsupported, so I think changing it in either
    > of those permanent ways would be confusing/misleading.
    >
    > For example, USB devices have multiple states ... minimally, an
    > unconfigured state, and a configured state. Some have multiple
    > configurations. Only configured states can be wakeup-capable.
    > So a given device might have three states, but support wakeup
    > except in one of them...

    If so, we can change "unsupported" to "unconfigurable" or "inoperable"
    which can cover the states "unconfigured", "unsupported" and other
    unknown states. :-).
    >
    > - Dave
    >


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

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

    On Fri, 2008-01-04 at 11:38 -0500, Alan Stern wrote:
    > On Fri, 4 Jan 2008, David Brownell wrote:
    >
    > > > > This patch changes empty output to "unsupported" in order that a user knows
    > > > > wakeup feature isn't supported by this device when he/she
    > > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
    > > > > thanks.
    > > >
    > > > What about simply removing "wakuep" file if wakeup is not supported?

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

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

    This should be an option.
    >
    > Alan Stern
    >


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

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

    On Monday, 7 of January 2008, Yi Yang wrote:
    > On Fri, 2008-01-04 at 08:09 -0800, David Brownell wrote:
    > > > > This patch changes empty output to "unsupported" in order that a user knows
    > > > > wakeup feature isn't supported by this device when he/she
    > > > > 'cat /sys/devices/.../power/wakeup', please consider to apply,
    > > > > thanks.
    > > >
    > > > What about simply removing "wakuep" file if wakeup is not supported?

    > >
    > > It may not *stay* unsupported, so I think changing it in either
    > > of those permanent ways would be confusing/misleading.
    > >
    > > For example, USB devices have multiple states ... minimally, an
    > > unconfigured state, and a configured state. Some have multiple
    > > configurations. Only configured states can be wakeup-capable.
    > > So a given device might have three states, but support wakeup
    > > except in one of them...

    > If so, we can change "unsupported" to "unconfigurable" or "inoperable"
    > which can cover the states "unconfigured", "unsupported" and other
    > unknown states. :-).


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

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

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

    On Fri, 2008-01-04 at 08:31 -0800, David Brownell wrote:
    > > This patch changes empty output to "unsupported" in order that a user knows
    > > wakeup feature isn't supported by this device when he/she
    > > 'cat /sys/devices/.../power/wakeup', please consider to apply, thanks.

    >
    > I don't much like this patch. Not just for the technical reasons
    > mentioned in my previous note ... but also because it doesn't update
    > the documention at the top, which clearly states that "\n" is
    > returned for "temporary or permanent inability to issue wakeup".
    > And then it gives the example I gave before ...
    >
    > Though I suppose a patch that changes the *entire* userspace interface,
    > (which includes its documentation, and all out-of-tree users) would have
    > shown more clearly why it wasn't a good idea.

    Really, "\n" should be changed to show that change.
    Anyway, "\n" isn't a good indicator for that state. :-)
    >
    >
    > > --- a/drivers/base/power/sysfs.c 2008-01-04 16:50:54.000000000 +0800
    > > +++ b/drivers/base/power/sysfs.c 2008-01-04 17:14:42.000000000 +0800
    > > @@ -49,7 +49,7 @@ wake_show(struct device * dev, struct de
    > > {
    > > return sprintf(buf, "%s\n", device_can_wakeup(dev)
    > > ? (device_may_wakeup(dev) ? enabled : disabled)
    > > - : "");
    > > + : "unsupported");
    > > }
    > >
    > > static ssize_t
    > >
    > >


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

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

    On Fri, 2008-01-04 at 18:20 +0100, Oliver Neukum wrote:
    > Am Freitag, 4. Januar 2008 17:52:14 schrieb Olivier Galibert:
    > > On Fri, Jan 04, 2008 at 11:38:29AM -0500, Alan Stern wrote:
    > > > How about changing it to say "unavailable"? That doesn't imply
    > > > permanence.

    > >
    > > How about not changing a userland-visible interface gratuitously?

    >

    Why not do if we can make it better?
    > Very good idea.
    >
    > Regards
    > Oliver


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

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

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

    >
    > How about not changing a userland-visible interface gratuitously?

    "empty" can't tell a user the state of wakeup of a device, so it is
    necessary to change it.
    >
    > OG.
    >


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

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

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

    > > If so, we can change "unsupported" to "unconfigurable" or "inoperable"
    > > which can cover the states "unconfigured", "unsupported" and other
    > > unknown states. :-).

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

    Which applications are using this interface? if they assume "\n" means
    the unconfigurable or unavailable state, this is very fragile.
    >
    > Thanks,
    > Rafael


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

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

    On Sunday 06 January 2008, Yi Yang wrote:
    >
    > > How about not changing a userland-visible interface gratuitously?

    >
    > "empty" can't tell a user the state of wakeup of a device, so it is
    > necessary to change it.


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

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


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

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

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

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

    > Which applications are using this interface? if they assume "\n" means
    > the unconfigurable or unavailable state, this is very fragile.


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

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

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

  17. [PATCH 2.6.24-rc8] 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
    ---
    cpufreq.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    --- a/drivers/cpufreq/cpufreq.c 2008-01-23 05:02:28.000000000 +0800
    +++ b/drivers/cpufreq/cpufreq.c 2008-01-23 06:11:21.000000000 +0800
    @@ -1608,7 +1608,7 @@ static int __cpufreq_set_policy(struct c
    memcpy(&policy->cpuinfo, &data->cpuinfo,
    sizeof(struct cpufreq_cpuinfo));

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


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

  18. [PATCH 2.6.24] 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
    ---
    cpuid.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++++++ +++++++++++++-
    1 file changed, 232 insertions(+), 1 deletion(-)

    --- a/arch/x86/kernel/cpuid.c 2008-01-28 01:28:48.000000000 +0800
    +++ b/arch/x86/kernel/cpuid.c 2008-01-29 06:27:15.000000000 +0800
    @@ -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);


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

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

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

    Data is annotated _cpuintidata

    but

    > +

    Data is annotated _cpuintidata

    > @@ -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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Yi Yang wrote:
    > 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.


    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 1 of 3 1 2 3 LastLast