lkml.org 
[lkml]   [2015]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()
Date
On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
> On Wed, Feb 25, 2015 at 02:30:49PM +0000, Daniel Lezcano wrote:
> > On 02/24/2015 06:58 PM, Lorenzo Pieralisi wrote:
> > > The changes in commit:
> > >
> > > 381063133246 ("PM / sleep: Re-implement suspend-to-idle handling")
> > >
> > > let suspend-to-idle code bypass the cpuidle_select() function to
> > > enter the deepest idle state. The sanity checks carried out in
> > > cpuidle_select() are bypassed too and this can cause breakage
> > > on systems that try to suspend-to-idle with no registered cpuidle
> > > driver.
> > >
> > > This patch factors out a function cpuidle_device_disabled() that
> > > is used to carry out sanity checks (ie CPUidle is disabled on the
> > > cpu executing the code) in both cpuidle_select() and cpuidle_enter_freeze()
> > > so that the checks are unified and carried out in both control paths.
> > >
> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > > drivers/cpuidle/cpuidle.c | 18 +++++++++++++-----
> > > 1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index f47edc6c..344fe6c 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -44,6 +44,12 @@ void disable_cpuidle(void)
> > > off = 1;
> > > }
> > >
> > > +static bool cpuidle_device_disabled(struct cpuidle_driver *drv,
> > > + struct cpuidle_device *dev)
> > > +{
> > > + return (off || !initialized || !drv || !dev || !dev->enabled);
> > > +}
> >
> > This is getting a bit fuzzy IMO. What means disabled ? :)
>
> Well, that's just the current checks in cpuidle_select() (that by
> the way is supposed to return an index) merged together with a function
> name, to reuse the same checks in cpuidle_enter_freeze().
> I have no problem leaving the checks as they are at the moment and
> replicate them in cpuidle_enter_freeze() but given your remark below,
> we should do something different in there.

Maybe something like the patch below (untested)?

Rafael


---
drivers/cpuidle/cpuidle.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,18 @@ void disable_cpuidle(void)
off = 1;
}

+int cpuidle_check_availability(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+ if (off || !initialized)
+ return -ENODEV;
+
+ if (!drv || !dev || !dev->enabled)
+ return -EBUSY;
+
+ return 0;
+}
+
/**
* cpuidle_play_dead - cpu off-lining
*
@@ -76,8 +88,13 @@ static int cpuidle_find_deepest_state(st
struct cpuidle_device *dev, bool freeze)
{
unsigned int latency_req = 0;
- int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
+ int i, ret;
+
+ ret = cpuidle_check_availability(drv, dev);
+ if (ret)
+ return ret;

+ ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
struct cpuidle_state *s = &drv->states[i];
struct cpuidle_state_usage *su = &dev->states_usage[i];
@@ -205,13 +222,8 @@ int cpuidle_enter_state(struct cpuidle_d
*/
int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
{
- if (off || !initialized)
- return -ENODEV;
-
- if (!drv || !dev || !dev->enabled)
- return -EBUSY;
-
- return cpuidle_curr_governor->select(drv, dev);
+ int ret = cpuidle_check_availability(drv, dev);
+ return ret ? ret : cpuidle_curr_governor->select(drv, dev);
}

/**


\
 
 \ /
  Last update: 2015-02-26 00:41    [W:0.107 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site