| Message ID | 1296235076-2570-1-git-send-email-ricardo.salveti@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Andy Whitcroft |
| Headers | show |
On 01/28/2011 10:17 AM, Ricardo Salveti de Araujo wrote: > In case in user has a OMAP3630< ES1.2 the kernel should warn the user > about the ERRATUM, but using pr_warning instead of WARN_ON is already > enough, as there is nothing else the user can do besides changing the > board. > > With this we avoid having the following calltrace while booting with some > Beagle xM revisions: > WARNING: at /home/apw/build/ubuntu-natty2/ubuntu-natty2/arch/arm/mach-omap2/cpuidle34xx.c:468 omap_init_power_states+0x230/0x238() > omap_init_power_states: core off state C7 disabled due to i583 > Modules linked in: > [<c00596f4>] (unwind_backtrace+0x0/0xfc) from [<c04f29a8>] (dump_stack+0x18/0x1c) > [<c04f29a8>] (dump_stack+0x18/0x1c) from [<c008510c>] (warn_slowpath_common+0x5c/0x6c) > [<c008510c>] (warn_slowpath_common+0x5c/0x6c) from [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) > [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) from [<c00676f4>] (omap_init_power_states+0x230/0x238) > [<c00676f4>] (omap_init_power_states+0x230/0x238) from [<c00131a0>] (omap3_idle_init+0x74/0x18c) > [<c00131a0>] (omap3_idle_init+0x74/0x18c) from [<c00126b4>] (omap3_pm_init+0x1ac/0x308) > [<c00126b4>] (omap3_pm_init+0x1ac/0x308) from [<c00474c0>] (do_one_initcall+0x3c/0x1b4) > [<c00474c0>] (do_one_initcall+0x3c/0x1b4) from [<c0008d58>] (kernel_init+0xe0/0x178) > [<c0008d58>] (kernel_init+0xe0/0x178) from [<c00532c8>] (kernel_thread_exit+0x0/0x8) > ---[ end trace e639b107cbbc60f1 ]--- > > Signed-off-by: Ricardo Salveti de Araujo<ricardo.salveti@canonical.com> > --- > arch/arm/mach-omap2/cpuidle34xx.c | 2 +- > arch/arm/mach-omap2/pm34xx.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c > index f7b22a1..54ec28e 100644 > --- a/arch/arm/mach-omap2/cpuidle34xx.c > +++ b/arch/arm/mach-omap2/cpuidle34xx.c > @@ -464,7 +464,7 @@ void omap_init_power_states(void) > if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583)) { > omap3_power_states[OMAP3_STATE_C7].valid = 0; > cpuidle_params_table[OMAP3_STATE_C7].valid = 0; > - WARN_ONCE(1, "%s: core off state C7 disabled due to i583\n", > + pr_warning("%s: core off state C7 disabled due to i583\n", > __func__); > } > } > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c > index 8cbbeade..d1ad1c1 100644 > --- a/arch/arm/mach-omap2/pm34xx.c > +++ b/arch/arm/mach-omap2/pm34xx.c > @@ -927,8 +927,7 @@ void omap3_pm_off_mode_enable(int enable) > pwrst->pwrdm == core_pwrdm&& > state == PWRDM_POWER_OFF) { > pwrst->next_state = PWRDM_POWER_RET; > - WARN_ONCE(1, > - "%s: Core OFF disabled due to errata i583\n", > + pr_warning("%s: Core OFF disabled due to errata i583\n", > __func__); > } else { > pwrst->next_state = state; Is this really necessary ? While the WARN_ONCE() is a bit more verbose, it has no deleterious impact. We'd rather not carry any more delta from upstream then we have to. rtg
On Sat, Jan 29, 2011 at 4:42 AM, Tim Gardner <tim.gardner@canonical.com> wrote: > On 01/28/2011 10:17 AM, Ricardo Salveti de Araujo wrote: >> >> In case in user has a OMAP3630< ES1.2 the kernel should warn the user >> about the ERRATUM, but using pr_warning instead of WARN_ON is already >> enough, as there is nothing else the user can do besides changing the >> board. >> >> With this we avoid having the following calltrace while booting with some >> Beagle xM revisions: >> WARNING: at >> /home/apw/build/ubuntu-natty2/ubuntu-natty2/arch/arm/mach-omap2/cpuidle34xx.c:468 >> omap_init_power_states+0x230/0x238() >> omap_init_power_states: core off state C7 disabled due to i583 >> Modules linked in: >> [<c00596f4>] (unwind_backtrace+0x0/0xfc) from [<c04f29a8>] >> (dump_stack+0x18/0x1c) >> [<c04f29a8>] (dump_stack+0x18/0x1c) from [<c008510c>] >> (warn_slowpath_common+0x5c/0x6c) >> [<c008510c>] (warn_slowpath_common+0x5c/0x6c) from [<c00851c0>] >> (warn_slowpath_fmt+0x38/0x40) >> [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) from [<c00676f4>] >> (omap_init_power_states+0x230/0x238) >> [<c00676f4>] (omap_init_power_states+0x230/0x238) from [<c00131a0>] >> (omap3_idle_init+0x74/0x18c) >> [<c00131a0>] (omap3_idle_init+0x74/0x18c) from [<c00126b4>] >> (omap3_pm_init+0x1ac/0x308) >> [<c00126b4>] (omap3_pm_init+0x1ac/0x308) from [<c00474c0>] >> (do_one_initcall+0x3c/0x1b4) >> [<c00474c0>] (do_one_initcall+0x3c/0x1b4) from [<c0008d58>] >> (kernel_init+0xe0/0x178) >> [<c0008d58>] (kernel_init+0xe0/0x178) from [<c00532c8>] >> (kernel_thread_exit+0x0/0x8) >> ---[ end trace e639b107cbbc60f1 ]--- >> >> Signed-off-by: Ricardo Salveti de Araujo<ricardo.salveti@canonical.com> >> --- >> arch/arm/mach-omap2/cpuidle34xx.c | 2 +- >> arch/arm/mach-omap2/pm34xx.c | 3 +-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c >> b/arch/arm/mach-omap2/cpuidle34xx.c >> index f7b22a1..54ec28e 100644 >> --- a/arch/arm/mach-omap2/cpuidle34xx.c >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c >> @@ -464,7 +464,7 @@ void omap_init_power_states(void) >> if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583)) { >> omap3_power_states[OMAP3_STATE_C7].valid = 0; >> cpuidle_params_table[OMAP3_STATE_C7].valid = 0; >> - WARN_ONCE(1, "%s: core off state C7 disabled due to >> i583\n", >> + pr_warning("%s: core off state C7 disabled due to i583\n", >> __func__); >> } >> } >> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c >> index 8cbbeade..d1ad1c1 100644 >> --- a/arch/arm/mach-omap2/pm34xx.c >> +++ b/arch/arm/mach-omap2/pm34xx.c >> @@ -927,8 +927,7 @@ void omap3_pm_off_mode_enable(int enable) >> pwrst->pwrdm == core_pwrdm&& >> state == PWRDM_POWER_OFF) { >> pwrst->next_state = PWRDM_POWER_RET; >> - WARN_ONCE(1, >> - "%s: Core OFF disabled due to errata >> i583\n", >> + pr_warning("%s: Core OFF disabled due to errata >> i583\n", >> __func__); >> } else { >> pwrst->next_state = state; > > Is this really necessary ? While the WARN_ONCE() is a bit more verbose, it > has no deleterious impact. We'd rather not carry any more delta from > upstream then we have to. > I agree here, how about send it out to upstream if it is very annoy to our end users? We can easy to cherry-pick it back then. Thanks,
On Sun, Jan 30, 2011 at 10:04 AM, Bryan Wu <bryan.wu@canonical.com> wrote: > On Sat, Jan 29, 2011 at 4:42 AM, Tim Gardner <tim.gardner@canonical.com> wrote: >> On 01/28/2011 10:17 AM, Ricardo Salveti de Araujo wrote: >>> >>> In case in user has a OMAP3630< ES1.2 the kernel should warn the user >>> about the ERRATUM, but using pr_warning instead of WARN_ON is already >>> enough, as there is nothing else the user can do besides changing the >>> board. >>> >>> With this we avoid having the following calltrace while booting with some >>> Beagle xM revisions: >>> WARNING: at >>> /home/apw/build/ubuntu-natty2/ubuntu-natty2/arch/arm/mach-omap2/cpuidle34xx.c:468 >>> omap_init_power_states+0x230/0x238() >>> omap_init_power_states: core off state C7 disabled due to i583 >>> Modules linked in: >>> [<c00596f4>] (unwind_backtrace+0x0/0xfc) from [<c04f29a8>] >>> (dump_stack+0x18/0x1c) >>> [<c04f29a8>] (dump_stack+0x18/0x1c) from [<c008510c>] >>> (warn_slowpath_common+0x5c/0x6c) >>> [<c008510c>] (warn_slowpath_common+0x5c/0x6c) from [<c00851c0>] >>> (warn_slowpath_fmt+0x38/0x40) >>> [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) from [<c00676f4>] >>> (omap_init_power_states+0x230/0x238) >>> [<c00676f4>] (omap_init_power_states+0x230/0x238) from [<c00131a0>] >>> (omap3_idle_init+0x74/0x18c) >>> [<c00131a0>] (omap3_idle_init+0x74/0x18c) from [<c00126b4>] >>> (omap3_pm_init+0x1ac/0x308) >>> [<c00126b4>] (omap3_pm_init+0x1ac/0x308) from [<c00474c0>] >>> (do_one_initcall+0x3c/0x1b4) >>> [<c00474c0>] (do_one_initcall+0x3c/0x1b4) from [<c0008d58>] >>> (kernel_init+0xe0/0x178) >>> [<c0008d58>] (kernel_init+0xe0/0x178) from [<c00532c8>] >>> (kernel_thread_exit+0x0/0x8) >>> ---[ end trace e639b107cbbc60f1 ]--- >>> >>> Signed-off-by: Ricardo Salveti de Araujo<ricardo.salveti@canonical.com> >>> --- >>> arch/arm/mach-omap2/cpuidle34xx.c | 2 +- >>> arch/arm/mach-omap2/pm34xx.c | 3 +-- >>> 2 files changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c >>> b/arch/arm/mach-omap2/cpuidle34xx.c >>> index f7b22a1..54ec28e 100644 >>> --- a/arch/arm/mach-omap2/cpuidle34xx.c >>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c >>> @@ -464,7 +464,7 @@ void omap_init_power_states(void) >>> if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583)) { >>> omap3_power_states[OMAP3_STATE_C7].valid = 0; >>> cpuidle_params_table[OMAP3_STATE_C7].valid = 0; >>> - WARN_ONCE(1, "%s: core off state C7 disabled due to >>> i583\n", >>> + pr_warning("%s: core off state C7 disabled due to i583\n", >>> __func__); >>> } >>> } >>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c >>> index 8cbbeade..d1ad1c1 100644 >>> --- a/arch/arm/mach-omap2/pm34xx.c >>> +++ b/arch/arm/mach-omap2/pm34xx.c >>> @@ -927,8 +927,7 @@ void omap3_pm_off_mode_enable(int enable) >>> pwrst->pwrdm == core_pwrdm&& >>> state == PWRDM_POWER_OFF) { >>> pwrst->next_state = PWRDM_POWER_RET; >>> - WARN_ONCE(1, >>> - "%s: Core OFF disabled due to errata >>> i583\n", >>> + pr_warning("%s: Core OFF disabled due to errata >>> i583\n", >>> __func__); >>> } else { >>> pwrst->next_state = state; >> >> Is this really necessary ? While the WARN_ONCE() is a bit more verbose, it >> has no deleterious impact. We'd rather not carry any more delta from >> upstream then we have to. >> > > I agree here, how about send it out to upstream if it is very annoy to > our end users? > We can easy to cherry-pick it back then. Already did, just need to do a minor change and it'll probably be accepted. Cheers,
On Fri, Jan 28, 2011 at 01:42:25PM -0700, Tim Gardner wrote: > Is this really necessary ? While the WARN_ONCE() is a bit more > verbose, it has no deleterious impact. We'd rather not carry any > more delta from upstream then we have to. I am inclined to suggest this is a good thing as we will be triggering apport for something which essentially is unfixable. I suspect that as this will also trigger kerneloops that upstream will take the change. Indeed, it sounds like it will go upstream from the rest of the thread. -apw
On Sun, Jan 30, 2011 at 2:07 PM, Andy Whitcroft <apw@canonical.com> wrote: > On Fri, Jan 28, 2011 at 01:42:25PM -0700, Tim Gardner wrote: > >> Is this really necessary ? While the WARN_ONCE() is a bit more >> verbose, it has no deleterious impact. We'd rather not carry any >> more delta from upstream then we have to. > > I am inclined to suggest this is a good thing as we will be triggering > apport for something which essentially is unfixable. I suspect that as > this will also trigger kerneloops that upstream will take the change. > Indeed, it sounds like it will go upstream from the rest of the thread. Patch queued at Kevin Hilman's for_2.6.39/pm-misc branch, so it should be upstream soon, but not for 38. Cheers,
On 01/31/2011 08:24 PM, Ricardo Salveti wrote: > On Sun, Jan 30, 2011 at 2:07 PM, Andy Whitcroft<apw@canonical.com> wrote: >> On Fri, Jan 28, 2011 at 01:42:25PM -0700, Tim Gardner wrote: >> >>> Is this really necessary ? While the WARN_ONCE() is a bit more >>> verbose, it has no deleterious impact. We'd rather not carry any >>> more delta from upstream then we have to. >> >> I am inclined to suggest this is a good thing as we will be triggering >> apport for something which essentially is unfixable. I suspect that as >> this will also trigger kerneloops that upstream will take the change. >> Indeed, it sounds like it will go upstream from the rest of the thread. > > Patch queued at Kevin Hilman's for_2.6.39/pm-misc branch, so it should > be upstream soon, but not for 38. > > Cheers, I agree with Andy's reasoning. Applied and pushed
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index f7b22a1..54ec28e 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -464,7 +464,7 @@ void omap_init_power_states(void) if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583)) { omap3_power_states[OMAP3_STATE_C7].valid = 0; cpuidle_params_table[OMAP3_STATE_C7].valid = 0; - WARN_ONCE(1, "%s: core off state C7 disabled due to i583\n", + pr_warning("%s: core off state C7 disabled due to i583\n", __func__); } } diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c index 8cbbeade..d1ad1c1 100644 --- a/arch/arm/mach-omap2/pm34xx.c +++ b/arch/arm/mach-omap2/pm34xx.c @@ -927,8 +927,7 @@ void omap3_pm_off_mode_enable(int enable) pwrst->pwrdm == core_pwrdm && state == PWRDM_POWER_OFF) { pwrst->next_state = PWRDM_POWER_RET; - WARN_ONCE(1, - "%s: Core OFF disabled due to errata i583\n", + pr_warning("%s: Core OFF disabled due to errata i583\n", __func__); } else { pwrst->next_state = state;
In case in user has a OMAP3630 < ES1.2 the kernel should warn the user about the ERRATUM, but using pr_warning instead of WARN_ON is already enough, as there is nothing else the user can do besides changing the board. With this we avoid having the following calltrace while booting with some Beagle xM revisions: WARNING: at /home/apw/build/ubuntu-natty2/ubuntu-natty2/arch/arm/mach-omap2/cpuidle34xx.c:468 omap_init_power_states+0x230/0x238() omap_init_power_states: core off state C7 disabled due to i583 Modules linked in: [<c00596f4>] (unwind_backtrace+0x0/0xfc) from [<c04f29a8>] (dump_stack+0x18/0x1c) [<c04f29a8>] (dump_stack+0x18/0x1c) from [<c008510c>] (warn_slowpath_common+0x5c/0x6c) [<c008510c>] (warn_slowpath_common+0x5c/0x6c) from [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) [<c00851c0>] (warn_slowpath_fmt+0x38/0x40) from [<c00676f4>] (omap_init_power_states+0x230/0x238) [<c00676f4>] (omap_init_power_states+0x230/0x238) from [<c00131a0>] (omap3_idle_init+0x74/0x18c) [<c00131a0>] (omap3_idle_init+0x74/0x18c) from [<c00126b4>] (omap3_pm_init+0x1ac/0x308) [<c00126b4>] (omap3_pm_init+0x1ac/0x308) from [<c00474c0>] (do_one_initcall+0x3c/0x1b4) [<c00474c0>] (do_one_initcall+0x3c/0x1b4) from [<c0008d58>] (kernel_init+0xe0/0x178) [<c0008d58>] (kernel_init+0xe0/0x178) from [<c00532c8>] (kernel_thread_exit+0x0/0x8) ---[ end trace e639b107cbbc60f1 ]--- Signed-off-by: Ricardo Salveti de Araujo <ricardo.salveti@canonical.com> --- arch/arm/mach-omap2/cpuidle34xx.c | 2 +- arch/arm/mach-omap2/pm34xx.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)