[PATCH 0/3] PM / Sleep: Suspend core code cleanups

[PATCH 0/3] PM / Sleep: Suspend core code cleanups

[Rafael J. Wysocki]

Hi,

The following three patches clean up the core system suspend code.

[1/3] - Unify and improve kerneldoc comments in kernel/power/suspend.c.
[2/3] - Make enter_state() in kernel/power/suspend.c static.
[3/3] - Drop suspend_stats_update() and move its code to the caller.

They are on top of the patch at:

http://marc.info/?l=linux-kernel&m=132891673021129&w=4

and are v3.4 candidates.

Thanks,
Rafael

[PATCH 1/3] PM / Sleep: Unify kerneldoc comments in kernel/power/suspend.c

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The kerneldoc comments in kernel/power/suspend.c are not formatted
in the same way and the quality of some of them is questionable.
Unify the formatting and improve the contents.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/suspend.c |   56 +++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

Index: linux/kernel/power/suspend.c
===================================================================
--- linux.orig/kernel/power/suspend.c
+++ linux/kernel/power/suspend.c
@@ -37,8 +37,8 @@ const char *const pm_states[PM_SUSPEND_M
 static const struct platform_suspend_ops *suspend_ops;
 
 /**
- *	suspend_set_ops - Set the global suspend method table.
- *	@ops:	Pointer to ops structure.
+ * suspend_set_ops - Set the global suspend method table.
+ * @ops: Suspend operations to use.
  */
 void suspend_set_ops(const struct platform_suspend_ops *ops)
 {
@@ -58,11 +58,11 @@ bool valid_state(suspend_state_t state)
 }
 
 /**
- * suspend_valid_only_mem - generic memory-only valid callback
+ * suspend_valid_only_mem - Generic memory-only valid callback.
  *
- * Platform drivers that implement mem suspend only and only need
- * to check for that in their .valid callback can use this instead
- * of rolling their own .valid callback.
+ * Platform drivers that implement mem suspend only and only need to check for
+ * that in their .valid() callback can use this instead of rolling their own
+ * .valid() callback.
  */
 int suspend_valid_only_mem(suspend_state_t state)
 {
@@ -83,10 +83,11 @@ static int suspend_test(int level)
 }
 
 /**
- *	suspend_prepare - Do prep work before entering low-power state.
+ * suspend_prepare - Prepare for entering system sleep state.
  *
- *	This is common code that is called for each state that we're entering.
- *	Run suspend notifiers, allocate a console and stop all processes.
+ * Common code run for every system sleep state that can be entered (except for
+ * hibernation).  Run suspend notifiers, allocate the "suspend" console and
+ * freeze processes.
  */
 static int suspend_prepare(void)
 {
@@ -131,9 +132,9 @@ void __attribute__ ((weak)) arch_suspend
 }
 
 /**
- * suspend_enter - enter the desired system sleep state.
- * @state: State to enter
- * @wakeup: Returns information that suspend should not be entered again.
+ * suspend_enter - Make the system enter the given sleep state.
+ * @state: System sleep state to enter.
+ * @wakeup: Returns information that the sleep state should not be re-entered.
  *
  * This function should be called after devices have been suspended.
  */
@@ -199,9 +200,8 @@ static int suspend_enter(suspend_state_t
 }
 
 /**
- *	suspend_devices_and_enter - suspend devices and enter the desired system
- *				    sleep state.
- *	@state:		  state to enter
+ * suspend_devices_and_enter - Suspend devices and enter system sleep state.
+ * @state: System sleep state to enter.
  */
 int suspend_devices_and_enter(suspend_state_t state)
 {
@@ -251,10 +251,10 @@ int suspend_devices_and_enter(suspend_st
 }
 
 /**
- *	suspend_finish - Do final work before exiting suspend sequence.
+ * suspend_finish - Clean up before finishing the suspend sequence.
  *
- *	Call platform code to clean up, restart processes, and free the
- *	console that we've allocated. This is not called for suspend-to-disk.
+ * Call platform code to clean up, restart processes, and free the console that
+ * we've allocated. This routine is not called for hibernation.
  */
 static void suspend_finish(void)
 {
@@ -265,14 +265,12 @@ static void suspend_finish(void)
 }
 
 /**
- *	enter_state - Do common work of entering low-power state.
- *	@state:		pm_state structure for state we're entering.
+ * enter_state - Do common work needed to enter system sleep state.
+ * @state: System sleep state to enter.
  *
- *	Make sure we're the only ones trying to enter a sleep state. Fail
- *	if someone has beat us to it, since we don't want anything weird to
- *	happen when we wake up.
- *	Then, do the setup for suspend, enter the state, and cleaup (after
- *	we've woken up).
+ * Make sure that no one else is trying to put the system into a sleep state.
+ * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
+ * system enter the given sleep state and clean up after wakeup.
  */
 int enter_state(suspend_state_t state)
 {
@@ -310,11 +308,11 @@ int enter_state(suspend_state_t state)
 }
 
 /**
- *	pm_suspend - Externally visible function for suspending system.
- *	@state:		Enumerated value of state to enter.
+ * pm_suspend - Externally visible function for suspending the system.
+ * @state: System sleep state to enter.
  *
- *	Determine whether or not value is within range, get state
- *	structure, and enter (above).
+ * Check if the value of @state represents one of the supported states,
+ * execute enter_state() and update system suspend statistics.
  */
 int pm_suspend(suspend_state_t state)
 {

[PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The enter_state() function in kernel/power/suspend.c should be
static and state_store() in kernel/power/suspend.c should call
pm_suspend() instead of it, so make that happen (which also reduces
code duplication related to suspend statistics).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/main.c    |    6 +-----
 kernel/power/power.h   |    2 --
 kernel/power/suspend.c |    2 +-
 3 files changed, 2 insertions(+), 8 deletions(-)

Index: linux/kernel/power/main.c
===================================================================
--- linux.orig/kernel/power/main.c
+++ linux/kernel/power/main.c
@@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
 #ifdef CONFIG_SUSPEND
 	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
 		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
-			break;
-	}
-	if (state < PM_SUSPEND_MAX && *s) {
-		error = enter_state(state);
-		suspend_stats_update(error);
+			error = pm_suspend(state);
 	}
 #endif
 
Index: linux/kernel/power/power.h
===================================================================
--- linux.orig/kernel/power/power.h
+++ linux/kernel/power/power.h
@@ -177,13 +177,11 @@ extern const char *const pm_states[];
 
 extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
-extern int enter_state(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
 	return -ENOSYS;
 }
-static inline int enter_state(suspend_state_t state) { return -ENOSYS; }
 static inline bool valid_state(suspend_state_t state) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
Index: linux/kernel/power/suspend.c
===================================================================
--- linux.orig/kernel/power/suspend.c
+++ linux/kernel/power/suspend.c
@@ -272,7 +272,7 @@ static void suspend_finish(void)
  * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
  * system enter the given sleep state and clean up after wakeup.
  */
-int enter_state(suspend_state_t state)
+static int enter_state(suspend_state_t state)
 {
 	int error;
 

[PATCH 3/3] PM / Sleep: Drop suspend_stats_update()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Since suspend_stats_update() is only called from pm_suspend(),
move its code directly into that function and remove the static
inline definition from include/linux/suspend.h.  Clean up
pm_suspend() in the process.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/suspend.h |   16 ----------------
 kernel/power/suspend.c  |   18 ++++++++++++------
 2 files changed, 12 insertions(+), 22 deletions(-)

Index: linux/include/linux/suspend.h
===================================================================
--- linux.orig/include/linux/suspend.h
+++ linux/include/linux/suspend.h
@@ -95,22 +95,6 @@ static inline void dpm_save_failed_step(
 }
 
 /**
- * suspend_stats_update - Update success/failure statistics of suspend-to-ram
- *
- * @error: Value returned by enter_state() function
- */
-static inline void suspend_stats_update(int error)
-{
-	if (error) {
-		suspend_stats.fail++;
-		dpm_save_failed_errno(error);
-	} else {
-		suspend_stats.success++;
-	}
-}
-
-
-/**
  * struct platform_suspend_ops - Callbacks for managing platform dependent
  *	system sleep states.
  *
Index: linux/kernel/power/suspend.c
===================================================================
--- linux.orig/kernel/power/suspend.c
+++ linux/kernel/power/suspend.c
@@ -316,12 +316,18 @@ static int enter_state(suspend_state_t s
  */
 int pm_suspend(suspend_state_t state)
 {
-	int ret;
-	if (state > PM_SUSPEND_ON && state < PM_SUSPEND_MAX) {
-		ret = enter_state(state);
-		suspend_stats_update(ret);
-		return ret;
+	int error;
+
+	if (state <= PM_SUSPEND_ON || state >= PM_SUSPEND_MAX)
+		return -EINVAL;
+
+	error = enter_state(state);
+	if (error) {
+		suspend_stats.fail++;
+		dpm_save_failed_errno(error);
+	} else {
+		suspend_stats.success++;
 	}
-	return -EINVAL;
+	return error;
 }
 EXPORT_SYMBOL(pm_suspend);

Re: [PATCH 1/3] PM / Sleep: Unify kerneldoc comments in kernel/power/suspend.c

[Srivatsa S. Bhat]

On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The kerneldoc comments in kernel/power/suspend.c are not formatted
> in the same way and the quality of some of them is questionable.
> Unify the formatting and improve the contents.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---


Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

>  kernel/power/suspend.c |   56 +++++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 29 deletions(-)
> 
> Index: linux/kernel/power/suspend.c
> ===================================================================
> --- linux.orig/kernel/power/suspend.c
> +++ linux/kernel/power/suspend.c
> @@ -37,8 +37,8 @@ const char *const pm_states[PM_SUSPEND_M
>  static const struct platform_suspend_ops *suspend_ops;
> 
>  /**
> - *	suspend_set_ops - Set the global suspend method table.
> - *	@ops:	Pointer to ops structure.
> + * suspend_set_ops - Set the global suspend method table.
> + * @ops: Suspend operations to use.
>   */
>  void suspend_set_ops(const struct platform_suspend_ops *ops)
>  {
> @@ -58,11 +58,11 @@ bool valid_state(suspend_state_t state)
>  }
> 
>  /**
> - * suspend_valid_only_mem - generic memory-only valid callback
> + * suspend_valid_only_mem - Generic memory-only valid callback.
>   *
> - * Platform drivers that implement mem suspend only and only need
> - * to check for that in their .valid callback can use this instead
> - * of rolling their own .valid callback.
> + * Platform drivers that implement mem suspend only and only need to check for
> + * that in their .valid() callback can use this instead of rolling their own
> + * .valid() callback.
>   */
>  int suspend_valid_only_mem(suspend_state_t state)
>  {
> @@ -83,10 +83,11 @@ static int suspend_test(int level)
>  }
> 
>  /**
> - *	suspend_prepare - Do prep work before entering low-power state.
> + * suspend_prepare - Prepare for entering system sleep state.
>   *
> - *	This is common code that is called for each state that we're entering.
> - *	Run suspend notifiers, allocate a console and stop all processes.
> + * Common code run for every system sleep state that can be entered (except for
> + * hibernation).  Run suspend notifiers, allocate the "suspend" console and
> + * freeze processes.
>   */
>  static int suspend_prepare(void)
>  {
> @@ -131,9 +132,9 @@ void __attribute__ ((weak)) arch_suspend
>  }
> 
>  /**
> - * suspend_enter - enter the desired system sleep state.
> - * @state: State to enter
> - * @wakeup: Returns information that suspend should not be entered again.
> + * suspend_enter - Make the system enter the given sleep state.
> + * @state: System sleep state to enter.
> + * @wakeup: Returns information that the sleep state should not be re-entered.
>   *
>   * This function should be called after devices have been suspended.
>   */
> @@ -199,9 +200,8 @@ static int suspend_enter(suspend_state_t
>  }
> 
>  /**
> - *	suspend_devices_and_enter - suspend devices and enter the desired system
> - *				    sleep state.
> - *	@state:		  state to enter
> + * suspend_devices_and_enter - Suspend devices and enter system sleep state.
> + * @state: System sleep state to enter.
>   */
>  int suspend_devices_and_enter(suspend_state_t state)
>  {
> @@ -251,10 +251,10 @@ int suspend_devices_and_enter(suspend_st
>  }
> 
>  /**
> - *	suspend_finish - Do final work before exiting suspend sequence.
> + * suspend_finish - Clean up before finishing the suspend sequence.
>   *
> - *	Call platform code to clean up, restart processes, and free the
> - *	console that we've allocated. This is not called for suspend-to-disk.
> + * Call platform code to clean up, restart processes, and free the console that
> + * we've allocated. This routine is not called for hibernation.
>   */
>  static void suspend_finish(void)
>  {
> @@ -265,14 +265,12 @@ static void suspend_finish(void)
>  }
> 
>  /**
> - *	enter_state - Do common work of entering low-power state.
> - *	@state:		pm_state structure for state we're entering.
> + * enter_state - Do common work needed to enter system sleep state.
> + * @state: System sleep state to enter.
>   *
> - *	Make sure we're the only ones trying to enter a sleep state. Fail
> - *	if someone has beat us to it, since we don't want anything weird to
> - *	happen when we wake up.
> - *	Then, do the setup for suspend, enter the state, and cleaup (after
> - *	we've woken up).
> + * Make sure that no one else is trying to put the system into a sleep state.
> + * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
> + * system enter the given sleep state and clean up after wakeup.
>   */
>  int enter_state(suspend_state_t state)
>  {
> @@ -310,11 +308,11 @@ int enter_state(suspend_state_t state)
>  }
> 
>  /**
> - *	pm_suspend - Externally visible function for suspending system.
> - *	@state:		Enumerated value of state to enter.
> + * pm_suspend - Externally visible function for suspending the system.
> + * @state: System sleep state to enter.
>   *
> - *	Determine whether or not value is within range, get state
> - *	structure, and enter (above).
> + * Check if the value of @state represents one of the supported states,
> + * execute enter_state() and update system suspend statistics.
>   */
>  int pm_suspend(suspend_state_t state)
>  {
> 

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Srivatsa S. Bhat]

On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The enter_state() function in kernel/power/suspend.c should be
> static and state_store() in kernel/power/suspend.c should call
> pm_suspend() instead of it, so make that happen (which also reduces
> code duplication related to suspend statistics).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/main.c    |    6 +-----
>  kernel/power/power.h   |    2 --
>  kernel/power/suspend.c |    2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> Index: linux/kernel/power/main.c
> ===================================================================
> --- linux.orig/kernel/power/main.c
> +++ linux/kernel/power/main.c
> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
>  #ifdef CONFIG_SUSPEND
>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> -			break;
> -	}
> -	if (state < PM_SUSPEND_MAX && *s) {
> -		error = enter_state(state);
> -		suspend_stats_update(error);
> +			error = pm_suspend(state);


This code will not stop after calling pm_suspend(). It will try more iterations
in the for loop right?

We can probably keep the 'for' loop as it is, and just replace the 'if' block
following the 'for' loop by: error = pm_suspend(state);

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] PM / Sleep: Drop suspend_stats_update()

[Srivatsa S. Bhat]

On 02/12/2012 04:36 AM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Since suspend_stats_update() is only called from pm_suspend(),
> move its code directly into that function and remove the static
> inline definition from include/linux/suspend.h.  Clean up
> pm_suspend() in the process.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---


Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

>  include/linux/suspend.h |   16 ----------------
>  kernel/power/suspend.c  |   18 ++++++++++++------
>  2 files changed, 12 insertions(+), 22 deletions(-)
> 
> Index: linux/include/linux/suspend.h
> ===================================================================
> --- linux.orig/include/linux/suspend.h
> +++ linux/include/linux/suspend.h
> @@ -95,22 +95,6 @@ static inline void dpm_save_failed_step(
>  }
> 
>  /**
> - * suspend_stats_update - Update success/failure statistics of suspend-to-ram
> - *
> - * @error: Value returned by enter_state() function
> - */
> -static inline void suspend_stats_update(int error)
> -{
> -	if (error) {
> -		suspend_stats.fail++;
> -		dpm_save_failed_errno(error);
> -	} else {
> -		suspend_stats.success++;
> -	}
> -}
> -
> -
> -/**
>   * struct platform_suspend_ops - Callbacks for managing platform dependent
>   *	system sleep states.
>   *
> Index: linux/kernel/power/suspend.c
> ===================================================================
> --- linux.orig/kernel/power/suspend.c
> +++ linux/kernel/power/suspend.c
> @@ -316,12 +316,18 @@ static int enter_state(suspend_state_t s
>   */
>  int pm_suspend(suspend_state_t state)
>  {
> -	int ret;
> -	if (state > PM_SUSPEND_ON && state < PM_SUSPEND_MAX) {
> -		ret = enter_state(state);
> -		suspend_stats_update(ret);
> -		return ret;
> +	int error;
> +
> +	if (state <= PM_SUSPEND_ON || state >= PM_SUSPEND_MAX)
> +		return -EINVAL;
> +
> +	error = enter_state(state);
> +	if (error) {
> +		suspend_stats.fail++;
> +		dpm_save_failed_errno(error);
> +	} else {
> +		suspend_stats.success++;
>  	}
> -	return -EINVAL;
> +	return error;
>  }
>  EXPORT_SYMBOL(pm_suspend);
> 

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Rafael J. Wysocki]

On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The enter_state() function in kernel/power/suspend.c should be
> > static and state_store() in kernel/power/suspend.c should call
> > pm_suspend() instead of it, so make that happen (which also reduces
> > code duplication related to suspend statistics).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/main.c    |    6 +-----
> >  kernel/power/power.h   |    2 --
> >  kernel/power/suspend.c |    2 +-
> >  3 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > Index: linux/kernel/power/main.c
> > ===================================================================
> > --- linux.orig/kernel/power/main.c
> > +++ linux/kernel/power/main.c
> > @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
> >  #ifdef CONFIG_SUSPEND
> >  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> >  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> > -			break;
> > -	}
> > -	if (state < PM_SUSPEND_MAX && *s) {
> > -		error = enter_state(state);
> > -		suspend_stats_update(error);
> > +			error = pm_suspend(state);
> 
> 
> This code will not stop after calling pm_suspend(). It will try more iterations
> in the for loop right?

Well, only one string in pm_states[] can be matched at a time, but I agree that
it doesn't make sense to continue the loop after we've found a match.

> We can probably keep the 'for' loop as it is, and just replace the 'if' block
> following the 'for' loop by: error = pm_suspend(state);

I think the patch below is correct.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static

The enter_state() function in kernel/power/suspend.c should be
static and state_store() in kernel/power/suspend.c should call
pm_suspend() instead of it, so make that happen (which also reduces
code duplication related to suspend statistics).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/main.c    |    8 +++-----
 kernel/power/power.h   |    2 --
 kernel/power/suspend.c |    2 +-
 3 files changed, 4 insertions(+), 8 deletions(-)

Index: linux/kernel/power/main.c
===================================================================
--- linux.orig/kernel/power/main.c
+++ linux/kernel/power/main.c
@@ -291,12 +291,10 @@ static ssize_t state_store(struct kobjec
 
 #ifdef CONFIG_SUSPEND
 	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
-		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
+		if (*s && len == strlen(*s) && !strncmp(buf, *s, len)) {
+			error = pm_suspend(state);
 			break;
-	}
-	if (state < PM_SUSPEND_MAX && *s) {
-		error = enter_state(state);
-		suspend_stats_update(error);
+		}
 	}
 #endif
 
Index: linux/kernel/power/power.h
===================================================================
--- linux.orig/kernel/power/power.h
+++ linux/kernel/power/power.h
@@ -177,13 +177,11 @@ extern const char *const pm_states[];
 
 extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
-extern int enter_state(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
 	return -ENOSYS;
 }
-static inline int enter_state(suspend_state_t state) { return -ENOSYS; }
 static inline bool valid_state(suspend_state_t state) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
Index: linux/kernel/power/suspend.c
===================================================================
--- linux.orig/kernel/power/suspend.c
+++ linux/kernel/power/suspend.c
@@ -272,7 +272,7 @@ static void suspend_finish(void)
  * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
  * system enter the given sleep state and clean up after wakeup.
  */
-int enter_state(suspend_state_t state)
+static int enter_state(suspend_state_t state)
 {
 	int error;
 

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Srivatsa S. Bhat]

On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:

> On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
>> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
>>
>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>
>>> The enter_state() function in kernel/power/suspend.c should be
>>> static and state_store() in kernel/power/suspend.c should call
>>> pm_suspend() instead of it, so make that happen (which also reduces
>>> code duplication related to suspend statistics).
>>>
>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>> ---
>>>  kernel/power/main.c    |    6 +-----
>>>  kernel/power/power.h   |    2 --
>>>  kernel/power/suspend.c |    2 +-
>>>  3 files changed, 2 insertions(+), 8 deletions(-)
>>>
>>> Index: linux/kernel/power/main.c
>>> ===================================================================
>>> --- linux.orig/kernel/power/main.c
>>> +++ linux/kernel/power/main.c
>>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
>>>  #ifdef CONFIG_SUSPEND
>>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
>>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
>>> -			break;
>>> -	}
>>> -	if (state < PM_SUSPEND_MAX && *s) {
>>> -		error = enter_state(state);
>>> -		suspend_stats_update(error);
>>> +			error = pm_suspend(state);
>>
>>
>> This code will not stop after calling pm_suspend(). It will try more iterations
>> in the for loop right?
> 
> Well, only one string in pm_states[] can be matched at a time, but I agree that
> it doesn't make sense to continue the loop after we've found a match.
> 
>> We can probably keep the 'for' loop as it is, and just replace the 'if' block
>> following the 'for' loop by: error = pm_suspend(state);
> 
> I think the patch below is correct.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
> 
> The enter_state() function in kernel/power/suspend.c should be
> static and state_store() in kernel/power/suspend.c should call
> pm_suspend() instead of it, so make that happen (which also reduces
> code duplication related to suspend statistics).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---


Yeah, this version of the patch will work fine.

Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

>  kernel/power/main.c    |    8 +++-----
>  kernel/power/power.h   |    2 --
>  kernel/power/suspend.c |    2 +-
>  3 files changed, 4 insertions(+), 8 deletions(-)
> 
> Index: linux/kernel/power/main.c
> ===================================================================
> --- linux.orig/kernel/power/main.c
> +++ linux/kernel/power/main.c
> @@ -291,12 +291,10 @@ static ssize_t state_store(struct kobjec
> 
>  #ifdef CONFIG_SUSPEND
>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> -		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> +		if (*s && len == strlen(*s) && !strncmp(buf, *s, len)) {
> +			error = pm_suspend(state);
>  			break;
> -	}
> -	if (state < PM_SUSPEND_MAX && *s) {
> -		error = enter_state(state);
> -		suspend_stats_update(error);
> +		}
>  	}
>  #endif
> 
> Index: linux/kernel/power/power.h
> ===================================================================
> --- linux.orig/kernel/power/power.h
> +++ linux/kernel/power/power.h
> @@ -177,13 +177,11 @@ extern const char *const pm_states[];
> 
>  extern bool valid_state(suspend_state_t state);
>  extern int suspend_devices_and_enter(suspend_state_t state);
> -extern int enter_state(suspend_state_t state);
>  #else /* !CONFIG_SUSPEND */
>  static inline int suspend_devices_and_enter(suspend_state_t state)
>  {
>  	return -ENOSYS;
>  }
> -static inline int enter_state(suspend_state_t state) { return -ENOSYS; }
>  static inline bool valid_state(suspend_state_t state) { return false; }
>  #endif /* !CONFIG_SUSPEND */
> 
> Index: linux/kernel/power/suspend.c
> ===================================================================
> --- linux.orig/kernel/power/suspend.c
> +++ linux/kernel/power/suspend.c
> @@ -272,7 +272,7 @@ static void suspend_finish(void)
>   * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
>   * system enter the given sleep state and clean up after wakeup.
>   */
> -int enter_state(suspend_state_t state)
> +static int enter_state(suspend_state_t state)
>  {
>  	int error;
> 
> 

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Rafael J. Wysocki]

On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
> On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:
> 
> > On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
> >> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
> >>
> >>> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>>
> >>> The enter_state() function in kernel/power/suspend.c should be
> >>> static and state_store() in kernel/power/suspend.c should call
> >>> pm_suspend() instead of it, so make that happen (which also reduces
> >>> code duplication related to suspend statistics).
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> ---
> >>>  kernel/power/main.c    |    6 +-----
> >>>  kernel/power/power.h   |    2 --
> >>>  kernel/power/suspend.c |    2 +-
> >>>  3 files changed, 2 insertions(+), 8 deletions(-)
> >>>
> >>> Index: linux/kernel/power/main.c
> >>> ===================================================================
> >>> --- linux.orig/kernel/power/main.c
> >>> +++ linux/kernel/power/main.c
> >>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
> >>>  #ifdef CONFIG_SUSPEND
> >>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> >>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> >>> -			break;
> >>> -	}
> >>> -	if (state < PM_SUSPEND_MAX && *s) {
> >>> -		error = enter_state(state);
> >>> -		suspend_stats_update(error);
> >>> +			error = pm_suspend(state);
> >>
> >>
> >> This code will not stop after calling pm_suspend(). It will try more iterations
> >> in the for loop right?
> > 
> > Well, only one string in pm_states[] can be matched at a time, but I agree that
> > it doesn't make sense to continue the loop after we've found a match.
> > 
> >> We can probably keep the 'for' loop as it is, and just replace the 'if' block
> >> following the 'for' loop by: error = pm_suspend(state);
> > 
> > I think the patch below is correct.
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
> > 
> > The enter_state() function in kernel/power/suspend.c should be
> > static and state_store() in kernel/power/suspend.c should call
> > pm_suspend() instead of it, so make that happen (which also reduces
> > code duplication related to suspend statistics).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> 
> 
> Yeah, this version of the patch will work fine.
> 
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Thanks!

I wonder if that should be Reviewed-by, tough?  You evidently have reviewed
the patch (actually, all three of them).

Rafael


> >  kernel/power/main.c    |    8 +++-----
> >  kernel/power/power.h   |    2 --
> >  kernel/power/suspend.c |    2 +-
> >  3 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > Index: linux/kernel/power/main.c
> > ===================================================================
> > --- linux.orig/kernel/power/main.c
> > +++ linux/kernel/power/main.c
> > @@ -291,12 +291,10 @@ static ssize_t state_store(struct kobjec
> > 
> >  #ifdef CONFIG_SUSPEND
> >  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> > -		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> > +		if (*s && len == strlen(*s) && !strncmp(buf, *s, len)) {
> > +			error = pm_suspend(state);
> >  			break;
> > -	}
> > -	if (state < PM_SUSPEND_MAX && *s) {
> > -		error = enter_state(state);
> > -		suspend_stats_update(error);
> > +		}
> >  	}
> >  #endif
> > 
> > Index: linux/kernel/power/power.h
> > ===================================================================
> > --- linux.orig/kernel/power/power.h
> > +++ linux/kernel/power/power.h
> > @@ -177,13 +177,11 @@ extern const char *const pm_states[];
> > 
> >  extern bool valid_state(suspend_state_t state);
> >  extern int suspend_devices_and_enter(suspend_state_t state);
> > -extern int enter_state(suspend_state_t state);
> >  #else /* !CONFIG_SUSPEND */
> >  static inline int suspend_devices_and_enter(suspend_state_t state)
> >  {
> >  	return -ENOSYS;
> >  }
> > -static inline int enter_state(suspend_state_t state) { return -ENOSYS; }
> >  static inline bool valid_state(suspend_state_t state) { return false; }
> >  #endif /* !CONFIG_SUSPEND */
> > 
> > Index: linux/kernel/power/suspend.c
> > ===================================================================
> > --- linux.orig/kernel/power/suspend.c
> > +++ linux/kernel/power/suspend.c
> > @@ -272,7 +272,7 @@ static void suspend_finish(void)
> >   * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
> >   * system enter the given sleep state and clean up after wakeup.
> >   */
> > -int enter_state(suspend_state_t state)
> > +static int enter_state(suspend_state_t state)
> >  {
> >  	int error;
> > 
> > 
> 
> 
> 

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Srivatsa S. Bhat]

On 02/13/2012 08:42 PM, Rafael J. Wysocki wrote:

> On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
>> On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:
>>
>>> On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
>>>> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
>>>>
>>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>
>>>>> The enter_state() function in kernel/power/suspend.c should be
>>>>> static and state_store() in kernel/power/suspend.c should call
>>>>> pm_suspend() instead of it, so make that happen (which also reduces
>>>>> code duplication related to suspend statistics).
>>>>>
>>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>>>> ---
>>>>>  kernel/power/main.c    |    6 +-----
>>>>>  kernel/power/power.h   |    2 --
>>>>>  kernel/power/suspend.c |    2 +-
>>>>>  3 files changed, 2 insertions(+), 8 deletions(-)
>>>>>
>>>>> Index: linux/kernel/power/main.c
>>>>> ===================================================================
>>>>> --- linux.orig/kernel/power/main.c
>>>>> +++ linux/kernel/power/main.c
>>>>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
>>>>>  #ifdef CONFIG_SUSPEND
>>>>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
>>>>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
>>>>> -			break;
>>>>> -	}
>>>>> -	if (state < PM_SUSPEND_MAX && *s) {
>>>>> -		error = enter_state(state);
>>>>> -		suspend_stats_update(error);
>>>>> +			error = pm_suspend(state);
>>>>
>>>>
>>>> This code will not stop after calling pm_suspend(). It will try more iterations
>>>> in the for loop right?
>>>
>>> Well, only one string in pm_states[] can be matched at a time, but I agree that
>>> it doesn't make sense to continue the loop after we've found a match.
>>>
>>>> We can probably keep the 'for' loop as it is, and just replace the 'if' block
>>>> following the 'for' loop by: error = pm_suspend(state);
>>>
>>> I think the patch below is correct.
>>>
>>> Thanks,
>>> Rafael
>>>
>>> ---
>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>> Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
>>>
>>> The enter_state() function in kernel/power/suspend.c should be
>>> static and state_store() in kernel/power/suspend.c should call
>>> pm_suspend() instead of it, so make that happen (which also reduces
>>> code duplication related to suspend statistics).
>>>
>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>> ---
>>
>>
>> Yeah, this version of the patch will work fine.
>>
>> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> Thanks!
> 
> I wonder if that should be Reviewed-by, tough?  You evidently have reviewed
> the patch (actually, all three of them).
> 


Anything is fine :-) It is not very clear to me when to use Reviewed-by and
when to use Acked-by.. so I randomly chose one of them.. :-)
But please enlighten me as to when to use which one, so that in the future, I
can use the right one :-)

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Rafael J. Wysocki]

On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
> On 02/13/2012 08:42 PM, Rafael J. Wysocki wrote:
> 
> > On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
> >> On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:
> >>
> >>> On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
> >>>> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
> >>>>
> >>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>>>>
> >>>>> The enter_state() function in kernel/power/suspend.c should be
> >>>>> static and state_store() in kernel/power/suspend.c should call
> >>>>> pm_suspend() instead of it, so make that happen (which also reduces
> >>>>> code duplication related to suspend statistics).
> >>>>>
> >>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>>>> ---
> >>>>>  kernel/power/main.c    |    6 +-----
> >>>>>  kernel/power/power.h   |    2 --
> >>>>>  kernel/power/suspend.c |    2 +-
> >>>>>  3 files changed, 2 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> Index: linux/kernel/power/main.c
> >>>>> ===================================================================
> >>>>> --- linux.orig/kernel/power/main.c
> >>>>> +++ linux/kernel/power/main.c
> >>>>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
> >>>>>  #ifdef CONFIG_SUSPEND
> >>>>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> >>>>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> >>>>> -			break;
> >>>>> -	}
> >>>>> -	if (state < PM_SUSPEND_MAX && *s) {
> >>>>> -		error = enter_state(state);
> >>>>> -		suspend_stats_update(error);
> >>>>> +			error = pm_suspend(state);
> >>>>
> >>>>
> >>>> This code will not stop after calling pm_suspend(). It will try more iterations
> >>>> in the for loop right?
> >>>
> >>> Well, only one string in pm_states[] can be matched at a time, but I agree that
> >>> it doesn't make sense to continue the loop after we've found a match.
> >>>
> >>>> We can probably keep the 'for' loop as it is, and just replace the 'if' block
> >>>> following the 'for' loop by: error = pm_suspend(state);
> >>>
> >>> I think the patch below is correct.
> >>>
> >>> Thanks,
> >>> Rafael
> >>>
> >>> ---
> >>> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>> Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
> >>>
> >>> The enter_state() function in kernel/power/suspend.c should be
> >>> static and state_store() in kernel/power/suspend.c should call
> >>> pm_suspend() instead of it, so make that happen (which also reduces
> >>> code duplication related to suspend statistics).
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> ---
> >>
> >>
> >> Yeah, this version of the patch will work fine.
> >>
> >> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > 
> > Thanks!
> > 
> > I wonder if that should be Reviewed-by, tough?  You evidently have reviewed
> > the patch (actually, all three of them).
> > 
> 
> 
> Anything is fine :-) It is not very clear to me when to use Reviewed-by and
> when to use Acked-by.. so I randomly chose one of them.. :-)
> But please enlighten me as to when to use which one, so that in the future, I
> can use the right one :-)

"Acked-by" means, roughly, "I have no objection" or "looks good to me",
depending on who's saying that, but it doesn't imply that you've had more than
a cursory look at the patch in question.  "Reviewied-by", in contrast, means
something like "I have reviewed the patch in detail and haven't found anything
wrong in it" (which obviously means that you have no objection too, because
otherwise you'd have found _something_ wrong in the patch).

So, "Acked-by" from anyone other than the relevant maintainer is just an
"I'm for" declaration, while "Reviewied-by" from _anyone_ carries some actual
weight. 

Thanks,
Rafael

Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

[Srivatsa S. Bhat]

On 02/13/2012 09:23 PM, Rafael J. Wysocki wrote:

> On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
>> On 02/13/2012 08:42 PM, Rafael J. Wysocki wrote:
>>
>>> On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
>>>> On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:
>>>>
>>>>> On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
>>>>>> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>>
>>>>>>> The enter_state() function in kernel/power/suspend.c should be
>>>>>>> static and state_store() in kernel/power/suspend.c should call
>>>>>>> pm_suspend() instead of it, so make that happen (which also reduces
>>>>>>> code duplication related to suspend statistics).
>>>>>>>
>>>>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>> ---
>>>>>>>  kernel/power/main.c    |    6 +-----
>>>>>>>  kernel/power/power.h   |    2 --
>>>>>>>  kernel/power/suspend.c |    2 +-
>>>>>>>  3 files changed, 2 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> Index: linux/kernel/power/main.c
>>>>>>> ===================================================================
>>>>>>> --- linux.orig/kernel/power/main.c
>>>>>>> +++ linux/kernel/power/main.c
>>>>>>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
>>>>>>>  #ifdef CONFIG_SUSPEND
>>>>>>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
>>>>>>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
>>>>>>> -			break;
>>>>>>> -	}
>>>>>>> -	if (state < PM_SUSPEND_MAX && *s) {
>>>>>>> -		error = enter_state(state);
>>>>>>> -		suspend_stats_update(error);
>>>>>>> +			error = pm_suspend(state);
>>>>>>
>>>>>>
>>>>>> This code will not stop after calling pm_suspend(). It will try more iterations
>>>>>> in the for loop right?
>>>>>
>>>>> Well, only one string in pm_states[] can be matched at a time, but I agree that
>>>>> it doesn't make sense to continue the loop after we've found a match.
>>>>>
>>>>>> We can probably keep the 'for' loop as it is, and just replace the 'if' block
>>>>>> following the 'for' loop by: error = pm_suspend(state);
>>>>>
>>>>> I think the patch below is correct.
>>>>>
>>>>> Thanks,
>>>>> Rafael
>>>>>
>>>>> ---
>>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>>> Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
>>>>>
>>>>> The enter_state() function in kernel/power/suspend.c should be
>>>>> static and state_store() in kernel/power/suspend.c should call
>>>>> pm_suspend() instead of it, so make that happen (which also reduces
>>>>> code duplication related to suspend statistics).
>>>>>
>>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>>>> ---
>>>>
>>>>
>>>> Yeah, this version of the patch will work fine.
>>>>
>>>> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>>
>>> Thanks!
>>>
>>> I wonder if that should be Reviewed-by, tough?  You evidently have reviewed
>>> the patch (actually, all three of them).
>>>
>>
>>
>> Anything is fine :-) It is not very clear to me when to use Reviewed-by and
>> when to use Acked-by.. so I randomly chose one of them.. :-)
>> But please enlighten me as to when to use which one, so that in the future, I
>> can use the right one :-)
> 
> "Acked-by" means, roughly, "I have no objection" or "looks good to me",
> depending on who's saying that, but it doesn't imply that you've had more than
> a cursory look at the patch in question.  "Reviewied-by", in contrast, means
> something like "I have reviewed the patch in detail and haven't found anything
> wrong in it" (which obviously means that you have no objection too, because
> otherwise you'd have found _something_ wrong in the patch).
> 
> So, "Acked-by" from anyone other than the relevant maintainer is just an
> "I'm for" declaration, while "Reviewied-by" from _anyone_ carries some actual
> weight. 
> 

Wow! Nice :-) Thanks for the great explanation!
I'll keep this in mind from the next time onwards :-)

Regards,
Srivatsa S. Bhat