Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 02/25/2014 04:47 AM, Preeti U Murthy wrote:
> Hi Daniel,
>
> On 02/24/2014 07:25 PM, Daniel Lezcano wrote:
>> ---
>>   drivers/cpuidle/cpuidle.c |  114 ++++++++++++++++++++++++++++++++++------------
>>   include/linux/cpuidle.h   |   19 +++++++
>>   2 files changed, 105 insertions(+), 28 deletions(-)
>>
>> Index: cpuidle-next/drivers/cpuidle/cpuidle.c
>> ===================================================================
>> --- cpuidle-next.orig/drivers/cpuidle/cpuidle.c
>> +++ cpuidle-next/drivers/cpuidle/cpuidle.c
>> @@ -65,6 +65,26 @@ int cpuidle_play_dead(void)
>>   }
>>
>>   /**
>> + * cpuidle_enabled - check if the cpuidle framework is ready
>> + * @dev: cpuidle device for this cpu
>> + * @drv: cpuidle driver for this cpu
>> + *
>> + * Return 0 on success, otherwise:
>> + * -NODEV : the cpuidle framework is not available
>> + * -EBUSY : the cpuidle framework is not initialized
>> + */
>> +int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +	if (off || !initialized)
>> +		return -ENODEV;
>> +
>> +	if (!drv || !dev || !dev->enabled)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>>    * cpuidle_enter_state - enter the state and update stats
>>    * @dev: cpuidle device for this cpu
>>    * @drv: cpuidle driver for this cpu
>> @@ -108,6 +128,63 @@ int cpuidle_enter_state(struct cpuidle_d
>>   }
>>
>>   /**
>> + * cpuidle_select - ask the cpuidle framework to choose an idle state
>> + *
>> + * @drv: the cpuidle driver
>> + * @dev: the cpuidle device
>> + *
>> + * Returns the index of the idle state.
>> + */
>> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +	return cpuidle_curr_governor->select(drv, dev);
>> +}
>> +
>> +/**
>> + * cpuidle_enter - enter into the specified idle state
>> + *
>> + * @drv:   the cpuidle driver tied with the cpu
>> + * @dev:   the cpuidle device
>> + * @index: the index in the idle state table
>> + *
>> + * Returns the index in the idle state, < 0 in case of error.
>> + * The error code depends on the backend driver
>> + */
>> +int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>> +		  int index)
>> +{
>> +	int entered_state;
>> +	bool broadcast = !!(drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP);
>> +
>> +	if (broadcast)
>> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> +
>> +	if (cpuidle_state_is_coupled(dev, drv, index))
>> +		entered_state = cpuidle_enter_state_coupled(dev, drv, index);
>> +	else
>> +		entered_state = cpuidle_enter_state(dev, drv, index);
>> +
>> +	if (broadcast)
>> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> +
>> +	return entered_state;
>> +}
>> +
>> +/**
>> + * cpuidle_reflect - tell the underlying governor what was the state
>> + * we were in
>> + *
>> + * @dev  : the cpuidle device
>> + * @index: the index in the idle state table
>> + *
>> + */
>> +void cpuidle_reflect(struct cpuidle_device *dev, int index)
>> +{
>> +	if (cpuidle_curr_governor->reflect)
>> +		cpuidle_curr_governor->reflect(dev, index);
>> +}
>> +
>> +/**
>>    * cpuidle_idle_call - the main idle loop
>>    *
>>    * NOTE: no locks or semaphores should be used here
>> @@ -116,51 +193,32 @@ int cpuidle_enter_state(struct cpuidle_d
>>   int cpuidle_idle_call(void)
>>   {
>>   	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>> -	struct cpuidle_driver *drv;
>> +	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>>   	int next_state, entered_state;
>> -	bool broadcast;
>>
>> -	if (off || !initialized)
>> -		return -ENODEV;
>> -
>> -	/* check if the device is ready */
>> -	if (!dev || !dev->enabled)
>> -		return -EBUSY;
>> -
>> -	drv = cpuidle_get_cpu_driver(dev);
>> +	next_state = cpuidle_enabled(drv, dev);
>
> Accepting the return of cpuidle_enabled() into a parameter named
> "next_state" looks misleading. You are not returning any idle state. You
> could use ret/enabled to accept the return value here perhaps?

Yes, it was to save an extra variable. I can replace it by a 'ret'.

>> +	if (next_state < 0)
>> +		return next_state;
>>
>>   	/* ask the governor for the next state */
>> -	next_state = cpuidle_curr_governor->select(drv, dev);
>> +	next_state = cpuidle_select(drv, dev);
>> +
>>   	if (need_resched()) {
>>   		dev->last_residency = 0;
>>   		/* give the governor an opportunity to reflect on the outcome */
>> -		if (cpuidle_curr_governor->reflect)
>> -			cpuidle_curr_governor->reflect(dev, next_state);
>> +		cpuidle_reflect(dev, next_state);
>>   		local_irq_enable();
>>   		return 0;
>>   	}
>>
>>   	trace_cpu_idle_rcuidle(next_state, dev->cpu);
>>
>> -	broadcast = !!(drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP);
>> -
>> -	if (broadcast)
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> -
>> -	if (cpuidle_state_is_coupled(dev, drv, next_state))
>> -		entered_state = cpuidle_enter_state_coupled(dev, drv,
>> -							    next_state);
>> -	else
>> -		entered_state = cpuidle_enter_state(dev, drv, next_state);
>> -
>> -	if (broadcast)
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> +	entered_state = cpuidle_enter(drv, dev, next_state);
>
> Shouldn't the return value be checked here, considering you are
> expecting the driver to return an error code. Another reason I mention
> this is that since you would have done BROADCAST_ENTRY and if this call
> to the broadcast framework succeeds, you will have to do a
> BROADCAST_EXIT irrespective of if the driver could put the CPU to that
> idle state or not. So even if cpuidle_enter() fails, you will need to do
> a clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu),
> although you will have to skip over cpuidle_reflect().
>
> So are there drivers which can return an error code when we call into
> the enter function of the idle states?

Except for the acpi noodle plate driver, no backend driver is returning 
an error.

> If not, you can probably avoid
> checking the error code return value of cpuidle_enter() altogether and
> make it simpler. Its not being checked in the current code too.

Yeah, that is the point. I don't want to mix the changes with this 
patchset. I agree, the code must be fixed but I prefer to do that in a 
separate patch.

Thanks for the review

   -- Daniel

>
> Thanks
>
> Regards
> Preeti U Murthy
>>
>>   	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>>
>>   	/* give the governor an opportunity to reflect on the outcome */
>> -	if (cpuidle_curr_governor->reflect)
>> -		cpuidle_curr_governor->reflect(dev, entered_state);
>> +	cpuidle_reflect(dev, entered_state);
>>
>>   	return 0;
>>   }
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog