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

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

[Jindong Yue]

Broadcast device is switched to oneshot mode in
tick_switch_to_oneshot() -> tick_broadcast_switch_to_oneshot().

If build broadcast clock event device driver as module, and
install it after system enters oneshot mode, then it will
stay in periodic mode forever.

This patch allows such broadcast device switch to oneshot
mode when register.

Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
---

v2 changes:
- Remove below condition check before switch new installed
  broadcast device to oneshot mode:
  dev->event_handler != tick_handle_oneshot_broadcast
- Put tick_clock_notify() after check system not runs in
  oneshot mode

---
 kernel/time/tick-broadcast.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 36d7464c8962..c5e67685b0af 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
@@ -107,6 +108,19 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
 	tick_broadcast_device.evtdev = dev;
 	if (!cpumask_empty(tick_broadcast_mask))
 		tick_broadcast_start_periodic(dev);
+
+	if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return;
+
+	/*
+	 * If system already runs in oneshot mode, switch new registered
+	 * broadcast device to oneshot mode explicitly if possiable.
+	 */
+	if (tick_broadcast_oneshot_active()) {
+		tick_broadcast_switch_to_oneshot();
+		return;
+	}
+
 	/*
 	 * Inform all cpus about this. We might be in a situation
 	 * where we did not switch to oneshot mode because the per cpu
@@ -115,8 +129,7 @@ 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)
-		tick_clock_notify();
+	tick_clock_notify();
 }
 
 /*
-- 
2.17.1

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

[Thomas Gleixner]

On Wed, Mar 31 2021 at 09:10, Jindong Yue wrote:
> --- 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);

Leftover ...
  
>  /*
>   * Debugging: see timer_list.c
> @@ -107,6 +108,19 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
>  	tick_broadcast_device.evtdev = dev;
>  	if (!cpumask_empty(tick_broadcast_mask))
>  		tick_broadcast_start_periodic(dev);
> +
> +	if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
> +		return;
> +
> +	/*
> +	 * If system already runs in oneshot mode, switch new registered

the system .... the newly registered

> +	 * broadcast device to oneshot mode explicitly if possiable.

s/possible/possiable/

But the 'if possible' makes no sense here. The above check for
CLOCK_EVT_FEAT_ONESHOT established that it _is_ possible. So just remove
the 'if ...'.

> +	 */
> +	if (tick_broadcast_oneshot_active()) {
> +		tick_broadcast_switch_to_oneshot();
> +		return;
> +	}

Thanks,

        tglx

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

[J.d. Yue]

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Wednesday, March 31, 2021 4:07 PM
> To: J.d. Yue <jindong.yue@nxp.com>; fweisbec@gmail.com; mingo@kernel.org
> Cc: linux-kernel@vger.kernel.org; J.d. Yue <jindong.yue@nxp.com>
> Subject: [EXT] Re: [PATCH v2] tick/broadcast: Allow later registered device
> enter oneshot mode
> 
> Caution: EXT Email
> 
> On Wed, Mar 31 2021 at 09:10, Jindong Yue wrote:
> > --- 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);
> 
> Leftover ...

Removed in v3.

> 
> >  /*
> >   * Debugging: see timer_list.c
> > @@ -107,6 +108,19 @@ void tick_install_broadcast_device(struct
> clock_event_device *dev)
> >       tick_broadcast_device.evtdev = dev;
> >       if (!cpumask_empty(tick_broadcast_mask))
> >               tick_broadcast_start_periodic(dev);
> > +
> > +     if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
> > +             return;
> > +
> > +     /*
> > +      * If system already runs in oneshot mode, switch new registered
> 
> the system .... the newly registered
> 
> > +      * broadcast device to oneshot mode explicitly if possiable.
> 
> s/possible/possiable/
> 
> But the 'if possible' makes no sense here. The above check for
> CLOCK_EVT_FEAT_ONESHOT established that it _is_ possible. So just remove
> the 'if ...'.

I should be more careful ...
Please check v3 patch.

Thanks
Jindong

> 
> > +      */
> > +     if (tick_broadcast_oneshot_active()) {
> > +             tick_broadcast_switch_to_oneshot();
> > +             return;
> > +     }
> 
> Thanks,
> 
>         tglx