PM: boot time suspend selftest
Subsystems (3)
PATCH 1/1 PM: boot time suspend selftest
From: David Brownell <[email protected]>
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.
---
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(-)
Documentation/kernel-parameters.txt +8 -1
@@ -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
kernel/power/Kconfig +11 -0
@@ -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
kernel/power/main.c +185 -0
212 lines not on this page · raw message
Quoted 9 lines · David Brownell
From: David Brownell <[email protected]>
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.
Quoted 8 lines
- 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
Quoted 274 lines
Signed-off-by David Brownell <[email protected]>---
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(-)Documentation/kernel-parameters.txt +8
-1@@ -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 connectionkernel/power/Kconfig +11
-0@@ -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 || BROKENkernel/power/main.c +185
-0@@ -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_stif (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 <linux/rtc.h> + +/* +* 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 */
Quoted 3 lines · Rafael J. Wysocki
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.
Quoted 1 line
2) Does it apply to the current linux-next?
No idea.
Quoted 3 lines · Rafael J. Wysocki
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.
Quoted 1 line
2) Does it apply to the current linux-next?
No idea.
Quoted 5 lines · David Brownell
Quoted 3 lines · Rafael J. Wysocki
Two questions:
1) How is it related to the analogous patch in the Ingo's tree?
Quoted 8 lines
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.
Quoted 1 line
2) Does it apply to the current linux-next?
No idea.
Could you check, please? The previous version didn't.
Thanks,
Rafael
Quoted 11 lines · On Thu, 29 May 2008 23:29:48 +0200 "Rafael J. Wysocki" <[email protected]> wrote:
Quoted 10 lines · David Brownell
Quoted 3 lines · Rafael J. Wysocki
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.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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:
Quoted 6 lines · Andrew Morton
Quoted 3 lines
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
Quoted 19 lines
***************
*** 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:
Quoted 6 lines · Andrew Morton
Quoted 3 lines
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
Quoted 19 lines
***************
*** 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:
Quoted 5 lines · David Brownell
Quoted 3 lines · Rafael J. Wysocki
Two questions:
1) How is it related to the analogous patch in the Ingo's tree?
Quoted 8 lines
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.
Quoted 1 line
2) Does it apply to the current linux-next?
No idea.
Could you check, please? The previous version didn't.
Thanks,
Rafael
Quoted 9 lines · David Brownell
From: David Brownell <[email protected]>
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.
Quoted 8 lines
- 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
Quoted 274 lines
Signed-off-by David Brownell <[email protected]>---
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(-)Documentation/kernel-parameters.txt +8
-1@@ -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 connectionkernel/power/Kconfig +11
-0@@ -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 || BROKENkernel/power/main.c +185
-0@@ -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_stif (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 <linux/rtc.h> + +/* +* 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 */
Quoted 39 lines · On Thu, 29 May 2008 13:33:41 -0700 David Brownell <[email protected]> wrote:
From: David Brownell <[email protected]>
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.
...
kernel/power/main.c +19
-0+#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...
kernel/power/main.c static inline int suspend_test(int level · line 163
Quoted 10 lines
kernel/power/main.c +10
-0+*/ + 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()
kernel/power/main.c static inline int suspend_test(int level · line 165
Quoted 2 lines
kernel/power/main.c +2
-0+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.
kernel/power/main.c static inline int suspend_test(int level · line 166
Quoted 1 line
kernel/power/main.c +1
-0+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.
Quoted 16 lines
kernel/power/main.c +13
-0+} + +#else + +static void suspend_test_start (void ) +{ +} + +static void suspend_test_finish (const char *label) +{ +} + +#endif
kernel/power/main.c static inline int suspend_test(int level
Quoted 10 lines
kernel/power/main.c +10
-0+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.
Quoted 61 lines
kernel/power/main.c +61
-0+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) +{
kernel/power/main.c static int __init pm_init(void) · line 661
Quoted 10 lines
kernel/power/main.c +10
-0+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.
<googles>
<finds http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-09/4078.html>
<can't work out why it is used here>
kernel/power/main.c static int __init pm_init(void) · line 674
Quoted 13 lines
kernel/power/main.c +13
-0+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.
Quoted 27 lines
kernel/power/main.c +27
-0+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); +
Quoted 39 lines · On Thu, 29 May 2008 13:33:41 -0700 David Brownell <[email protected]> wrote:
From: David Brownell <[email protected]>
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.
...
kernel/power/main.c +19
-0+#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...
kernel/power/main.c static inline int suspend_test(int level · line 163
Quoted 10 lines
kernel/power/main.c +10
-0+*/ + 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()
kernel/power/main.c static inline int suspend_test(int level · line 165
Quoted 2 lines
kernel/power/main.c +2
-0+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.
kernel/power/main.c static inline int suspend_test(int level · line 166
Quoted 1 line
kernel/power/main.c +1
-0+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.
Quoted 16 lines
kernel/power/main.c +13
-0+} + +#else + +static void suspend_test_start (void ) +{ +} + +static void suspend_test_finish (const char *label) +{ +} + +#endif
kernel/power/main.c static inline int suspend_test(int level
Quoted 10 lines
kernel/power/main.c +10
-0+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.
Quoted 61 lines
kernel/power/main.c +61
-0+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) +{
kernel/power/main.c static int __init pm_init(void) · line 661
Quoted 10 lines
kernel/power/main.c +10
-0+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.
<googles>
<finds http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-09/4078.html>
<can't work out why it is used here>
kernel/power/main.c static int __init pm_init(void) · line 674
Quoted 13 lines
kernel/power/main.c +13
-0+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.
Quoted 27 lines
kernel/power/main.c +27
-0+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); +
Quoted 4 lines · Andrew Morton
kernel/power/main.c static inline int suspend_test(int level · line 166
Quoted 1 line
kernel/power/main.c +1
-0+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".
Quoted 2 lines
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
Quoted 4 lines · Andrew Morton
kernel/power/main.c static inline int suspend_test(int level · line 166
Quoted 1 line
kernel/power/main.c +1
-0+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".
Quoted 2 lines
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
Quoted 9 lines · On Fri, 30 May 2008 12:59:48 +0200 Ingo Molnar <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
kernel/power/main.c static inline int suspend_test(int level · line 166
Quoted 1 line
kernel/power/main.c +1
-0+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...
PATCH 1/1 add a printk_init variant storing format strings in __initdata
In reply to Andrew Morton[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.
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)
include/linux/kernel.h +10 -0
@@ -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 ; }
kernel/printk.c +12 -0
@@ -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 ;
PATCH 1/1 add a printk_init variant storing format strings in __initdata
In reply to Andrew Morton[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.
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)
include/linux/kernel.h +10 -0
@@ -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 ; }
kernel/printk.c +12 -0
@@ -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 ;
Re: [PATCH] add a printk_init variant storing format strings in __initdata
Hi,
Quoted 6 lines · Andy Whitcroft
[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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy WhitcroftHi,
Quoted 6 lines · Andy Whitcroft
[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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy Whitcroftkernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 630
Quoted 13 lines
kernel/printk.c +12
-0+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
Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy Whitcroftkernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 630
Quoted 13 lines
kernel/printk.c +12
-0+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
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy WhitcroftQuoted 14 lines · On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> 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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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 toprintk()but marked__inititself. By ensuring that an__initvariant of printk is used we get proper section violation warnings when this is used incorrectly:
Quoted 9 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
__coldannotations.
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 :)
Quoted 41 lines
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)include/linux/kernel.h +10
-0@@ -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 ; }kernel/printk.c +1
-0@@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)return r; } +asmlinkage __init int __printk_init(const char *fmt, ...)
kernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 629
Quoted 10 lines
kernel/printk.c +10
-0+{ +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.
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy WhitcroftQuoted 14 lines · On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> 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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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 toprintk()but marked__inititself. By ensuring that an__initvariant of printk is used we get proper section violation warnings when this is used incorrectly:
Quoted 9 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
__coldannotations.
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 :)
Quoted 41 lines
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)include/linux/kernel.h +10
-0@@ -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 ; }kernel/printk.c +1
-0@@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)return r; } +asmlinkage __init int __printk_init(const char *fmt, ...)
kernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 629
Quoted 10 lines
kernel/printk.c +10
-0+{ +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.
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.
Quoted 105 lines · Andrew Morton
Quoted 14 lines · On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> 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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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 toprintk()but marked__inititself. By ensuring that an__initvariant of printk is used we get proper section violation warnings when this is used incorrectly:Quoted 9 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
__coldannotations.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__initfunction?oh, speaking of checkpatch: please use it :)
Quoted 41 lines
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)include/linux/kernel.h +10
-0@@ -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 ; }kernel/printk.c +1
-0@@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)return r; } +asmlinkage __init int __printk_init(const char *fmt, ...)kernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 629
Quoted 10 lines
kernel/printk.c +10
-0+{ +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()andprintk_cpuinit()and whatever.Which probably means that
__printk_init()can't be__init, unless all theCONFIG_settings which control__cpuinit,__meminitetc 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Kinds regards, MinChan Kim
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andrew MortonIt is important about embedded system, too.
So I add CC, linux-embedded.
Quoted 105 lines · Andrew Morton
Quoted 14 lines · On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <[email protected]> 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
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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 toprintk()but marked__inititself. By ensuring that an__initvariant of printk is used we get proper section violation warnings when this is used incorrectly:Quoted 9 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
__coldannotations.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__initfunction?oh, speaking of checkpatch: please use it :)
Quoted 41 lines
--- include/linux/kernel.h | 10 ++++++++++ kernel/printk.c | 12 ++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-)include/linux/kernel.h +10
-0@@ -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 ; }kernel/printk.c +1
-0@@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)return r; } +asmlinkage __init int __printk_init(const char *fmt, ...)kernel/printk.c asmlinkage int printk(const char *fmt, ...) · line 629
Quoted 10 lines
kernel/printk.c +10
-0+{ +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()andprintk_cpuinit()and whatever.Which probably means that
__printk_init()can't be__init, unless all theCONFIG_settings which control__cpuinit,__meminitetc 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Kinds regards, MinChan Kim
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andrew MortonQuoted 2 lines · Andrew Morton
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?
Signature · 1 line
dwmw2
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andrew MortonQuoted 2 lines · Andrew Morton
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?
Signature · 1 line
dwmw2
Re: [PATCH] add a printk_init variant storing format strings in __initdata
Quoted 9 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
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
__initis 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?
Quoted 6 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata
In reply to David WoodhouseQuoted 9 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
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
__initis 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?
Quoted 6 lines
WARNING:
vmlinux.o(.text+0x3): Section mismatch in reference from the
functionsomething()to the variable .init.data:__printk_init_fmt.31426
The functionsomething()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
Re: [PATCH] add a printk_init variant storing format strings in __initdata
Quoted 5 lines · Johannes Berg
Quoted 4 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
We're going to want to be able to call
printk_init()from modules. Please fix and test that, if we decide to proceed.
Quoted 7 lines
Quoted 5 lines
Can we fix that by making it an alias for printk in the module case?
The only reason we need it to be
__initis 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?
Signature · 1 line
dwmw2
Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Johannes BergQuoted 5 lines · Johannes Berg
Quoted 4 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
We're going to want to be able to call
printk_init()from modules. Please fix and test that, if we decide to proceed.
Quoted 7 lines
Quoted 5 lines
Can we fix that by making it an alias for printk in the module case?
The only reason we need it to be
__initis 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?
Signature · 1 line
dwmw2
Re: [PATCH] add a printk_init variant storing format strings in __initdata
Quoted 8 lines · David Woodhouse
Quoted 7 lines · Johannes Berg
Quoted 6 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
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?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The only reason we need it to be
__initis 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
Re: [linux-pm] [PATCH] add a printk_init variant storing format strings in __initdata
In reply to David WoodhouseQuoted 8 lines · David Woodhouse
Quoted 7 lines · Johannes Berg
Quoted 6 lines · David Woodhouse
Quoted 2 lines · Andrew Morton
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?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The only reason we need it to be
__initis 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
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andrew MortonQuoted 3 lines
Quoted 3 lines
When using printk from
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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.
Quoted 2 lines
Wouldn't it be great if checkpatch were to detect
fail-to-use-printk_init() in an__initfunction?
Can we have a 'correctpatch' to go with it someday perhaps ;)
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andrew MortonQuoted 3 lines
Quoted 3 lines
When using printk from
__initfunctions it would be desirable to place the printk format strings in__initdata. Add aprintk_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.
Quoted 2 lines
Wouldn't it be great if checkpatch were to detect
fail-to-use-printk_init() in an__initfunction?
Can we have a 'correctpatch' to go with it someday perhaps ;)
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy Whitcroftinclude/linux/kernel.h struct pid; · line 187
Quoted 5 lines · Andy Whitcroft
include/linux/kernel.h +5
-0+#define printk_init(fmt, args...) \ +do { \ + static char __printk_init_fmt[] __initdata = fmt; \ + __printk_init(__printk_init_fmt, ##args); \ +} while (0)
Hm, do these strings still get merged? Perhaps we want them in a .initdata.str section on their own?
Bonus points for letting the linker 'merge' such strings into the real .rodata.str section if they exist there too.
Signature · 1 line
dwmw2
Re: [PATCH] add a printk_init variant storing format strings in __initdata
In reply to Andy Whitcroftinclude/linux/kernel.h struct pid; · line 187
Quoted 5 lines · Andy Whitcroft
include/linux/kernel.h +5
-0+#define printk_init(fmt, args...) \ +do { \ + static char __printk_init_fmt[] __initdata = fmt; \ + __printk_init(__printk_init_fmt, ##args); \ +} while (0)
Hm, do these strings still get merged? Perhaps we want them in a .initdata.str section on their own?
Bonus points for letting the linker 'merge' such strings into the real .rodata.str section if they exist there too.
Signature · 1 line
dwmw2
Quoted 14 lines · Andrew Morton
kernel/power/main.c static int __init pm_init(void) · line 589
Quoted 10 lines
kernel/power/main.c +10
-0+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.
Quoted 1 line
- 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
Quoted 14 lines · Andrew Morton
kernel/power/main.c static int __init pm_init(void) · line 589
Quoted 10 lines
kernel/power/main.c +10
-0+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.
Quoted 1 line
- 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
Address some of Andrew's comments on the patch now in MMOTM with name "pm-boot-time-suspend-selftest.patch".
--- kernel/power/main.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
kernel/power/main.c +15 -6
@@ -160,10 +160,19 @@ static void suspend_test_finish(const chlong nj = jiffies - suspend_test_start_time;unsigned msec; - msec =jiffies_to_msecs ((nj >=0 ) ? nj : -nj); + msec =jiffies_to_msecs (abs (nj));pr_info ("PM: %s took %d.%03d seconds\n" , label, msec /1000 , msec %1000 ); - WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000)); + +/* Warning on suspend means the RTC alarm period needs to be +* larger -- the system was sooo slooowwww to suspend that the +* alarm (should have) fired before the system went to sleep! +* +* Warning on either suspend or resume also means the system +* has some performance issues. The stack dump of a WARN_ON +* is more likely to get the right attention than a printk... +*/ + WARN_ON(msec > (TEST_SUSPEND_SECONDS * 1000)); }#else
@@ -579,13 +588,13 @@ core_initcall(pm_init);static void __init test_wakealarm (struct rtc_device *rtc,suspend_state_t state) { -static char err_readtime []__initdata = +static char err_readtime[]__initdata =KERN_ERR "PM: can't read %s time, err %d\n" ; -static char err_wakealarm []__initdata = +static char err_wakealarm []__initdata =KERN_ERR "PM: can't set %s wakealarm, err %d\n" ; -static char err_suspend []__initdata = +static char err_suspend[]__initdata =KERN_ERR "PM: suspend test failed, error %d\n" ; -static char info_test []__initdata = +static char info_test[]__initdata =KERN_INFO "PM: test RTC wakeup from '%s' suspend\n" ;unsigned long now;
Address some of Andrew's comments on the patch now in MMOTM with name "pm-boot-time-suspend-selftest.patch".
--- kernel/power/main.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
kernel/power/main.c +15 -6
@@ -160,10 +160,19 @@ static void suspend_test_finish(const chlong nj = jiffies - suspend_test_start_time;unsigned msec; - msec =jiffies_to_msecs ((nj >=0 ) ? nj : -nj); + msec =jiffies_to_msecs (abs (nj));pr_info ("PM: %s took %d.%03d seconds\n" , label, msec /1000 , msec %1000 ); - WARN_ON_ONCE(msec > (TEST_SUSPEND_SECONDS * 1000)); + +/* Warning on suspend means the RTC alarm period needs to be +* larger -- the system was sooo slooowwww to suspend that the +* alarm (should have) fired before the system went to sleep! +* +* Warning on either suspend or resume also means the system +* has some performance issues. The stack dump of a WARN_ON +* is more likely to get the right attention than a printk... +*/ + WARN_ON(msec > (TEST_SUSPEND_SECONDS * 1000)); }#else
@@ -579,13 +588,13 @@ core_initcall(pm_init);static void __init test_wakealarm (struct rtc_device *rtc,suspend_state_t state) { -static char err_readtime []__initdata = +static char err_readtime[]__initdata =KERN_ERR "PM: can't read %s time, err %d\n" ; -static char err_wakealarm []__initdata = +static char err_wakealarm []__initdata =KERN_ERR "PM: can't set %s wakealarm, err %d\n" ; -static char err_suspend []__initdata = +static char err_suspend[]__initdata =KERN_ERR "PM: suspend test failed, error %d\n" ; -static char info_test []__initdata = +static char info_test[]__initdata =KERN_INFO "PM: test RTC wakeup from '%s' suspend\n" ;unsigned long now;
Quoted 7 lines · On Thu, 29 May 2008 13:33:41 -0700 David Brownell <[email protected]> wrote:
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.
Quoted 8 lines
- 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.
mips allmodconfig:
kernel/built-in.o(.init.text+0x5b30): In function `test_suspend':
: undefined reference to rtc_class' kernel/built-in.o(.init.text+0x5b38): In function test_suspend':
: undefined reference to rtc_class' kernel/built-in.o(.init.text+0x5ba4): In function test_suspend':
: undefined reference to rtc_class_open' kernel/built-in.o(.init.text+0x5c30): In function test_suspend':
: undefined reference to rtc_read_time' kernel/built-in.o(.init.text+0x5d50): In function test_suspend':
: undefined reference to rtc_set_alarm' kernel/built-in.o(.init.text+0x5f6c): In function test_suspend':
: undefined reference to `rtc_class_close'
some of these are already exported, so something else went wrong.
Quoted 7 lines · On Thu, 29 May 2008 13:33:41 -0700 David Brownell <[email protected]> wrote:
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.
Quoted 8 lines
- 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.
mips allmodconfig:
kernel/built-in.o(.init.text+0x5b30): In function `test_suspend':
: undefined reference to rtc_class' kernel/built-in.o(.init.text+0x5b38): In function test_suspend':
: undefined reference to rtc_class' kernel/built-in.o(.init.text+0x5ba4): In function test_suspend':
: undefined reference to rtc_class_open' kernel/built-in.o(.init.text+0x5c30): In function test_suspend':
: undefined reference to rtc_read_time' kernel/built-in.o(.init.text+0x5d50): In function test_suspend':
: undefined reference to rtc_set_alarm' kernel/built-in.o(.init.text+0x5f6c): In function test_suspend':
: undefined reference to `rtc_class_close'
some of these are already exported, so something else went wrong.