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

2015-02-25 Thread Rafael J. Wysocki 
On Thursday, February 26, 2015 12:50:58 AM Rafael J. Wysocki wrote:
> On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
> > On Wed, Feb 25, 2015 at 02:30:49PM +, 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 
> > > > Cc: Daniel Lezcano 
> > > > Signed-off-by: Lorenzo Pieralisi 
> > > > ---
> > > >   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)?

That's slightly inefficient for things that don't support ->enter_freeze
and, moreover, all negative return values of cpuidle_select() are equivalent,
so I ended up with something similar to the original Lorenzo's patch (on
top of https://patchwork.kernel.org/patch/5885171/ (still untested).

Rafael


---
 drivers/cpuidle/cpuidle.c |   23 ---
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,12 @@ void disable_cpuidle(void)
off = 1;
 }
 
+static bool cpuidle_not_available(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+   return off || !initialized || !drv || !dev || !dev->enabled;
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,9 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;
 
+   if (cpuidle_not_available(drv, dev))
+   goto fallback;
+
/*
 * Find the deepest state with ->enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -141,11 +150,14 @@ void cpuidle_enter_freeze(void)
 * at all and try to enter it normally.
 */
index = cpuidle_find_deepest_state(drv, dev, false);
-   if (index >= 0)
+   if (index >= 0) {
cpuidle_enter(drv, dev, index);
-   else
-   arch_cpu_idle();
+   /* Interrupts are enabled again here. */
+   return;
+   }
 
+ fallback:
+   arch_cpu_idle();
/* Interrupts are enabled again here. */
 }
 
@@ -205,12 +217,9 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-   if (off || !initialized)
+   if (cpuidle_not_available(drv, dev))
return -ENODEV;
 
-   if (!drv || !dev || !dev->enabled)
-   return -EBUSY;
-
return cpuidle_curr_governor->select(drv, dev);
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Rafael J. Wysocki 
On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
> On Wed, Feb 25, 2015 at 02:30:49PM +, 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 
> > > Cc: Daniel Lezcano 
> > > Signed-off-by: Lorenzo Pieralisi 
> > > ---
> > >   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 = >states[i];
struct cpuidle_state_usage *su = >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);
 }
 
 /**

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Lorenzo Pieralisi 
On Wed, Feb 25, 2015 at 02:30:49PM +, Daniel Lezcano wrote:

[...]

> > 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 ? :)
> 
> >   /**
> >* cpuidle_play_dead - cpu off-lining
> >*
> > @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
> > struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> > int index;
> 
> I think this is exploding before because of dev == NULL in the line above.

Actually not, cpuidle_get_cpu_driver() checks the dev pointer, so
we might end up with drv == NULL and dev == NULL, and the check I added
still applies and it is effective I think.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Lorenzo Pieralisi 
On Wed, Feb 25, 2015 at 02:30:49PM +, 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 
> > Cc: Daniel Lezcano 
> > Signed-off-by: Lorenzo Pieralisi 
> > ---
> >   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.

> 
> >   /**
> >* cpuidle_play_dead - cpu off-lining
> >*
> > @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
> > struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> > int index;
> 
> I think this is exploding before because of dev == NULL in the line above.

Yes, good point so my attempt at consolidating the sanity checks above
is not valid, but something has to be done regardless.

Lorenzo

> > +   if (cpuidle_device_disabled(drv, dev)) {
> > +   arch_cpu_idle();
> > +   return;
> > +   }
> > +
> > /*
> >  * Find the deepest state with ->enter_freeze present, which guarantees
> >  * that interrupts won't be enabled when it exits and allows the tick to
> > @@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, 
> > struct cpuidle_driver *drv,
> >*/
> >   int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> >   {
> > -   if (off || !initialized)
> > -   return -ENODEV;
> > -
> > -   if (!drv || !dev || !dev->enabled)
> > -   return -EBUSY;
> > +   if (cpuidle_device_disabled(drv, dev))
> > +   return -1;
> >
> > return cpuidle_curr_governor->select(drv, dev);
> >   }
> >
> 
> 
> -- 
>    Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Daniel Lezcano 

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 
Cc: Daniel Lezcano 
Signed-off-by: Lorenzo Pieralisi 
---
  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 ? :)


  /**
   * cpuidle_play_dead - cpu off-lining
   *
@@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;


I think this is exploding before because of dev == NULL in the line above.


+   if (cpuidle_device_disabled(drv, dev)) {
+   arch_cpu_idle();
+   return;
+   }
+
/*
 * Find the deepest state with ->enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct 
cpuidle_driver *drv,
   */
  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
  {
-   if (off || !initialized)
-   return -ENODEV;
-
-   if (!drv || !dev || !dev->enabled)
-   return -EBUSY;
+   if (cpuidle_device_disabled(drv, dev))
+   return -1;

return cpuidle_curr_governor->select(drv, dev);
  }




--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Daniel Lezcano 

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 r...@rjwysocki.net
Cc: Daniel Lezcano daniel.lezc...@linaro.org
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@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 ? :)


  /**
   * cpuidle_play_dead - cpu off-lining
   *
@@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;


I think this is exploding before because of dev == NULL in the line above.


+   if (cpuidle_device_disabled(drv, dev)) {
+   arch_cpu_idle();
+   return;
+   }
+
/*
 * Find the deepest state with -enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct 
cpuidle_driver *drv,
   */
  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
  {
-   if (off || !initialized)
-   return -ENODEV;
-
-   if (!drv || !dev || !dev-enabled)
-   return -EBUSY;
+   if (cpuidle_device_disabled(drv, dev))
+   return -1;

return cpuidle_curr_governor-select(drv, dev);
  }




--
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Lorenzo Pieralisi 
On Wed, Feb 25, 2015 at 02:30:49PM +, Daniel Lezcano wrote:

[...]

  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 ? :)
 
/**
 * cpuidle_play_dead - cpu off-lining
 *
  @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
  struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
  int index;
 
 I think this is exploding before because of dev == NULL in the line above.

Actually not, cpuidle_get_cpu_driver() checks the dev pointer, so
we might end up with drv == NULL and dev == NULL, and the check I added
still applies and it is effective I think.

Lorenzo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Lorenzo Pieralisi 
On Wed, Feb 25, 2015 at 02:30:49PM +, 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 r...@rjwysocki.net
  Cc: Daniel Lezcano daniel.lezc...@linaro.org
  Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@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.

 
/**
 * cpuidle_play_dead - cpu off-lining
 *
  @@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
  struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
  int index;
 
 I think this is exploding before because of dev == NULL in the line above.

Yes, good point so my attempt at consolidating the sanity checks above
is not valid, but something has to be done regardless.

Lorenzo

  +   if (cpuidle_device_disabled(drv, dev)) {
  +   arch_cpu_idle();
  +   return;
  +   }
  +
  /*
   * Find the deepest state with -enter_freeze present, which guarantees
   * that interrupts won't be enabled when it exits and allows the tick to
  @@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, 
  struct cpuidle_driver *drv,
 */
int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
{
  -   if (off || !initialized)
  -   return -ENODEV;
  -
  -   if (!drv || !dev || !dev-enabled)
  -   return -EBUSY;
  +   if (cpuidle_device_disabled(drv, dev))
  +   return -1;
 
  return cpuidle_curr_governor-select(drv, dev);
}
 
 
 
 -- 
   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Rafael J. Wysocki 
On Thursday, February 26, 2015 12:50:58 AM Rafael J. Wysocki wrote:
 On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
  On Wed, Feb 25, 2015 at 02:30:49PM +, 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 r...@rjwysocki.net
Cc: Daniel Lezcano daniel.lezc...@linaro.org
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@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)?

That's slightly inefficient for things that don't support -enter_freeze
and, moreover, all negative return values of cpuidle_select() are equivalent,
so I ended up with something similar to the original Lorenzo's patch (on
top of https://patchwork.kernel.org/patch/5885171/ (still untested).

Rafael


---
 drivers/cpuidle/cpuidle.c |   23 ---
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -44,6 +44,12 @@ void disable_cpuidle(void)
off = 1;
 }
 
+static bool cpuidle_not_available(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+   return off || !initialized || !drv || !dev || !dev-enabled;
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,9 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;
 
+   if (cpuidle_not_available(drv, dev))
+   goto fallback;
+
/*
 * Find the deepest state with -enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -141,11 +150,14 @@ void cpuidle_enter_freeze(void)
 * at all and try to enter it normally.
 */
index = cpuidle_find_deepest_state(drv, dev, false);
-   if (index = 0)
+   if (index = 0) {
cpuidle_enter(drv, dev, index);
-   else
-   arch_cpu_idle();
+   /* Interrupts are enabled again here. */
+   return;
+   }
 
+ fallback:
+   arch_cpu_idle();
/* Interrupts are enabled again here. */
 }
 
@@ -205,12 +217,9 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-   if (off || !initialized)
+   if (cpuidle_not_available(drv, dev))
return -ENODEV;
 
-   if (!drv || !dev || !dev-enabled)
-   return -EBUSY;
-
return cpuidle_curr_governor-select(drv, dev);
 }
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Rafael J. Wysocki 
On Wednesday, February 25, 2015 02:47:37 PM Lorenzo Pieralisi wrote:
 On Wed, Feb 25, 2015 at 02:30:49PM +, 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 r...@rjwysocki.net
   Cc: Daniel Lezcano daniel.lezc...@linaro.org
   Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@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);
 }
 
 /**

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-24 Thread Lorenzo Pieralisi 
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 
Cc: Daniel Lezcano 
Signed-off-by: Lorenzo Pieralisi 
---
 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);
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;
 
+   if (cpuidle_device_disabled(drv, dev)) {
+   arch_cpu_idle();
+   return;
+   }
+
/*
 * Find the deepest state with ->enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct 
cpuidle_driver *drv,
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-   if (off || !initialized)
-   return -ENODEV;
-
-   if (!drv || !dev || !dev->enabled)
-   return -EBUSY;
+   if (cpuidle_device_disabled(drv, dev))
+   return -1;
 
return cpuidle_curr_governor->select(drv, dev);
 }
-- 
2.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-24 Thread Lorenzo Pieralisi 
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 r...@rjwysocki.net
Cc: Daniel Lezcano daniel.lezc...@linaro.org
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@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);
+}
+
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -124,6 +130,11 @@ void cpuidle_enter_freeze(void)
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int index;
 
+   if (cpuidle_device_disabled(drv, dev)) {
+   arch_cpu_idle();
+   return;
+   }
+
/*
 * Find the deepest state with -enter_freeze present, which guarantees
 * that interrupts won't be enabled when it exits and allows the tick to
@@ -202,11 +213,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct 
cpuidle_driver *drv,
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
-   if (off || !initialized)
-   return -ENODEV;
-
-   if (!drv || !dev || !dev-enabled)
-   return -EBUSY;
+   if (cpuidle_device_disabled(drv, dev))
+   return -1;
 
return cpuidle_curr_governor-select(drv, dev);
 }
-- 
2.2.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/