* buggy usage of pm_idle by omap1 code @ 2011-12-17 3:24 Nicolas Pitre 2011-12-19 17:35 ` Tony Lindgren 0 siblings, 1 reply; 7+ messages in thread From: Nicolas Pitre @ 2011-12-17 3:24 UTC (permalink / raw) To: linux-arm-kernel I just noticed in mach-omap1/cpu.c:omap_pm_prepare() that the function pointer pm_idle is explicitly set to NULL for some vague reason. However this pointer is unconditionally dereferenced in kernel/process.c:cpu_idle(). This is just a oops waiting to happen. And if it doesn't happen in practice, then the code in omap_pm_prepare() must be useless in addition to being wrong. Nicolas ^ permalink raw reply [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-17 3:24 buggy usage of pm_idle by omap1 code Nicolas Pitre @ 2011-12-19 17:35 ` Tony Lindgren 2011-12-19 17:44 ` Nicolas Pitre 2011-12-19 21:34 ` Russell King - ARM Linux 0 siblings, 2 replies; 7+ messages in thread From: Tony Lindgren @ 2011-12-19 17:35 UTC (permalink / raw) To: linux-arm-kernel * Nicolas Pitre <nico@fluxnic.net> [111216 18:53]: > > I just noticed in mach-omap1/cpu.c:omap_pm_prepare() that the function > pointer pm_idle is explicitly set to NULL for some vague reason. > However this pointer is unconditionally dereferenced in > kernel/process.c:cpu_idle(). This is just a oops waiting to happen. > And if it doesn't happen in practice, then the code in omap_pm_prepare() > must be useless in addition to being wrong. Good catch. Here's a patch to deal with that issue, want to take this into your idle series? I'll try out your idle series today at some point. Regards, Tony From: Tony Lindgren <tony@atomide.com> Date: Mon, 19 Dec 2011 09:11:11 -0800 Subject: [PATCH] ARM: OMAP1: Fix pm_idle during suspend Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: clean up pm_idle calling, obey hlt_counter) removed a check for NULL pm_idle. On omap1 the system hits an equivalent of suspend during idle. If we are suspending, we need to make sure so we don't call omap1_pm_idle during the suspend process. Fix this by setting the pm_idle function to a an empty function while suspending. Reported-by: Nicolas Pitre <nico@fluxnic.net> Signed-off-by: Tony Lindgren <tony@atomide.com> diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c index 89ea20c..b551a62 100644 --- a/arch/arm/mach-omap1/pm.c +++ b/arch/arm/mach-omap1/pm.c @@ -584,6 +584,9 @@ static void omap_pm_init_proc(void) #endif /* DEBUG && CONFIG_PROC_FS */ static void (*saved_idle)(void) = NULL; +static void omap1_dummy_idle(void) +{ +} /* * omap_pm_prepare - Do preliminary suspend work. @@ -593,7 +596,7 @@ static int omap_pm_prepare(void) { /* We cannot sleep in idle until we have resumed */ saved_idle = pm_idle; - pm_idle = NULL; + pm_idle = omap1_dummy_idle; return 0; } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-19 17:35 ` Tony Lindgren @ 2011-12-19 17:44 ` Nicolas Pitre 2011-12-19 21:34 ` Russell King - ARM Linux 1 sibling, 0 replies; 7+ messages in thread From: Nicolas Pitre @ 2011-12-19 17:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, 19 Dec 2011, Tony Lindgren wrote: > * Nicolas Pitre <nico@fluxnic.net> [111216 18:53]: > > > > I just noticed in mach-omap1/cpu.c:omap_pm_prepare() that the function > > pointer pm_idle is explicitly set to NULL for some vague reason. > > However this pointer is unconditionally dereferenced in > > kernel/process.c:cpu_idle(). This is just a oops waiting to happen. > > And if it doesn't happen in practice, then the code in omap_pm_prepare() > > must be useless in addition to being wrong. > > Good catch. Here's a patch to deal with that issue, want to take this > into your idle series? Sure. > I'll try out your idle series today at some > point. Thanks. Nicolas ^ permalink raw reply [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-19 17:35 ` Tony Lindgren 2011-12-19 17:44 ` Nicolas Pitre @ 2011-12-19 21:34 ` Russell King - ARM Linux 2011-12-19 23:04 ` Nicolas Pitre 2011-12-19 23:20 ` Tony Lindgren 1 sibling, 2 replies; 7+ messages in thread From: Russell King - ARM Linux @ 2011-12-19 21:34 UTC (permalink / raw) To: linux-arm-kernel On Mon, Dec 19, 2011 at 09:35:19AM -0800, Tony Lindgren wrote: > Good catch. Here's a patch to deal with that issue, want to take this > into your idle series? I'll try out your idle series today at some > point. Two points: 1. It's good practice to call cpu_idle_wait() after modifying pm_idle, even on UP - it allows static checking of this. > From: Tony Lindgren <tony@atomide.com> > Date: Mon, 19 Dec 2011 09:11:11 -0800 > Subject: [PATCH] ARM: OMAP1: Fix pm_idle during suspend > > Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: > clean up pm_idle calling, obey hlt_counter) removed a check > for NULL pm_idle. > > On omap1 the system hits an equivalent of suspend during idle. > If we are suspending, we need to make sure so we don't call > omap1_pm_idle during the suspend process. > > Fix this by setting the pm_idle function to a an empty > function while suspending. 2. what's wrong with calling disable_hlt() to prevent the pm_idle function being called? Why solve it this complicated way of changing the pm_idle pointer when there's some infrastructure which can fix it for you already? > > Reported-by: Nicolas Pitre <nico@fluxnic.net> > Signed-off-by: Tony Lindgren <tony@atomide.com> > > diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c > index 89ea20c..b551a62 100644 > --- a/arch/arm/mach-omap1/pm.c > +++ b/arch/arm/mach-omap1/pm.c > @@ -584,6 +584,9 @@ static void omap_pm_init_proc(void) > #endif /* DEBUG && CONFIG_PROC_FS */ > > static void (*saved_idle)(void) = NULL; > +static void omap1_dummy_idle(void) > +{ > +} > > /* > * omap_pm_prepare - Do preliminary suspend work. > @@ -593,7 +596,7 @@ static int omap_pm_prepare(void) > { > /* We cannot sleep in idle until we have resumed */ > saved_idle = pm_idle; > - pm_idle = NULL; > + pm_idle = omap1_dummy_idle; > > return 0; > } > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-19 21:34 ` Russell King - ARM Linux @ 2011-12-19 23:04 ` Nicolas Pitre 2011-12-19 23:22 ` Tony Lindgren 2011-12-19 23:20 ` Tony Lindgren 1 sibling, 1 reply; 7+ messages in thread From: Nicolas Pitre @ 2011-12-19 23:04 UTC (permalink / raw) To: linux-arm-kernel On Mon, 19 Dec 2011, Russell King - ARM Linux wrote: > On Mon, Dec 19, 2011 at 09:35:19AM -0800, Tony Lindgren wrote: > > Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: > > clean up pm_idle calling, obey hlt_counter) removed a check > > for NULL pm_idle. > > > > On omap1 the system hits an equivalent of suspend during idle. > > If we are suspending, we need to make sure so we don't call > > omap1_pm_idle during the suspend process. > > > > Fix this by setting the pm_idle function to a an empty > > function while suspending. > > 2. what's wrong with calling disable_hlt() to prevent the pm_idle function > being called? Why solve it this complicated way of changing the pm_idle > pointer when there's some infrastructure which can fix it for you already? Absolutely. I must have been stairing at the whole thing for too long, as I obviously did exactly that already elsewhere. So I propose this alternative: ARM: OMAP1: Fix pm_idle during suspend Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: clean up pm_idle calling, obey hlt_counter) removed a check for NULL pm_idle. Replace the NULL assignment in the OMAP1 code with disable_hlt() to be in sync with the core code and provide the intended behavior. Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c index 89ea20ca0c..6c6a2dc554 100644 --- a/arch/arm/mach-omap1/pm.c +++ b/arch/arm/mach-omap1/pm.c @@ -583,8 +583,6 @@ static void omap_pm_init_proc(void) #endif /* DEBUG && CONFIG_PROC_FS */ -static void (*saved_idle)(void) = NULL; - /* * omap_pm_prepare - Do preliminary suspend work. * @@ -592,8 +590,7 @@ static void (*saved_idle)(void) = NULL; static int omap_pm_prepare(void) { /* We cannot sleep in idle until we have resumed */ - saved_idle = pm_idle; - pm_idle = NULL; + disable_hlt(); return 0; } @@ -630,7 +627,7 @@ static int omap_pm_enter(suspend_state_t state) static void omap_pm_finish(void) { - pm_idle = saved_idle; + enable_hlt(); } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-19 23:04 ` Nicolas Pitre @ 2011-12-19 23:22 ` Tony Lindgren 0 siblings, 0 replies; 7+ messages in thread From: Tony Lindgren @ 2011-12-19 23:22 UTC (permalink / raw) To: linux-arm-kernel * Nicolas Pitre <nico@fluxnic.net> [111219 14:32]: > On Mon, 19 Dec 2011, Russell King - ARM Linux wrote: > > > On Mon, Dec 19, 2011 at 09:35:19AM -0800, Tony Lindgren wrote: > > > Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: > > > clean up pm_idle calling, obey hlt_counter) removed a check > > > for NULL pm_idle. > > > > > > On omap1 the system hits an equivalent of suspend during idle. > > > If we are suspending, we need to make sure so we don't call > > > omap1_pm_idle during the suspend process. > > > > > > Fix this by setting the pm_idle function to a an empty > > > function while suspending. > > > > 2. what's wrong with calling disable_hlt() to prevent the pm_idle function > > being called? Why solve it this complicated way of changing the pm_idle > > pointer when there's some infrastructure which can fix it for you already? > > Absolutely. I must have been stairing at the whole thing for too long, > as I obviously did exactly that already elsewhere. > > So I propose this alternative: > > ARM: OMAP1: Fix pm_idle during suspend > > Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: > clean up pm_idle calling, obey hlt_counter) removed a check > for NULL pm_idle. > > Replace the NULL assignment in the OMAP1 code with disable_hlt() > to be in sync with the core code and provide the intended behavior. > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> Yeh this is better. Looks like you beat me to it by few minutes: Acked-by: Tony Lindgren <tony@atomide.com> > diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c > index 89ea20ca0c..6c6a2dc554 100644 > --- a/arch/arm/mach-omap1/pm.c > +++ b/arch/arm/mach-omap1/pm.c > @@ -583,8 +583,6 @@ static void omap_pm_init_proc(void) > > #endif /* DEBUG && CONFIG_PROC_FS */ > > -static void (*saved_idle)(void) = NULL; > - > /* > * omap_pm_prepare - Do preliminary suspend work. > * > @@ -592,8 +590,7 @@ static void (*saved_idle)(void) = NULL; > static int omap_pm_prepare(void) > { > /* We cannot sleep in idle until we have resumed */ > - saved_idle = pm_idle; > - pm_idle = NULL; > + disable_hlt(); > > return 0; > } > @@ -630,7 +627,7 @@ static int omap_pm_enter(suspend_state_t state) > > static void omap_pm_finish(void) > { > - pm_idle = saved_idle; > + enable_hlt(); > } > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* buggy usage of pm_idle by omap1 code 2011-12-19 21:34 ` Russell King - ARM Linux 2011-12-19 23:04 ` Nicolas Pitre @ 2011-12-19 23:20 ` Tony Lindgren 1 sibling, 0 replies; 7+ messages in thread From: Tony Lindgren @ 2011-12-19 23:20 UTC (permalink / raw) To: linux-arm-kernel * Russell King - ARM Linux <linux@arm.linux.org.uk> [111219 13:02]: > On Mon, Dec 19, 2011 at 09:35:19AM -0800, Tony Lindgren wrote: > > Good catch. Here's a patch to deal with that issue, want to take this > > into your idle series? I'll try out your idle series today at some > > point. > > Two points: > > 1. It's good practice to call cpu_idle_wait() after modifying pm_idle, > even on UP - it allows static checking of this. Hmm, looks like we're not using that anywhere right now. Should we have a function or macro set_arm_idle() that does that automatically? > > From: Tony Lindgren <tony@atomide.com> > > Date: Mon, 19 Dec 2011 09:11:11 -0800 > > Subject: [PATCH] ARM: OMAP1: Fix pm_idle during suspend > > > > Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: > > clean up pm_idle calling, obey hlt_counter) removed a check > > for NULL pm_idle. > > > > On omap1 the system hits an equivalent of suspend during idle. > > If we are suspending, we need to make sure so we don't call > > omap1_pm_idle during the suspend process. > > > > Fix this by setting the pm_idle function to a an empty > > function while suspending. > > 2. what's wrong with calling disable_hlt() to prevent the pm_idle function > being called? Why solve it this complicated way of changing the pm_idle > pointer when there's some infrastructure which can fix it for you already? Thanks that allows removal of few lines of code. Updated patch below. Regards, Tony From: Tony Lindgren <tony@atomide.com> Date: Mon, 19 Dec 2011 09:11:11 -0800 Subject: [PATCH] Subject: [PATCH] ARM: OMAP1: Fix pm_idle during suspend Commit 9ccdac3662dbf3c75e8f8851a214bdf7d365a4bd ([ARM] idle: clean up pm_idle calling, obey hlt_counter) removed a check for NULL pm_idle. On omap1 the system hits an equivalent of suspend during idle. If we are suspending, we need to make sure so we don't call omap1_pm_idle during the suspend process. Fix this by using disable_hlt and enable_hlt for the suspend as pointed out by Russell King <linux@arm.linux.org.uk>. This allows us to remove the saved_idle. Reported-by: Nicolas Pitre <nico@fluxnic.net> Signed-off-by: Tony Lindgren <tony@atomide.com> --- a/arch/arm/mach-omap1/pm.c +++ b/arch/arm/mach-omap1/pm.c @@ -583,8 +583,6 @@ static void omap_pm_init_proc(void) #endif /* DEBUG && CONFIG_PROC_FS */ -static void (*saved_idle)(void) = NULL; - /* * omap_pm_prepare - Do preliminary suspend work. * @@ -592,8 +590,7 @@ static void (*saved_idle)(void) = NULL; static int omap_pm_prepare(void) { /* We cannot sleep in idle until we have resumed */ - saved_idle = pm_idle; - pm_idle = NULL; + disable_hlt(); return 0; } @@ -630,7 +627,7 @@ static int omap_pm_enter(suspend_state_t state) static void omap_pm_finish(void) { - pm_idle = saved_idle; + enable_hlt(); } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-12-19 23:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-17 3:24 buggy usage of pm_idle by omap1 code Nicolas Pitre 2011-12-19 17:35 ` Tony Lindgren 2011-12-19 17:44 ` Nicolas Pitre 2011-12-19 21:34 ` Russell King - ARM Linux 2011-12-19 23:04 ` Nicolas Pitre 2011-12-19 23:22 ` Tony Lindgren 2011-12-19 23:20 ` Tony Lindgren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).