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

[Sudeep Holla <sudeep.holla@arm.com>]

On 26/06/15 13:34, Preeti U Murthy wrote:
> 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).
>

I tested the updated patch and found issues. I am seeing some random
behaviour(with GENERIC_CLOCKEVENTS_BROADCAST=y TICK_ONESHOT=y):
1. sometimes all the CPUs have entered deeper idle states(though very
    rare, finding it difficult to hit this scenario)
2. few other times I see one CPU in shallow state which matches the
    above explanation of missing hrtimer programming.

Regards,
Sudeep