[PATCH] tick/broadcast: Allow later probed device enter oneshot mode

[PATCH] tick/broadcast: Allow later probed device enter oneshot mode

[Jindong Yue]

Broadcast device is switched to oneshot mode in
hrtimer_switch_to_hres() -> tick_broadcast_switch_to_oneshot().
After high resolution timers are enabled, new installed
broadcast device has no chance to switch mode.

This issue happens in below situation:
In order to make broadcast clock source driver build as module,
use module_platform_driver() to replace TIMER_OF_DECLARE().
This will make clock source driver probed later than
high resolution timers enabled.

Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
---
 kernel/time/tick-broadcast.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e51778c312f1..f38a7544cb5b 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -47,6 +47,7 @@ static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc)
 static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
 # endif
 #endif
+static void tick_handle_oneshot_broadcast(struct clock_event_device *dev);
 
 /*
  * Debugging: see timer_list.c
@@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
 	 * notification the systems stays stuck in periodic mode
 	 * forever.
 	 */
-	if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
+	if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
 		tick_clock_notify();
+
+		/*
+		 * If new broadcast device is installed after high resolution
+		 * timers enabled, it can not switch to oneshot mode anymore.
+		 * Here give it a chance.
+		 */
+		if (tick_broadcast_oneshot_active() &&
+		    dev->event_handler != tick_handle_oneshot_broadcast) {
+			tick_broadcast_switch_to_oneshot();
+		}
+	}
+
 }
 
 /*
-- 
2.17.1

Re: [PATCH] tick/broadcast: Allow later probed device enter oneshot mode

[Thomas Gleixner]

On Mon, Mar 29 2021 at 13:25, Jindong Yue wrote:
 
>  /*
>   * Debugging: see timer_list.c
> @@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
>  	 * notification the systems stays stuck in periodic mode
>  	 * forever.
>  	 */
> -	if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
> +	if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
>  		tick_clock_notify();

If the kernel runs in oneshot mode already, then calling
tick_clock_notify() does not make sense.

> +		/*
> +		 * If new broadcast device is installed after high resolution
> +		 * timers enabled, it can not switch to oneshot mode anymore.

This is not restricted to high resolution timers. The point is that
the mode is ONESHOT which might be either HIGHRES or NOHZ or both.

> +		 */
> +		if (tick_broadcast_oneshot_active() &&
> +		    dev->event_handler != tick_handle_oneshot_broadcast) {

How would that condition ever be false for a newly installed device?

> +			tick_broadcast_switch_to_oneshot();
> +		}

So what you really want is:

        if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
        	return;

        if (tick_broadcast_oneshot_active()) {
		tick_broadcast_switch_to_oneshot();
                return;
        }

        tick_clock_notify();

Thanks,

        tglx

RE: [EXT] Re: [PATCH] tick/broadcast: Allow later probed device enter oneshot mode

[J.d. Yue]

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Tuesday, March 30, 2021 5:18 AM
> To: J.d. Yue <jindong.yue@nxp.com>
> Cc: linux-kernel@vger.kernel.org; fweisbec@gmail.com; mingo@kernel.org
> Subject: [EXT] Re: [PATCH] tick/broadcast: Allow later probed device enter
> oneshot mode
> 
> Caution: EXT Email
> 
> On Mon, Mar 29 2021 at 13:25, Jindong Yue wrote:
> 
> >  /*
> >   * Debugging: see timer_list.c
> > @@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct
> clock_event_device *dev)
> >        * notification the systems stays stuck in periodic mode
> >        * forever.
> >        */
> > -     if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
> > +     if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
> >               tick_clock_notify();
> 
> If the kernel runs in oneshot mode already, then calling
> tick_clock_notify() does not make sense.

Yes, there should be more check before tick_clock_notify().

> 
> > +             /*
> > +              * If new broadcast device is installed after high resolution
> > +              * timers enabled, it can not switch to oneshot mode
> anymore.
> 
> This is not restricted to high resolution timers. The point is that the mode is
> ONESHOT which might be either HIGHRES or NOHZ or both.

Got it, after kernel enters ONESHOT mode, new registered broadcast device can't be switched to ONESHOT mode. 

> 
> > +              */
> > +             if (tick_broadcast_oneshot_active() &&
> > +                 dev->event_handler != tick_handle_oneshot_broadcast)
> > + {
> 
> How would that condition ever be false for a newly installed device?

Okay, will remove this condition.

> 
> > +                     tick_broadcast_switch_to_oneshot();
> > +             }
> 
> So what you really want is:
> 
>         if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
>                 return;
> 
>         if (tick_broadcast_oneshot_active()) {
>                 tick_broadcast_switch_to_oneshot();
>                 return;
>         }
> 
>         tick_clock_notify();
> 
> Thanks,
> 
>         tglx

Thanks for your quick review and advice. I will test this code locally and send V2 patch later.
Jindong