Re: [PATCH 2/2] drivers: cpuidle: add driver/device checks in cpuidle_enter_freeze()

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

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);
 }
 
 /**