[patch 2.6.26-rc4-git] PM: boot time suspend selftest - Kernel

This is a discussion on [patch 2.6.26-rc4-git] PM: boot time suspend selftest - Kernel ; From: David Brownell Boot-time test for system suspend states (STR or standby). The generic RTC framework triggers wakeup alarms, which are used to exit those states. - Measures some aspects of suspend time ... this uses "jiffies" until someone converts ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 22

Thread: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

  1. [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    From: David Brownell

    Boot-time test for system suspend states (STR or standby). The generic
    RTC framework triggers wakeup alarms, which are used to exit those states.

    - Measures some aspects of suspend time ... this uses "jiffies" until
    someone converts it to use a timebase that works properly even while
    timer IRQs are disabled.

    - Triggered by a command line parameter. By default nothing even
    vaguely troublesome will happen, but "test_suspend=mem" will give
    you a brief STR test during system boot. (Or you may need to use
    "test_suspend=standby" instead, if your hardware needs that.)

    This isn't without problems. It fires early enough during boot that for
    example both PCMCIA and MMC stacks have misbehaved. The workaround in
    those cases was to boot without such media cards inserted.

    Signed-off-by: David Brownell
    ---
    I've used this on some ARM boards, and Ingo says it's been a big
    help avoiding (upstream) regressions for x86.git work. Based on
    some early work from Pavel, who wanted a pony.

    Documentation/kernel-parameters.txt | 9 +
    kernel/power/Kconfig | 11 ++
    kernel/power/main.c | 185 ++++++++++++++++++++++++++++++++++
    3 files changed, 204 insertions(+), 1 deletion(-)

    --- a/Documentation/kernel-parameters.txt 2008-05-29 13:14:07.000000000 -0700
    +++ b/Documentation/kernel-parameters.txt 2008-05-29 13:14:08.000000000 -0700
    @@ -87,7 +87,8 @@ parameter is applicable:
    SH SuperH architecture is enabled.
    SMP The kernel is an SMP kernel.
    SPARC Sparc architecture is enabled.
    - SWSUSP Software suspend is enabled.
    + SWSUSP Software suspend (hibernation) is enabled.
    + SUSPEND System suspend states are enabled.
    TS Appropriate touchscreen support is enabled.
    USB USB support is enabled.
    USBHID USB Human Interface Device support is enabled.
    @@ -2035,6 +2036,12 @@ and is between 256 and 4096 characters.

    tdfx= [HW,DRM]

    + test_suspend= [SUSPEND]
    + Specify "mem" (for Suspend-to-RAM) or "standby" (for
    + standby suspend) as the system sleep state to briefly
    + enter during system startup. The system is woken from
    + this state using a wakeup-capable RTC alarm.
    +
    thash_entries= [KNL,NET]
    Set number of hash buckets for TCP connection

    --- a/kernel/power/Kconfig 2008-05-29 13:14:07.000000000 -0700
    +++ b/kernel/power/Kconfig 2008-05-29 13:14:08.000000000 -0700
    @@ -94,6 +94,17 @@ config SUSPEND
    powered and thus its contents are preserved, such as the
    suspend-to-RAM state (e.g. the ACPI S3 state).

    +config PM_TEST_SUSPEND
    + bool "Test suspend/resume and wakealarm during bootup"
    + depends on SUSPEND && PM_DEBUG && RTC_LIB=y
    + ---help---
    + This option will let you suspend your machine during bootup, and
    + make it wake up a few seconds later using an RTC wakeup alarm.
    + Enable this with a kernel parameter like "test_suspend=mem".
    +
    + You probably want to have your system's RTC driver statically
    + linked, ensuring that it's available when this test runs.
    +
    config SUSPEND_FREEZER
    bool "Enable freezer for suspend to RAM/standby" \
    if ARCH_WANTS_FREEZER_CONTROL || BROKEN
    --- a/kernel/power/main.c 2008-05-29 13:14:07.000000000 -0700
    +++ b/kernel/power/main.c 2008-05-29 13:14:08.000000000 -0700
    @@ -132,6 +132,52 @@ static inline int suspend_test(int level

    #ifdef CONFIG_SUSPEND

    +#ifdef CONFIG_PM_TEST_SUSPEND
    +
    +/*
    + * We test the system suspend code by setting an RTC wakealarm a short
    + * time in the future, then suspending. Suspending the devices won't
    + * normally take long ... some systems only need a few milliseconds.
    + *
    + * The time it takes is system-specific though, so when we test this
    + * during system bootup we allow a LOT of time.
    + */
    +#define TEST_SUSPEND_SECONDS 5
    +
    +static unsigned long suspend_test_start_time;
    +
    +static void suspend_test_start(void)
    +{
    + /* FIXME Use better timebase than "jiffies", ideally a clocksource.
    + * What we want is a hardware counter that will work correctly even
    + * during the irqs-are-off stages of the suspend/resume cycle...
    + */
    + suspend_test_start_time = jiffies;
    +}
    +
    +static void suspend_test_finish(const char *label)
    +{
    + long nj = jiffies - suspend_test_start_time;
    + unsigned msec;
    +
    + msec = jiffies_to_msecs((nj >= 0) ? nj : -nj);
    + pr_info("PM: %s took %d.%03d seconds\n", label,
    + msec / 1000, msec % 1000);
    + WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000));
    +}
    +
    +#else
    +
    +static void suspend_test_start(void)
    +{
    +}
    +
    +static void suspend_test_finish(const char *label)
    +{
    +}
    +
    +#endif
    +
    /* This is just an arbitrary number */
    #define FREE_PAGE_NUMBER (100)

    @@ -264,11 +310,14 @@ int suspend_devices_and_enter(suspend_st
    goto Close;
    }
    suspend_console();
    +
    + suspend_test_start();
    error = device_suspend(PMSG_SUSPEND);
    if (error) {
    printk(KERN_ERR "PM: Some devices failed to suspend\n");
    goto Resume_console;
    }
    + suspend_test_finish("suspend devices");

    if (suspend_test(TEST_DEVICES))
    goto Resume_devices;
    @@ -291,7 +340,9 @@ int suspend_devices_and_enter(suspend_st
    if (suspend_ops->finish)
    suspend_ops->finish();
    Resume_devices:
    + suspend_test_start();
    device_resume();
    + suspend_test_finish("resume devices");
    Resume_console:
    resume_console();
    Close:
    @@ -515,3 +566,137 @@ static int __init pm_init(void)
    }

    core_initcall(pm_init);
    +
    +
    +#ifdef CONFIG_PM_TEST_SUSPEND
    +
    +#include
    +
    +/*
    + * To test system suspend, we need a hands-off mechanism to resume the
    + * system. RTCs wake alarms are a common self-contained mechanism.
    + */
    +
    +static void __init test_wakealarm(struct rtc_device *rtc, suspend_state_t state)
    +{
    + static char err_readtime [] __initdata =
    + KERN_ERR "PM: can't read %s time, err %d\n";
    + static char err_wakealarm [] __initdata =
    + KERN_ERR "PM: can't set %s wakealarm, err %d\n";
    + static char err_suspend [] __initdata =
    + KERN_ERR "PM: suspend test failed, error %d\n";
    + static char info_test [] __initdata =
    + KERN_INFO "PM: test RTC wakeup from '%s' suspend\n";
    +
    + unsigned long now;
    + struct rtc_wkalrm alm;
    + int status;
    +
    + /* this may fail if the RTC hasn't been initialized */
    + status = rtc_read_time(rtc, &alm.time);
    + if (status < 0) {
    + printk(err_readtime, rtc->dev.bus_id, status);
    + return;
    + }
    + rtc_tm_to_time(&alm.time, &now);
    +
    + memset(&alm, 0, sizeof alm);
    + rtc_time_to_tm(now + TEST_SUSPEND_SECONDS, &alm.time);
    + alm.enabled = true;
    +
    + status = rtc_set_alarm(rtc, &alm);
    + if (status < 0) {
    + printk(err_wakealarm, rtc->dev.bus_id, status);
    + return;
    + }
    +
    + if (state == PM_SUSPEND_MEM) {
    + printk(info_test, pm_states[state]);
    + status = pm_suspend(state);
    + if (status == -ENODEV)
    + state = PM_SUSPEND_STANDBY;
    + }
    + if (state == PM_SUSPEND_STANDBY) {
    + printk(info_test, pm_states[state]);
    + status = pm_suspend(state);
    + }
    + if (status < 0)
    + printk(err_suspend, status);
    +}
    +
    +static int __init has_wakealarm(struct device *dev, void *name_ptr)
    +{
    + struct rtc_device *candidate = to_rtc_device(dev);
    +
    + if (!candidate->ops->set_alarm)
    + return 0;
    + if (!device_may_wakeup(candidate->dev.parent))
    + return 0;
    +
    + *(char **)name_ptr = dev->bus_id;
    + return 1;
    +}
    +
    +/*
    + * Kernel options like "test_suspend=mem" force suspend/resume sanity tests
    + * at startup time. They're normally disabled, for faster boot and because
    + * we can't know which states really work on this particular system.
    + */
    +static suspend_state_t test_state __initdata = PM_SUSPEND_ON;
    +
    +static char warn_bad_state[] __initdata =
    + KERN_WARNING "PM: can't test '%s' suspend state\n";
    +
    +static int __init setup_test_suspend(char *value)
    +{
    + unsigned i;
    +
    + /* "=mem" ==> "mem" */
    + value++;
    + for (i = 0; i < PM_SUSPEND_MAX; i++) {
    + if (!pm_states[i])
    + continue;
    + if (strcmp(pm_states[i], value) != 0)
    + continue;
    + test_state = (__force suspend_state_t) i;
    + return 0;
    + }
    + printk(warn_bad_state, value);
    + return 0;
    +}
    +__setup("test_suspend", setup_test_suspend);
    +
    +static int __init test_suspend(void)
    +{
    + static char warn_no_rtc[] __initdata =
    + KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
    +
    + char *pony = NULL;
    + struct rtc_device *rtc = NULL;
    +
    + /* PM is initialized by now; is that state testable? */
    + if (test_state == PM_SUSPEND_ON)
    + goto done;
    + if (!valid_state(test_state)) {
    + printk(warn_bad_state, pm_states[test_state]);
    + goto done;
    + }
    +
    + /* RTCs have initialized by now too ... can we use one? */
    + class_find_device(rtc_class, &pony, has_wakealarm);
    + if (pony)
    + rtc = rtc_class_open(pony);
    + if (!rtc) {
    + printk(warn_no_rtc);
    + goto done;
    + }
    +
    + /* go for it */
    + test_wakealarm(rtc, test_state);
    + rtc_class_close(rtc);
    +done:
    + return 0;
    +}
    +late_initcall(test_suspend);
    +
    +#endif /* CONFIG_PM_TEST_SUSPEND */
    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thursday, 29 of May 2008, David Brownell wrote:
    > From: David Brownell
    >
    > Boot-time test for system suspend states (STR or standby). The generic
    > RTC framework triggers wakeup alarms, which are used to exit those states.
    >
    > - Measures some aspects of suspend time ... this uses "jiffies" until
    > someone converts it to use a timebase that works properly even while
    > timer IRQs are disabled.
    >
    > - Triggered by a command line parameter. By default nothing even
    > vaguely troublesome will happen, but "test_suspend=mem" will give
    > you a brief STR test during system boot. (Or you may need to use
    > "test_suspend=standby" instead, if your hardware needs that.)
    >
    > This isn't without problems. It fires early enough during boot that for
    > example both PCMCIA and MMC stacks have misbehaved. The workaround in
    > those cases was to boot without such media cards inserted.


    Two questions:

    1) How is it related to the analogous patch in the Ingo's tree?
    2) Does it apply to the current linux-next?

    Thanks,
    Rafael


    > Signed-off-by: David Brownell
    > ---
    > I've used this on some ARM boards, and Ingo says it's been a big
    > help avoiding (upstream) regressions for x86.git work. Based on
    > some early work from Pavel, who wanted a pony.
    >
    > Documentation/kernel-parameters.txt | 9 +
    > kernel/power/Kconfig | 11 ++
    > kernel/power/main.c | 185 ++++++++++++++++++++++++++++++++++
    > 3 files changed, 204 insertions(+), 1 deletion(-)
    >
    > --- a/Documentation/kernel-parameters.txt 2008-05-29 13:14:07.000000000 -0700
    > +++ b/Documentation/kernel-parameters.txt 2008-05-29 13:14:08.000000000 -0700
    > @@ -87,7 +87,8 @@ parameter is applicable:
    > SH SuperH architecture is enabled.
    > SMP The kernel is an SMP kernel.
    > SPARC Sparc architecture is enabled.
    > - SWSUSP Software suspend is enabled.
    > + SWSUSP Software suspend (hibernation) is enabled.
    > + SUSPEND System suspend states are enabled.
    > TS Appropriate touchscreen support is enabled.
    > USB USB support is enabled.
    > USBHID USB Human Interface Device support is enabled.
    > @@ -2035,6 +2036,12 @@ and is between 256 and 4096 characters.
    >
    > tdfx= [HW,DRM]
    >
    > + test_suspend= [SUSPEND]
    > + Specify "mem" (for Suspend-to-RAM) or "standby" (for
    > + standby suspend) as the system sleep state to briefly
    > + enter during system startup. The system is woken from
    > + this state using a wakeup-capable RTC alarm.
    > +
    > thash_entries= [KNL,NET]
    > Set number of hash buckets for TCP connection
    >
    > --- a/kernel/power/Kconfig 2008-05-29 13:14:07.000000000 -0700
    > +++ b/kernel/power/Kconfig 2008-05-29 13:14:08.000000000 -0700
    > @@ -94,6 +94,17 @@ config SUSPEND
    > powered and thus its contents are preserved, such as the
    > suspend-to-RAM state (e.g. the ACPI S3 state).
    >
    > +config PM_TEST_SUSPEND
    > + bool "Test suspend/resume and wakealarm during bootup"
    > + depends on SUSPEND && PM_DEBUG && RTC_LIB=y
    > + ---help---
    > + This option will let you suspend your machine during bootup, and
    > + make it wake up a few seconds later using an RTC wakeup alarm.
    > + Enable this with a kernel parameter like "test_suspend=mem".
    > +
    > + You probably want to have your system's RTC driver statically
    > + linked, ensuring that it's available when this test runs.
    > +
    > config SUSPEND_FREEZER
    > bool "Enable freezer for suspend to RAM/standby" \
    > if ARCH_WANTS_FREEZER_CONTROL || BROKEN
    > --- a/kernel/power/main.c 2008-05-29 13:14:07.000000000 -0700
    > +++ b/kernel/power/main.c 2008-05-29 13:14:08.000000000 -0700
    > @@ -132,6 +132,52 @@ static inline int suspend_test(int level
    >
    > #ifdef CONFIG_SUSPEND
    >
    > +#ifdef CONFIG_PM_TEST_SUSPEND
    > +
    > +/*
    > + * We test the system suspend code by setting an RTC wakealarm a short
    > + * time in the future, then suspending. Suspending the devices won't
    > + * normally take long ... some systems only need a few milliseconds.
    > + *
    > + * The time it takes is system-specific though, so when we test this
    > + * during system bootup we allow a LOT of time.
    > + */
    > +#define TEST_SUSPEND_SECONDS 5
    > +
    > +static unsigned long suspend_test_start_time;
    > +
    > +static void suspend_test_start(void)
    > +{
    > + /* FIXME Use better timebase than "jiffies", ideally a clocksource.
    > + * What we want is a hardware counter that will work correctly even
    > + * during the irqs-are-off stages of the suspend/resume cycle...
    > + */
    > + suspend_test_start_time = jiffies;
    > +}
    > +
    > +static void suspend_test_finish(const char *label)
    > +{
    > + long nj = jiffies - suspend_test_start_time;
    > + unsigned msec;
    > +
    > + msec = jiffies_to_msecs((nj >= 0) ? nj : -nj);
    > + pr_info("PM: %s took %d.%03d seconds\n", label,
    > + msec / 1000, msec % 1000);
    > + WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000));
    > +}
    > +
    > +#else
    > +
    > +static void suspend_test_start(void)
    > +{
    > +}
    > +
    > +static void suspend_test_finish(const char *label)
    > +{
    > +}
    > +
    > +#endif
    > +
    > /* This is just an arbitrary number */
    > #define FREE_PAGE_NUMBER (100)
    >
    > @@ -264,11 +310,14 @@ int suspend_devices_and_enter(suspend_st
    > goto Close;
    > }
    > suspend_console();
    > +
    > + suspend_test_start();
    > error = device_suspend(PMSG_SUSPEND);
    > if (error) {
    > printk(KERN_ERR "PM: Some devices failed to suspend\n");
    > goto Resume_console;
    > }
    > + suspend_test_finish("suspend devices");
    >
    > if (suspend_test(TEST_DEVICES))
    > goto Resume_devices;
    > @@ -291,7 +340,9 @@ int suspend_devices_and_enter(suspend_st
    > if (suspend_ops->finish)
    > suspend_ops->finish();
    > Resume_devices:
    > + suspend_test_start();
    > device_resume();
    > + suspend_test_finish("resume devices");
    > Resume_console:
    > resume_console();
    > Close:
    > @@ -515,3 +566,137 @@ static int __init pm_init(void)
    > }
    >
    > core_initcall(pm_init);
    > +
    > +
    > +#ifdef CONFIG_PM_TEST_SUSPEND
    > +
    > +#include
    > +
    > +/*
    > + * To test system suspend, we need a hands-off mechanism to resume the
    > + * system. RTCs wake alarms are a common self-contained mechanism.
    > + */
    > +
    > +static void __init test_wakealarm(struct rtc_device *rtc, suspend_state_t state)
    > +{
    > + static char err_readtime [] __initdata =
    > + KERN_ERR "PM: can't read %s time, err %d\n";
    > + static char err_wakealarm [] __initdata =
    > + KERN_ERR "PM: can't set %s wakealarm, err %d\n";
    > + static char err_suspend [] __initdata =
    > + KERN_ERR "PM: suspend test failed, error %d\n";
    > + static char info_test [] __initdata =
    > + KERN_INFO "PM: test RTC wakeup from '%s' suspend\n";
    > +
    > + unsigned long now;
    > + struct rtc_wkalrm alm;
    > + int status;
    > +
    > + /* this may fail if the RTC hasn't been initialized */
    > + status = rtc_read_time(rtc, &alm.time);
    > + if (status < 0) {
    > + printk(err_readtime, rtc->dev.bus_id, status);
    > + return;
    > + }
    > + rtc_tm_to_time(&alm.time, &now);
    > +
    > + memset(&alm, 0, sizeof alm);
    > + rtc_time_to_tm(now + TEST_SUSPEND_SECONDS, &alm.time);
    > + alm.enabled = true;
    > +
    > + status = rtc_set_alarm(rtc, &alm);
    > + if (status < 0) {
    > + printk(err_wakealarm, rtc->dev.bus_id, status);
    > + return;
    > + }
    > +
    > + if (state == PM_SUSPEND_MEM) {
    > + printk(info_test, pm_states[state]);
    > + status = pm_suspend(state);
    > + if (status == -ENODEV)
    > + state = PM_SUSPEND_STANDBY;
    > + }
    > + if (state == PM_SUSPEND_STANDBY) {
    > + printk(info_test, pm_states[state]);
    > + status = pm_suspend(state);
    > + }
    > + if (status < 0)
    > + printk(err_suspend, status);
    > +}
    > +
    > +static int __init has_wakealarm(struct device *dev, void *name_ptr)
    > +{
    > + struct rtc_device *candidate = to_rtc_device(dev);
    > +
    > + if (!candidate->ops->set_alarm)
    > + return 0;
    > + if (!device_may_wakeup(candidate->dev.parent))
    > + return 0;
    > +
    > + *(char **)name_ptr = dev->bus_id;
    > + return 1;
    > +}
    > +
    > +/*
    > + * Kernel options like "test_suspend=mem" force suspend/resume sanity tests
    > + * at startup time. They're normally disabled, for faster boot and because
    > + * we can't know which states really work on this particular system.
    > + */
    > +static suspend_state_t test_state __initdata = PM_SUSPEND_ON;
    > +
    > +static char warn_bad_state[] __initdata =
    > + KERN_WARNING "PM: can't test '%s' suspend state\n";
    > +
    > +static int __init setup_test_suspend(char *value)
    > +{
    > + unsigned i;
    > +
    > + /* "=mem" ==> "mem" */
    > + value++;
    > + for (i = 0; i < PM_SUSPEND_MAX; i++) {
    > + if (!pm_states[i])
    > + continue;
    > + if (strcmp(pm_states[i], value) != 0)
    > + continue;
    > + test_state = (__force suspend_state_t) i;
    > + return 0;
    > + }
    > + printk(warn_bad_state, value);
    > + return 0;
    > +}
    > +__setup("test_suspend", setup_test_suspend);
    > +
    > +static int __init test_suspend(void)
    > +{
    > + static char warn_no_rtc[] __initdata =
    > + KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
    > +
    > + char *pony = NULL;
    > + struct rtc_device *rtc = NULL;
    > +
    > + /* PM is initialized by now; is that state testable? */
    > + if (test_state == PM_SUSPEND_ON)
    > + goto done;
    > + if (!valid_state(test_state)) {
    > + printk(warn_bad_state, pm_states[test_state]);
    > + goto done;
    > + }
    > +
    > + /* RTCs have initialized by now too ... can we use one? */
    > + class_find_device(rtc_class, &pony, has_wakealarm);
    > + if (pony)
    > + rtc = rtc_class_open(pony);
    > + if (!rtc) {
    > + printk(warn_no_rtc);
    > + goto done;
    > + }
    > +
    > + /* go for it */
    > + test_wakealarm(rtc, test_state);
    > + rtc_class_close(rtc);
    > +done:
    > + return 0;
    > +}
    > +late_initcall(test_suspend);
    > +
    > +#endif /* CONFIG_PM_TEST_SUSPEND */

    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thursday, 29 of May 2008, David Brownell wrote:
    > On Thursday 29 May 2008, Rafael J. Wysocki wrote:
    > > Two questions:
    > >
    > > 1) How is it related to the analogous patch in the Ingo's tree?

    >
    > AFAIK -- just that it's an updated version. Comments, some
    > code flow, but mostly having the "test_suspend=mode" parameter
    > default to "safe -- no testing" and allows the choice of modes.
    >
    >
    > > 2) Does it apply to the current linux-next?

    >
    > No idea.


    Could you check, please? The previous version didn't.

    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/

  4. Re: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thursday 29 May 2008, Rafael J. Wysocki wrote:
    > Two questions:
    >
    > 1) How is it related to the analogous patch in the Ingo's tree?


    AFAIK -- just that it's an updated version. Comments, some
    code flow, but mostly having the "test_suspend=mode" parameter
    default to "safe -- no testing" and allows the choice of modes.


    > 2) Does it apply to the current linux-next?


    No idea.



    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thu, 29 May 2008 23:29:48 +0200
    "Rafael J. Wysocki" wrote:

    > On Thursday, 29 of May 2008, David Brownell wrote:
    > > On Thursday 29 May 2008, Rafael J. Wysocki wrote:
    > > > Two questions:
    > > >
    > > > 1) How is it related to the analogous patch in the Ingo's tree?

    > >
    > > AFAIK -- just that it's an updated version. Comments, some
    > > code flow, but mostly having the "test_suspend=mode" parameter
    > > default to "safe -- no testing" and allows the choice of modes.
    > >
    > >
    > > > 2) Does it apply to the current linux-next?

    > >
    > > No idea.

    >
    > Could you check, please? The previous version didn't.
    >


    There's just one trivial reject against the PCI tree's
    pm-introduce-new-top-level-suspend-and-hibernation-callbacks:

    ***************
    *** 293,299 ****
    if (suspend_ops->finish)
    suspend_ops->finish();
    Resume_devices:
    device_resume();
    Resume_console:
    resume_console();
    Close:
    --- 342,350 ----
    if (suspend_ops->finish)
    suspend_ops->finish();
    Resume_devices:
    + suspend_test_start();
    device_resume();
    + suspend_test_finish("resume devices");
    Resume_console:
    resume_console();
    Close:

    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thu, 29 May 2008 13:33:41 -0700
    David Brownell wrote:

    > From: David Brownell
    >
    > Boot-time test for system suspend states (STR or standby). The generic
    > RTC framework triggers wakeup alarms, which are used to exit those states.
    >
    > - Measures some aspects of suspend time ... this uses "jiffies" until
    > someone converts it to use a timebase that works properly even while
    > timer IRQs are disabled.
    >
    > - Triggered by a command line parameter. By default nothing even
    > vaguely troublesome will happen, but "test_suspend=mem" will give
    > you a brief STR test during system boot. (Or you may need to use
    > "test_suspend=standby" instead, if your hardware needs that.)
    >
    > This isn't without problems. It fires early enough during boot that for
    > example both PCMCIA and MMC stacks have misbehaved. The workaround in
    > those cases was to boot without such media cards inserted.
    >
    > ...
    >
    > +#ifdef CONFIG_PM_TEST_SUSPEND
    > +
    > +/*
    > + * We test the system suspend code by setting an RTC wakealarm a short
    > + * time in the future, then suspending. Suspending the devices won't
    > + * normally take long ... some systems only need a few milliseconds.
    > + *
    > + * The time it takes is system-specific though, so when we test this
    > + * during system bootup we allow a LOT of time.
    > + */
    > +#define TEST_SUSPEND_SECONDS 5
    > +
    > +static unsigned long suspend_test_start_time;
    > +
    > +static void suspend_test_start(void)
    > +{
    > + /* FIXME Use better timebase than "jiffies", ideally a clocksource.
    > + * What we want is a hardware counter that will work correctly even
    > + * during the irqs-are-off stages of the suspend/resume cycle...
    > + */
    > + suspend_test_start_time = jiffies;
    > +}
    > +
    > +static void suspend_test_finish(const char *label)
    > +{
    > + long nj = jiffies - suspend_test_start_time;
    > + unsigned msec;
    > +
    > + msec = jiffies_to_msecs((nj >= 0) ? nj : -nj);


    abs()

    > + pr_info("PM: %s took %d.%03d seconds\n", label,
    > + msec / 1000, msec % 1000);


    Can it really take a negative amount of time? If so, this message will
    convert that to a positive duration.

    Confused.

    > + WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000));


    We should have a comment here explaining what we're warning about. Why
    would it take more that five seconds?

    Better might be to just add a nice printk - I don't think we need the
    stack trace here.

    > +}
    > +
    > +#else
    > +
    > +static void suspend_test_start(void)
    > +{
    > +}
    > +
    > +static void suspend_test_finish(const char *label)
    > +{
    > +}
    > +
    > +#endif
    >
    > ...
    >
    > +static void __init test_wakealarm(struct rtc_device *rtc, suspend_state_t state)
    > +{
    > + static char err_readtime [] __initdata =
    > + KERN_ERR "PM: can't read %s time, err %d\n";
    > + static char err_wakealarm [] __initdata =
    > + KERN_ERR "PM: can't set %s wakealarm, err %d\n";
    > + static char err_suspend [] __initdata =
    > + KERN_ERR "PM: suspend test failed, error %d\n";
    > + static char info_test [] __initdata =
    > + KERN_INFO "PM: test RTC wakeup from '%s' suspend\n";


    - One tab before the variable space is a waste of space. Two tabs is
    just extravagant.

    - The space before the [] shouldn't be there. checkpatch misses this.

    - This way of defining printk control strings is weird, and will (I
    assume) defeat gcc printk arg checking.

    I _assume_ it was done so that the strings could be moved into
    .init.data, thus saving a few bytes at runtime?

    I wonder if that's a good tradeoff. It would be nice to teach gcc
    how to do this, but that sounds improbable.

    > + unsigned long now;
    > + struct rtc_wkalrm alm;
    > + int status;
    > +
    > + /* this may fail if the RTC hasn't been initialized */
    > + status = rtc_read_time(rtc, &alm.time);
    > + if (status < 0) {
    > + printk(err_readtime, rtc->dev.bus_id, status);
    > + return;
    > + }
    > + rtc_tm_to_time(&alm.time, &now);
    > +
    > + memset(&alm, 0, sizeof alm);
    > + rtc_time_to_tm(now + TEST_SUSPEND_SECONDS, &alm.time);
    > + alm.enabled = true;
    > +
    > + status = rtc_set_alarm(rtc, &alm);
    > + if (status < 0) {
    > + printk(err_wakealarm, rtc->dev.bus_id, status);
    > + return;
    > + }
    > +
    > + if (state == PM_SUSPEND_MEM) {
    > + printk(info_test, pm_states[state]);
    > + status = pm_suspend(state);
    > + if (status == -ENODEV)
    > + state = PM_SUSPEND_STANDBY;
    > + }
    > + if (state == PM_SUSPEND_STANDBY) {
    > + printk(info_test, pm_states[state]);
    > + status = pm_suspend(state);
    > + }
    > + if (status < 0)
    > + printk(err_suspend, status);
    > +}
    > +
    > +static int __init has_wakealarm(struct device *dev, void *name_ptr)
    > +{
    > + struct rtc_device *candidate = to_rtc_device(dev);
    > +
    > + if (!candidate->ops->set_alarm)
    > + return 0;
    > + if (!device_may_wakeup(candidate->dev.parent))
    > + return 0;
    > +
    > + *(char **)name_ptr = dev->bus_id;
    > + return 1;
    > +}
    > +
    > +/*
    > + * Kernel options like "test_suspend=mem" force suspend/resume sanity tests
    > + * at startup time. They're normally disabled, for faster boot and because
    > + * we can't know which states really work on this particular system.
    > + */
    > +static suspend_state_t test_state __initdata = PM_SUSPEND_ON;
    > +
    > +static char warn_bad_state[] __initdata =
    > + KERN_WARNING "PM: can't test '%s' suspend state\n";
    > +
    > +static int __init setup_test_suspend(char *value)
    > +{
    > + unsigned i;
    > +
    > + /* "=mem" ==> "mem" */
    > + value++;
    > + for (i = 0; i < PM_SUSPEND_MAX; i++) {
    > + if (!pm_states[i])
    > + continue;
    > + if (strcmp(pm_states[i], value) != 0)
    > + continue;
    > + test_state = (__force suspend_state_t) i;


    I don't think I ever knew what __force does, and whoever added it
    forgot to comment it.







    > + return 0;
    > + }
    > + printk(warn_bad_state, value);
    > + return 0;
    > +}
    > +__setup("test_suspend", setup_test_suspend);
    > +
    > +static int __init test_suspend(void)
    > +{
    > + static char warn_no_rtc[] __initdata =
    > + KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
    > +
    > + char *pony = NULL;


    whinny.

    > + struct rtc_device *rtc = NULL;
    > +
    > + /* PM is initialized by now; is that state testable? */
    > + if (test_state == PM_SUSPEND_ON)
    > + goto done;
    > + if (!valid_state(test_state)) {
    > + printk(warn_bad_state, pm_states[test_state]);
    > + goto done;
    > + }
    > +
    > + /* RTCs have initialized by now too ... can we use one? */
    > + class_find_device(rtc_class, &pony, has_wakealarm);
    > + if (pony)
    > + rtc = rtc_class_open(pony);
    > + if (!rtc) {
    > + printk(warn_no_rtc);
    > + goto done;
    > + }
    > +
    > + /* go for it */
    > + test_wakealarm(rtc, test_state);
    > + rtc_class_close(rtc);
    > +done:
    > + return 0;
    > +}
    > +late_initcall(test_suspend);
    > +


    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thursday 29 May 2008, Andrew Morton wrote:
    >
    > > Could you check, please? *The previous version didn't.
    > >

    >
    > There's just one trivial reject against the PCI tree's
    > pm-introduce-new-top-level-suspend-and-hibernation-callbacks:


    Thanks Andrew ... in that case I'm not going to worry
    about any "-next" issues for now.

    - Dave


    > ***************
    > *** 293,299 ****
    > * ******if (suspend_ops->finish)
    > * **************suspend_ops->finish();
    > * *Resume_devices:
    > * ******device_resume();
    > * *Resume_console:
    > * ******resume_console();
    > * *Close:
    > --- 342,350 ----
    > * ******if (suspend_ops->finish)
    > * **************suspend_ops->finish();
    > * *Resume_devices:
    > + ******suspend_test_start();
    > * ******device_resume();
    > + ******suspend_test_finish("resume devices");
    > * *Resume_console:
    > * ******resume_console();
    > * *Close:



    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest


    * Andrew Morton wrote:

    > > + WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000));

    >
    > We should have a comment here explaining what we're warning about.
    > Why would it take more that five seconds?


    i asked for that because we had regressions in the past in the form of
    "it takes one minute to resume".

    > Better might be to just add a nice printk - I don't think we need the
    > stack trace here.


    please keep the warn-on so that it can be detected automatically. Adding
    yet another printk just complicates the automated answer to the "is this
    kernel that just booted up fine or not" question.

    In fact i'd love to have the analogue to /proc/lockdep_debug's
    "debug_locks: 0" output. I.e. the kernel should know it via one central
    flag whether any bugs that need human review have been detected so far.

    Say /proc/sys/kernel/kernel_is_buggy. This value could even be
    multi-level: a WARN_ON() increases it by +1, a kernel crash increases it
    by +1000. ( That way i could run overnight tests that will only stop on
    a kernel_is_buggy >= 1000 condition, while it could ignore simpler
    WARN_ON()s. )

    Ingo
    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Fri, 30 May 2008 12:59:48 +0200 Ingo Molnar wrote:

    > * Andrew Morton wrote:
    >
    > > > + WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000));

    > >
    > > We should have a comment here explaining what we're warning about.
    > > Why would it take more that five seconds?

    >
    > i asked for that because we had regressions in the past in the form of
    > "it takes one minute to resume".


    It is not possible for a reader to learn this from either the kernel
    source nor from the changelogs.

    Hence...

    --
    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. [PATCH] add a printk_init variant storing format strings in __initdata


    [As gcc seems unable to help us out selecting the appropriate data segment
    for the code, how about we did something like this?]

    When using printk from __init functions it would be desirable to place
    the printk format strings in __initdata. Add a printk_init() variant
    which does this.

    This printk_init() is necessarily a #define so that we can declare the
    format string in static scope and mark it __initdata. We then call a
    newly introduced __printk_init() variant which is identicle to printk() but
    marked __init itself. By ensuring that an __init variant of printk is used
    we get proper section violation warnings when this is used incorrectly:

    WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
    function something() to the variable .init.data:__printk_init_fmt.31426
    The function something() references
    the variable __initdata __printk_init_fmt.31426.
    This is often because something lacks a __initdata
    annotation or the annotation of __printk_init_fmt.31426 is wrong.

    Note I have followed printk's pattern for __cold annotations.

    Signed-off-by: Andy Whitcroft
    ---
    include/linux/kernel.h | 10 ++++++++++
    kernel/printk.c | 12 ++++++++++++
    2 files changed, 22 insertions(+), 0 deletions(-)
    diff --git a/include/linux/kernel.h b/include/linux/kernel.h
    index 792bf0a..7754196 100644
    --- a/include/linux/kernel.h
    +++ b/include/linux/kernel.h
    @@ -180,6 +180,13 @@ struct pid;
    extern struct pid *session_of_pgrp(struct pid *pgrp);

    #ifdef CONFIG_PRINTK
    +#define printk_init(fmt, args...) \
    +do { \
    + static char __printk_init_fmt[] __initdata = fmt; \
    + __printk_init(__printk_init_fmt, ##args); \
    +} while (0)
    +asmlinkage int __printk_init(const char * fmt, ...)
    + __attribute__ ((format (printf, 1, 2))) __cold;
    asmlinkage int vprintk(const char *fmt, va_list args)
    __attribute__ ((format (printf, 1, 0)));
    asmlinkage int printk(const char * fmt, ...)
    @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
    extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
    unsigned int interval_msec);
    #else
    +asmlinkage int printk_init(const char * fmt, ...)
    + __attribute__ ((format (printf, 1, 2))) __cold;
    +static inline int __cold printk_init(const char *s, ...) { return 0; }
    static inline int vprintk(const char *s, va_list args)
    __attribute__ ((format (printf, 1, 0)));
    static inline int vprintk(const char *s, va_list args) { return 0; }
    diff --git a/kernel/printk.c b/kernel/printk.c
    index 8fb01c3..992a5c0 100644
    --- a/kernel/printk.c
    +++ b/kernel/printk.c
    @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
    return r;
    }

    +asmlinkage __init int __printk_init(const char *fmt, ...)
    +{
    + va_list args;
    + int r;
    +
    + va_start(args, fmt);
    + r = vprintk(fmt, args);
    + va_end(args);
    +
    + return r;
    +}
    +
    /* cpu currently holding logbuf_lock */
    static volatile unsigned int printk_cpu = UINT_MAX;

    --
    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: [patch 2.6.26-rc4-git] PM: boot time suspend selftest

    On Thu, May 29, 2008 at 04:22:57PM -0700, Andrew Morton wrote:

    > > +static void __init test_wakealarm(struct rtc_device *rtc, suspend_state_t state)
    > > +{
    > > + static char err_readtime [] __initdata =
    > > + KERN_ERR "PM: can't read %s time, err %d\n";
    > > + static char err_wakealarm [] __initdata =
    > > + KERN_ERR "PM: can't set %s wakealarm, err %d\n";
    > > + static char err_suspend [] __initdata =
    > > + KERN_ERR "PM: suspend test failed, error %d\n";
    > > + static char info_test [] __initdata =
    > > + KERN_INFO "PM: test RTC wakeup from '%s' suspend\n";

    >
    > - One tab before the variable space is a waste of space. Two tabs is
    > just extravagant.
    >
    > - The space before the [] shouldn't be there. checkpatch misses this.


    Ok, this should be picked up in the next release:

    ERROR: space prohibited before open square bracket '['
    #1: FILE: Z152.c:1:
    + static char info_test [] __initdata =

    -apw
    --
    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: [PATCH] add a printk_init variant storing format strings in __initdata

    Hi,

    Andy Whitcroft writes:

    > [As gcc seems unable to help us out selecting the appropriate data segment
    > for the code, how about we did something like this?]
    >
    > When using printk from __init functions it would be desirable to place
    > the printk format strings in __initdata. Add a printk_init() variant
    > which does this.


    http://lkml.org/lkml/2008/2/4/185

    I don't know if `desirable' is the technical argument Sam wants to hear

    Hannes
    --
    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] add a printk_init variant storing format strings in __initdata


    > +asmlinkage __init int __printk_init(const char *fmt, ...)
    > +{
    > + va_list args;
    > + int r;
    > +
    > + va_start(args, fmt);
    > + r = vprintk(fmt, args);
    > + va_end(args);
    > +
    > + return r;
    > +}
    > +


    Wouldn't you have to export that?

    johannes

    -----BEGIN PGP SIGNATURE-----
    Comment: Johannes Berg (powerbook)

    iQIVAwUASEWEK6Vg1VMiehFYAQLwgg/+KKqdxxoTlI8Sy0ah6x+zoLd2n8dscVuM
    x4SjkdsMbocL7g2+PcL0upK2+w9LjylBpl4zAviE5Q62nhMIww jkeIGeQU54Y1q6
    eBV1c5NEp6xS8+9JQYBXizkTVpkUQb0BfKne2O1+TPqLctFuhg xafvRrjjAQERW/
    2ZsBRe6Q6dO8nIZBX+PywduOXohsDIiV6cLezBTJCxlw3GHn3I IAvz7JkvcpkEd3
    kH4//Uoz66crUasmr01nR7phXub+MzM335//UBk66dFi5KteNApXXiYRYGvaQA+d
    TErBJJ1K5/Grk3RaKUsk5QVl6NbCMSt6YyaxQIgizTbPXeMoQH0rEuk+4CJz h17J
    T+jOoLGNVQaHS1SyZgI3kgCZWj+aHAgRSw8VYbWJI4xtBtIFXK KGnxDcN/saFxLH
    caimbzv6rMrXng8g6dwpDnkU8Bzy94unKeR4yEVWQ8GM+54q4P aM6Fa0dyRe41gK
    lymg//lgoB6sIWgCym0pj6fHql90gMamFMT88u7ZOcKEf82DFv/odW5ITjbkG/xU
    rj0dlTlWvU4AaqcQqcAxU2SZ2fn+y/eD4DZku5xoAhha4E6naGk7Slt4dTjpB4Ui
    is97P8yrPGopaFZGjsW98U84sRa9tRzdkwG0lSt8lFP0Q8U1SX HjtrQ5EjfM3/vu
    Alz3zy/rUlg=
    =YgC3
    -----END PGP SIGNATURE-----


  14. Re: [PATCH] add a printk_init variant storing format strings in __initdata

    On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft wrote:

    >
    > [As gcc seems unable to help us out selecting the appropriate data segment
    > for the code, how about we did something like this?]
    >
    > When using printk from __init functions it would be desirable to place
    > the printk format strings in __initdata. Add a printk_init() variant
    > which does this.
    >
    > This printk_init() is necessarily a #define so that we can declare the
    > format string in static scope and mark it __initdata. We then call a
    > newly introduced __printk_init() variant which is identicle to printk() but
    > marked __init itself. By ensuring that an __init variant of printk is used
    > we get proper section violation warnings when this is used incorrectly:
    >
    > WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
    > function something() to the variable .init.data:__printk_init_fmt.31426
    > The function something() references
    > the variable __initdata __printk_init_fmt.31426.
    > This is often because something lacks a __initdata
    > annotation or the annotation of __printk_init_fmt.31426 is wrong.
    >
    > Note I have followed printk's pattern for __cold annotations.
    >


    Ho hum. This give everyone another way in which to bury everyone else
    with patches.

    Wouldn't it be great if checkpatch were to detect
    fail-to-use-printk_init() in an __init function?

    oh, speaking of checkpatch: please use it

    > ---
    > include/linux/kernel.h | 10 ++++++++++
    > kernel/printk.c | 12 ++++++++++++
    > 2 files changed, 22 insertions(+), 0 deletions(-)
    > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
    > index 792bf0a..7754196 100644
    > --- a/include/linux/kernel.h
    > +++ b/include/linux/kernel.h
    > @@ -180,6 +180,13 @@ struct pid;
    > extern struct pid *session_of_pgrp(struct pid *pgrp);
    >
    > #ifdef CONFIG_PRINTK
    > +#define printk_init(fmt, args...) \
    > +do { \
    > + static char __printk_init_fmt[] __initdata = fmt; \
    > + __printk_init(__printk_init_fmt, ##args); \
    > +} while (0)
    > +asmlinkage int __printk_init(const char * fmt, ...)
    > + __attribute__ ((format (printf, 1, 2))) __cold;
    > asmlinkage int vprintk(const char *fmt, va_list args)
    > __attribute__ ((format (printf, 1, 0)));
    > asmlinkage int printk(const char * fmt, ...)
    > @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
    > extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
    > unsigned int interval_msec);
    > #else
    > +asmlinkage int printk_init(const char * fmt, ...)
    > + __attribute__ ((format (printf, 1, 2))) __cold;
    > +static inline int __cold printk_init(const char *s, ...) { return 0; }
    > static inline int vprintk(const char *s, va_list args)
    > __attribute__ ((format (printf, 1, 0)));
    > static inline int vprintk(const char *s, va_list args) { return 0; }
    > diff --git a/kernel/printk.c b/kernel/printk.c
    > index 8fb01c3..992a5c0 100644
    > --- a/kernel/printk.c
    > +++ b/kernel/printk.c
    > @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
    > return r;
    > }
    >
    > +asmlinkage __init int __printk_init(const char *fmt, ...)
    > +{
    > + va_list args;
    > + int r;
    > +
    > + va_start(args, fmt);
    > + r = vprintk(fmt, args);
    > + va_end(args);
    > +
    > + return r;
    > +}


    We're going to want to be able to call printk_init() from modules.
    Please fix and test that, if we decide to proceed.

    Oh, and we're going to need printk_meminit() and printk_cpuinit() and
    whatever.

    Which probably means that __printk_init() can't be __init, unless all
    the CONFIG_ settings which control __cpuinit, __meminit etc are blowing
    in the right direction.

    It would be good if we could get some idea of the savings here, because
    boy this is going to be a pain.

    --
    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: [PATCH] add a printk_init variant storing format strings in __initdata

    It is important about embedded system, too.
    So I add CC, linux-embedded.

    On Wed, Jun 4, 2008 at 5:16 PM, Andrew Morton wrote:
    > On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft wrote:
    >
    >>
    >> [As gcc seems unable to help us out selecting the appropriate data segment
    >> for the code, how about we did something like this?]
    >>
    >> When using printk from __init functions it would be desirable to place
    >> the printk format strings in __initdata. Add a printk_init() variant
    >> which does this.
    >>
    >> This printk_init() is necessarily a #define so that we can declare the
    >> format string in static scope and mark it __initdata. We then call a
    >> newly introduced __printk_init() variant which is identicle to printk() but
    >> marked __init itself. By ensuring that an __init variant of printk is used
    >> we get proper section violation warnings when this is used incorrectly:
    >>
    >> WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
    >> function something() to the variable .init.data:__printk_init_fmt.31426
    >> The function something() references
    >> the variable __initdata __printk_init_fmt.31426.
    >> This is often because something lacks a __initdata
    >> annotation or the annotation of __printk_init_fmt.31426 is wrong.
    >>
    >> Note I have followed printk's pattern for __cold annotations.
    >>

    >
    > Ho hum. This give everyone another way in which to bury everyone else
    > with patches.
    >
    > Wouldn't it be great if checkpatch were to detect
    > fail-to-use-printk_init() in an __init function?
    >
    > oh, speaking of checkpatch: please use it
    >
    >> ---
    >> include/linux/kernel.h | 10 ++++++++++
    >> kernel/printk.c | 12 ++++++++++++
    >> 2 files changed, 22 insertions(+), 0 deletions(-)
    >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
    >> index 792bf0a..7754196 100644
    >> --- a/include/linux/kernel.h
    >> +++ b/include/linux/kernel.h
    >> @@ -180,6 +180,13 @@ struct pid;
    >> extern struct pid *session_of_pgrp(struct pid *pgrp);
    >>
    >> #ifdef CONFIG_PRINTK
    >> +#define printk_init(fmt, args...) \
    >> +do { \
    >> + static char __printk_init_fmt[] __initdata = fmt; \
    >> + __printk_init(__printk_init_fmt, ##args); \
    >> +} while (0)
    >> +asmlinkage int __printk_init(const char * fmt, ...)
    >> + __attribute__ ((format (printf, 1, 2))) __cold;
    >> asmlinkage int vprintk(const char *fmt, va_list args)
    >> __attribute__ ((format (printf, 1, 0)));
    >> asmlinkage int printk(const char * fmt, ...)
    >> @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
    >> extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
    >> unsigned int interval_msec);
    >> #else
    >> +asmlinkage int printk_init(const char * fmt, ...)
    >> + __attribute__ ((format (printf, 1, 2))) __cold;
    >> +static inline int __cold printk_init(const char *s, ...) { return 0; }
    >> static inline int vprintk(const char *s, va_list args)
    >> __attribute__ ((format (printf, 1, 0)));
    >> static inline int vprintk(const char *s, va_list args) { return 0; }
    >> diff --git a/kernel/printk.c b/kernel/printk.c
    >> index 8fb01c3..992a5c0 100644
    >> --- a/kernel/printk.c
    >> +++ b/kernel/printk.c
    >> @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
    >> return r;
    >> }
    >>
    >> +asmlinkage __init int __printk_init(const char *fmt, ...)
    >> +{
    >> + va_list args;
    >> + int r;
    >> +
    >> + va_start(args, fmt);
    >> + r = vprintk(fmt, args);
    >> + va_end(args);
    >> +
    >> + return r;
    >> +}

    >
    > We're going to want to be able to call printk_init() from modules.
    > Please fix and test that, if we decide to proceed.
    >
    > Oh, and we're going to need printk_meminit() and printk_cpuinit() and
    > whatever.
    >
    > Which probably means that __printk_init() can't be __init, unless all
    > the CONFIG_ settings which control __cpuinit, __meminit etc are blowing
    > in the right direction.
    >
    > It would be good if we could get some idea of the savings here, because
    > boy this is going to be a pain.
    >
    > --
    > 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/
    >




    --
    Kinds regards,
    MinChan Kim
    --
    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: [PATCH] add a printk_init variant storing format strings in __initdata

    On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
    > We're going to want to be able to call printk_init() from modules.
    > Please fix and test that, if we decide to proceed.


    Can we fix that by making it an alias for printk in the module case?

    The only reason we need it to be __init is so that we get the section
    warnings when you use it from non-init code, right? Won't we get the
    warning when non-init code refers to the string in initdata anyway?

    --
    dwmw2

    --
    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. Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata

    On Wed, 2008-06-04 at 11:10 +0200, Johannes Berg wrote:
    > On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
    > > On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
    > > > We're going to want to be able to call printk_init() from modules.
    > > > Please fix and test that, if we decide to proceed.

    > >
    > > Can we fix that by making it an alias for printk in the module case?
    > >
    > > The only reason we need it to be __init is so that we get the section
    > > warnings when you use it from non-init code, right? Won't we get the
    > > warning when non-init code refers to the string in initdata anyway?

    >
    > In fact, wasn't the warning Andy showed such a warning?


    Hm, yes it was. Why do we need __printk_init() to be anything other than
    an alias for printk, then?

    --
    dwmw2

    --
    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. Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata

    On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
    > On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
    > > We're going to want to be able to call printk_init() from modules.
    > > Please fix and test that, if we decide to proceed.

    >
    > Can we fix that by making it an alias for printk in the module case?
    >
    > The only reason we need it to be __init is so that we get the section
    > warnings when you use it from non-init code, right? Won't we get the
    > warning when non-init code refers to the string in initdata anyway?


    In fact, wasn't the warning Andy showed such a warning?

    > WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
    > function something() to the variable .init.data:__printk_init_fmt..31426
    > The function something() references
    > the variable __initdata __printk_init_fmt.31426.
    > This is often because something lacks a __initdata
    > annotation or the annotation of __printk_init_fmt.31426 is wrong.


    johannes

    -----BEGIN PGP SIGNATURE-----
    Comment: Johannes Berg (powerbook)

    iQIVAwUASEZb/KVg1VMiehFYAQJ50g/8DUKSWg8SuR69Wx9hjM1Y7os78sPT8S3h
    4xANMMqnUdqRCIF4ZHn0FU/FY82Y4MuyHs6p8/yDIWkEylO+Yy7F+Gomf3d24qSR
    oGO1iftM79zlyS//9xgiZ+1AgQ79Mi1sQNRNDmkAFxwYaZaUpEDuKQeJn1mEXZsA
    pmbntH4uKYBep7P+dMR7xDKoeWzL/j28K5Tc/wyq8oa8q22NRm2QASRpRS8Qavjx
    VaWGhophMSg4Igtm+KXVrsgc9NBV4qPqPv+BTkN0azuHFcPDw7 8Pn9mBe3FtXBkj
    lVeSl7Oy5jPDOl4IG2TfQvwZb3QbMAFJO3v3ppOZnNUMK+sdD4 F375I0XvddiuGJ
    XUee3gW7m74/qL/fQ6zh1+RlLZfK7aMRa7KrQFmWHbDnBJbN+7Xyyz8S5AxwqKWY
    xsKtqKQCQ+CE6ZC0QQsaWpoWYs7ZGywsnRoKjnJHfCaI/yCMHIhr4Cmjf/M8y8b4
    25aTXrxYm7rJjPMSERBbwYeQ7Le4P+xYwYRFWMvvG7A9QbhJRZ 5YsOjjXqoxKb+z
    z3Rn3Cg0TKqOHvIzVbT2owaZeoh+5Ilj6VX6qjZfsql3C8x+k9 lCGp5voAet7mqH
    BNLQqvHw+EeN7Jk26KDmnicHvQqFOqLd9eJ1FTfCDIQnytLZ00 t+jFYSpfE/jk6x
    9KRDmqEwBwM=
    =Kxfh
    -----END PGP SIGNATURE-----


  19. Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata

    On Wed, 2008-06-04 at 10:17 +0100, David Woodhouse wrote:
    > On Wed, 2008-06-04 at 11:10 +0200, Johannes Berg wrote:
    > > On Wed, 2008-06-04 at 09:59 +0100, David Woodhouse wrote:
    > > > On Wed, 2008-06-04 at 01:16 -0700, Andrew Morton wrote:
    > > > > We're going to want to be able to call printk_init() from modules.
    > > > > Please fix and test that, if we decide to proceed.
    > > >
    > > > Can we fix that by making it an alias for printk in the module case?
    > > >
    > > > The only reason we need it to be __init is so that we get the section
    > > > warnings when you use it from non-init code, right? Won't we get the
    > > > warning when non-init code refers to the string in initdata anyway?

    > >
    > > In fact, wasn't the warning Andy showed such a warning?

    >
    > Hm, yes it was. Why do we need __printk_init() to be anything other than
    > an alias for printk, then?


    It looks to me like we wouldn't need it at all, just the printk_init()
    macro that puts the format string into the right section.

    johannes

    -----BEGIN PGP SIGNATURE-----
    Comment: Johannes Berg (powerbook)

    iQIVAwUASEZzm6Vg1VMiehFYAQLShQ//X4m7jDVmXmXB+kZPS9uC4/iZM9/g34yT
    v4FsJ1v+Po+Cydi3dHO7oJvD0k5Y9VZyuYIWo2EjFaxsNSCwm4 0a1ZgkQf7n3+fx
    sHmMrqEoX1UE7/1WUBuGej3zDxVduXmfdw+baGxs/5GFuiqTCcrWOTb4AJ2LasvY
    lNnEtvyIb9UW5xlUl6y+B91ze+oOk24euMaUgApxB4/S9bjRTzp1BdlpS5bN1ei6
    h2rvUdY63SOGJhgvZd2Gbf73UQAoWbE2VxO1UgGk3eBc90BhRY sJo/1kZc3yEj8G
    YA5x8BGWOropweNDBZDTxWK6iDIPGEy+Q3FVC1NpqGNGvIC5qy juWTiuIYErmuWp
    qk1aNGZgTS8Jxd0kvUPerptek2EVIjkqWtuebWMUjITX4vO8JQ e3LOvud4ubfGD8
    98mhlQruLVTjNcWnZISnyzvZ4F/iu+KoA3BRNVjKgG+O2tluuah9YbXZk03vOf81
    bAgo0bV8KBO1/CJBXWUptya+VrnPQLJBAKUrfyAzGPzhbvAsNDmlNthmWKXem2j e
    H8hi/gyhj30MntZ8aR70N0IuDV2MCkq7bc4Vw4NeUEc1TqEFao5w1Ig deDtYUfDf
    2wKgFsjSCDTkel8r/xzecO79RyFpN/kXfezUroOvk+Vk9Oymu6c6Cop6aCmH2lOe
    pexxYjz7d5E=
    =uyc0
    -----END PGP SIGNATURE-----


  20. Re: [PATCH] add a printk_init variant storing format strings in __initdata

    > > When using printk from __init functions it would be desirable to place
    > > the printk format strings in __initdata. Add a printk_init() variant
    > > which does this.


    Russell played with this years ago I seem to remember and broke the ARM
    gcc in the process. If we have any compilers generating near pointer
    references for string constants then suddenely hiding them in another
    section is going to be interesting.

    > Wouldn't it be great if checkpatch were to detect
    > fail-to-use-printk_init() in an __init function?


    Can we have a 'correctpatch' to go with it someday perhaps
    --
    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 2 1 2 LastLast