[PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Santosh Shilimkar]

Commit 77b0d60{clockevents: Leave the broadcast device in shutdown mode
when not needed} was intended to leave the broadcast device in shutdown mode
when the per-cpu clockevent devices are always running.

This breaks the systems where per cpu clock events stop in low power states.

Hence revert 77b0d60 and implement the same requirement with use
of C3STOP feature flag.

Not-yet-signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
---
The 3.4-rc1 kernel introduced a regression with cpuidle and after debugging it
arrowed down the issue to commit 77b0d60 {clockevents: Leave the broadcast
device in shutdown mode when not needed}.

On OMAP, the percpu clock-events die in the low power states and
hence the broadcast clock-event needs to be active. With 77b0d60 
patch I was seeing that broadcast device was not getting programmed
while entering in low power states and hence system locks up.

I am not completely sure about this fix and the patch is RFC.

 kernel/time/tick-broadcast.c |    4 ----
 kernel/time/tick-oneshot.c   |    3 ++-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e883f57..fd4a7b1 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -575,15 +575,11 @@ void ntick_broadcast_switch_to_oneshot(void)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-	if (cpumask_empty(tick_get_broadcast_mask()))
-		goto end;
 
 	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
 	bc = tick_broadcast_device.evtdev;
 	if (bc)
 		tick_broadcast_setup_oneshot(bc);
-
-end:
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 8241090..318ec0c 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -82,7 +82,8 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
 	td->mode = TICKDEV_MODE_ONESHOT;
 	dev->event_handler = handler;
 	clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
-	tick_broadcast_switch_to_oneshot();
+	if (dev->features & CLOCK_EVT_FEAT_C3STOP)
+		tick_broadcast_switch_to_oneshot();
 	return 0;
 }
 
-- 
1.7.5.4

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Suresh Siddha]

On Mon, 2012-04-09 at 11:33 +0530, Santosh Shilimkar wrote:
> Commit 77b0d60{clockevents: Leave the broadcast device in shutdown mode
> when not needed} was intended to leave the broadcast device in shutdown mode
> when the per-cpu clockevent devices are always running.
> 
> This breaks the systems where per cpu clock events stop in low power states.
> 
> Hence revert 77b0d60 and implement the same requirement with use
> of C3STOP feature flag.

Problem you encountered is not related to leaving the broadcast device
in shutdown mode. Problem is that we didn't track the mode change to
oneshot and later during idle entry/exit, when we request the broadcast
device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
tick_broadcast_oneshot_control() returns with out doing much as it
thinks broadcast device is in periodic mode.

Can you please check if the appended patch fixes the issue? I could
reproduce the issue on my NHM platform which doesn't have always running
apic timer and with the appended fix, all is well with cpu's going into
tickless idle etc. Thanks.
---

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: clockevents: track broadcast device mode change in tick_broadcast_switch_to_oneshot()

In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
"clockevents: Leave the broadcast device in shutdown mode when not needed",
we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.

This breaks the platforms which need broadcast device oneshot services during
deep idle states. tick_broadcast_oneshot_control() thinks that it is
in periodic mode and fails to take proper decisions based on the
CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
idle entry/exit.

Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
before leaving the broadcast HW device in shutdown mode if there are no active
requests for the moment.

Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 kernel/time/tick-broadcast.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e883f57..bf57abd 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
+
+	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
+
 	if (cpumask_empty(tick_get_broadcast_mask()))
 		goto end;
 
-	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
 	bc = tick_broadcast_device.evtdev;
 	if (bc)
 		tick_broadcast_setup_oneshot(bc);

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Shilimkar, Santosh]

On Tue, Apr 10, 2012 at 4:11 AM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Mon, 2012-04-09 at 11:33 +0530, Santosh Shilimkar wrote:
>> Commit 77b0d60{clockevents: Leave the broadcast device in shutdown mode
>> when not needed} was intended to leave the broadcast device in shutdown mode
>> when the per-cpu clockevent devices are always running.
>>
>> This breaks the systems where per cpu clock events stop in low power states.
>>
>> Hence revert 77b0d60 and implement the same requirement with use
>> of C3STOP feature flag.
>
> Problem you encountered is not related to leaving the broadcast device
> in shutdown mode. Problem is that we didn't track the mode change to
> oneshot and later during idle entry/exit, when we request the broadcast
> device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
> tick_broadcast_oneshot_control() returns with out doing much as it
> thinks broadcast device is in periodic mode.
>
You are right. In my last traces I didn't track the broadcast device state
fully. Did some more tracing and indeed the issue was the broadcast
device mode.

> Can you please check if the appended patch fixes the issue? I could
> reproduce the issue on my NHM platform which doesn't have always running
> apic timer and with the appended fix, all is well with cpu's going into
> tickless idle etc. Thanks.
> ---
>
Yes it does work on OMAP too.

> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: clockevents: track broadcast device mode change in tick_broadcast_switch_to_oneshot()
>
> In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
> "clockevents: Leave the broadcast device in shutdown mode when not needed",
> we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
> with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.
>
> This breaks the platforms which need broadcast device oneshot services during
> deep idle states. tick_broadcast_oneshot_control() thinks that it is
> in periodic mode and fails to take proper decisions based on the
> CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
> idle entry/exit.
>
> Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
> before leaving the broadcast HW device in shutdown mode if there are no active
> requests for the moment.
>
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
Thanks for the patch. Feel free to add,
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>	

Regards
Santosh

[tip:timers/urgent] clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()

[tip-bot for Suresh Siddha]

Commit-ID:  fa4da365bc7772c2cd6d5405bdf151612455f957
Gitweb:     http://git.kernel.org/tip/fa4da365bc7772c2cd6d5405bdf151612455f957
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Mon, 9 Apr 2012 15:41:44 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 10 Apr 2012 11:42:07 +0200

clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()

In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
"clockevents: Leave the broadcast device in shutdown mode when not needed",
we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.

This breaks the platforms which need broadcast device oneshot services during
deep idle states. tick_broadcast_oneshot_control() thinks that it is
in periodic mode and fails to take proper decisions based on the
CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
idle entry/exit.

Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
before leaving the broadcast HW device in shutdown mode if there are no active
requests for the moment.

Reported-and-tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: johnstul@us.ibm.com
Link: http://lkml.kernel.org/r/1334011304.12400.81.camel@sbsiddha-desk.sc.intel.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/tick-broadcast.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e883f57..bf57abd 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
+
+	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
+
 	if (cpumask_empty(tick_get_broadcast_mask()))
 		goto end;
 
-	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
 	bc = tick_broadcast_device.evtdev;
 	if (bc)
 		tick_broadcast_setup_oneshot(bc);

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Feng Tang]

Hi Suresh,

> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Date: 2012/4/10
> Subject: Re: [PATCH] clockevents: Leave broadcast device shtudown only
> if the current device is always running.
> To: Santosh Shilimkar <santosh.shilimkar@ti.com>
> 抄送： tglx@linutronix.de, linux-kernel@vger.kernel.org, johnstul@us.ibm.com
> 
> 
> On Mon, 2012-04-09 at 11:33 +0530, Santosh Shilimkar wrote:
> > Commit 77b0d60{clockevents: Leave the broadcast device in shutdown mode
> > when not needed} was intended to leave the broadcast device in shutdown mode
> > when the per-cpu clockevent devices are always running.
> >
> > This breaks the systems where per cpu clock events stop in low power states.
> >
> > Hence revert 77b0d60 and implement the same requirement with use
> > of C3STOP feature flag.
> 
> Problem you encountered is not related to leaving the broadcast device
> in shutdown mode. Problem is that we didn't track the mode change to
> oneshot and later during idle entry/exit, when we request the broadcast
> device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
> tick_broadcast_oneshot_control() returns with out doing much as it
> thinks broadcast device is in periodic mode.

Just curious, IIUC, the tick_broadcast_switch_to_oneshot() is only called
during CPU init process in boot time or bring a offlined cpu back on. Why
is it related to the deep idle?

Thanks,
Feng
> 
> Can you please check if the appended patch fixes the issue? I could
> reproduce the issue on my NHM platform which doesn't have always running
> apic timer and with the appended fix, all is well with cpu's going into
> tickless idle etc. Thanks.
> ---
> 
> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: clockevents: track broadcast device mode change in
> tick_broadcast_switch_to_oneshot()
> 
> In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
> "clockevents: Leave the broadcast device in shutdown mode when not needed",
> we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
> with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.
> 
> This breaks the platforms which need broadcast device oneshot services during
> deep idle states. tick_broadcast_oneshot_control() thinks that it is
> in periodic mode and fails to take proper decisions based on the
> CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
> idle entry/exit.
> 
> Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
> before leaving the broadcast HW device in shutdown mode if there are no active
> requests for the moment.
> 
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
>  kernel/time/tick-broadcast.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index e883f57..bf57abd 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
>        unsigned long flags;
> 
>        raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> +
> +       tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
> +
>        if (cpumask_empty(tick_get_broadcast_mask()))
>                goto end;
> 
> -       tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
>        bc = tick_broadcast_device.evtdev;
>        if (bc)
>                tick_broadcast_setup_oneshot(bc);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Thomas Gleixner]

On Wed, 11 Apr 2012, Feng Tang wrote:
> > Problem you encountered is not related to leaving the broadcast device
> > in shutdown mode. Problem is that we didn't track the mode change to
> > oneshot and later during idle entry/exit, when we request the broadcast
> > device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
> > tick_broadcast_oneshot_control() returns with out doing much as it
> > thinks broadcast device is in periodic mode.
> 
> Just curious, IIUC, the tick_broadcast_switch_to_oneshot() is only called
> during CPU init process in boot time or bring a offlined cpu back on. Why
> is it related to the deep idle?

Because a deep idle state might require the broadcast device and w/o
Suresh's patch it can't use it because it's in the wrong state.

Thanks,

	tglx

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Feng Tang]

Hi Thomas,

On Wed, Apr 11, 2012 at 12:10:59PM +0200, Thomas Gleixner wrote:
> On Wed, 11 Apr 2012, Feng Tang wrote:
> > > Problem you encountered is not related to leaving the broadcast device
> > > in shutdown mode. Problem is that we didn't track the mode change to
> > > oneshot and later during idle entry/exit, when we request the broadcast
> > > device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
> > > tick_broadcast_oneshot_control() returns with out doing much as it
> > > thinks broadcast device is in periodic mode.
> > 
> > Just curious, IIUC, the tick_broadcast_switch_to_oneshot() is only called
> > during CPU init process in boot time or bring a offlined cpu back on. Why
> > is it related to the deep idle?
> 
> Because a deep idle state might require the broadcast device and w/o
> Suresh's patch it can't use it because it's in the wrong state.
>

Thanks for the explaination, I understand the root cause now.

And regarding the original commit 77b0d60c5
    clockevents: Leave the broadcast device in shutdown mode when not needed

@@ -575,11 +575,15 @@ void tick_broadcast_switch_to_oneshot(void)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
+	if (cpumask_empty(tick_get_broadcast_mask()))
+		goto end;

  	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
 	bc = tick_broadcast_device.evtdev;
 	if (bc)
 		tick_broadcast_setup_oneshot(bc);
+
+end:
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 
The tick_broadcast_switch_to_oneshot() get called mostly at boot time or
during cpu hotplug. And when it is called at boot time, the tick_broadcast_mask
is alwasys 0 at the point, thus this check is always true and skip the
following oneshot setting up for broadcast device, which sounds a little
buggy.

Thanks,
Feng

> 
> Thanks,
> 
> 	tglx

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Suresh Siddha]

On Thu, 2012-04-12 at 00:04 +0800, Feng Tang wrote:
> Thanks for the explaination, I understand the root cause now.
> 
> And regarding the original commit 77b0d60c5
>     clockevents: Leave the broadcast device in shutdown mode when not needed
> 
> @@ -575,11 +575,15 @@ void tick_broadcast_switch_to_oneshot(void)
>  	unsigned long flags;
>  
>  	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> +	if (cpumask_empty(tick_get_broadcast_mask()))
> +		goto end;
> 
>   	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
>  	bc = tick_broadcast_device.evtdev;
>  	if (bc)
>  		tick_broadcast_setup_oneshot(bc);
> +
> +end:
>  	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
>  }
>  
> The tick_broadcast_switch_to_oneshot() get called mostly at boot time or
> during cpu hotplug. And when it is called at boot time, the tick_broadcast_mask
> is alwasys 0 at the point,

On the correctly working platforms, that is the case. But there are
cases where APIC timer calibration fails, in which case APIC is marked
as CLOCK_EVT_FEAT_DUMMY and in this scenario, broadcast mask will be
non-zero.

>  thus this check is always true and skip the
> following oneshot setting up for broadcast device, 

On most platforms yes, but there are some buggy exceptions which the
code is trying to handle.

thanks,
suresh

Re: [tip:timers/urgent] clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()

[Alex Shi]

On Tue, Apr 10, 2012 at 5:49 PM, tip-bot for Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> Commit-ID:  fa4da365bc7772c2cd6d5405bdf151612455f957
> Gitweb:     http://git.kernel.org/tip/fa4da365bc7772c2cd6d5405bdf151612455f957
> Author:     Suresh Siddha <suresh.b.siddha@intel.com>
> AuthorDate: Mon, 9 Apr 2012 15:41:44 -0700
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Tue, 10 Apr 2012 11:42:07 +0200
>
> clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()
>
> In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
> "clockevents: Leave the broadcast device in shutdown mode when not needed",
> we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
> with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.
>
> This breaks the platforms which need broadcast device oneshot services during
> deep idle states. tick_broadcast_oneshot_control() thinks that it is
> in periodic mode and fails to take proper decisions based on the
> CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
> idle entry/exit.
>
> Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
> before leaving the broadcast HW device in shutdown mode if there are no active
> requests for the moment.
>

This patch recovered the loopback netperf testing on NHM machine, but
on SNB machine, It isn't need a broadcost timer at all.

Do you think it is better to remove oneshot broadcast on WSM/SNB machine?

Re: [tip:timers/urgent] clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()

[Suresh Siddha]

On Thu, 2012-04-12 at 09:27 +0800, Alex Shi wrote:
> This patch recovered the loopback netperf testing on NHM machine, but
> on SNB machine, It isn't need a broadcost timer at all.
> 
> Do you think it is better to remove oneshot broadcast on WSM/SNB machine?

on WSM/SNB, we already don't use broadcast timer and always use
apictimer instead.

thanks,
suresh

Re: [tip:timers/urgent] clockevents: tTack broadcast device mode change in tick_broadcast_switch_to_oneshot()

[Alex Shi]

On 04/12/2012 12:35 PM, Suresh Siddha wrote:

> On Thu, 2012-04-12 at 09:27 +0800, Alex Shi wrote:
>> This patch recovered the loopback netperf testing on NHM machine, but
>> on SNB machine, It isn't need a broadcost timer at all.
>>
>> Do you think it is better to remove oneshot broadcast on WSM/SNB machine?
> 
> on WSM/SNB, we already don't use broadcast timer and always use
> apictimer instead.


Yes, you are right. I omitted this.

Tick Device: mode:     1
Broadcast device
Clock Event Device: hpet
...
tick_broadcast_mask: 00000000
tick_broadcast_oneshot_mask: 00000000


> 
> thanks,
> suresh
> 

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Santosh Shilimkar]

Suresh,

On Tuesday 10 April 2012 04:11 AM, Suresh Siddha wrote:
> On Mon, 2012-04-09 at 11:33 +0530, Santosh Shilimkar wrote:
>> Commit 77b0d60{clockevents: Leave the broadcast device in shutdown mode
>> when not needed} was intended to leave the broadcast device in shutdown mode
>> when the per-cpu clockevent devices are always running.
>>
>> This breaks the systems where per cpu clock events stop in low power states.
>>
>> Hence revert 77b0d60 and implement the same requirement with use
>> of C3STOP feature flag.
> 
> Problem you encountered is not related to leaving the broadcast device
> in shutdown mode. Problem is that we didn't track the mode change to
> oneshot and later during idle entry/exit, when we request the broadcast
> device services using CLOCK_EVT_NOTIFY_BROADCAST_ENTER etc,
> tick_broadcast_oneshot_control() returns with out doing much as it
> thinks broadcast device is in periodic mode.
> 
> Can you please check if the appended patch fixes the issue? I could
> reproduce the issue on my NHM platform which doesn't have always running
> apic timer and with the appended fix, all is well with cpu's going into
> tickless idle etc. Thanks.
> ---
> 
> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: clockevents: track broadcast device mode change in tick_broadcast_switch_to_oneshot()
> 
> In the commit 77b0d60c5adf39c74039e2142a1d3cd1e4d53799,
> "clockevents: Leave the broadcast device in shutdown mode when not needed",
> we were bailing out too quickly in tick_broadcast_switch_to_oneshot(),
> with out tracking the broadcast device mode change to 'TICKDEV_MODE_ONESHOT'.
> 
> This breaks the platforms which need broadcast device oneshot services during
> deep idle states. tick_broadcast_oneshot_control() thinks that it is
> in periodic mode and fails to take proper decisions based on the
> CLOCK_EVT_NOTIFY_BROADCAST_[ENTER, EXIT] notifications during deep
> idle entry/exit.
> 
> Fix this by tracking the broadcast device mode as 'TICKDEV_MODE_ONESHOT',
> before leaving the broadcast HW device in shutdown mode if there are no active
> requests for the moment.
> 
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
>  kernel/time/tick-broadcast.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index e883f57..bf57abd 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
>  	unsigned long flags;
>  
>  	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> +
> +	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
> +
>  	if (cpumask_empty(tick_get_broadcast_mask()))
>  		goto end;
>  
> -	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
>  	bc = tick_broadcast_device.evtdev;
>  	if (bc)
>  		tick_broadcast_setup_oneshot(bc);
> 
> 
Last time when I tried your patch, some how he ethernet IRQ masked
the issue.

The problem is still as before. If you make a minimal kernel
configuration and possibly have only local timer, broadcast timer
and say console device as CPU wakeup sources, you should hang in
the boot process itself(Assuming CPUIDLE driver is built-in)

The main issue is, you are not letting the one time setup
needed for the broadcast device. Even with above change,
the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
mode and the event handler is pointing to clockevents_handle_noop()

So the very first 'CLOCK_EVT_NOTIFY_BROADCAST_ENTER' call won't
program the broadcast device with next event and CPU won't wakeup
from low power. if you are lucky and have some other wakeup source,
system will move further and eventually the handler and
mode of the broad-cast device get set correctly.

At the minimal below things needs to be done to avoid the boot-hang.

-----------------------
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index bf57abd..3b08fb9 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -577,13 +580,16 @@ void tick_broadcast_switch_to_oneshot(void)
        raw_spin_lock_irqsave(&tick_broadcast_lock, flags);

        tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
+       bc = tick_broadcast_device.evtdev;
+       if (bc) {
+               bc->event_handler = tick_handle_oneshot_broadcast;
+               clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT);
+       }

        if (cpumask_empty(tick_get_broadcast_mask()))
                goto end;

-       bc = tick_broadcast_device.evtdev;
-       if (bc)
-               tick_broadcast_setup_oneshot(bc);
+       tick_broadcast_setup_oneshot(bc);

 end:
        raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
-----------------------

More I think of this, seems that the original patch approach
itself was not right.

What you intended can be easily handled by avoiding
the tick_broadcast_switch_to_oneshot() on machines where
local timers are always running. And that was precisely
I tried in $subject patch [1] and that should solve
the problem what you have at the same time maintain the
existing functionality intact.

Let me know if I am missing something here. Bottom line is
the clock-event broadcasting is still broken after your
change(s) and needs to be fixed.

Regards
Santosh

[1] https://lkml.org/lkml/2012/4/9/13

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Suresh Siddha]

On Sun, 2012-04-15 at 18:27 +0530, Santosh Shilimkar wrote:
> On Tuesday 10 April 2012 04:11 AM, Suresh Siddha wrote:
> > @@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
> >  	unsigned long flags;
> >  
> >  	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> > +
> > +	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
> > +
> >  	if (cpumask_empty(tick_get_broadcast_mask()))
> >  		goto end;
> >  
> > -	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
> >  	bc = tick_broadcast_device.evtdev;
> >  	if (bc)
> >  		tick_broadcast_setup_oneshot(bc);
> > 
> > 
> Last time when I tried your patch, some how he ethernet IRQ masked
> the issue.
> 
> The problem is still as before. If you make a minimal kernel
> configuration and possibly have only local timer, broadcast timer
> and say console device as CPU wakeup sources, you should hang in
> the boot process itself(Assuming CPUIDLE driver is built-in)
> 
> The main issue is, you are not letting the one time setup
> needed for the broadcast device. Even with above change,
> the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
> mode and the event handler is pointing to clockevents_handle_noop()

On x86, idle driver (like drivers/idle/intel_idle.c:
__setup_broadcast_timer()) calls clockevents_notify() with
CLOCK_EVT_NOTIFY_BROADCAST_ON, during which we do
tick_broadcast_setup_oneshot(). See tick_do_broadcast_on_off() which
does this. That should setup the evtdev handler, mode etc.

And during the next CLOCK_EVT_NOTIFY_BROADCAST_ENTER we program the next
broadcast event.

> So the very first 'CLOCK_EVT_NOTIFY_BROADCAST_ENTER' call won't
> program the broadcast device with next event and CPU won't wakeup
> from low power. if you are lucky and have some other wakeup source,
> system will move further and eventually the handler and
> mode of the broad-cast device get set correctly.

I am confused. Can you elaborate on how on the next spurious wakeup
event, broad-cast device handler, mode is set?

It sounds like the CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications are
missing from your idle driver. But I am confused how it will work on the
next spurious wakeup etc with out the calls to
tick_do_broadcast_on_off()? Perhaps the next spurious wakeup correcting
everything is just theory and you didn't see it in practice?

Anyways, can you add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications in
your idle driver to see if it addresses the problem. That is the correct
thing to do here.

thanks,
suresh

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Santosh Shilimkar]

On Tuesday 17 April 2012 03:33 AM, Suresh Siddha wrote:
> On Sun, 2012-04-15 at 18:27 +0530, Santosh Shilimkar wrote:
>> On Tuesday 10 April 2012 04:11 AM, Suresh Siddha wrote:
>>> @@ -575,10 +575,12 @@ void tick_broadcast_switch_to_oneshot(void)
>>>  	unsigned long flags;
>>>  
>>>  	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>>> +
>>> +	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
>>> +
>>>  	if (cpumask_empty(tick_get_broadcast_mask()))
>>>  		goto end;
>>>  
>>> -	tick_broadcast_device.mode = TICKDEV_MODE_ONESHOT;
>>>  	bc = tick_broadcast_device.evtdev;
>>>  	if (bc)
>>>  		tick_broadcast_setup_oneshot(bc);
>>>
>>>
>> Last time when I tried your patch, some how he ethernet IRQ masked
>> the issue.
>>
>> The problem is still as before. If you make a minimal kernel
>> configuration and possibly have only local timer, broadcast timer
>> and say console device as CPU wakeup sources, you should hang in
>> the boot process itself(Assuming CPUIDLE driver is built-in)
>>
>> The main issue is, you are not letting the one time setup
>> needed for the broadcast device. Even with above change,
>> the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
>> mode and the event handler is pointing to clockevents_handle_noop()
> 
> On x86, idle driver (like drivers/idle/intel_idle.c:
> __setup_broadcast_timer()) calls clockevents_notify() with
> CLOCK_EVT_NOTIFY_BROADCAST_ON, during which we do
> tick_broadcast_setup_oneshot(). See tick_do_broadcast_on_off() which
> does this. That should setup the evtdev handler, mode etc.
> 
> And during the next CLOCK_EVT_NOTIFY_BROADCAST_ENTER we program the next
> broadcast event.
>
I see.

>> So the very first 'CLOCK_EVT_NOTIFY_BROADCAST_ENTER' call won't
>> program the broadcast device with next event and CPU won't wakeup
>> from low power. if you are lucky and have some other wakeup source,
>> system will move further and eventually the handler and
>> mode of the broad-cast device get set correctly.
> 
> I am confused. Can you elaborate on how on the next spurious wakeup
> event, broad-cast device handler, mode is set?
> 
No it doesn't set it. I was wrong in quoting that. The system was
working because of other wakeup sources. I checked the broadcast
events and they were not happening.

> 
> Anyways, can you add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications in
> your idle driver to see if it addresses the problem. That is the correct
> thing to do here.
> 
Before your commit 77b0d60c{clockevents: Leave the broadcast device..},
my idle driver was working without CLOCK_EVT_NOTIFY_BROADCAST_ON
notifier. Now it's clear that it was relying on the default
'tick_broadcast_setup_oneshot()' which was happening during boot.

And ofcource notifying explicitly with BROADCAST_ON does the
tick setup. Will update my idle driver accordingly but can
you please explain me why the proposed patch [1] is not correct
approach ? It should be also do what you intended to do
with minimal change, right ?

Regards
Santosh
[1] https://lkml.org/lkml/2012/4/9/13

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Suresh Siddha]

On Tue, 2012-04-17 at 13:44 +0530, Santosh Shilimkar wrote:
> On Tuesday 17 April 2012 03:33 AM, Suresh Siddha wrote:
> > On Sun, 2012-04-15 at 18:27 +0530, Santosh Shilimkar wrote:
> >> The main issue is, you are not letting the one time setup
> >> needed for the broadcast device. Even with above change,
> >> the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
> >> mode and the event handler is pointing to clockevents_handle_noop()

If you see this, handler was not even set to the periodic mode handler.
So the periodic mode for the broadcast device also was not working
before (even with out the recent change). 

In tick_check_broadcast_device(), we do this:

        if (!cpumask_empty(tick_get_broadcast_mask()))
                tick_broadcast_start_periodic(dev);

So with out the CLOCK_EVT_NOTIFY_BROADCAST_ON notification, we won't be
even setting up the periodic mode correctly.

I just extended this to the oneshot mode too, exposing the issue in your
cpuidle code.

> > 
> > On x86, idle driver (like drivers/idle/intel_idle.c:
> > __setup_broadcast_timer()) calls clockevents_notify() with
> > CLOCK_EVT_NOTIFY_BROADCAST_ON, during which we do
> > tick_broadcast_setup_oneshot(). See tick_do_broadcast_on_off() which
> > does this. That should setup the evtdev handler, mode etc.
> > 
> > And during the next CLOCK_EVT_NOTIFY_BROADCAST_ENTER we program the next
> > broadcast event.
> >
> I see.
> 
> > 
> > Anyways, can you add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications in
> > your idle driver to see if it addresses the problem. That is the correct
> > thing to do here.
> > 
> Before your commit 77b0d60c{clockevents: Leave the broadcast device..},
> my idle driver was working without CLOCK_EVT_NOTIFY_BROADCAST_ON
> notifier. Now it's clear that it was relying on the default
> 'tick_broadcast_setup_oneshot()' which was happening during boot.
> 
> And ofcource notifying explicitly with BROADCAST_ON does the
> tick setup. Will update my idle driver accordingly but can
> you please explain me why the proposed patch [1] is not correct
> approach ? It should be also do what you intended to do
> with minimal change, right ?

Please see the above. Essentially irrespective of the recent change,
periodic mode was also broken. Probably no one cared about that mode and
we didn't notice that issue so far. Anyhow, the correct thing to do here
is to add those notifications to the cpuidle driver. Can you please
check if the appended/untested patch fixes your issue? If so, please
push this to your driver. thanks.
---
From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: arm: omap4: cpuidle: add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications

Add the missing CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF clockevent notifications.
This will setup the broadcast timer in either periodic/oneshot modes
correctly. Recent clockevent infrastructure change "leave the broadcast
device in shutdown mode when not needed" exposed this bug leading to boot
hangs in oneshot mode. Prior to this, periodic broadcast mode was also
broken. This change fixes both the periodic/oneshot broadcast modes.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 arch/arm/mach-omap2/cpuidle44xx.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c
index f386cbe..cd89f58 100644
--- a/arch/arm/mach-omap2/cpuidle44xx.c
+++ b/arch/arm/mach-omap2/cpuidle44xx.c
@@ -161,7 +161,16 @@ static inline struct omap4_idle_statedata *_fill_cstate_usage(
 	return cx;
 }
 
+static void __setup_broadcast_timer(void *arg)
+{
+	unsigned long reason = (unsigned long)arg;
+	int cpu = smp_processor_id();
+
+	reason = reason ?
+		CLOCK_EVT_NOTIFY_BROADCAST_ON : CLOCK_EVT_NOTIFY_BROADCAST_OFF;
 
+	clockevents_notify(reason, &cpu);
+}
 
 /**
  * omap4_idle_init - Init routine for OMAP4 idle
@@ -214,6 +223,8 @@ int __init omap4_idle_init(void)
 	cpuidle_register_driver(&omap4_idle_driver);
 
 	dev->state_count = OMAP4_NUM_STATES;
+
+	on_each_cpu(__setup_broadcast_timer, (void *) true, 1);
 	if (cpuidle_register_device(dev)) {
 		pr_err("%s: CPUidle register device failed\n", __func__);
 			return -EIO;

Re: [PATCH] clockevents: Leave broadcast device shtudown only if the current device is always running.

[Shilimkar, Santosh]

On Wed, Apr 18, 2012 at 12:41 AM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Tue, 2012-04-17 at 13:44 +0530, Santosh Shilimkar wrote:
>> On Tuesday 17 April 2012 03:33 AM, Suresh Siddha wrote:
>> > On Sun, 2012-04-15 at 18:27 +0530, Santosh Shilimkar wrote:
>> >> The main issue is, you are not letting the one time setup
>> >> needed for the broadcast device. Even with above change,
>> >> the 'tick_broadcast_device.evtdev' is still in SHUTDOWN
>> >> mode and the event handler is pointing to clockevents_handle_noop()
>
> If you see this, handler was not even set to the periodic mode handler.
> So the periodic mode for the broadcast device also was not working
> before (even with out the recent change).
>
> In tick_check_broadcast_device(), we do this:
>
>        if (!cpumask_empty(tick_get_broadcast_mask()))
>                tick_broadcast_start_periodic(dev);
>
> So with out the CLOCK_EVT_NOTIFY_BROADCAST_ON notification, we won't be
> even setting up the periodic mode correctly.
>
> I just extended this to the oneshot mode too, exposing the issue in your
> cpuidle code.
>
Periodic mode for broad-cast device was any way not much important and hence
the issue was hidden I guess.

>> >
>> > On x86, idle driver (like drivers/idle/intel_idle.c:
>> > __setup_broadcast_timer()) calls clockevents_notify() with
>> > CLOCK_EVT_NOTIFY_BROADCAST_ON, during which we do
>> > tick_broadcast_setup_oneshot(). See tick_do_broadcast_on_off() which
>> > does this. That should setup the evtdev handler, mode etc.
>> >
>> > And during the next CLOCK_EVT_NOTIFY_BROADCAST_ENTER we program the next
>> > broadcast event.
>> >
>> I see.
>>
>> >
>> > Anyways, can you add CLOCK_EVT_NOTIFY_BROADCAST_ON/OFF notifications in
>> > your idle driver to see if it addresses the problem. That is the correct
>> > thing to do here.
>> >
>> Before your commit 77b0d60c{clockevents: Leave the broadcast device..},
>> my idle driver was working without CLOCK_EVT_NOTIFY_BROADCAST_ON
>> notifier. Now it's clear that it was relying on the default
>> 'tick_broadcast_setup_oneshot()' which was happening during boot.
>>
>> And ofcource notifying explicitly with BROADCAST_ON does the
>> tick setup. Will update my idle driver accordingly but can
>> you please explain me why the proposed patch [1] is not correct
>> approach ? It should be also do what you intended to do
>> with minimal change, right ?
>
> Please see the above. Essentially irrespective of the recent change,
> periodic mode was also broken. Probably no one cared about that mode and
> we didn't notice that issue so far. Anyhow, the correct thing to do here
> is to add those notifications to the cpuidle driver. Can you please
> check if the appended/untested patch fixes your issue? If so, please
> push this to your driver. thanks.
>
As I mentioned earlier, I have already tried my driver  with
CLOCK_EVT_NOTIFY_BROADCAST_ON update yesterday and pushed
a patch for the same.
Thanks for the periodic mode point which i missed previously.

Regards
Santosh