Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

On 06/25/2015 07:25 PM, Thomas Gleixner wrote:
> But there is a case which is not correctly handled right now. That's
> what you are trying to solve in the wrong way.
> 
> If
>    GENERIC_CLOCKEVENTS_BROADCAST=n
> 
> or
> 
>    GENERIC_CLOCKEVENTS_BROADCAST=y and no broadcast device is available,
> 
> AND cpu local tick device has the C3STOP flag set,
> 
> then we have no way to tell the idle code that going deep is not
> allowed.
> 
> So we need to be smarter than blindly changing a return
> value. Completely untested patch below.
> 
> Thanks,
> 
> 	tglx
> ---
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index 4191b5623a28..8731a58dd747 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -63,11 +63,7 @@ extern void tick_broadcast_control(enum tick_broadcast_mode mode);
>  static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
>  #endif /* BROADCAST */
> 
> -#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
>  extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
> -#else
> -static inline int tick_broadcast_oneshot_control(enum tick_broadcast_state state) { return 0; }
> -#endif
> 
>  static inline void tick_broadcast_enable(void)
>  {
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index d39f32cdd1b5..52c8d01b956b 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -662,24 +662,15 @@ static void broadcast_shutdown_local(struct clock_event_device *bc,
>  	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
>  }
> 
> -/**
> - * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
> - * @state:	The target state (enter/exit)
> - *
> - * The system enters/leaves a state, where affected devices might stop
> - * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
> - *
> - * Called with interrupts disabled, so clockevents_lock is not
> - * required here because the local clock event device cannot go away
> - * under us.
> - */
> -int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
>  {
>  	struct clock_event_device *bc, *dev;
> -	struct tick_device *td;
>  	int cpu, ret = 0;
>  	ktime_t now;
> 
> +	if (!tick_broadcast_device.evtdev)
> +		return -EBUSY;
> +
>  	/*
>  	 * Periodic mode does not care about the enter/exit of power
>  	 * states
> @@ -687,15 +678,7 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
>  	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
>  		return 0;
> 
> -	/*
> -	 * We are called with preemtion disabled from the depth of the
> -	 * idle code, so we can't be moved away.
> -	 */
> -	td = this_cpu_ptr(&tick_cpu_device);
> -	dev = td->evtdev;
> -
> -	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
> -		return 0;
> +	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
> 
>  	raw_spin_lock(&tick_broadcast_lock);
>  	bc = tick_broadcast_device.evtdev;
> @@ -938,6 +921,13 @@ bool tick_broadcast_oneshot_available(void)
>  	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
>  }
> 
> +#else
> +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> +{
> +	if (!tick_broadcast_device.evtdev)
> +		return -EBUSY;
> +	return 0;
> +}
>  #endif
> 
>  void __init tick_broadcast_init(void)
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index 17f144450050..f66c11a30348 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -342,6 +342,27 @@ out_bc:
>  	tick_install_broadcast_device(newdev);
>  }
> 
> +/**
> + * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
> + * @state:	The target state (enter/exit)
> + *
> + * The system enters/leaves a state, where affected devices might stop
> + * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
> + *
> + * Called with interrupts disabled, so clockevents_lock is not
> + * required here because the local clock event device cannot go away
> + * under us.
> + */
> +int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> +{
> +	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
> +
> +	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
> +		return 0;
> +
> +	return __tick_broadcast_oneshot_control(state);
> +}
> +
>  #ifdef CONFIG_HOTPLUG_CPU
>  /*
>   * Transfer the do_timer job away from a dying cpu.
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 42fdf4958bcc..a4a8d4e9baa1 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -71,4 +71,14 @@ extern void tick_cancel_sched_timer(int cpu);
>  static inline void tick_cancel_sched_timer(int cpu) { }
>  #endif
> 
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> +extern int __tick_broadcast_oneshot_control(enum tick_broadcast_state state);
> +#else
> +static inline int
> +__tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> +{
> +	return -EBUSY;
> +}
> +#endif
> +
>  #endif

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>