* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] [not found] ` <4D234F8E.7080102@gmail.com> @ 2011-01-04 22:57 ` Rafael J. Wysocki 2011-01-05 21:36 ` Jiri Slaby 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-04 22:57 UTC (permalink / raw) To: Jiri Slaby Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On Tuesday, January 04, 2011, Jiri Slaby wrote: > On 01/04/2011 02:40 PM, Jiri Slaby wrote: > > On 12/24/2010 01:58 AM, akpm@linux-foundation.org wrote: > >> The mm-of-the-moment snapshot 2010-12-23-16-58 has been uploaded to > > > > Hi, this kernel regresses with respect to suspend to ram in comparison > > with mmotm 2010-12-16-14-56. > > > > This is OK: > > echo devices > /sys/power/pm_test > > pm-suspend > > This hangs at suspend phase: > > echo platform > /sys/power/pm_test > > pm-suspend Hmm. So it looks like _PTS hangs? > > Note that this kernel is based on next-20101221. Should I try newer (and > > clean) -next? > > Ok, bisected down to: > 16dc39c98a6ca56a27f22f7ac6731d8223237a2e is first bad commit > commit 16dc39c98a6ca56a27f22f7ac6731d8223237a2e > Author: Len Brown <len.brown@intel.com> > Date: Thu Dec 16 23:12:23 2010 -0500 > > ACPI: use ioremap_cache() > > Although the temporary boot-time ACPI table mappings > were set up with CPU caching enabled, the permanent table > mappings and AML run-time region memory accesses were > set up with ioremap(), which on x86 is a synonym for > ioremap_nocache(). > > Changing this to ioremap_cache() improves performance as > seen when accessing the tables via acpidump, > or /sys/firmware/acpi/tables. It should also improve > AML run-time performance. > > No change on ia64. > > Reported-by: Jack Steiner <steiner@sgi.com> > Signed-off-by: Len Brown <len.brown@intel.com> > > :040000 040000 be35c5e8f214f10f94688c1a27f33ecfb8505220 > 52581222d0edf190f160f3e5aa5d2c1af8e76988 M arch > :040000 040000 ccdca0d41938b8312e946cde3c01c59b32d1c17c > 96ccf2357f2ac4a31d19cc41f5728d9f87b6cac0 M drivers > > Revert of that patch fixes the problem. Can you send a dmesg output and acpidump output from the failing system, please? Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-04 22:57 ` suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] Rafael J. Wysocki @ 2011-01-05 21:36 ` Jiri Slaby 2011-01-05 22:39 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Jiri Slaby @ 2011-01-05 21:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On 01/04/2011 11:57 PM, Rafael J. Wysocki wrote: > On Tuesday, January 04, 2011, Jiri Slaby wrote: >> On 01/04/2011 02:40 PM, Jiri Slaby wrote: >>> On 12/24/2010 01:58 AM, akpm@linux-foundation.org wrote: >>>> The mm-of-the-moment snapshot 2010-12-23-16-58 has been uploaded to >>> >>> Hi, this kernel regresses with respect to suspend to ram in comparison >>> with mmotm 2010-12-16-14-56. >>> >>> This is OK: >>> echo devices > /sys/power/pm_test >>> pm-suspend >>> This hangs at suspend phase: >>> echo platform > /sys/power/pm_test >>> pm-suspend > > Hmm. So it looks like _PTS hangs? No _PST here :). >>> Note that this kernel is based on next-20101221. Should I try newer (and >>> clean) -next? >> >> Ok, bisected down to: >> 16dc39c98a6ca56a27f22f7ac6731d8223237a2e is first bad commit >> commit 16dc39c98a6ca56a27f22f7ac6731d8223237a2e ... >> Revert of that patch fixes the problem. > > Can you send a dmesg output and acpidump output from the failing system, please? Here they are: http://www.fi.muni.cz/~xslaby/sklad/panics/dmesg-no-susp.txt http://www.fi.muni.cz/~xslaby/sklad/adump.txt They are captured from the mmotm minus 16dc39c98a6ca56 if that's OK? Ignore the BUG there, it's caused by qemu and fixed in later -next. regards, -- js ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-05 21:36 ` Jiri Slaby @ 2011-01-05 22:39 ` Rafael J. Wysocki 2011-01-05 22:46 ` Jiri Slaby 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-05 22:39 UTC (permalink / raw) To: Jiri Slaby Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On Wednesday, January 05, 2011, Jiri Slaby wrote: > On 01/04/2011 11:57 PM, Rafael J. Wysocki wrote: > > On Tuesday, January 04, 2011, Jiri Slaby wrote: > >> On 01/04/2011 02:40 PM, Jiri Slaby wrote: > >>> On 12/24/2010 01:58 AM, akpm@linux-foundation.org wrote: > >>>> The mm-of-the-moment snapshot 2010-12-23-16-58 has been uploaded to > >>> > >>> Hi, this kernel regresses with respect to suspend to ram in comparison > >>> with mmotm 2010-12-16-14-56. > >>> > >>> This is OK: > >>> echo devices > /sys/power/pm_test > >>> pm-suspend > >>> This hangs at suspend phase: > >>> echo platform > /sys/power/pm_test > >>> pm-suspend > > > > Hmm. So it looks like _PTS hangs? > > No _PST here :). I really meant _PTS and yes, there is one and it does quite a lot of interesting stuff. Are you able to collect serial console (or equivalent) logs from that machine? Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-05 22:39 ` Rafael J. Wysocki @ 2011-01-05 22:46 ` Jiri Slaby 2011-01-05 23:28 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Jiri Slaby @ 2011-01-05 22:46 UTC (permalink / raw) To: Rafael J. Wysocki Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On 01/05/2011 11:39 PM, Rafael J. Wysocki wrote: > On Wednesday, January 05, 2011, Jiri Slaby wrote: >> On 01/04/2011 11:57 PM, Rafael J. Wysocki wrote: >>> On Tuesday, January 04, 2011, Jiri Slaby wrote: >>>> On 01/04/2011 02:40 PM, Jiri Slaby wrote: >>>>> On 12/24/2010 01:58 AM, akpm@linux-foundation.org wrote: >>>>>> The mm-of-the-moment snapshot 2010-12-23-16-58 has been uploaded to >>>>> >>>>> Hi, this kernel regresses with respect to suspend to ram in comparison >>>>> with mmotm 2010-12-16-14-56. >>>>> >>>>> This is OK: >>>>> echo devices > /sys/power/pm_test >>>>> pm-suspend >>>>> This hangs at suspend phase: >>>>> echo platform > /sys/power/pm_test >>>>> pm-suspend >>> >>> Hmm. So it looks like _PTS hangs? >> >> No _PST here :). > > I really meant _PTS and yes, there is one and it does quite a lot of > interesting stuff. Yeah, I grepped for _PTS, but in a wrong file. My bad. > Are you able to collect serial console (or equivalent) logs from that machine? Unfortunately I don't have a second machine with serial port. What exactly you want to dig out of the logs? Will netconsole work at that phase? regards, -- js ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-05 22:46 ` Jiri Slaby @ 2011-01-05 23:28 ` Rafael J. Wysocki 2011-01-06 9:24 ` Jiri Slaby [not found] ` <1294305504-5787-1-git-send-email-jslaby@suse.cz> 0 siblings, 2 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-05 23:28 UTC (permalink / raw) To: Jiri Slaby Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On Wednesday, January 05, 2011, Jiri Slaby wrote: > On 01/05/2011 11:39 PM, Rafael J. Wysocki wrote: > > On Wednesday, January 05, 2011, Jiri Slaby wrote: > >> On 01/04/2011 11:57 PM, Rafael J. Wysocki wrote: > >>> On Tuesday, January 04, 2011, Jiri Slaby wrote: > >>>> On 01/04/2011 02:40 PM, Jiri Slaby wrote: > >>>>> On 12/24/2010 01:58 AM, akpm@linux-foundation.org wrote: > >>>>>> The mm-of-the-moment snapshot 2010-12-23-16-58 has been uploaded to > >>>>> > >>>>> Hi, this kernel regresses with respect to suspend to ram in comparison > >>>>> with mmotm 2010-12-16-14-56. > >>>>> > >>>>> This is OK: > >>>>> echo devices > /sys/power/pm_test > >>>>> pm-suspend > >>>>> This hangs at suspend phase: > >>>>> echo platform > /sys/power/pm_test > >>>>> pm-suspend > >>> > >>> Hmm. So it looks like _PTS hangs? > >> > >> No _PST here :). > > > > I really meant _PTS and yes, there is one and it does quite a lot of > > interesting stuff. > > Yeah, I grepped for _PTS, but in a wrong file. My bad. > > > Are you able to collect serial console (or equivalent) logs from that machine? > > Unfortunately I don't have a second machine with serial port. What > exactly you want to dig out of the logs? Will netconsole work at that phase? I don't think so, the adapter will have been already suspended. Hmm. I guess you can boot with no_console_suspend in the kernel command line and use the video console to get some debug info. At this point we would like to know where exactly it hangs, so you'd need to enable AML tracing in the kernel and basically see what's printed last. :-) Two more questions: (1) Does the failing machine power off correctly with the kernel that fails to suspend? (2) Is the failing suspend behavior reproducible if you run "echo disk > /sys/power/state" instead of "pm-suspend" in the platform test? Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-05 23:28 ` Rafael J. Wysocki @ 2011-01-06 9:24 ` Jiri Slaby 2011-01-06 15:45 ` Rafael J. Wysocki [not found] ` <1294305504-5787-1-git-send-email-jslaby@suse.cz> 1 sibling, 1 reply; 12+ messages in thread From: Jiri Slaby @ 2011-01-06 9:24 UTC (permalink / raw) To: Rafael J. Wysocki Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On 01/06/2011 12:28 AM, Rafael J. Wysocki wrote: > Two more questions: > (1) Does the failing machine power off correctly with the kernel that fails to > suspend? Poweroff is OK. > (2) Is the failing suspend behavior reproducible if you run > "echo disk > /sys/power/state" instead of "pm-suspend" in the platform test? Thanks for the hint no_console_suspend with hibernate shows somwthing (opposing to suspend). I've sent a patch for the oops which occured due to ioremap change. The ioremap change still needs to be reverted, obviously. The oops in question: http://www.fi.muni.cz/~xslaby/sklad/panics/suspend-oops.png /proc/mtrr: reg00: base=0x0b0000000 ( 2816MB), size= 256MB, count=1: uncachable reg01: base=0x0c0000000 ( 3072MB), size= 1024MB, count=1: uncachable reg02: base=0x000000000 ( 0MB), size= 4096MB, count=1: write-back reg03: base=0x100000000 ( 4096MB), size= 2048MB, count=1: write-back reg04: base=0x180000000 ( 6144MB), size= 1024MB, count=1: write-back reg05: base=0x1c0000000 ( 7168MB), size= 128MB, count=1: write-back reg06: base=0x1c8000000 ( 7296MB), size= 64MB, count=1: write-back reg07: base=0x0af600000 ( 2806MB), size= 2MB, count=1: uncachable /debug/x86/pat_memtype_list: PAT memtype list: uncached-minus @ 0xaf5b0000-0xaf5b7000 uncached-minus @ 0xaf5be000-0xaf5c1000 uncached-minus @ 0xaf5be000-0xaf5c1000 uncached-minus @ 0xaf5be000-0xaf5bf000 uncached-minus @ 0xb0200000-0xb0201000 write-combining @ 0xd0000000-0xe0000000 write-combining @ 0xd0001000-0xd0021000 write-combining @ 0xd0030000-0xd0530000 uncached-minus @ 0xe0000000-0xf0000000 uncached-minus @ 0xfe6ff000-0xfe700000 uncached-minus @ 0xfe7e0000-0xfe800000 uncached-minus @ 0xfea00000-0xfea80000 uncached-minus @ 0xfeb40000-0xfeb60000 uncached-minus @ 0xfeb78000-0xfeb7c000 uncached-minus @ 0xfeb7c000-0xfeb7d000 uncached-minus @ 0xfeb7d000-0xfeb7e000 uncached-minus @ 0xfeb7f000-0xfeb80000 uncached-minus @ 0xfeb7f000-0xfeb80000 uncached-minus @ 0xfeb80000-0xfec00000 uncached-minus @ 0xfeb80000-0xfec00000 uncached-minus @ 0xfed00000-0xfed01000 uncached-minus @ 0xfed1f000-0xfed20000 uncached-minus @ 0xffff0000-0xffff1000 regards, -- js ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] 2011-01-06 9:24 ` Jiri Slaby @ 2011-01-06 15:45 ` Rafael J. Wysocki 0 siblings, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-06 15:45 UTC (permalink / raw) To: Jiri Slaby Cc: akpm, mm-commits, LKML, Linux-pm mailing list, Brown, Len, steiner, ACPI Devel Maling List, Len Brown On Thursday, January 06, 2011, Jiri Slaby wrote: > On 01/06/2011 12:28 AM, Rafael J. Wysocki wrote: > > Two more questions: > > (1) Does the failing machine power off correctly with the kernel that fails to > > suspend? > > Poweroff is OK. > > > (2) Is the failing suspend behavior reproducible if you run > > "echo disk > /sys/power/state" instead of "pm-suspend" in the platform test? > > Thanks for the hint no_console_suspend with hibernate shows somwthing > (opposing to suspend). I've sent a patch for the oops which occured due > to ioremap change. Can you send a link to the patch, please? > The ioremap change still needs to be reverted, obviously. Does it need to be reverted with your patch applied too? Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <1294305504-5787-1-git-send-email-jslaby@suse.cz>]
* Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code [not found] ` <1294305504-5787-1-git-send-email-jslaby@suse.cz> @ 2011-01-06 15:57 ` Rafael J. Wysocki 2011-01-06 16:09 ` Jiri Slaby 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-06 15:57 UTC (permalink / raw) To: Jiri Slaby Cc: akpm, linux-kernel, jirislaby, ACPI Devel Maling List, Linux-pm mailing list, Matthew Garrett, Len Brown On Thursday, January 06, 2011, Jiri Slaby wrote: > When ioremap fails (which might happen for some reason), If it happens, something is seriously wrong (see below). BTW, to keep things in context, please post fixes like this in the same thread in which you reported the problem. At lease please retain the CC list from there. > we nicely oops in suspend_nvs_save due to NULL dereference by memcpy in > there. Fail gracefully instead. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > --- > drivers/acpi/sleep.c | 5 ++--- > include/linux/suspend.h | 4 ++-- > kernel/power/nvs.c | 8 +++++++- > 3 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index c423231..f94c9a9 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -124,8 +124,7 @@ static int acpi_pm_freeze(void) > static int acpi_pm_pre_suspend(void) > { > acpi_pm_freeze(); > - suspend_nvs_save(); > - return 0; > + return suspend_nvs_save(); > } > > /** > @@ -151,7 +150,7 @@ static int acpi_pm_prepare(void) > { > int error = __acpi_pm_prepare(); > if (!error) > - acpi_pm_pre_suspend(); > + error = acpi_pm_pre_suspend(); > > return error; > } > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index c1f4998..3ac2551 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -262,7 +262,7 @@ static inline bool system_entering_hibernation(void) { return false; } > extern int suspend_nvs_register(unsigned long start, unsigned long size); > extern int suspend_nvs_alloc(void); > extern void suspend_nvs_free(void); > -extern void suspend_nvs_save(void); > +extern int suspend_nvs_save(void); > extern void suspend_nvs_restore(void); > #else /* CONFIG_SUSPEND_NVS */ > static inline int suspend_nvs_register(unsigned long a, unsigned long b) > @@ -271,7 +271,7 @@ static inline int suspend_nvs_register(unsigned long a, unsigned long b) > } > static inline int suspend_nvs_alloc(void) { return 0; } > static inline void suspend_nvs_free(void) {} > -static inline void suspend_nvs_save(void) {} > +static inline int suspend_nvs_save(void) {} > static inline void suspend_nvs_restore(void) {} > #endif /* CONFIG_SUSPEND_NVS */ > > diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c > index 1836db6..57c6fab 100644 > --- a/kernel/power/nvs.c > +++ b/kernel/power/nvs.c > @@ -105,7 +105,7 @@ int suspend_nvs_alloc(void) > /** > * suspend_nvs_save - save NVS memory regions > */ > -void suspend_nvs_save(void) > +int suspend_nvs_save(void) > { > struct nvs_page *entry; > > @@ -114,8 +114,14 @@ void suspend_nvs_save(void) > list_for_each_entry(entry, &nvs_list, node) > if (entry->data) { > entry->kaddr = ioremap(entry->phys_start, entry->size); I wonder what happens if you simply change the ioremap() here to ioremap_nocache() without any other modifications? It _really_ shouldn't fail here, because the NVS pages are known to be present. > + if (!entry->kaddr) { > + suspend_nvs_free(); > + return -ENOMEM; > + } > memcpy(entry->data, entry->kaddr, entry->size); > } > + > + return 0; > } > > /** > Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code 2011-01-06 15:57 ` [PATCH 1/1] PM: fix oops in suspend/hibernate code Rafael J. Wysocki @ 2011-01-06 16:09 ` Jiri Slaby 2011-01-06 16:31 ` Jiri Slaby 2011-01-06 16:38 ` Rafael J. Wysocki 0 siblings, 2 replies; 12+ messages in thread From: Jiri Slaby @ 2011-01-06 16:09 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Jiri Slaby, akpm, linux-kernel, ACPI Devel Maling List, Linux-pm mailing list, Matthew Garrett, Len Brown On 01/06/2011 04:57 PM, Rafael J. Wysocki wrote: > On Thursday, January 06, 2011, Jiri Slaby wrote: >> When ioremap fails (which might happen for some reason), > > If it happens, something is seriously wrong (see below). I agree that something is broken, however ioremap may fail for dozen of reasons. Ignoring the retval is a *bad* idea and it took me a while to sort out what is wrong. Especially if one has no console like throughout suspend. If it was handled properly, I would know immediately. (There should be a message printed out which I forgot to add.) > BTW, to keep things in context, please post fixes like this in the same thread > in which you reported the problem. At lease please retain the CC list from > there. I actually did, there is: In-Reply-To: <201101060028.43342.rjw@sisk.pl> and it successfully threaded to the conversation for me in TB. >> we nicely oops in suspend_nvs_save due to NULL dereference by memcpy in >> there. Fail gracefully instead. >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> >> --- >> drivers/acpi/sleep.c | 5 ++--- >> include/linux/suspend.h | 4 ++-- >> kernel/power/nvs.c | 8 +++++++- >> 3 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c >> index c423231..f94c9a9 100644 >> --- a/drivers/acpi/sleep.c >> +++ b/drivers/acpi/sleep.c >> @@ -124,8 +124,7 @@ static int acpi_pm_freeze(void) >> static int acpi_pm_pre_suspend(void) >> { >> acpi_pm_freeze(); >> - suspend_nvs_save(); >> - return 0; >> + return suspend_nvs_save(); >> } >> >> /** >> @@ -151,7 +150,7 @@ static int acpi_pm_prepare(void) >> { >> int error = __acpi_pm_prepare(); >> if (!error) >> - acpi_pm_pre_suspend(); >> + error = acpi_pm_pre_suspend(); >> >> return error; >> } >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h >> index c1f4998..3ac2551 100644 >> --- a/include/linux/suspend.h >> +++ b/include/linux/suspend.h >> @@ -262,7 +262,7 @@ static inline bool system_entering_hibernation(void) { return false; } >> extern int suspend_nvs_register(unsigned long start, unsigned long size); >> extern int suspend_nvs_alloc(void); >> extern void suspend_nvs_free(void); >> -extern void suspend_nvs_save(void); >> +extern int suspend_nvs_save(void); >> extern void suspend_nvs_restore(void); >> #else /* CONFIG_SUSPEND_NVS */ >> static inline int suspend_nvs_register(unsigned long a, unsigned long b) >> @@ -271,7 +271,7 @@ static inline int suspend_nvs_register(unsigned long a, unsigned long b) >> } >> static inline int suspend_nvs_alloc(void) { return 0; } >> static inline void suspend_nvs_free(void) {} >> -static inline void suspend_nvs_save(void) {} >> +static inline int suspend_nvs_save(void) {} >> static inline void suspend_nvs_restore(void) {} >> #endif /* CONFIG_SUSPEND_NVS */ >> >> diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c >> index 1836db6..57c6fab 100644 >> --- a/kernel/power/nvs.c >> +++ b/kernel/power/nvs.c >> @@ -105,7 +105,7 @@ int suspend_nvs_alloc(void) >> /** >> * suspend_nvs_save - save NVS memory regions >> */ >> -void suspend_nvs_save(void) >> +int suspend_nvs_save(void) >> { >> struct nvs_page *entry; >> >> @@ -114,8 +114,14 @@ void suspend_nvs_save(void) >> list_for_each_entry(entry, &nvs_list, node) >> if (entry->data) { >> entry->kaddr = ioremap(entry->phys_start, entry->size); > > I wonder what happens if you simply change the ioremap() here to > ioremap_nocache() without any other modifications? ioremap *is* ioremap_nocache on x86. And that's the conflict it complains about I guess? Don't you mean ioremap_cache? > It _really_ shouldn't fail here, because the NVS pages are known to be present. It fails because of conflicting maps as can be seen in the photo. At least I think so. regards, -- js ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code 2011-01-06 16:09 ` Jiri Slaby @ 2011-01-06 16:31 ` Jiri Slaby 2011-01-06 16:38 ` Rafael J. Wysocki 1 sibling, 0 replies; 12+ messages in thread From: Jiri Slaby @ 2011-01-06 16:31 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Jiri Slaby, akpm, linux-kernel, ACPI Devel Maling List, Linux-pm mailing list, Matthew Garrett, Len Brown On 01/06/2011 05:09 PM, Jiri Slaby wrote: >>> --- a/kernel/power/nvs.c >>> +++ b/kernel/power/nvs.c >>> @@ -105,7 +105,7 @@ int suspend_nvs_alloc(void) >>> /** >>> * suspend_nvs_save - save NVS memory regions >>> */ >>> -void suspend_nvs_save(void) >>> +int suspend_nvs_save(void) >>> { >>> struct nvs_page *entry; >>> >>> @@ -114,8 +114,14 @@ void suspend_nvs_save(void) >>> list_for_each_entry(entry, &nvs_list, node) >>> if (entry->data) { >>> entry->kaddr = ioremap(entry->phys_start, entry->size); >> >> I wonder what happens if you simply change the ioremap() here to >> ioremap_nocache() without any other modifications? > > ioremap *is* ioremap_nocache on x86. And that's the conflict it > complains about I guess? Don't you mean ioremap_cache? Using ioremap_cache indeed fixes the problem... thanks, -- js ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code 2011-01-06 16:09 ` Jiri Slaby 2011-01-06 16:31 ` Jiri Slaby @ 2011-01-06 16:38 ` Rafael J. Wysocki 2011-01-06 21:01 ` Len Brown 1 sibling, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2011-01-06 16:38 UTC (permalink / raw) To: Jiri Slaby Cc: Jiri Slaby, akpm, linux-kernel, ACPI Devel Maling List, Linux-pm mailing list, Matthew Garrett, Len Brown On Thursday, January 06, 2011, Jiri Slaby wrote: > On 01/06/2011 04:57 PM, Rafael J. Wysocki wrote: > > On Thursday, January 06, 2011, Jiri Slaby wrote: > >> When ioremap fails (which might happen for some reason), > > > > If it happens, something is seriously wrong (see below). > > I agree that something is broken, however ioremap may fail for dozen of > reasons. Ignoring the retval is a *bad* idea and it took me a while to > sort out what is wrong. Especially if one has no console like throughout > suspend. If it was handled properly, I would know immediately. (There > should be a message printed out which I forgot to add.) It wasn't handled, because it _never_ failed previously. The ACPI mapping change apparently revealed a deeper problem. I'm not saying the patch isn't useful, though, and I'm going to take it for 2.6.38 (perhaps with minor modifications). > > BTW, to keep things in context, please post fixes like this in the same thread > > in which you reported the problem. At lease please retain the CC list from > > there. > > I actually did, there is: > In-Reply-To: <201101060028.43342.rjw@sisk.pl> > and it successfully threaded to the conversation for me in TB. But you trimmed the CC line, didn't you? Which caused my filter to put the patch into a different folder. :-) > >> we nicely oops in suspend_nvs_save due to NULL dereference by memcpy in > >> there. Fail gracefully instead. > >> > >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> > >> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > >> --- > >> drivers/acpi/sleep.c | 5 ++--- > >> include/linux/suspend.h | 4 ++-- > >> kernel/power/nvs.c | 8 +++++++- > >> 3 files changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > >> index c423231..f94c9a9 100644 > >> --- a/drivers/acpi/sleep.c > >> +++ b/drivers/acpi/sleep.c > >> @@ -124,8 +124,7 @@ static int acpi_pm_freeze(void) > >> static int acpi_pm_pre_suspend(void) > >> { > >> acpi_pm_freeze(); > >> - suspend_nvs_save(); > >> - return 0; > >> + return suspend_nvs_save(); > >> } > >> > >> /** > >> @@ -151,7 +150,7 @@ static int acpi_pm_prepare(void) > >> { > >> int error = __acpi_pm_prepare(); > >> if (!error) > >> - acpi_pm_pre_suspend(); > >> + error = acpi_pm_pre_suspend(); > >> > >> return error; > >> } > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h > >> index c1f4998..3ac2551 100644 > >> --- a/include/linux/suspend.h > >> +++ b/include/linux/suspend.h > >> @@ -262,7 +262,7 @@ static inline bool system_entering_hibernation(void) { return false; } > >> extern int suspend_nvs_register(unsigned long start, unsigned long size); > >> extern int suspend_nvs_alloc(void); > >> extern void suspend_nvs_free(void); > >> -extern void suspend_nvs_save(void); > >> +extern int suspend_nvs_save(void); > >> extern void suspend_nvs_restore(void); > >> #else /* CONFIG_SUSPEND_NVS */ > >> static inline int suspend_nvs_register(unsigned long a, unsigned long b) > >> @@ -271,7 +271,7 @@ static inline int suspend_nvs_register(unsigned long a, unsigned long b) > >> } > >> static inline int suspend_nvs_alloc(void) { return 0; } > >> static inline void suspend_nvs_free(void) {} > >> -static inline void suspend_nvs_save(void) {} > >> +static inline int suspend_nvs_save(void) {} > >> static inline void suspend_nvs_restore(void) {} > >> #endif /* CONFIG_SUSPEND_NVS */ > >> > >> diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c > >> index 1836db6..57c6fab 100644 > >> --- a/kernel/power/nvs.c > >> +++ b/kernel/power/nvs.c > >> @@ -105,7 +105,7 @@ int suspend_nvs_alloc(void) > >> /** > >> * suspend_nvs_save - save NVS memory regions > >> */ > >> -void suspend_nvs_save(void) > >> +int suspend_nvs_save(void) > >> { > >> struct nvs_page *entry; > >> > >> @@ -114,8 +114,14 @@ void suspend_nvs_save(void) > >> list_for_each_entry(entry, &nvs_list, node) > >> if (entry->data) { > >> entry->kaddr = ioremap(entry->phys_start, entry->size); > > > > I wonder what happens if you simply change the ioremap() here to > > ioremap_nocache() without any other modifications? > > ioremap *is* ioremap_nocache on x86. And that's the conflict it > complains about I guess? Don't you mean ioremap_cache? Yes, I meant ioremap_cache(), sorry. Using ioremap_cache() here fixes the problem for Len (he's seeing the same issue on his test machine). The question is why it helps, though. My theory is that we have mapped the same area already using ioremap_cache() and now we're trying to map it again using ioremap_nocache(), hence the conflict. I need to confirm this. > > It _really_ shouldn't fail here, because the NVS pages are known to be present. > > It fails because of conflicting maps as can be seen in the photo. At > least I think so. Yes, I think so too. Which is _suspicious_. Thanks, Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code 2011-01-06 16:38 ` Rafael J. Wysocki @ 2011-01-06 21:01 ` Len Brown 0 siblings, 0 replies; 12+ messages in thread From: Len Brown @ 2011-01-06 21:01 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Jiri Slaby, Jiri Slaby, akpm, linux-kernel, ACPI Devel Maling List, Linux-pm mailing list, Matthew Garrett > ... My theory is that we have mapped the > same area already using ioremap_cache() and now we're trying to map it again > using ioremap_nocache(), hence the conflict. I need to confirm this. On my test box. BIOS-e820: 0000000077626000 - 0000000077632000 (ACPI NVS) yet there are at least two tables (FACS and SSDT) that live in that region, and there are several run-time AML memory opregions residing in that range too. So the region has already been mapped by acpi_os_map_memory() (now using ioremap_cache()) before suspend_nvs_save() runs. Is there a reason that suspend_nvs_save() requires a non-cached mapping? cheers, Len Brown, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-01-06 21:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201012240132.oBO1W8Ub022207@imap1.linux-foundation.org>
[not found] ` <4D232352.2030809@gmail.com>
[not found] ` <4D234F8E.7080102@gmail.com>
2011-01-04 22:57 ` suspend hangs at platform phase [was: mmotm 2010-12-23-16-58 uploaded] Rafael J. Wysocki
2011-01-05 21:36 ` Jiri Slaby
2011-01-05 22:39 ` Rafael J. Wysocki
2011-01-05 22:46 ` Jiri Slaby
2011-01-05 23:28 ` Rafael J. Wysocki
2011-01-06 9:24 ` Jiri Slaby
2011-01-06 15:45 ` Rafael J. Wysocki
[not found] ` <1294305504-5787-1-git-send-email-jslaby@suse.cz>
2011-01-06 15:57 ` [PATCH 1/1] PM: fix oops in suspend/hibernate code Rafael J. Wysocki
2011-01-06 16:09 ` Jiri Slaby
2011-01-06 16:31 ` Jiri Slaby
2011-01-06 16:38 ` Rafael J. Wysocki
2011-01-06 21:01 ` Len Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox