| Message ID | 1350066773-14589-1-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 10/12/2012 11:32 AM, Tim Gardner wrote: > From: Suresh Siddha <suresh.b.siddha@intel.com> > > BugLink: http://bugs.launchpad.net/bugs/1065076 > > During resume, tick_resume_broadcast() programs the broadcast timer in > oneshot mode unconditionally. On the platforms where broadcast timer > is not really required, this will generate spurious broadcast timer > ticks upon resume. For example, on the always running apic timer > platforms with HPET, I see spurious hpet tick once every ~5minutes > (which is the 32-bit hpet counter wraparound time). > > Similar to boot time, during resume make the oneshot mode setting of > the broadcast clock event device conditional on the state of active > broadcast users. > > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> > Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Tested-by: svenjoac@gmx.de > Cc: torvalds@linux-foundation.org > Cc: rjw@sisk.pl > Link: http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > kernel/time/tick-broadcast.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index fd4a7b1..f61d678 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -346,7 +346,8 @@ int tick_resume_broadcast(void) > tick_get_broadcast_mask()); > break; > case TICKDEV_MODE_ONESHOT: > - broadcast = tick_resume_broadcast_oneshot(bc); > + if (!cpumask_empty(tick_get_broadcast_mask())) > + broadcast = tick_resume_broadcast_oneshot(bc); > break; > } > } >
On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: > From: Suresh Siddha <suresh.b.siddha@intel.com> > > BugLink: http://bugs.launchpad.net/bugs/1065076 > > During resume, tick_resume_broadcast() programs the broadcast timer in > oneshot mode unconditionally. On the platforms where broadcast timer > is not really required, this will generate spurious broadcast timer > ticks upon resume. For example, on the always running apic timer > platforms with HPET, I see spurious hpet tick once every ~5minutes > (which is the 32-bit hpet counter wraparound time). > > Similar to boot time, during resume make the oneshot mode setting of > the broadcast clock event device conditional on the state of active > broadcast users. > > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> > Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Tested-by: svenjoac@gmx.de > Cc: torvalds@linux-foundation.org > Cc: rjw@sisk.pl > Link: http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> I'm rather confused. The bug report is about a failure to resume, but the commit message describes fixing a spurious timer interrupt. On the bug the reporter states that upstream kernel 3.6 is still affected, and this patch was merged during 3.4. There's also no verification that I can see that backporting this patch to 3.2 fixes the issue. So how did you arrive at the conclusion that this patch fixes the bug?
On 10/12/2012 01:06 PM, Seth Forshee wrote: > On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: >> From: Suresh Siddha <suresh.b.siddha@intel.com> >> >> BugLink: http://bugs.launchpad.net/bugs/1065076 >> >> During resume, tick_resume_broadcast() programs the broadcast timer in >> oneshot mode unconditionally. On the platforms where broadcast timer >> is not really required, this will generate spurious broadcast timer >> ticks upon resume. For example, on the always running apic timer >> platforms with HPET, I see spurious hpet tick once every ~5minutes >> (which is the 32-bit hpet counter wraparound time). >> >> Similar to boot time, during resume make the oneshot mode setting of >> the broadcast clock event device conditional on the state of active >> broadcast users. >> >> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> >> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Tested-by: svenjoac@gmx.de >> Cc: torvalds@linux-foundation.org >> Cc: rjw@sisk.pl >> Link: http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >> (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) >> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > > I'm rather confused. > > The bug report is about a failure to resume, but the commit message > describes fixing a spurious timer interrupt. On the bug the reporter > states that upstream kernel 3.6 is still affected, and this patch was > merged during 3.4. There's also no verification that I can see that > backporting this patch to 3.2 fixes the issue. > > So how did you arrive at the conclusion that this patch fixes the bug? > Well, I found the patch from the upstream bug report after it was merged in Linus' tree. However, you're right in that it doesn't really seem related (aside from some folks reporting success). I'll get Joe to build a test kernel and harvest the results. rtg
On 10/15/2012 12:05 PM, Tim Gardner wrote: > On 10/12/2012 01:06 PM, Seth Forshee wrote: >> On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: >>> From: Suresh Siddha <suresh.b.siddha@intel.com> >>> >>> BugLink: http://bugs.launchpad.net/bugs/1065076 >>> >>> During resume, tick_resume_broadcast() programs the broadcast timer in >>> oneshot mode unconditionally. On the platforms where broadcast timer >>> is not really required, this will generate spurious broadcast timer >>> ticks upon resume. For example, on the always running apic timer >>> platforms with HPET, I see spurious hpet tick once every ~5minutes >>> (which is the 32-bit hpet counter wraparound time). >>> >>> Similar to boot time, during resume make the oneshot mode setting of >>> the broadcast clock event device conditional on the state of active >>> broadcast users. >>> >>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> >>> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> Tested-by: svenjoac@gmx.de >>> Cc: torvalds@linux-foundation.org >>> Cc: rjw@sisk.pl >>> Link: http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com >>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>> (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) >>> >>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> I'm rather confused. >> >> The bug report is about a failure to resume, but the commit message >> describes fixing a spurious timer interrupt. On the bug the reporter >> states that upstream kernel 3.6 is still affected, and this patch was >> merged during 3.4. There's also no verification that I can see that >> backporting this patch to 3.2 fixes the issue. >> >> So how did you arrive at the conclusion that this patch fixes the bug? >> > Well, I found the patch from the upstream bug report after it was merged > in Linus' tree. However, you're right in that it doesn't really seem > related (aside from some folks reporting success). I'll get Joe to build > a test kernel and harvest the results. > > rtg Will do, Tim.
On 10/15/2012 12:05 PM, Tim Gardner wrote: > On 10/12/2012 01:06 PM, Seth Forshee wrote: >> On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: >>> From: Suresh Siddha <suresh.b.siddha@intel.com> >>> >>> BugLink: http://bugs.launchpad.net/bugs/1065076 >>> >>> During resume, tick_resume_broadcast() programs the broadcast timer in >>> oneshot mode unconditionally. On the platforms where broadcast timer >>> is not really required, this will generate spurious broadcast timer >>> ticks upon resume. For example, on the always running apic timer >>> platforms with HPET, I see spurious hpet tick once every ~5minutes >>> (which is the 32-bit hpet counter wraparound time). >>> >>> Similar to boot time, during resume make the oneshot mode setting of >>> the broadcast clock event device conditional on the state of active >>> broadcast users. >>> >>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> >>> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> Tested-by: svenjoac@gmx.de >>> Cc: torvalds@linux-foundation.org >>> Cc: rjw@sisk.pl >>> Link: http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com >>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>> (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) >>> >>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> I'm rather confused. >> >> The bug report is about a failure to resume, but the commit message >> describes fixing a spurious timer interrupt. On the bug the reporter >> states that upstream kernel 3.6 is still affected, and this patch was >> merged during 3.4. There's also no verification that I can see that >> backporting this patch to 3.2 fixes the issue. >> >> So how did you arrive at the conclusion that this patch fixes the bug? >> > Well, I found the patch from the upstream bug report after it was merged > in Linus' tree. However, you're right in that it doesn't really seem > related (aside from some folks reporting success). I'll get Joe to build > a test kernel and harvest the results. > > rtg It looks like the patch from the upstream bug report is different than upstream commit a6371f8. The patch from the upstream bug report is: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065076/+attachment/3394539/+files/new_s2ram.patch However, this patch hasn't been submitted upstream yet. Would you like me to build a test kernel with this patch? Thanks, Joe
On 10/15/2012 11:09 AM, Joseph Salisbury wrote: > On 10/15/2012 12:05 PM, Tim Gardner wrote: >> On 10/12/2012 01:06 PM, Seth Forshee wrote: >>> On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: >>>> From: Suresh Siddha <suresh.b.siddha@intel.com> >>>> >>>> BugLink: http://bugs.launchpad.net/bugs/1065076 >>>> >>>> During resume, tick_resume_broadcast() programs the broadcast timer in >>>> oneshot mode unconditionally. On the platforms where broadcast timer >>>> is not really required, this will generate spurious broadcast timer >>>> ticks upon resume. For example, on the always running apic timer >>>> platforms with HPET, I see spurious hpet tick once every ~5minutes >>>> (which is the 32-bit hpet counter wraparound time). >>>> >>>> Similar to boot time, during resume make the oneshot mode setting of >>>> the broadcast clock event device conditional on the state of active >>>> broadcast users. >>>> >>>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> >>>> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> Tested-by: svenjoac@gmx.de >>>> Cc: torvalds@linux-foundation.org >>>> Cc: rjw@sisk.pl >>>> Link: >>>> http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com >>>> >>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>>> (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) >>>> >>>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >>> I'm rather confused. >>> >>> The bug report is about a failure to resume, but the commit message >>> describes fixing a spurious timer interrupt. On the bug the reporter >>> states that upstream kernel 3.6 is still affected, and this patch was >>> merged during 3.4. There's also no verification that I can see that >>> backporting this patch to 3.2 fixes the issue. >>> >>> So how did you arrive at the conclusion that this patch fixes the bug? >>> >> Well, I found the patch from the upstream bug report after it was merged >> in Linus' tree. However, you're right in that it doesn't really seem >> related (aside from some folks reporting success). I'll get Joe to build >> a test kernel and harvest the results. >> >> rtg > It looks like the patch from the upstream bug report is different than > upstream commit a6371f8. The patch from the upstream bug report is: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065076/+attachment/3394539/+files/new_s2ram.patch > > > However, this patch hasn't been submitted upstream yet. Would you like > me to build a test kernel with this patch? > > Thanks, > > Joe > Lets try with the cherry-picked patch first (a6371f80230eaaafd7eef7efeedaa9509bdc982d)
On 10/15/2012 01:43 PM, Tim Gardner wrote: > On 10/15/2012 11:09 AM, Joseph Salisbury wrote: >> On 10/15/2012 12:05 PM, Tim Gardner wrote: >>> On 10/12/2012 01:06 PM, Seth Forshee wrote: >>>> On Fri, Oct 12, 2012 at 12:32:53PM -0600, Tim Gardner wrote: >>>>> From: Suresh Siddha <suresh.b.siddha@intel.com> >>>>> >>>>> BugLink: http://bugs.launchpad.net/bugs/1065076 >>>>> >>>>> During resume, tick_resume_broadcast() programs the broadcast >>>>> timer in >>>>> oneshot mode unconditionally. On the platforms where broadcast timer >>>>> is not really required, this will generate spurious broadcast timer >>>>> ticks upon resume. For example, on the always running apic timer >>>>> platforms with HPET, I see spurious hpet tick once every ~5minutes >>>>> (which is the 32-bit hpet counter wraparound time). >>>>> >>>>> Similar to boot time, during resume make the oneshot mode setting of >>>>> the broadcast clock event device conditional on the state of active >>>>> broadcast users. >>>>> >>>>> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com> >>>>> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>>> Tested-by: svenjoac@gmx.de >>>>> Cc: torvalds@linux-foundation.org >>>>> Cc: rjw@sisk.pl >>>>> Link: >>>>> http://lkml.kernel.org/r/1334802459.28674.209.camel@sbsiddha-desk.sc.intel.com >>>>> >>>>> >>>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>>>> (cherry picked from commit a6371f80230eaaafd7eef7efeedaa9509bdc982d) >>>>> >>>>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >>>> I'm rather confused. >>>> >>>> The bug report is about a failure to resume, but the commit message >>>> describes fixing a spurious timer interrupt. On the bug the reporter >>>> states that upstream kernel 3.6 is still affected, and this patch was >>>> merged during 3.4. There's also no verification that I can see that >>>> backporting this patch to 3.2 fixes the issue. >>>> >>>> So how did you arrive at the conclusion that this patch fixes the bug? >>>> >>> Well, I found the patch from the upstream bug report after it was >>> merged >>> in Linus' tree. However, you're right in that it doesn't really seem >>> related (aside from some folks reporting success). I'll get Joe to >>> build >>> a test kernel and harvest the results. >>> >>> rtg >> It looks like the patch from the upstream bug report is different than >> upstream commit a6371f8. The patch from the upstream bug report is: >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065076/+attachment/3394539/+files/new_s2ram.patch >> >> >> >> However, this patch hasn't been submitted upstream yet. Would you like >> me to build a test kernel with this patch? >> >> Thanks, >> >> Joe >> > > Lets try with the cherry-picked patch first > (a6371f80230eaaafd7eef7efeedaa9509bdc982d) > Ok. I'll let you know the testing results.
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index fd4a7b1..f61d678 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -346,7 +346,8 @@ int tick_resume_broadcast(void) tick_get_broadcast_mask()); break; case TICKDEV_MODE_ONESHOT: - broadcast = tick_resume_broadcast_oneshot(bc); + if (!cpumask_empty(tick_get_broadcast_mask())) + broadcast = tick_resume_broadcast_oneshot(bc); break; } }