| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <530C39AE.2050905@linaro.org>
Date: Tue, 25 Feb 2014 07:35:26 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
CC: mingo@...nel.org, peterz@...radead.org, tglx@...utronix.de,
rjw@...ysocki.net, nicolas.pitre@...aro.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function
into smaller functions
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists