Patchwork [Precise,SRU] tick: Fix the spurious broadcast timer ticks after resume

login
register
mail settings
Submitter Tim Gardner
Date Oct. 12, 2012, 6:32 p.m.
Message ID <1350066773-14589-1-git-send-email-tim.gardner@canonical.com>
Download mbox | patch
Permalink /patch/191176/
State New
Headers show

Comments

Tim Gardner - Oct. 12, 2012, 6:32 p.m.
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(-)
Brad Figg - Oct. 12, 2012, 6:54 p.m.
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;
>  		}
>  	}
>
Seth Forshee - Oct. 12, 2012, 7:06 p.m.
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?
Tim Gardner - Oct. 15, 2012, 4:05 p.m.
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
Joseph Salisbury - Oct. 15, 2012, 4:52 p.m.
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.
Joseph Salisbury - Oct. 15, 2012, 5:09 p.m.
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
Tim Gardner - Oct. 15, 2012, 5:43 p.m.
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)
Joseph Salisbury - Oct. 15, 2012, 5:46 p.m.
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.

Patch

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;
 		}
 	}