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

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

On 06/26/2015 01:17 PM, Thomas Gleixner wrote:
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index d39f32cdd1b5..281ce29d295e 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -662,46 +662,39 @@ 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;
> 
> -	/*
> -	 * Periodic mode does not care about the enter/exit of power
> -	 * states
> -	 */
> -	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
> -		return 0;
> +	if (!tick_broadcast_device.evtdev)
> +		return -EBUSY;
> 
> -	/*
> -	 * 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;
>  	cpu = smp_processor_id();
> 
>  	if (state == TICK_BROADCAST_ENTER) {
> +		/*
> +		 * If the current CPU owns the hrtimer broadcast
> +		 * mechanism, it cannot go deep idle and we do not add
> +		 * the CPU to the broadcast mask. We don't have to go
> +		 * through the EXIT path as the local timer is not
> +		 * shutdown.
> +		 */
> +		ret = broadcast_needs_cpu(bc, cpu);
> +
> +		/*
> +		 * If the hrtimer broadcast check tells us that the
> +		 * cpu cannot go deep idle, or if the broadcast device
> +		 * is in periodic mode, we just return.
> +		 */
> +		if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
> +			goto out;

The check on PERIODIC mode is good, but I don't see the point of moving
broadcast_needs_cpu() up above. broadcast_shutdown_local() calls
broadcast_needs_cpu() internally.

Besides, by jumping to 'out', we will miss programming the broadcast
hrtimer in tick_broadcast_set_event() below, if the cpu happen to be the
broadcast cpu(which is why it was not allowed to go to deep idle).

> +
>  		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
>  			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));
>  			broadcast_shutdown_local(bc, dev);
> @@ -717,16 +710,6 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
>  			    dev->next_event.tv64 < bc->next_event.tv64)
>  				tick_broadcast_set_event(bc, cpu, dev->next_event);
>  		}
> -		/*
> -		 * If the current CPU owns the hrtimer broadcast
> -		 * mechanism, it cannot go deep idle and we remove the
> -		 * CPU from the broadcast mask. We don't have to go
> -		 * through the EXIT path as the local timer is not
> -		 * shutdown.
> -		 */
> -		ret = broadcast_needs_cpu(bc, cpu);
> -		if (ret)
> -			cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
>  	} else {
>  		if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) {
>  			clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT);
> @@ -938,6 +921,13 @@ bool tick_broadcast_oneshot_available(void)
>  	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
>  }
> 

Regards
Preeti U Murthy