[PATCH 1/2] acpi choose sleep state help

[PATCH 1/2] acpi choose sleep state help

[Shaohua Li]

Based on David's patch
http://marc.info/?l=linux-acpi&m=117873972806360&w=2
I slightly changed it.

Add a helper routine, which gets the sleep state of a ACPI device.

Index: 2.6.22-rc2/drivers/acpi/sleep/main.c
===================================================================
--- 2.6.22-rc2.orig/drivers/acpi/sleep/main.c	2007-05-23 09:15:14.000000000 +0800
+++ 2.6.22-rc2/drivers/acpi/sleep/main.c	2007-06-19 09:19:09.000000000 +0800
@@ -34,6 +34,8 @@ static u32 acpi_suspend_states[] = {
 
 static int init_8259A_after_S1;
 
+static int acpi_target_sleep_state = ACPI_STATE_S0;
+
 /**
  *	acpi_pm_prepare - Do preliminary suspend work.
  *	@pm_state:		suspend state we're entering.
@@ -54,6 +56,7 @@ static int acpi_pm_prepare(suspend_state
 		printk("acpi_pm_prepare does not support %d \n", pm_state);
 		return -EPERM;
 	}
+	acpi_target_sleep_state = acpi_state;
 	return acpi_sleep_prepare(acpi_state);
 }
 
@@ -140,6 +143,7 @@ static int acpi_pm_finish(suspend_state_
 		printk("Broken toshiba laptop -> kicking interrupts\n");
 		init_8259A(0);
 	}
+	acpi_target_sleep_state = ACPI_STATE_S0;
 	return 0;
 }
 
@@ -184,6 +188,7 @@ static struct pm_ops acpi_pm_ops = {
 #ifdef CONFIG_SOFTWARE_SUSPEND
 static int acpi_hibernation_prepare(void)
 {
+	acpi_target_sleep_state = ACPI_STATE_S4;
 	return acpi_sleep_prepare(ACPI_STATE_S4);
 }
 
@@ -215,6 +220,7 @@ static void acpi_hibernation_finish(void
 		printk("Broken toshiba laptop -> kicking interrupts\n");
 		init_8259A(0);
 	}
+	acpi_target_sleep_state = ACPI_STATE_S0;
 }
 
 static struct hibernation_ops acpi_hibernation_ops = {
@@ -224,6 +230,49 @@ static struct hibernation_ops acpi_hiber
 };
 #endif				/* CONFIG_SOFTWARE_SUSPEND */
 
+int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state)
+{
+	char sxd[] = "_SxD";
+	unsigned long d_min, d_max;
+	struct acpi_device *dev;
+
+	/*
+	 * If the sleep state is S0, we will return D3, but if the device has
+	 * _S0W, we will use the value from _S0W
+	 */
+	d_min = ACPI_STATE_D3;
+	d_max = ACPI_STATE_D3;
+
+	/* If present, _SxD methods give the minimum D-state we may use
+	 * for each S-state ... with lowest latency state switching.
+	 *
+	 * We rely on acpi_evaluate_integer() not clobbering the integer
+	 * provided -- that's our fault recovery, we ignore retval.
+	 */
+	sxd[2] = '0' + acpi_target_sleep_state;
+	if (acpi_target_sleep_state > ACPI_STATE_S0)
+		acpi_evaluate_integer(handle, sxd, NULL, &d_min);
+
+	/*
+	 * If _PRW says we can wake from the upcoming system state, the _SxD
+	 * value can wake ... and we'll assume a wakeup-aware driver.  If _SxW
+	 * methods exist (ACPI 3.x), they give the lowest power D-state that
+	 * can also wake the system.  _S0W can be valid.
+	 */
+	if (acpi_bus_get_device(handle, &dev)) {
+		printk(KERN_ERR"ACPI handle hasn't context\n");
+		return d_max;
+	}
+	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
+	    (dev->wakeup.state.enabled &&
+	     dev->wakeup.sleep_state <= acpi_target_sleep_state)) {
+		sxd[3] = 'W';
+		acpi_evaluate_integer(handle, sxd, NULL, &d_max);
+	}
+	/* choose a state within d_min to d_max, we just choose the max */
+	return d_max;
+}
+
 /*
  * Toshiba fails to preserve interrupts over S1, reinitialization
  * of 8259 is needed after S1 resume.
Index: 2.6.22-rc2/include/acpi/acpi_bus.h
===================================================================
--- 2.6.22-rc2.orig/include/acpi/acpi_bus.h	2007-05-23 09:15:14.000000000 +0800
+++ 2.6.22-rc2/include/acpi/acpi_bus.h	2007-06-19 09:23:54.000000000 +0800
@@ -364,6 +364,8 @@ acpi_handle acpi_get_child(acpi_handle, 
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);
 #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
 
+int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state);
+
 #endif				/* CONFIG_ACPI */
 
 #endif /*__ACPI_BUS_H__*/

Re: [PATCH 1/2] acpi choose sleep state help

[Rafael J. Wysocki]

On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> Based on David's patch
> http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> I slightly changed it.
> 
> Add a helper routine, which gets the sleep state of a ACPI device.

Is it going to work with the recent code ordering changes?  I mean,
acpi_pm_prepare() is now called after device_suspend() (and analogously for
the hibernation), so the target ACPI state is not known when the drivers'
.suspend() routines are being called.

> Index: 2.6.22-rc2/drivers/acpi/sleep/main.c
> ===================================================================
> --- 2.6.22-rc2.orig/drivers/acpi/sleep/main.c	2007-05-23 09:15:14.000000000 +0800
> +++ 2.6.22-rc2/drivers/acpi/sleep/main.c	2007-06-19 09:19:09.000000000 +0800
> @@ -34,6 +34,8 @@ static u32 acpi_suspend_states[] = {
>  
>  static int init_8259A_after_S1;
>  
> +static int acpi_target_sleep_state = ACPI_STATE_S0;
> +
>  /**
>   *	acpi_pm_prepare - Do preliminary suspend work.
>   *	@pm_state:		suspend state we're entering.
> @@ -54,6 +56,7 @@ static int acpi_pm_prepare(suspend_state
>  		printk("acpi_pm_prepare does not support %d \n", pm_state);
>  		return -EPERM;
>  	}
> +	acpi_target_sleep_state = acpi_state;
>  	return acpi_sleep_prepare(acpi_state);
>  }
>  
> @@ -140,6 +143,7 @@ static int acpi_pm_finish(suspend_state_
>  		printk("Broken toshiba laptop -> kicking interrupts\n");
>  		init_8259A(0);
>  	}
> +	acpi_target_sleep_state = ACPI_STATE_S0;
>  	return 0;
>  }
>  
> @@ -184,6 +188,7 @@ static struct pm_ops acpi_pm_ops = {
>  #ifdef CONFIG_SOFTWARE_SUSPEND
>  static int acpi_hibernation_prepare(void)
>  {
> +	acpi_target_sleep_state = ACPI_STATE_S4;
>  	return acpi_sleep_prepare(ACPI_STATE_S4);
>  }
>  
> @@ -215,6 +220,7 @@ static void acpi_hibernation_finish(void
>  		printk("Broken toshiba laptop -> kicking interrupts\n");
>  		init_8259A(0);
>  	}
> +	acpi_target_sleep_state = ACPI_STATE_S0;

This will clash with the recent Pavel's patch that removes the Toshiba
quirk from the hibernation code path

http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.22-rc5/patches/35-ACPI-remove-S1-workaround-from-hibernation-code-path.patch

>  }
>  
>  static struct hibernation_ops acpi_hibernation_ops = {
> @@ -224,6 +230,49 @@ static struct hibernation_ops acpi_hiber
>  };
>  #endif				/* CONFIG_SOFTWARE_SUSPEND */
>  
> +int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state)
> +{

The second argument doesn't seem to be used.  Is that intentional and if so,
then why?

> +	char sxd[] = "_SxD";
> +	unsigned long d_min, d_max;
> +	struct acpi_device *dev;
> +
> +	/*
> +	 * If the sleep state is S0, we will return D3, but if the device has
> +	 * _S0W, we will use the value from _S0W
> +	 */

Hmm, is the comment right?  Why should we return D3 for S0?

> +	d_min = ACPI_STATE_D3;
> +	d_max = ACPI_STATE_D3;
> +
> +	/* If present, _SxD methods give the minimum D-state we may use
> +	 * for each S-state ... with lowest latency state switching.
> +	 *
> +	 * We rely on acpi_evaluate_integer() not clobbering the integer
> +	 * provided -- that's our fault recovery, we ignore retval.
> +	 */
> +	sxd[2] = '0' + acpi_target_sleep_state;
> +	if (acpi_target_sleep_state > ACPI_STATE_S0)
> +		acpi_evaluate_integer(handle, sxd, NULL, &d_min);
> +
> +	/*
> +	 * If _PRW says we can wake from the upcoming system state, the _SxD
> +	 * value can wake ... and we'll assume a wakeup-aware driver.  If _SxW
> +	 * methods exist (ACPI 3.x), they give the lowest power D-state that
> +	 * can also wake the system.  _S0W can be valid.
> +	 */
> +	if (acpi_bus_get_device(handle, &dev)) {
> +		printk(KERN_ERR"ACPI handle hasn't context\n");
> +		return d_max;
> +	}
> +	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> +	    (dev->wakeup.state.enabled &&
> +	     dev->wakeup.sleep_state <= acpi_target_sleep_state)) {
> +		sxd[3] = 'W';

Looks ugly.  Why don't we call the array 'method_name' or something like this?

> +		acpi_evaluate_integer(handle, sxd, NULL, &d_max);
> +	}
> +	/* choose a state within d_min to d_max, we just choose the max */
> +	return d_max;
> +}
> +
>  /*
>   * Toshiba fails to preserve interrupts over S1, reinitialization
>   * of 8259 is needed after S1 resume.
> Index: 2.6.22-rc2/include/acpi/acpi_bus.h
> ===================================================================
> --- 2.6.22-rc2.orig/include/acpi/acpi_bus.h	2007-05-23 09:15:14.000000000 +0800
> +++ 2.6.22-rc2/include/acpi/acpi_bus.h	2007-06-19 09:23:54.000000000 +0800
> @@ -364,6 +364,8 @@ acpi_handle acpi_get_child(acpi_handle, 
>  acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);
>  #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
>  
> +int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state);
> +
>  #endif				/* CONFIG_ACPI */
>  
>  #endif /*__ACPI_BUS_H__*/
> -

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 1/2] acpi choose sleep state help

[Rafael J. Wysocki]

On Tuesday, 19 June 2007 13:52, Rafael J. Wysocki wrote:
> On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > Based on David's patch
> > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > I slightly changed it.
> > 
> > Add a helper routine, which gets the sleep state of a ACPI device.
> 
> Is it going to work with the recent code ordering changes?  I mean,
> acpi_pm_prepare() is now called after device_suspend() (and analogously for
> the hibernation), so the target ACPI state is not known when the drivers'
> .suspend() routines are being called.
> 
> > Index: 2.6.22-rc2/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- 2.6.22-rc2.orig/drivers/acpi/sleep/main.c	2007-05-23 09:15:14.000000000 +0800
> > +++ 2.6.22-rc2/drivers/acpi/sleep/main.c	2007-06-19 09:19:09.000000000 +0800
> > @@ -34,6 +34,8 @@ static u32 acpi_suspend_states[] = {
> >  
> >  static int init_8259A_after_S1;
> >  
> > +static int acpi_target_sleep_state = ACPI_STATE_S0;
> > +
> >  /**
> >   *	acpi_pm_prepare - Do preliminary suspend work.
> >   *	@pm_state:		suspend state we're entering.
> > @@ -54,6 +56,7 @@ static int acpi_pm_prepare(suspend_state
> >  		printk("acpi_pm_prepare does not support %d \n", pm_state);
> >  		return -EPERM;
> >  	}
> > +	acpi_target_sleep_state = acpi_state;
> >  	return acpi_sleep_prepare(acpi_state);
> >  }
> >  
> > @@ -140,6 +143,7 @@ static int acpi_pm_finish(suspend_state_
> >  		printk("Broken toshiba laptop -> kicking interrupts\n");
> >  		init_8259A(0);
> >  	}
> > +	acpi_target_sleep_state = ACPI_STATE_S0;
> >  	return 0;
> >  }
> >  
> > @@ -184,6 +188,7 @@ static struct pm_ops acpi_pm_ops = {
> >  #ifdef CONFIG_SOFTWARE_SUSPEND
> >  static int acpi_hibernation_prepare(void)
> >  {
> > +	acpi_target_sleep_state = ACPI_STATE_S4;
> >  	return acpi_sleep_prepare(ACPI_STATE_S4);
> >  }
> >  
> > @@ -215,6 +220,7 @@ static void acpi_hibernation_finish(void
> >  		printk("Broken toshiba laptop -> kicking interrupts\n");
> >  		init_8259A(0);
> >  	}
> > +	acpi_target_sleep_state = ACPI_STATE_S0;
> 
> This will clash with the recent Pavel's patch that removes the Toshiba
> quirk from the hibernation code path
> 
> http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.22-rc5/patches/35-ACPI-remove-S1-workaround-from-hibernation-code-path.patch

The patch has been moved to

http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.22-rc5/patches/34-ACPI-remove-S1-workaround-from-hibernation-code-path.patch

Sorry for the inconvenience.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 1/2] acpi choose sleep state help

[Shaohua Li]

On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > Based on David's patch
> > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > I slightly changed it.
> > 
> > Add a helper routine, which gets the sleep state of a ACPI device.
> 
> Is it going to work with the recent code ordering changes?  I mean,
> acpi_pm_prepare() is now called after device_suspend() (and analogously for
> the hibernation), so the target ACPI state is not known when the drivers'
> .suspend() routines are being called.
Not. Could pm_message_t have a member indicating the suspend state?

> 
> > Index: 2.6.22-rc2/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- 2.6.22-rc2.orig/drivers/acpi/sleep/main.c	2007-05-23 09:15:14.000000000 +0800
> > +++ 2.6.22-rc2/drivers/acpi/sleep/main.c	2007-06-19 09:19:09.000000000 +0800
> > @@ -34,6 +34,8 @@ static u32 acpi_suspend_states[] = {
> >  
> >  static int init_8259A_after_S1;
> >  
> > +static int acpi_target_sleep_state = ACPI_STATE_S0;
> > +
> >  /**
> >   *	acpi_pm_prepare - Do preliminary suspend work.
> >   *	@pm_state:		suspend state we're entering.
> > @@ -54,6 +56,7 @@ static int acpi_pm_prepare(suspend_state
> >  		printk("acpi_pm_prepare does not support %d \n", pm_state);
> >  		return -EPERM;
> >  	}
> > +	acpi_target_sleep_state = acpi_state;
> >  	return acpi_sleep_prepare(acpi_state);
> >  }
> >  
> > @@ -140,6 +143,7 @@ static int acpi_pm_finish(suspend_state_
> >  		printk("Broken toshiba laptop -> kicking interrupts\n");
> >  		init_8259A(0);
> >  	}
> > +	acpi_target_sleep_state = ACPI_STATE_S0;
> >  	return 0;
> >  }
> >  
> > @@ -184,6 +188,7 @@ static struct pm_ops acpi_pm_ops = {
> >  #ifdef CONFIG_SOFTWARE_SUSPEND
> >  static int acpi_hibernation_prepare(void)
> >  {
> > +	acpi_target_sleep_state = ACPI_STATE_S4;
> >  	return acpi_sleep_prepare(ACPI_STATE_S4);
> >  }
> >  
> > @@ -215,6 +220,7 @@ static void acpi_hibernation_finish(void
> >  		printk("Broken toshiba laptop -> kicking interrupts\n");
> >  		init_8259A(0);
> >  	}
> > +	acpi_target_sleep_state = ACPI_STATE_S0;
> 
> This will clash with the recent Pavel's patch that removes the Toshiba
> quirk from the hibernation code path
> 
> http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.22-rc5/patches/35-ACPI-remove-S1-workaround-from-hibernation-code-path.patch
> 
> >  }
> >  
> >  static struct hibernation_ops acpi_hibernation_ops = {
> > @@ -224,6 +230,49 @@ static struct hibernation_ops acpi_hiber
> >  };
> >  #endif				/* CONFIG_SOFTWARE_SUSPEND */
> >  
> > +int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state)
> > +{
> 
> The second argument doesn't seem to be used.  Is that intentional and if so,
> then why?
I hope it gives the suspend state, but it appears it's not used.

> > +	char sxd[] = "_SxD";
> > +	unsigned long d_min, d_max;
> > +	struct acpi_device *dev;
> > +
> > +	/*
> > +	 * If the sleep state is S0, we will return D3, but if the device has
> > +	 * _S0W, we will use the value from _S0W
> > +	 */
> 
> Hmm, is the comment right?  Why should we return D3 for S0?
Yes. It could be runtime device suspend. In that case we return D3 by
default if acpi gives the correct _S0W, we use state from it.

> > +	d_min = ACPI_STATE_D3;
> > +	d_max = ACPI_STATE_D3;
> > +
> > +	/* If present, _SxD methods give the minimum D-state we may use
> > +	 * for each S-state ... with lowest latency state switching.
> > +	 *
> > +	 * We rely on acpi_evaluate_integer() not clobbering the integer
> > +	 * provided -- that's our fault recovery, we ignore retval.
> > +	 */
> > +	sxd[2] = '0' + acpi_target_sleep_state;
> > +	if (acpi_target_sleep_state > ACPI_STATE_S0)
> > +		acpi_evaluate_integer(handle, sxd, NULL, &d_min);
> > +
> > +	/*
> > +	 * If _PRW says we can wake from the upcoming system state, the _SxD
> > +	 * value can wake ... and we'll assume a wakeup-aware driver.  If _SxW
> > +	 * methods exist (ACPI 3.x), they give the lowest power D-state that
> > +	 * can also wake the system.  _S0W can be valid.
> > +	 */
> > +	if (acpi_bus_get_device(handle, &dev)) {
> > +		printk(KERN_ERR"ACPI handle hasn't context\n");
> > +		return d_max;
> > +	}
> > +	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> > +	    (dev->wakeup.state.enabled &&
> > +	     dev->wakeup.sleep_state <= acpi_target_sleep_state)) {
> > +		sxd[3] = 'W';
> 
> Looks ugly.  Why don't we call the array 'method_name' or something like this?
Will fix it.

Thanks,
Shaohua

[RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > Based on David's patch
> > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > I slightly changed it.
> > > 
> > > Add a helper routine, which gets the sleep state of a ACPI device.
> > 
> > Is it going to work with the recent code ordering changes?  I mean,
> > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > the hibernation), so the target ACPI state is not known when the drivers'
> > .suspend() routines are being called.
> Not. Could pm_message_t have a member indicating the suspend state?

Well, I thought about that, but I did't know what people on linux-pm would
think about that.

Alternatively, we could introduce a pm_target() global PM operation that will
set the target sleep state for the entire system.

I think we should discuss that on linux-pm before any decision is made.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:

> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.
> 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.
> 
> I think we should discuss that on linux-pm before any decision is made.

Pardon me for asking what may be a dumb question.  Why does ACPI (or
anything else) need to know the target system state in order to decide
how a device should be suspended or whether wakeup should be enabled?

Alan Stern

Re: [linux-pm] [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Wednesday, 20 June 2007 16:08, Alan Stern wrote:
> On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:
> 
> > > Not. Could pm_message_t have a member indicating the suspend state?
> > 
> > Well, I thought about that, but I did't know what people on linux-pm would
> > think about that.
> > 
> > Alternatively, we could introduce a pm_target() global PM operation that will
> > set the target sleep state for the entire system.
> > 
> > I think we should discuss that on linux-pm before any decision is made.
> 
> Pardon me for asking what may be a dumb question.  Why does ACPI (or
> anything else) need to know the target system state in order to decide
> how a device should be suspended or whether wakeup should be enabled?

The question isn't so dumb. ;-)

For each device (handled by it) ACPI defines _SxD and _SxW methods returning
the highest power (lowest number) D-state supported by the device in the
(system-wide) state Sx and the lowest power (highest number) D-state in which
the device can wake up the system being in the Sx sleep state, respectively.
The target power state of the device should be determined on the basis of these
values (along with the device's wake up setting) and they depend on the target
system sleep state.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Len Brown]

On Wednesday 20 June 2007 07:32, Rafael J. Wysocki wrote:
> On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > Based on David's patch
> > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > I slightly changed it.
> > > > 
> > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > 
> > > Is it going to work with the recent code ordering changes?  I mean,
> > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > the hibernation), so the target ACPI state is not known when the drivers'
> > > .suspend() routines are being called.
> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.
> 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.
> 
> I think we should discuss that on linux-pm before any decision is made.

okay, this thread include linux-pm....

I support the proposal that pm_message_t include the target state
in addition to the phase of entering that state.
The reasoning is simple, device drivers that receive a pm_message_t
will do different things depending on the target state.
The example at hand is ACPI devices that need to know how deep a D-state
to go into based on the S-state, and this in-turn depends on if they
are enabled as wakeup devices or not.

thanks,
-Len

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Alan Stern wrote:
> On Wed, 20 Jun 2007, Rafael J. Wysocki wrote:
> 
> Pardon me for asking what may be a dumb question.  Why does ACPI (or
> anything else) need to know the target system state in order to decide
> how a device should be suspended or whether wakeup should be enabled?

Because the things that distinguish different states are
the particular resources that are available in that state.

A device that needs resource X to be active (e.g. to wake
that part of the system) can't stay active in states where
X is unavailable.  A device that won't issue wakeup events
can probably enter lower power states.

I think it'd be pure confusion to care about whether wakeup
"should" be enabled in this state versus that one.  Enable it
when it's requested and possible, but otherwise there's nothing
to be done.  Runtime PM policies will mostly avoid device states
where wakeup can't work (unless it's disabled for that device),
but if the system enters a state where it can't, tough!


One canonical example is portions of the clock tree that
are available in one state but not another.  My pet example
being the USB clock, often 48 MHz, not being available in
deeper sleep states ... e.g.

  http://lkml.org/lkml/2007/3/22/241
  http://lkml.org/lkml/2007/3/22/242

It's routine for system power states to care about clocks
like that.  PXA 25x docs are probably where I first ran
across that issue, but docs for pretty much any SOC will
talk first about clocks when discussing power management.
(Current flows when clocks tick ...)


Another is power domains.  ACPI talks about those (but not
clocks!) as board level things ... e.g. turn off this power
supply on the mainboard.  Newer SOCs must manage these for
on-chip devices too, since newer manufacturing processes
(with smaller feature sizes) involve higher leakage current
(flowing even when clocks don't tick).

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Rafael J. Wysocki wrote:
> On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > Based on David's patch
> > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > I slightly changed it.
> > > > 
> > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > 
> > > Is it going to work with the recent code ordering changes?  I mean,
> > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > the hibernation), so the target ACPI state is not known when the drivers'
> > > .suspend() routines are being called.
>
> > Not. Could pm_message_t have a member indicating the suspend state?
> 
> Well, I thought about that, but I did't know what people on linux-pm would
> think about that.

Let's get rid of pm_message_t entirely.  Didn't we already discuss
how the main reasons for it will vanish if drivers get new PM methods?

 
> Alternatively, we could introduce a pm_target() global PM operation that will
> set the target sleep state for the entire system.

I hope you mean "get the target state"!!

If drivers actually need a handle on that state, that'd be a fair
approach; make it an opaque type though, platform-specific.

But actually I don't see much point to having such a struct.  What
matters is the attributes of the target state (what resources will
be present, especially), and that rarely needs to be indicated by
any kind of cookie.  Consider the "current" task ... it's implicit,
always present (except in IRQ contexts), and hardly ever accessed
despite being more fundamental than "target PM state".

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Wednesday 20 June 2007, Len Brown wrote:

> I support the proposal that pm_message_t include the target state
> in addition to the phase of entering that state.
> The reasoning is simple, device drivers that receive a pm_message_t
> will do different things depending on the target state.

They should do that based on attributes of the target state,
not any particular notion (e.g. from ACPI) of what the states
may be ...


> The example at hand is ACPI devices that need to know how deep a D-state
> to go into based on the S-state, and this in-turn depends on if they
> are enabled as wakeup devices or not.

None of that needs to involve growing pm_message_t (yeech!),
or even knowing that this system's PM infrastructure uses ACPI.

The ACPI bits need to know about ACPI target state, agreed, but
such stuff can (and should!) be interfaces that normal drivers
never see.

In fact, that's why pci_choose_state() exists.  Its implementation
has always sucked, sure, but not the notion that ACPI can answer
the question following APCI rules, while other PCI platforms can
answer the question using other (non-ACPI) rules.  And likewise
for other device types, including SOC devices.

- Dave

Re: [PATCH 1/2] acpi choose sleep state help

[David Brownell]

On Tuesday 19 June 2007, Shaohua Li wrote:
> > > +int acpi_pm_choose_sleep_state(acpi_handle handle, pm_message_t state)
> > > +{
> > 
> > The second argument doesn't seem to be used.  Is that intentional and if so,
> > then why?

That's what the platform hook API uses.  It is indeed a
useless parameter.  Removing it should be a separate patch.


> I hope it gives the suspend state, but it appears it's not used.

Eventually the API should be fixed to not include a pm_message_t ...
which should be called a "message" not a "state" (in every driver!).

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 09:04, David Brownell wrote:
> On Wednesday 20 June 2007, Rafael J. Wysocki wrote:
> > On Wednesday, 20 June 2007 08:18, Shaohua Li wrote:
> > > On Tue, 2007-06-19 at 13:52 +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, 19 June 2007 04:33, Shaohua Li wrote:
> > > > > Based on David's patch
> > > > > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
> > > > > I slightly changed it.
> > > > > 
> > > > > Add a helper routine, which gets the sleep state of a ACPI device.
> > > > 
> > > > Is it going to work with the recent code ordering changes?  I mean,
> > > > acpi_pm_prepare() is now called after device_suspend() (and analogously for
> > > > the hibernation), so the target ACPI state is not known when the drivers'
> > > > .suspend() routines are being called.
> >
> > > Not. Could pm_message_t have a member indicating the suspend state?
> > 
> > Well, I thought about that, but I did't know what people on linux-pm would
> > think about that.
> 
> Let's get rid of pm_message_t entirely.  Didn't we already discuss
> how the main reasons for it will vanish if drivers get new PM methods?
> 
>  
> > Alternatively, we could introduce a pm_target() global PM operation that will
> > set the target sleep state for the entire system.
> 
> I hope you mean "get the target state"!!
> 
> If drivers actually need a handle on that state, that'd be a fair
> approach; make it an opaque type though, platform-specific.
> 
> But actually I don't see much point to having such a struct.  What
> matters is the attributes of the target state (what resources will
> be present, especially), and that rarely needs to be indicated by
> any kind of cookie.  Consider the "current" task ... it's implicit,
> always present (except in IRQ contexts), and hardly ever accessed
> despite being more fundamental than "target PM state".

The issue at hand is that some device drivers may need to know what the
target sleep state of the system will be when their .suspend() routines are
being executed.  Currently, there's no means of passing that information to the
drivers and my question is how to do this.

IMO it can be done in two different ways:
1) via a .suspend() argument
2) via a global variable that the drivers can read.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Pavel Machek]

Hi!

> > > > Not. Could pm_message_t have a member indicating the suspend state?
> > > 
> > > Well, I thought about that, but I did't know what people on linux-pm would
> > > think about that.
> > 
> > Let's get rid of pm_message_t entirely.  Didn't we already discuss
> > how the main reasons for it will vanish if drivers get new PM methods?
> > 
> >  
> > > Alternatively, we could introduce a pm_target() global PM operation that will
> > > set the target sleep state for the entire system.
> > 
> > I hope you mean "get the target state"!!
> > 
> > If drivers actually need a handle on that state, that'd be a fair
> > approach; make it an opaque type though, platform-specific.
> > 
> > But actually I don't see much point to having such a struct.  What
> > matters is the attributes of the target state (what resources will
> > be present, especially), and that rarely needs to be indicated by
> > any kind of cookie.  Consider the "current" task ... it's implicit,
> > always present (except in IRQ contexts), and hardly ever accessed
> > despite being more fundamental than "target PM state".
> 
> The issue at hand is that some device drivers may need to know what the
> target sleep state of the system will be when their .suspend() routines are
> being executed.  Currently, there's no means of passing that information to the
> drivers and my question is how to do this.
> 
> IMO it can be done in two different ways:
> 1) via a .suspend() argument
> 2) via a global variable that the drivers can read.

Just do 1). Global variables are ugly, and we already have space in
pm_message_t. 
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

Hi,

On Thursday, 21 June 2007 15:03, Pavel Machek wrote:
> Hi!
> 
> > > > > Not. Could pm_message_t have a member indicating the suspend state?
> > > > 
> > > > Well, I thought about that, but I did't know what people on linux-pm would
> > > > think about that.
> > > 
> > > Let's get rid of pm_message_t entirely.  Didn't we already discuss
> > > how the main reasons for it will vanish if drivers get new PM methods?
> > > 
> > >  
> > > > Alternatively, we could introduce a pm_target() global PM operation that will
> > > > set the target sleep state for the entire system.
> > > 
> > > I hope you mean "get the target state"!!
> > > 
> > > If drivers actually need a handle on that state, that'd be a fair
> > > approach; make it an opaque type though, platform-specific.
> > > 
> > > But actually I don't see much point to having such a struct.  What
> > > matters is the attributes of the target state (what resources will
> > > be present, especially), and that rarely needs to be indicated by
> > > any kind of cookie.  Consider the "current" task ... it's implicit,
> > > always present (except in IRQ contexts), and hardly ever accessed
> > > despite being more fundamental than "target PM state".
> > 
> > The issue at hand is that some device drivers may need to know what the
> > target sleep state of the system will be when their .suspend() routines are
> > being executed.  Currently, there's no means of passing that information to the
> > drivers and my question is how to do this.
> > 
> > IMO it can be done in two different ways:
> > 1) via a .suspend() argument
> > 2) via a global variable that the drivers can read.
> 
> Just do 1). Global variables are ugly, and we already have space in
> pm_message_t. 

Well, this is what Len voted for, I think.  David is against it.

I also think that the cleanest way would be to pass that as an argument
to .suspend(), but currently pm_message_t. is passed by value and if we
made it a real struct (ie. with more than one field), that would also become
ugly, IMHO.

So, can we make pm_message_t consist only of the target state?

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> The issue at hand is that some device drivers may need to know what the
> target sleep state of the system will be when their .suspend() routines are
> being executed.  Currently, there's no means of passing that information to the
> drivers and my question is how to do this.

Actually what they need to know is some *attribute* of that state.

They really don't care what the state is.  The $SUBJECT patch isn't
driver code ... it's for platform hooks that expose attributes to
the drivers.  Specifically, it's ACPI code, talking to drivers that
must run on non-ACPI systems.  Any driver that thinks it needs to
understand anything about ACPI states is sadly broken.

Remember also that the Linux "states" (in /sys/power/state) are an
inadequate representation of what most hardware can do.  Common
hardware can support a lot more low power sleep modes than the two
states Linux currently defines ... a limitation inherited from
first APM, and them more recently ACPI, which doesn't fit embedded
systems well at all.

- Dave


-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, Rafael J. Wysocki wrote:

> > Just do 1). Global variables are ugly, and we already have space in
> > pm_message_t. 
> 
> Well, this is what Len voted for, I think.  David is against it.
> 
> I also think that the cleanest way would be to pass that as an argument
> to .suspend(), but currently pm_message_t. is passed by value and if we
> made it a real struct (ie. with more than one field), that would also become
> ugly, IMHO.
> 
> So, can we make pm_message_t consist only of the target state?

You're both repeating the mistakes from two years ago.

You can't use a simple type to describe a target system state.  While 
it might work for ACPI states, it won't work on general (i.e., 
non-ACPI) systems.  It's probably a mistake even to use a data 
structure to describe a system state, since the requirements are so 
complex.  The only reasonable approach is to describe it in code.

What you can do is this: Pick a small enumerated set of labels for some 
selected system states.  For example:

enum pm_system_state {
	PM_STATE_RUNNING,
	PM_STATE_STANDBY,
	PM_STATE_SUSPEND,
	PM_STATE_HIBERNATE,
};

(It might be preferable to make the list more configurable, perhaps 
even allow changes at runtime.  Never mind all that for now.)

These are merely labels, they don't actually describe anything.  To use 
them, you would have to pass them to a subsystem routine for 
interpretation.  For example, pci_select_state() might pass one of 
these things to an ACPI routine, which would know what ACPI state 
corresponded to the given pm_system_state and would be able to say what 
D-state would be appropriate for a given PCI device.  On a non-ACPI 
platform, pci_select_state() would have to call a different routine -- 
something platform-dependent -- to do the same job.


On the other hand, maybe you don't need anything like this at all.  
What would happen if a PCI driver put its device into a D-state which 
wasn't supported under the target ACPI state?  Would it be so terrible?
I can imagine that the requested wakeup functionality might not be 
available, but would it prevent the device from working properly when 
it was resumed?

Alan Stern

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

> > IMO it can be done in two different ways:
> > 1) via a .suspend() argument
> > 2) via a global variable that the drivers can read.

For sufficiently small values of "two" that is.  

Other solutions that have been described on the PM list include

  3) Providing accessors to the information actually needed
     in drivers ... e.g. say whether this clock or power domain
     will be available in that target state.

  4) Act more like "current" ... there's a function returning
     whatever "state" struct is settled on.  (But ideally
     without the pseudo-global.)

I'm amused that nobody really reacted to the technical comments in
my previous posts on this thread.  That's unfortunate, since from
where I sit it feels to me like everyone else is a johnny-come-lately
on this issue, and is now grasping at the quickest and dirtiest ways
to work around the issue instead of coming to grasp with the various
underlying issues.

IMO #3 is strongly preferable.


> Just do 1). Global variables are ugly, and we already have space in
> pm_message_t. 

There is no space in the ugly pm_message_t structure.  Adding to
that would involve creating a **larger** structure and passing it
around on the stack all the time.

Pavel, I know that for some perverse reason you actually like
that structure, and the notion of passing it around *BY VALUE*
instead of by reference.  But that approach has never been
universally acclaimed, and has in fact always had opposition;
the only way you got it merged in the first place was to send
in mountains of patches and ignore the negative feedback.

But I really thought the discussion on new PM methods, back a
couple months now, was going to finally get rid of that.

- Dave

Re: Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, David Brownell wrote:

> If drivers actually need a handle on that state, that'd be a fair
> approach; make it an opaque type though, platform-specific.
> 
> But actually I don't see much point to having such a struct.  What
> matters is the attributes of the target state (what resources will
> be present, especially), and that rarely needs to be indicated by
> any kind of cookie.

Your meaning isn't clear.  You just said there isn't much point in 
having a struct, and now you say that there rarely needs to be a 
cookie.  Did you mean to say that a cookie can do a better job of 
encapsulating the information than a struct can?

System suspend is different from runtime suspend in that the 
requirements are passed from the top down: "The whole system is going 
to enter this state, so prepare yourself".  Not "All your children are 
in low-power states, so feel free to reduce your own power level".  A 
driver can't rely on purely local information to know what will happen.

If a driver wants to find out whether some resource will be available
in the target system state, the only way is to ask the resource's
provider.  Hence the driver needs to have some cookie (representing the
target state) that it can pass to the provider.

Alan Stern

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Alan Stern wrote:
> On Thu, 21 Jun 2007, David Brownell wrote:
> 
> > If drivers actually need a handle on that state, that'd be a fair
> > approach; make it an opaque type though, platform-specific.
> > 
> > But actually I don't see much point to having such a struct.  What
> > matters is the attributes of the target state (what resources will
> > be present, especially), and that rarely needs to be indicated by
> > any kind of cookie.
> 
> Your meaning isn't clear.  You just said there isn't much point in 
> having a struct, and now you say that there rarely needs to be a 
> cookie.  Did you mean to say that a cookie can do a better job of 
> encapsulating the information than a struct can?

Struct, cookie, it's all the same thing.  Nothing is really needed.
The contents would be platform-specific, so there's no point in
having a struct vs some other identifier.

Remember that system suspend/sleep states are global transitions, so
there's no value in having a struct/cookie/... to help distinguish
between multiple concurrent transitions.  There is at most one such
transition under way at a time.  It's implicit.

There's no point in having *ANY* identifier.  That's good, since
updating all the APIs to pass such an identifier -- top to bottom of
all driver and framework stacks -- would be a lot of make-work.  Not
much code currently cares about how the various states differ.

As I think Linus also observed not that long ago, most driver writers
are having a hard time even understanding how to recover from power-off
"suspend" states.  Now, we need to get way beyond that for at least
some systems.  And to help those systems create the base of experience
that other developers can build on.  A key part of that is ensuring
that smarter drivers can be written, while not inflicting complexity
on other systems (and driver writers still struggling with 'resume').


> System suspend is different from runtime suspend in that the 
> requirements are passed from the top down: "The whole system is going 
> to enter this state, so prepare yourself".  Not "All your children are 
> in low-power states, so feel free to reduce your own power level".  A 
> driver can't rely on purely local information to know what will happen.

Right, the same global info applies ... globally.

 
> If a driver wants to find out whether some resource will be available
> in the target system state, the only way is to ask the resource's
> provider.  Hence the driver needs to have some cookie (representing the
> target state) that it can pass to the provider.

Not true.  The provider knows the target state.  Just ask it whether
the resource will be available.  It doesn't need a cookie to distinguish
between multiple target states, since there can be only one.

ACPI-aware providers will of course have to live within the constraints
of ACPI, which include knowing that there are only a handful of possible
states -- numbered S0, S1, S3, S4, S5 -- and that details of those states
are computed by interpreting AML bytecodes.  Those providers can talk to
the ACPI subsystem to get whatever info they need ... not just calling the
AML interpreter, but other details like what S-state is involved.

Providers for non-ACPI systems will naturally work quite differently,
and (by observation!) provide different kinds of resources.  Like clocks,
to pick just one of the more basic examples.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:

> So, can we make pm_message_t consist only of the target state?

Let's just finally get rid of pm_message_t.

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, David Brownell wrote:

> > If a driver wants to find out whether some resource will be available
> > in the target system state, the only way is to ask the resource's
> > provider.  Hence the driver needs to have some cookie (representing the
> > target state) that it can pass to the provider.
> 
> Not true.  The provider knows the target state.  Just ask it whether
> the resource will be available.  It doesn't need a cookie to distinguish
> between multiple target states, since there can be only one.

_How_ does the provider know what the next target state is?  Right now
there's no way for that information to get from the PM core to the
provider other than pm_message_t, and the pm_message_t will generally
be passed to the provider _after_ it is passed to the lower-level
drivers.

There could be a global next_pm_system_state() routine.  It would have
to return _something_ -- and I think a cookie would be better than a
struct.

There are other possible ways to disseminate the information.  The 
details don't matter much, and relatively few drivers would care.  
However the form of the information is a legitimate concern at this 
point.

Alan Stern

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Alan Stern wrote:
> On Thu, 21 Jun 2007, David Brownell wrote:
> 
> > > If a driver wants to find out whether some resource will be available
> > > in the target system state, the only way is to ask the resource's
> > > provider.  Hence the driver needs to have some cookie (representing the
> > > target state) that it can pass to the provider.
> > 
> > Not true.  The provider knows the target state.  Just ask it whether
> > the resource will be available.  It doesn't need a cookie to distinguish
> > between multiple target states, since there can be only one.
> 
> _How_ does the provider know what the next target state is?

That's an interface between the provider and the platform's PM code.
Remember those two patches?

  http://lkml.org/lkml/2007/3/22/241
  http://lkml.org/lkml/2007/3/22/242

The second one does that by coupling one platform's pm_ops to its
clock framework using an internal interface.  That will be typical
for any SOC system, where the difference between states is mostly
just which oscillators/PLLs are active ... pm_ops being essentially
tasked with turning some stuff off.

Of course, I believe we need to move away from "suspend_state_t"
being effectively just "standby" or "STR" (or "ON") so that more
of the hardware capabilities can be exposed.   Systems that have,
say, seven different hardware states can't fit into Linux today.
(Related, I think that target *run* states are under-appreciated.
That's the general runtime PM issue.  Interfaces should work for
run-state transitions as well as sleep-state ones...)


> Right now 
> there's no way for that information to get from the PM core to the
> provider other than pm_message_t, and the pm_message_t will generally
> be passed to the provider _after_ it is passed to the lower-level
> drivers.

No, providers don't get a pm_message_t ... that goes to drivers.


> There could be a global next_pm_system_state() routine.  It would have
> to return _something_ -- and I think a cookie would be better than a
> struct.

But *should* there be such a routine?  Interpreting it would
necessarily be very platform-specific.  Why should anyone care
about platform-specific calls ... except people writing the
platform-specific code to implement and use those calls?
Is there any way that driver code could ever make use of such
a suspend_state_t value?


> There are other possible ways to disseminate the information.  The 
> details don't matter much, and relatively few drivers would care.  
> However the form of the information is a legitimate concern at this 
> point.

I still disagree.  Has anyone even proposed an example of a driver
caring about "what the target sleep state is"?  Every example we
have seen in the past several years is an example where the relevant
detail is something *else* ... something which is only loosely
associated with that state, in a very platform-specific way.

The same PCI chip may need to act very differently based on whether
the system uses ACPI (or not), and how it's wired on that particular
mainboard.  That's platform-specific behavior which is not coupled
directly to a sleep state.

- Dave


-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, David Brownell wrote:

> On Thursday 21 June 2007, Alan Stern wrote:
> > On Thu, 21 Jun 2007, David Brownell wrote:
> > 
> > > > If a driver wants to find out whether some resource will be available
> > > > in the target system state, the only way is to ask the resource's
> > > > provider.  Hence the driver needs to have some cookie (representing the
> > > > target state) that it can pass to the provider.
> > > 
> > > Not true.  The provider knows the target state.  Just ask it whether
> > > the resource will be available.  It doesn't need a cookie to distinguish
> > > between multiple target states, since there can be only one.
> > 
> > _How_ does the provider know what the next target state is?
> 
> That's an interface between the provider and the platform's PM code.
> Remember those two patches?
> 
>   http://lkml.org/lkml/2007/3/22/241
>   http://lkml.org/lkml/2007/3/22/242
> 
> The second one does that by coupling one platform's pm_ops to its
> clock framework using an internal interface.  That will be typical
> for any SOC system, where the difference between states is mostly
> just which oscillators/PLLs are active ... pm_ops being essentially
> tasked with turning some stuff off.

Now we're getting back to the question which started this thread.  
That patch takes care of one platform, but now consider an ACPI system.  
How should the ACPI core learn what the next target system state will
be?  And once it possess that knowledge, what's the best way for it to
tell various subsystems which device states/functions will be
supported?

> Of course, I believe we need to move away from "suspend_state_t"
> being effectively just "standby" or "STR" (or "ON") so that more
> of the hardware capabilities can be exposed.   Systems that have,
> say, seven different hardware states can't fit into Linux today.

I agree that the list of system states should be configurable and 
perhaps even adjustable at runtime.  However this is somewhat OT.

> (Related, I think that target *run* states are under-appreciated.
> That's the general runtime PM issue.  Interfaces should work for
> run-state transitions as well as sleep-state ones...)

Perhaps something like this: Resource providers have an API whereby
drivers can find out what resources either are currently available or
will be available in the next system state (a little awkward to specify
which is needed).  Then drivers decide on a new device state based on
the type of change requested and the available resources, and notify
the providers via a second API about any change in resource usage.

> > There could be a global next_pm_system_state() routine.  It would have
> > to return _something_ -- and I think a cookie would be better than a
> > struct.
> 
> But *should* there be such a routine?  Interpreting it would
> necessarily be very platform-specific.  Why should anyone care
> about platform-specific calls ... except people writing the
> platform-specific code to implement and use those calls?

You forgot one thing.  Yes, the code that makes those calls and uses
the results would be platform-specific.  But the code that gets called
would be part of the PM core and hence not platform-specific -- even
though the values it returns are.

Now perhaps I'm beating a dead horse and the existing pm_ops callbacks 
already provide the necessary notifications.  If so I'll shut up.

> I still disagree.  Has anyone even proposed an example of a driver
> caring about "what the target sleep state is"?  Every example we
> have seen in the past several years is an example where the relevant
> detail is something *else* ... something which is only loosely
> associated with that state, in a very platform-specific way.

Granted that the association is loose and platform-specific.  
Nevertheless, it is non-zero.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Pavel Machek]

Hi!

> > > IMO it can be done in two different ways:
> > > 1) via a .suspend() argument
> > > 2) via a global variable that the drivers can read.
> 
> For sufficiently small values of "two" that is.  
> 
> Other solutions that have been described on the PM list include
> 
>   3) Providing accessors to the information actually needed
>      in drivers ... e.g. say whether this clock or power domain
>      will be available in that target state.
> 
>   4) Act more like "current" ... there's a function returning
>      whatever "state" struct is settled on.  (But ideally
>      without the pseudo-global.)
> 
> I'm amused that nobody really reacted to the technical comments in
> my previous posts on this thread.  That's unfortunate, since from
> where I sit it feels to me like everyone else is a johnny-come-lately
> on this issue, and is now grasping at the quickest and dirtiest ways
> to work around the issue instead of coming to grasp with the various
> underlying issues.

Well, rest of the word is still using PC here, so johny-come-lately
may not be completely unexpected.

Could you push framework for some embedded system you care about? OLPC
by chance?

> IMO #3 is strongly preferable.

3) actually looks ok to me. For acpi it would mean

int acpi_state_we_are_entering(void)

returning S1..S4, right?

> But I really thought the discussion on new PM methods, back a
> couple months now, was going to finally get rid of that.

Umm, well, when someone gets to implement that...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 17:23, Alan Stern wrote:
> On Thu, 21 Jun 2007, Rafael J. Wysocki wrote:
> 
> > > Just do 1). Global variables are ugly, and we already have space in
> > > pm_message_t. 
> > 
> > Well, this is what Len voted for, I think.  David is against it.
> > 
> > I also think that the cleanest way would be to pass that as an argument
> > to .suspend(), but currently pm_message_t. is passed by value and if we
> > made it a real struct (ie. with more than one field), that would also become
> > ugly, IMHO.
> > 
> > So, can we make pm_message_t consist only of the target state?
> 
> You're both repeating the mistakes from two years ago.
> 
> You can't use a simple type to describe a target system state.  While 
> it might work for ACPI states, it won't work on general (i.e., 
> non-ACPI) systems.

I don't want to _describe_ system sleep states this way.

> It's probably a mistake even to use a data  structure to describe a system
> state, since the requirements are so complex.

We already have defined two system sleep states that we think all of the
architectures may support: 'standby' and 'suspend'.  Why don't we assign an
integer to each of them?

> The only reasonable approach is to describe it in code. 
> 
> What you can do is this: Pick a small enumerated set of labels for some 
> selected system states.  For example:
> 
> enum pm_system_state {
> 	PM_STATE_RUNNING,

What for?

> 	PM_STATE_STANDBY,
> 	PM_STATE_SUSPEND,
> 	PM_STATE_HIBERNATE,

Different thing.

> };
> 
> (It might be preferable to make the list more configurable, perhaps 
> even allow changes at runtime.  Never mind all that for now.)

Let me repeat: we _only_ need to tell drivers what the target system sleep
state will be.  No less, no more.

We can do this using the existing pm_message_t, perhaps slightly modifying it,
or anything else, and in fact I don't care much what that will be.  I'd only
prefer to use a .suspend() argument for this purpose, but if you think that
has to be a global variable for whatever practical reason, I'm fine with that
too.

> These are merely labels, they don't actually describe anything.  To use 
> them, you would have to pass them to a subsystem routine for 
> interpretation.  For example, pci_select_state() might pass one of 
> these things to an ACPI routine, which would know what ACPI state 
> corresponded to the given pm_system_state and would be able to say what 
> D-state would be appropriate for a given PCI device.  On a non-ACPI 
> platform, pci_select_state() would have to call a different routine -- 
> something platform-dependent -- to do the same job.
> 
> 
> On the other hand, maybe you don't need anything like this at all.  
> What would happen if a PCI driver put its device into a D-state which 
> wasn't supported under the target ACPI state?  Would it be so terrible?
> I can imagine that the requested wakeup functionality might not be 
> available, but would it prevent the device from working properly when 
> it was resumed?

Yes, I think so, in general.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 18:35, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> 
> > So, can we make pm_message_t consist only of the target state?
> 
> Let's just finally get rid of pm_message_t.

Fine.  How to we solve the problem at hand, then?


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Alan Stern wrote:
> On Thu, 21 Jun 2007, David Brownell wrote:
> > On Thursday 21 June 2007, Alan Stern wrote:
> > > 
> > > _How_ does the provider know what the next target state is?
> > 
> > That's an interface between the provider and the platform's PM code.
> > Remember those two patches?
> > ...
> 
> Now we're getting back to the question which started this thread.  
> That patch takes care of one platform, but now consider an ACPI system.  
> How should the ACPI core learn what the next target system state will
> be? 

The original patch -- to which the previous $SUBJECT patch was
an update -- did more or less the same thing:  pm_ops recorded
the ACPI target state ...


> And once it possess that knowledge, what's the best way for it to 
> tell various subsystems which device states/functions will be
> supported?

... and then exposed a method returning ACPI_STATE_Sx values,
called by non-core ACPI code.

At which point your narrative falters.  What it's exposed to is
not a subsystem ... but ACPI versions of hooks invoked by the
subsystem.  For PCI, to be specific.

That is, the knowledge of that target sleep state never leaves
that platform's PM code.  (In this case, ACPI; including its PCI
support, which lives in drivers/pci not drivers/acpi.)  Because
no subsystem other than ACPI itself should care how ACPI does any
of its PM stuff.


> > Of course, I believe we need to move away from "suspend_state_t"
> > being effectively just "standby" or "STR" (or "ON") so that more
> > of the hardware capabilities can be exposed.   Systems that have,
> > say, seven different hardware states can't fit into Linux today.
> 
> I agree that the list of system states should be configurable and 
> perhaps even adjustable at runtime.  However this is somewhat OT.

Only slightly; remember, a main reason Linux has so few states is
that PCs don't use any more.  There are a number of PC-specific
models and assumptions lurking in this area.  One of them is that
the target sleep state is anything more than a stepping-stone to
the important platform-specific information.


> > (Related, I think that target *run* states are under-appreciated.
> > That's the general runtime PM issue.  Interfaces should work for
> > run-state transitions as well as sleep-state ones...)
> 
> Perhaps something like this: Resource providers have an API whereby
> drivers can find out what resources either are currently available or
> will be available in the next system state (a little awkward to specify
> which is needed).  Then drivers decide on a new device state based on
> the type of change requested and the available resources, and notify
> the providers via a second API about any change in resource usage.

Resource providers have interfaces (*) they expose; yes.  *IF* those
resources change availability based on system states, there must be
interfaces drivers can use to notice that.  The providers already have
interfaces to manage resources, and won't need new cals for that. 

The calls to expose whether a given in-use resource must be released
or modified would be simplest if they're just query calls made by
driver suspend()/resume() methods, but there's also something to be
said for callbacks in certain contexts.

(*) "API" == "Application Programming Interface", a userspace thing.
    So I avoid using that TLA for anything inside the OS kernel.

 
> > > There could be a global next_pm_system_state() routine.  It would have
> > > to return _something_ -- and I think a cookie would be better than a
> > > struct.
> > 
> > But *should* there be such a routine?  Interpreting it would
> > necessarily be very platform-specific.  Why should anyone care
> > about platform-specific calls ... except people writing the
> > platform-specific code to implement and use those calls?
> 
> You forgot one thing.  Yes, the code that makes those calls and uses
> the results would be platform-specific.  But the code that gets called
> would be part of the PM core and hence not platform-specific -- even
> though the values it returns are.

That's the kind of notion that waves red flags at me.  If it's
returning platform-specific values and types, to platform-specific
code, is there any realistic way to view that code as not being
specific to that platform?  Why bother trying to package it as a
core capability, except to show that it's possible to discard
type information at will?

Sure, ACPI_STATE_Sx values aren't declared as "__bitwise", and so
they are very amenable to type punning with values from other
platforms.  But I'd call that sort of thing a "bug" not a "feature".
And I can't see a way to have that routine be part of the PM core
without relying on that bug in a very deep (and error prone) way.


> > I still disagree.  Has anyone even proposed an example of a driver
> > caring about "what the target sleep state is"?  Every example we
> > have seen in the past several years is an example where the relevant
> > detail is something *else* ... something which is only loosely
> > associated with that state, in a very platform-specific way.
> 
> Granted that the association is loose and platform-specific.  
> Nevertheless, it is non-zero.

So are the association with Earthlings, and the non-association with
Martians or Venusians.  ;)

The association is so weak that trying to build on it becomes
rapidly counterproductive.  Focus on the "something else" which
actually matters to the drivers.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 17:37, David Brownell wrote:
> 
> > > IMO it can be done in two different ways:
> > > 1) via a .suspend() argument
> > > 2) via a global variable that the drivers can read.
> 
> For sufficiently small values of "two" that is.  
> 
> Other solutions that have been described on the PM list include
> 
>   3) Providing accessors to the information actually needed
>      in drivers ... e.g. say whether this clock or power domain
>      will be available in that target state.

Well, you need to store that information somewhere.  The way in which
it will be provided to drivers is a secondary thing.

To me, the most important question is whether we want to pass that information
as a .suspend() argument or in any different way, which involves the use of a
global variable (or a set of variables) and that's 2).

>   4) Act more like "current" ... there's a function returning
>      whatever "state" struct is settled on.  (But ideally
>      without the pseudo-global.)

How would you be going to arrange that in practice?

> I'm amused that nobody really reacted to the technical comments in
> my previous posts on this thread.  That's unfortunate, since from
> where I sit it feels to me like everyone else is a johnny-come-lately
> on this issue, and is now grasping at the quickest and dirtiest ways
> to work around the issue instead of coming to grasp with the various
> underlying issues.
> 
> IMO #3 is strongly preferable.

Of course we can do that.  At least I don't have any objections.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Pavel Machek wrote:
> Hi!
> 
> > > > IMO it can be done in two different ways:
> > > > 1) via a .suspend() argument
> > > > 2) via a global variable that the drivers can read.
> > 
> > For sufficiently small values of "two" that is.  
> > 
> > Other solutions that have been described on the PM list include
> > 
> >   3) Providing accessors to the information actually needed
> >      in drivers ... e.g. say whether this clock or power domain
> >      will be available in that target state.
> > 
> >   4) Act more like "current" ... there's a function returning
> >      whatever "state" struct is settled on.  (But ideally
> >      without the pseudo-global.)
> > 
> > I'm amused that nobody really reacted to the technical comments in
> > my previous posts on this thread.  That's unfortunate, since from
> > where I sit it feels to me like everyone else is a johnny-come-lately
> > on this issue, and is now grasping at the quickest and dirtiest ways
> > to work around the issue instead of coming to grasp with the various
> > underlying issues.
> 
> Well, rest of the word is still using PC here, so johny-come-lately
> may not be completely unexpected.

True.  I could hardly escape noticing this problem though, given
what it takes to get USB remote wakeup working on various systems.
We've had a few years now to get that infrastructure in place.


> Could you push framework for some embedded system you care about? OLPC
> by chance?

I'll probably push those two patches (clock core, AT91 implementation)
since there seemed to be no objections.  I don't work on OLPC.  :)


> > IMO #3 is strongly preferable.
> 
> 3) actually looks ok to me. For acpi it would mean
> 
> int acpi_state_we_are_entering(void)
> 
> returning S1..S4, right?

My original patch included acpi_get_target_sleep_state()
returning ACPI_STATE_Sx values, yes.  However, that was
purely internal to ACPI-aware logic ... it was used to
implement pci_choose_state().

Drivers would never make such ACPI calls themselves, they'd
use pci_choose_state() instead.


And the clk_must_disable() call is another instance of the
same approach as pci_choose_state():  drivers getting access
to the PM-reated information they need, without needing to
use platform-specific calls or care about "what the target
sleep state is".

 
> > But I really thought the discussion on new PM methods, back a
> > couple months now, was going to finally get rid of that.
> 
> Umm, well, when someone gets to implement that...

Oh.  *THAT* little problem.  Sorry, I thought it was in the works.

- Dave

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 16:48, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> > The issue at hand is that some device drivers may need to know what the
> > target sleep state of the system will be when their .suspend() routines are
> > being executed.  Currently, there's no means of passing that information to the
> > drivers and my question is how to do this.
> 
> Actually what they need to know is some *attribute* of that state.

Generally, yes.

> They really don't care what the state is.  The $SUBJECT patch isn't
> driver code ... it's for platform hooks that expose attributes to
> the drivers.  Specifically, it's ACPI code, talking to drivers that
> must run on non-ACPI systems.  Any driver that thinks it needs to
> understand anything about ACPI states is sadly broken.

But finally it has to place the device into a specific state and that state
needs to be determined somehow.  And, if ACPI is involved, it needs to be
told at one point if the final system state is supposed to be S1 or S2 or S3
(I guess we can discard S2 safely) just to tell the driver which power states
of the device are suitable.

That's why I thought we might introduce an additional global pm_op that
could be used to provide ACPI with that information without engaging drivers in
passing it.

> Remember also that the Linux "states" (in /sys/power/state) are an
> inadequate representation of what most hardware can do.

Do we really want to introduce more system sleep states right at this point?

> Common hardware can support a lot more low power sleep modes than the two
> states Linux currently defines ... a limitation inherited from first APM, and
> them more recently ACPI, which doesn't fit embedded systems well at all.

I agree with that, but right now we have an ACPI-related problem at hand.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 20:51, Alan Stern wrote:
> On Thu, 21 Jun 2007, David Brownell wrote:
> 
> > On Thursday 21 June 2007, Alan Stern wrote:
> > > On Thu, 21 Jun 2007, David Brownell wrote:
> > > 
> > > > > If a driver wants to find out whether some resource will be available
> > > > > in the target system state, the only way is to ask the resource's
> > > > > provider.  Hence the driver needs to have some cookie (representing the
> > > > > target state) that it can pass to the provider.
> > > > 
> > > > Not true.  The provider knows the target state.  Just ask it whether
> > > > the resource will be available.  It doesn't need a cookie to distinguish
> > > > between multiple target states, since there can be only one.
> > > 
> > > _How_ does the provider know what the next target state is?
> > 
> > That's an interface between the provider and the platform's PM code.
> > Remember those two patches?
> > 
> >   http://lkml.org/lkml/2007/3/22/241
> >   http://lkml.org/lkml/2007/3/22/242
> > 
> > The second one does that by coupling one platform's pm_ops to its
> > clock framework using an internal interface.  That will be typical
> > for any SOC system, where the difference between states is mostly
> > just which oscillators/PLLs are active ... pm_ops being essentially
> > tasked with turning some stuff off.
> 
> Now we're getting back to the question which started this thread.  
> That patch takes care of one platform, but now consider an ACPI system.  
> How should the ACPI core learn what the next target system state will
> be?

Yes, this is what I'm interested in in the first place.

> And once it possess that knowledge, what's the best way for it to 
> tell various subsystems which device states/functions will be
> supported?

I think it can be asked for that via a pair of callbacks, like:

platform->lowest_power_state_the_device_can_be_in(dev, wakeup)

and

platform->highest_power_state_the_device_can_be_in(dev)

where 'wakeup' tells the platform whether we want this device to be able to
wake up the system.

> > Of course, I believe we need to move away from "suspend_state_t"
> > being effectively just "standby" or "STR" (or "ON") so that more
> > of the hardware capabilities can be exposed.   Systems that have,
> > say, seven different hardware states can't fit into Linux today.
> 
> I agree that the list of system states should be configurable and 
> perhaps even adjustable at runtime.  However this is somewhat OT.

Agreed.

> > (Related, I think that target *run* states are under-appreciated.
> > That's the general runtime PM issue.  Interfaces should work for
> > run-state transitions as well as sleep-state ones...)
> 
> Perhaps something like this: Resource providers have an API whereby
> drivers can find out what resources either are currently available or
> will be available in the next system state (a little awkward to specify
> which is needed).  Then drivers decide on a new device state based on
> the type of change requested and the available resources, and notify
> the providers via a second API about any change in resource usage.

Yes, something like this.
 
> > > There could be a global next_pm_system_state() routine.  It would have
> > > to return _something_ -- and I think a cookie would be better than a
> > > struct.
> > 
> > But *should* there be such a routine?  Interpreting it would
> > necessarily be very platform-specific.  Why should anyone care
> > about platform-specific calls ... except people writing the
> > platform-specific code to implement and use those calls?
> 
> You forgot one thing.  Yes, the code that makes those calls and uses
> the results would be platform-specific.  But the code that gets called
> would be part of the PM core and hence not platform-specific -- even
> though the values it returns are.
> 
> Now perhaps I'm beating a dead horse and the existing pm_ops callbacks 
> already provide the necessary notifications.  If so I'll shut up.

No, they don't.

pm_ops->prepare() is now called after the drivers' .suspend() routines have
been executed, so the ACPI core, for example, has no means of learning what
the next system state will be until all devices have been suspended.
 
> > I still disagree.  Has anyone even proposed an example of a driver
> > caring about "what the target sleep state is"?  Every example we
> > have seen in the past several years is an example where the relevant
> > detail is something *else* ... something which is only loosely
> > associated with that state, in a very platform-specific way.
> 
> Granted that the association is loose and platform-specific.  
> Nevertheless, it is non-zero.

Agreed.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> On Thursday, 21 June 2007 16:48, David Brownell wrote:

> > They really don't care what the state is.  The $SUBJECT patch isn't
> > driver code ... it's for platform hooks that expose attributes to
> > the drivers.  Specifically, it's ACPI code, talking to drivers that
> > must run on non-ACPI systems.  Any driver that thinks it needs to
> > understand anything about ACPI states is sadly broken.
> 
> But finally it has to place the device into a specific state and that state
> needs to be determined somehow. 

I suppose I'm still thinking that the approach in my original
patch works Just Fine.  Layering is kind of like this, going
from top to bottom (and omitting the go-to-pci-hardware stack,
and the initial ACPI pm hook before suspension starts):

	PM infrastructure ... calling suspend() for everything

	PCI bus support ... translates to PCI-specific typed call

	PCI driver ... suspend() calling pci_choose_state()

	ACPI support for PCI ... implementing choose_state()

	ACPI core code ... remembering ACPI_STATE_Sx, calling AML

That is, ACPI gets invoked at various points, but the driver and
core code doesn't need to know ACPI from Rumpelstiltskin.

- Dave

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:

> platform->lowest_power_state_the_device_can_be_in(dev, wakeup)
> 
> and
> 
> platform->highest_power_state_the_device_can_be_in(dev)
> 
> where 'wakeup' tells the platform whether we want this device to be able to
> wake up the system.

That won't work well except with ACPI, since few systems centralize
that knowledge.  Like ACPI does with AML ... but mostly for PCI.

Look at your average SOC chip spec and you'll see a lot of information
that lives most naturally in the device drivers, or sometimes in the
clock framework.



> pm_ops->prepare() is now called after the drivers' .suspend() routines have
> been executed, so the ACPI core, for example, has no means of learning what
> the next system state will be until all devices have been suspended.

Well that's a design mistake.  Remember that those suspend() methods
need to know the target system states, so that they can call the right
"_SxD" and "_SxW" methods.  There needs to be *SOME* call into the
platform code that can be used to track "x" for ACPI ... or whatever
is needed on other platforms.

What is "now" by the way ... something in the MM tree?  Or did that
sneak in while I wasn't looking?

- Dave

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 21:51, David Brownell wrote:
> On Thursday 21 June 2007, Alan Stern wrote:
> > On Thu, 21 Jun 2007, David Brownell wrote:
> > > On Thursday 21 June 2007, Alan Stern wrote:
> > > > 
> > > > _How_ does the provider know what the next target state is?
> > > 
> > > That's an interface between the provider and the platform's PM code.
> > > Remember those two patches?
> > > ...
> > 
> > Now we're getting back to the question which started this thread.  
> > That patch takes care of one platform, but now consider an ACPI system.  
> > How should the ACPI core learn what the next target system state will
> > be? 
> 
> The original patch -- to which the previous $SUBJECT patch was
> an update -- did more or less the same thing:  pm_ops recorded
> the ACPI target state ...
> 
> 
> > And once it possess that knowledge, what's the best way for it to 
> > tell various subsystems which device states/functions will be
> > supported?
> 
> ... and then exposed a method returning ACPI_STATE_Sx values,
> called by non-core ACPI code.

But we have changed the suspend code ordering and the state
recorded by pm_ops only becomes known to ACPI _after_ we have suspended
devices.

That's what this thread is all about, BTW.

> At which point your narrative falters.  What it's exposed to is
> not a subsystem ... but ACPI versions of hooks invoked by the
> subsystem.  For PCI, to be specific.
> 
> That is, the knowledge of that target sleep state never leaves
> that platform's PM code.  (In this case, ACPI; including its PCI
> support, which lives in drivers/pci not drivers/acpi.)  Because
> no subsystem other than ACPI itself should care how ACPI does any
> of its PM stuff.

All of this is fine, but we need some way to tell ACPI what the next sleep
state will be, because currently _we_ _have_ _not_ one.

So, do we introduce an additional pm_op to do that or are we going to do
something else?
 
> > > Of course, I believe we need to move away from "suspend_state_t"
> > > being effectively just "standby" or "STR" (or "ON") so that more
> > > of the hardware capabilities can be exposed.   Systems that have,
> > > say, seven different hardware states can't fit into Linux today.
> > 
> > I agree that the list of system states should be configurable and 
> > perhaps even adjustable at runtime.  However this is somewhat OT.
> 
> Only slightly; remember, a main reason Linux has so few states is
> that PCs don't use any more.  There are a number of PC-specific
> models and assumptions lurking in this area.  One of them is that
> the target sleep state is anything more than a stepping-stone to
> the important platform-specific information.

As far as my original question is concerned, this is OT.

> > > (Related, I think that target *run* states are under-appreciated.
> > > That's the general runtime PM issue.  Interfaces should work for
> > > run-state transitions as well as sleep-state ones...)
> > 
> > Perhaps something like this: Resource providers have an API whereby
> > drivers can find out what resources either are currently available or
> > will be available in the next system state (a little awkward to specify
> > which is needed).  Then drivers decide on a new device state based on
> > the type of change requested and the available resources, and notify
> > the providers via a second API about any change in resource usage.
> 
> Resource providers have interfaces (*) they expose; yes.  *IF* those
> resources change availability based on system states, there must be
> interfaces drivers can use to notice that.  The providers already have
> interfaces to manage resources, and won't need new cals for that. 
> 
> The calls to expose whether a given in-use resource must be released
> or modified would be simplest if they're just query calls made by
> driver suspend()/resume() methods, but there's also something to be
> said for callbacks in certain contexts.
> 
> (*) "API" == "Application Programming Interface", a userspace thing.
>     So I avoid using that TLA for anything inside the OS kernel.

Agreed.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:03, David Brownell wrote:
> On Thursday 21 June 2007, Pavel Machek wrote:
> > Hi!
> > 
> > > > > IMO it can be done in two different ways:
> > > > > 1) via a .suspend() argument
> > > > > 2) via a global variable that the drivers can read.
> > > 
> > > For sufficiently small values of "two" that is.  
> > > 
> > > Other solutions that have been described on the PM list include
> > > 
> > >   3) Providing accessors to the information actually needed
> > >      in drivers ... e.g. say whether this clock or power domain
> > >      will be available in that target state.
> > > 
> > >   4) Act more like "current" ... there's a function returning
> > >      whatever "state" struct is settled on.  (But ideally
> > >      without the pseudo-global.)
> > > 
> > > I'm amused that nobody really reacted to the technical comments in
> > > my previous posts on this thread.  That's unfortunate, since from
> > > where I sit it feels to me like everyone else is a johnny-come-lately
> > > on this issue, and is now grasping at the quickest and dirtiest ways
> > > to work around the issue instead of coming to grasp with the various
> > > underlying issues.
> > 
> > Well, rest of the word is still using PC here, so johny-come-lately
> > may not be completely unexpected.
> 
> True.  I could hardly escape noticing this problem though, given
> what it takes to get USB remote wakeup working on various systems.
> We've had a few years now to get that infrastructure in place.
> 
> 
> > Could you push framework for some embedded system you care about? OLPC
> > by chance?
> 
> I'll probably push those two patches (clock core, AT91 implementation)
> since there seemed to be no objections.  I don't work on OLPC.  :)
> 
> 
> > > IMO #3 is strongly preferable.
> > 
> > 3) actually looks ok to me. For acpi it would mean
> > 
> > int acpi_state_we_are_entering(void)
> > 
> > returning S1..S4, right?
> 
> My original patch included acpi_get_target_sleep_state()
> returning ACPI_STATE_Sx values, yes.  However, that was
> purely internal to ACPI-aware logic ... it was used to
> implement pci_choose_state().
> 
> Drivers would never make such ACPI calls themselves, they'd
> use pci_choose_state() instead.
> 
> 
> And the clk_must_disable() call is another instance of the
> same approach as pci_choose_state():  drivers getting access
> to the PM-reated information they need, without needing to
> use platform-specific calls or care about "what the target
> sleep state is".
> 
>  
> > > But I really thought the discussion on new PM methods, back a
> > > couple months now, was going to finally get rid of that.
> > 
> > Umm, well, when someone gets to implement that...
> 
> Oh.  *THAT* little problem.  Sorry, I thought it was in the works.

In fact it is, but I had no time to complete it yet.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:22, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> > On Thursday, 21 June 2007 16:48, David Brownell wrote:
> 
> > > They really don't care what the state is.  The $SUBJECT patch isn't
> > > driver code ... it's for platform hooks that expose attributes to
> > > the drivers.  Specifically, it's ACPI code, talking to drivers that
> > > must run on non-ACPI systems.  Any driver that thinks it needs to
> > > understand anything about ACPI states is sadly broken.
> > 
> > But finally it has to place the device into a specific state and that state
> > needs to be determined somehow. 
> 
> I suppose I'm still thinking that the approach in my original
> patch works Just Fine.  Layering is kind of like this, going
> from top to bottom (and omitting the go-to-pci-hardware stack,
> and the initial ACPI pm hook before suspension starts):

We're missing that hook right now. ;-)

> 	PM infrastructure ... calling suspend() for everything
> 
> 	PCI bus support ... translates to PCI-specific typed call
> 
> 	PCI driver ... suspend() calling pci_choose_state()
> 
> 	ACPI support for PCI ... implementing choose_state()
> 
> 	ACPI core code ... remembering ACPI_STATE_Sx, calling AML
> 
> That is, ACPI gets invoked at various points, but the driver and
> core code doesn't need to know ACPI from Rumpelstiltskin.

I agree with that, but we need to add a mechanism to tell the ACPI core
what it needs to know (ie. the target system sleep state) before we suspend
devices or while we are suspending them.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[David Brownell]

On Thursday 21 June 2007, Rafael J. Wysocki wrote:

> All of this is fine, but we need some way to tell ACPI what the next sleep
> state will be, because currently _we_ _have_ _not_ one.
> 
> So, do we introduce an additional pm_op to do that or are we going to do
> something else?

Simplest would be an additional op.

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:32, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> 
> > platform->lowest_power_state_the_device_can_be_in(dev, wakeup)
> > 
> > and
> > 
> > platform->highest_power_state_the_device_can_be_in(dev)
> > 
> > where 'wakeup' tells the platform whether we want this device to be able to
> > wake up the system.
> 
> That won't work well except with ACPI, since few systems centralize
> that knowledge.  Like ACPI does with AML ... but mostly for PCI.
> 
> Look at your average SOC chip spec and you'll see a lot of information
> that lives most naturally in the device drivers, or sometimes in the
> clock framework.
> 
> 
> 
> > pm_ops->prepare() is now called after the drivers' .suspend() routines have
> > been executed, so the ACPI core, for example, has no means of learning what
> > the next system state will be until all devices have been suspended.
> 
> Well that's a design mistake.

pm_ops->prepare() has to be called after device_suspend() for other reasons.

> Remember that those suspend() methods 
> need to know the target system states, so that they can call the right
> "_SxD" and "_SxW" methods.  There needs to be *SOME* call into the
> platform code that can be used to track "x" for ACPI ... or whatever
> is needed on other platforms.

YES!  And that's what I'm asking about: How are we going to handle that?

> What is "now" by the way ... something in the MM tree?  Or did that
> sneak in while I wasn't looking?

2.6.22-rc5 actually, and the patch was from Linus himself, acked by Len. ;-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Rafael J. Wysocki]

On Thursday, 21 June 2007 22:46, David Brownell wrote:
> On Thursday 21 June 2007, Rafael J. Wysocki wrote:
> 
> > All of this is fine, but we need some way to tell ACPI what the next sleep
> > state will be, because currently _we_ _have_ _not_ one.
> > 
> > So, do we introduce an additional pm_op to do that or are we going to do
> > something else?
> 
> Simplest would be an additional op.

OK

Does anyone have any objections to adding a new pm_op that will tell
the ACPI core (or generally, a platform core code) what target system sleep
state we are going to enter?

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFD] How to tell ACPI drivers what the target sleep state is (was: Re: [PATCH 1/2] acpi choose sleep state help)

[Alan Stern]

On Thu, 21 Jun 2007, Rafael J. Wysocki wrote:

> Does anyone have any objections to adding a new pm_op that will tell
> the ACPI core (or generally, a platform core code) what target system sleep
> state we are going to enter?

Go for it!

Alan Stern

[RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

Hi,

The appended patch adds the new pm_ops callback to be used to pass the target
system sleep state to the platform core (ACPI core in particular) and reworks
the ACPI PM operations to take this callback into account.

When I was working on this patch I thought it might be a good idea to do the
following additional changes:
* rename pm_ops to something more descriptive, like for example
  'platform_suspend_operations'
* move the definition of pm_ops (or whatever it will be called) to
  <linux/suspend.h>
* make the prepare(), enter() and finish() callbacks not take any arguments
* clean up the PM-related code in the ARM tree (that, and the previous one,
  would require someone to test the changes on these platforms, though)

Comments welcome.

Greetings,
Rafael

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

Some drivers may need to use ACPI to determine the low power states in which
to place their devices, but to provide the drivers with this information the
ACPI core needs to know what sleep state the system is going to enter.
Namely, the device's state should not be too high power for given system sleep
state and, if the device is supposed to be able to wake up the system, its state
should not be too low power for the wake up to be possible).  However,
pm_ops->prepare() is only called after the drivers' .suspend() callbacks have
been executed, so we need an additional means to pass the information of the
target system sleep state to the ACPI core.  For this purpose, we can introduce
an additional member function in 'struct pm_ops'.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/main.c |   84 ++++++++++++++++++++++++++++------------------
 include/linux/pm.h        |   53 +++++++++++++++++++----------
 kernel/power/main.c       |    6 ++-
 3 files changed, 93 insertions(+), 50 deletions(-)

Index: linux-2.6.22-rc5/include/linux/pm.h
===================================================================
--- linux-2.6.22-rc5.orig/include/linux/pm.h	2007-06-23 21:22:43.000000000 +0200
+++ linux-2.6.22-rc5/include/linux/pm.h	2007-06-23 21:26:29.000000000 +0200
@@ -110,37 +110,54 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
 /**
- * struct pm_ops - Callbacks for managing platform dependent suspend states.
- * @valid: Callback to determine whether the given state can be entered.
- *	Valid states are advertised in /sys/power/state but can still
- *	be rejected by prepare or enter if the conditions aren't right.
- *	There is a %pm_valid_only_mem function available that can be assigned
- *	to this if you only implement mem sleep.
+ * struct pm_ops - Callbacks for managing platform dependent system sleep
+ *	states.
  *
- * @prepare: Prepare the platform for the given suspend state. Can return a
- *	negative error code if necessary.
- *
- * @enter: Enter the given suspend state, must be assigned. Can return a
- *	negative error code if necessary.
- *
- * @finish: Called when the system has left the given state and all devices
- *	are resumed. The return value is ignored.
+ * @valid: Callback to determine if given system sleep state is supported by
+ *	the platform.  Valid (ie. supported) states are advertised in
+ *	/sys/power/state.  Note that it still may be impossible to enter given
+ *	system sleep state if the conditions aren't right.
+ *	There is the %pm_valid_only_mem function available that can be assigned
+ *	to this if the platform only supports mem sleep.
+ *
+ * @set_target: Tell the platform which system sleep state is going to be
+ *	entered.  The information passed to @set_target should be disregarded
+ *	by the platform as soon as @finish() is executed and if	@prepare()
+ *	fails.
+ *	This callback is optional.  However, if it is implemented, the
+ *	argument passed to @prepare(), @enter and @finish() must be ignored.
+ *
+ * @prepare: Prepare the platform for entering the system sleep state indicated
+ *	by @set_target().
+ *	This callback is optional.  It returns 0 on success or a negative
+ *	error code otherwise, in which case the system cannot enter given
+ *	sleep state.
+ *
+ * @enter: Enter the system sleep state indicated by @set_target().
+ *	This callback is mandatory.  It returns 0 on success or a negative
+ *	error code otherwise, in which case the system cannot enter given
+ *	sleep state.
+ *
+ * @finish: Called when the system has just left a sleep state.
+ *	This callback is optional, but should be implemented by platforms that
+ *	implement @prepare().  It is always called after @enter() (even if
+ *	@enter() fails).
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
+	int (*set_target)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 };
 
+extern struct pm_ops *pm_ops;
+
 /**
  * pm_set_ops - set platform dependent power management ops
  * @pm_ops: The new power management operations to set.
  */
 extern void pm_set_ops(struct pm_ops *pm_ops);
-extern struct pm_ops *pm_ops;
-extern int pm_suspend(suspend_state_t state);
-
 extern int pm_valid_only_mem(suspend_state_t state);
 
 /**
@@ -161,6 +178,8 @@ extern void arch_suspend_disable_irqs(vo
  */
 extern void arch_suspend_enable_irqs(void);
 
+extern int pm_suspend(suspend_state_t state);
+
 /*
  * Device power management
  */
Index: linux-2.6.22-rc5/kernel/power/main.c
===================================================================
--- linux-2.6.22-rc5.orig/kernel/power/main.c	2007-06-23 21:22:43.000000000 +0200
+++ linux-2.6.22-rc5/kernel/power/main.c	2007-06-23 21:32:31.000000000 +0200
@@ -15,7 +15,6 @@
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/init.h>
-#include <linux/pm.h>
 #include <linux/console.h>
 #include <linux/cpu.h>
 #include <linux/resume-trace.h>
@@ -161,6 +160,11 @@ int suspend_devices_and_enter(suspend_st
 	if (!pm_ops)
 		return -ENOSYS;
 
+	if (pm_ops->set_target) {
+		error = pm_ops->set_target(state);
+		if (error)
+			return error;
+	}
 	suspend_console();
 	error = device_suspend(PMSG_SUSPEND);
 	if (error) {
Index: linux-2.6.22-rc5/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.22-rc5.orig/drivers/acpi/sleep/main.c	2007-06-23 21:22:43.000000000 +0200
+++ linux-2.6.22-rc5/drivers/acpi/sleep/main.c	2007-06-23 23:01:27.000000000 +0200
@@ -34,34 +34,54 @@ static u32 acpi_suspend_states[] = {
 
 static int init_8259A_after_S1;
 
+extern int acpi_sleep_prepare(u32 acpi_state);
+extern void acpi_power_off(void);
+
+static u32 acpi_target_suspend_state = ACPI_STATE_S0;
+
+/**
+ *	acpi_pm_set_target - Set the target system sleep state to the state
+ *		associated with given @pm_state, if supported.
+ */
+
+static int acpi_pm_set_target(suspend_state_t pm_state)
+{
+	u32 acpi_state = acpi_suspend_states[pm_state];
+	int error = 0;
+
+	if (sleep_states[acpi_state]) {
+		acpi_target_suspend_state = acpi_state;
+	} else {
+		printk(KERN_ERR "ACPI does not support this state: %d\n",
+			pm_state);
+		error = -ENOSYS;
+	}
+	return error;
+}
+
 /**
  *	acpi_pm_prepare - Do preliminary suspend work.
- *	@pm_state:		suspend state we're entering.
+ *	@pm_state: ignored
  *
- *	Make sure we support the state. If we do, and we need it, set the
- *	firmware waking vector and do arch-specific nastiness to get the 
- *	wakeup code to the waking vector. 
+ *	If necessary, set the firmware waking vector and do arch-specific
+ *	nastiness to get the wakeup code to the waking vector.
  */
 
-extern int acpi_sleep_prepare(u32 acpi_state);
-extern void acpi_power_off(void);
-
 static int acpi_pm_prepare(suspend_state_t pm_state)
 {
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	int error = acpi_sleep_prepare(acpi_target_suspend_state);
 
-	if (!sleep_states[acpi_state]) {
-		printk("acpi_pm_prepare does not support %d \n", pm_state);
-		return -EPERM;
-	}
-	return acpi_sleep_prepare(acpi_state);
+	if (error)
+		acpi_target_suspend_state = ACPI_STATE_S0;
+
+	return error;
 }
 
 /**
  *	acpi_pm_enter - Actually enter a sleep state.
- *	@pm_state:		State we're entering.
+ *	@pm_state: ignored
  *
- *	Flush caches and go to sleep. For STR or STD, we have to call 
+ *	Flush caches and go to sleep. For STR or S2, we have to call
  *	arch-specific assembly, which in turn call acpi_enter_sleep_state().
  *	It's unfortunate, but it works. Please fix if you're feeling frisky.
  */
@@ -70,31 +90,32 @@ static int acpi_pm_enter(suspend_state_t
 {
 	acpi_status status = AE_OK;
 	unsigned long flags = 0;
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	u32 acpi_state = acpi_target_suspend_state;
 
 	ACPI_FLUSH_CPU_CACHE();
 
 	/* Do arch specific saving of state. */
-	if (pm_state > PM_SUSPEND_STANDBY) {
+	if (acpi_state == ACPI_STATE_S2 || acpi_state == ACPI_STATE_S3) {
 		int error = acpi_save_state_mem();
-		if (error)
+
+		if (error) {
+			acpi_target_suspend_state = ACPI_STATE_S0;
 			return error;
+		}
 	}
 
 	local_irq_save(flags);
 	acpi_enable_wakeup_device(acpi_state);
-	switch (pm_state) {
-	case PM_SUSPEND_STANDBY:
+	switch (acpi_state) {
+	case ACPI_STATE_S1:
 		barrier();
 		status = acpi_enter_sleep_state(acpi_state);
 		break;
 
-	case PM_SUSPEND_MEM:
+	case ACPI_STATE_S2:
+	case ACPI_STATE_S3:
 		do_suspend_lowlevel();
 		break;
-
-	default:
-		return -EINVAL;
 	}
 
 	/* ACPI 3.0 specs (P62) says that it's the responsabilty
@@ -114,12 +135,8 @@ static int acpi_pm_enter(suspend_state_t
 	local_irq_restore(flags);
 	printk(KERN_DEBUG "Back to C!\n");
 
-	/* restore processor state
-	 * We should only be here if we're coming back from STR or STD.
-	 * And, in the case of the latter, the memory image should have already
-	 * been loaded from disk.
-	 */
-	if (pm_state > PM_SUSPEND_STANDBY)
+	/* restore processor state */
+	if (acpi_state == ACPI_STATE_S2 || acpi_state == ACPI_STATE_S3)
 		acpi_restore_state_mem();
 
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
@@ -127,7 +144,7 @@ static int acpi_pm_enter(suspend_state_t
 
 /**
  *	acpi_pm_finish - Finish up suspend sequence.
- *	@pm_state:		State we're coming out of.
+ *	@pm_state: ignored
  *
  *	This is called after we wake back up (or if entering the sleep state
  *	failed). 
@@ -135,7 +152,7 @@ static int acpi_pm_enter(suspend_state_t
 
 static int acpi_pm_finish(suspend_state_t pm_state)
 {
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	u32 acpi_state = acpi_target_suspend_state;
 
 	acpi_leave_sleep_state(acpi_state);
 	acpi_disable_wakeup_device(acpi_state);
@@ -143,6 +160,8 @@ static int acpi_pm_finish(suspend_state_
 	/* reset firmware waking vector */
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
+	acpi_target_suspend_state = ACPI_STATE_S0;
+
 	if (init_8259A_after_S1) {
 		printk("Broken toshiba laptop -> kicking interrupts\n");
 		init_8259A(0);
@@ -183,6 +202,7 @@ static int acpi_pm_state_valid(suspend_s
 
 static struct pm_ops acpi_pm_ops = {
 	.valid = acpi_pm_state_valid,
+	.set_target = acpi_pm_set_target,
 	.prepare = acpi_pm_prepare,
 	.enter = acpi_pm_enter,
 	.finish = acpi_pm_finish,

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Alan Stern]

On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:

> Hi,
> 
> The appended patch adds the new pm_ops callback to be used to pass the target
> system sleep state to the platform core (ACPI core in particular) and reworks
> the ACPI PM operations to take this callback into account.
> 
> When I was working on this patch I thought it might be a good idea to do the
> following additional changes:
> * rename pm_ops to something more descriptive, like for example
>   'platform_suspend_operations'
> * move the definition of pm_ops (or whatever it will be called) to
>   <linux/suspend.h>
> * make the prepare(), enter() and finish() callbacks not take any arguments
> * clean up the PM-related code in the ARM tree (that, and the previous one,
>   would require someone to test the changes on these platforms, though)
> 
> Comments welcome.

Is this design okay with system states in which the CPU is able to run?

Right now the states we have are On, Standby, and Suspend, and the CPU
runs only in the On state.  But on some platforms there could be
multiple states in which the CPU is able to run, albeit with degraded 
performance.

So for something like Suspend the PM core tells the platform to enter 
the new state, and when the call returns the system has already left 
that state.  But with a low-performance On state, when the call returns 
the system will still be in the new state.

Is the PM core prepared to handle this difference?

Alan Stern

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

On Sunday, 24 June 2007 01:46, Alan Stern wrote:
> On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > The appended patch adds the new pm_ops callback to be used to pass the target
> > system sleep state to the platform core (ACPI core in particular) and reworks
> > the ACPI PM operations to take this callback into account.
> > 
> > When I was working on this patch I thought it might be a good idea to do the
> > following additional changes:
> > * rename pm_ops to something more descriptive, like for example
> >   'platform_suspend_operations'
> > * move the definition of pm_ops (or whatever it will be called) to
> >   <linux/suspend.h>
> > * make the prepare(), enter() and finish() callbacks not take any arguments
> > * clean up the PM-related code in the ARM tree (that, and the previous one,
> >   would require someone to test the changes on these platforms, though)
> > 
> > Comments welcome.
> 
> Is this design okay with system states in which the CPU is able to run?

Do you mean the patch or the suggestions above?

> Right now the states we have are On, Standby, and Suspend, and the CPU
> runs only in the On state.  But on some platforms there could be
> multiple states in which the CPU is able to run, albeit with degraded 
> performance.

I wouldn't call those system sleep states.  For example, ACPI defines system
sleep states as the states in which no instructions are executed by any CPUs
and I think that's reasonable.

Moreover, the ACPI spec insists that transitions between different sleep
states should be made through the On state.

> So for something like Suspend the PM core tells the platform to enter 
> the new state, and when the call returns the system has already left 
> that state.  But with a low-performance On state, when the call returns 
> the system will still be in the new state.
> 
> Is the PM core prepared to handle this difference?

No, I don't think so.  IMO, runtime power management is needed for the
low-performance On states.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Alan Stern]

On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:

> > Is this design okay with system states in which the CPU is able to run?
> 
> Do you mean the patch or the suggestions above?

The suggestions.

> > Right now the states we have are On, Standby, and Suspend, and the CPU
> > runs only in the On state.  But on some platforms there could be
> > multiple states in which the CPU is able to run, albeit with degraded 
> > performance.
> 
> I wouldn't call those system sleep states.  For example, ACPI defines system
> sleep states as the states in which no instructions are executed by any CPUs
> and I think that's reasonable.
> 
> Moreover, the ACPI spec insists that transitions between different sleep
> states should be made through the On state.

Okay.  But on non-ACPI systems, do we want to restrict the 
/sys/power/state interface to sleep states?  How then should the user 
tell the system to go to a low-performance run state?  Or should that 
be handled automatically within the kernel?

Alan Stern

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Sat, 2007-06-23 at 20:28 -0400, Alan Stern wrote:

> Okay.  But on non-ACPI systems, do we want to restrict the 
> /sys/power/state interface to sleep states?  How then should the user 
> tell the system to go to a low-performance run state?  Or should that 
> be handled automatically within the kernel?

The code for pasemi processors/boards has various idle loops right now
that put the processor into different states with different wakeup
latency, these are currently handled with a kernel parameter there.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Igor Stoppa]

Hi,
On Sat, 2007-06-23 at 20:28 -0400, ext Alan Stern wrote:
> On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> 
> > > Is this design okay with system states in which the CPU is able to
> run?
> > 
> > Do you mean the patch or the suggestions above?
> 
> The suggestions.
> 
> > > Right now the states we have are On, Standby, and Suspend, and the
> CPU
> > > runs only in the On state.  But on some platforms there could be
> > > multiple states in which the CPU is able to run, albeit with
> degraded 
> > > performance.
> > 
> > I wouldn't call those system sleep states.  For example, ACPI
> defines system
> > sleep states as the states in which no instructions are executed by
> any CPUs
> > and I think that's reasonable.
> > 
> > Moreover, the ACPI spec insists that transitions between different sleep
> > states should be made through the On state.
> 
> Okay.  But on non-ACPI systems, do we want to restrict the 
> /sys/power/state interface to sleep states?  How then should the user 
> tell the system to go to a low-performance run state?  Or should that 
> be handled automatically within the kernel?

That's how we do it since Nokia IT 770 where we have:
 
/sys/power/sleep_while_idle

and if that's set to 1 (default), then in the idle loop we try to hit
SoC retention.

Retention can be achieved if all the clocks have been released by their
respective users. If not all the clocks are free, the system oscillator
is not stopped and thereffore only some part will be in clock stop mode.
This, still, yelds significant power saving.

There is also an architecture specific (OMAP1 vs OMAP2) way to control
latency.

It's very rudimental and partially hackish: in OMAP2 it's mostly used to
workaround some missing wakeup dependencies (some are not provided by
the hw, some are not implemented properly by sw and should be fixed).

OMAP1: select ARM idle vs Big Sleep vs Deep Sleep

OMAP2: select ARM idle vs ARM retention, vs SoC retention

That's nice both from user's perspective (no need to worry about runtime
pm, it just works) and driver developer's perspective (if your driver is
for hw that has been properly wired, then using the clock fw interface
is enough).

So if you want to implement dependencies betweeen userspace and
kernelspace because your architecture has them builtin, it's ok, but
they should not be forced upon those arch that don't need them.


-- 
Cheers, Igor

Igor Stoppa <igor.stoppa@nokia.com>
(Nokia Multimedia - CP - OSSO / Helsinki, Finland)

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

On Sunday, 24 June 2007 02:28, Alan Stern wrote:
> On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> 
> > > Is this design okay with system states in which the CPU is able to run?
> > 
> > Do you mean the patch or the suggestions above?
> 
> The suggestions.
> 
> > > Right now the states we have are On, Standby, and Suspend, and the CPU
> > > runs only in the On state.  But on some platforms there could be
> > > multiple states in which the CPU is able to run, albeit with degraded 
> > > performance.
> > 
> > I wouldn't call those system sleep states.  For example, ACPI defines system
> > sleep states as the states in which no instructions are executed by any CPUs
> > and I think that's reasonable.
> > 
> > Moreover, the ACPI spec insists that transitions between different sleep
> > states should be made through the On state.
> 
> Okay.  But on non-ACPI systems, do we want to restrict the 
> /sys/power/state interface to sleep states?  How then should the user 
> tell the system to go to a low-performance run state?  Or should that 
> be handled automatically within the kernel?

I think that the /sys/power/state interface should be restricted to system
sleep states only and we should introduce another one for handling non-sleep
low-power states.

IMHO, the kernel can automatically transition to non-sleep low-power states,
but the users should be able to define the conditions for that.  Also, the user
should be able to force the transition if necessary/desired.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

On Sunday, 24 June 2007 13:49, Igor Stoppa wrote:
> Hi,
> On Sat, 2007-06-23 at 20:28 -0400, ext Alan Stern wrote:
> > On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> > 
> > > > Is this design okay with system states in which the CPU is able to
> > run?
> > > 
> > > Do you mean the patch or the suggestions above?
> > 
> > The suggestions.
> > 
> > > > Right now the states we have are On, Standby, and Suspend, and the
> > CPU
> > > > runs only in the On state.  But on some platforms there could be
> > > > multiple states in which the CPU is able to run, albeit with
> > degraded 
> > > > performance.
> > > 
> > > I wouldn't call those system sleep states.  For example, ACPI
> > defines system
> > > sleep states as the states in which no instructions are executed by
> > any CPUs
> > > and I think that's reasonable.
> > > 
> > > Moreover, the ACPI spec insists that transitions between different sleep
> > > states should be made through the On state.
> > 
> > Okay.  But on non-ACPI systems, do we want to restrict the 
> > /sys/power/state interface to sleep states?  How then should the user 
> > tell the system to go to a low-performance run state?  Or should that 
> > be handled automatically within the kernel?
> 
> That's how we do it since Nokia IT 770 where we have:
>  
> /sys/power/sleep_while_idle
> 
> and if that's set to 1 (default), then in the idle loop we try to hit
> SoC retention.
> 
> Retention can be achieved if all the clocks have been released by their
> respective users. If not all the clocks are free, the system oscillator
> is not stopped and thereffore only some part will be in clock stop mode.
> This, still, yelds significant power saving.
> 
> There is also an architecture specific (OMAP1 vs OMAP2) way to control
> latency.
> 
> It's very rudimental and partially hackish: in OMAP2 it's mostly used to
> workaround some missing wakeup dependencies (some are not provided by
> the hw, some are not implemented properly by sw and should be fixed).
> 
> OMAP1: select ARM idle vs Big Sleep vs Deep Sleep
> 
> OMAP2: select ARM idle vs ARM retention, vs SoC retention
> 
> That's nice both from user's perspective (no need to worry about runtime
> pm, it just works) and driver developer's perspective (if your driver is
> for hw that has been properly wired, then using the clock fw interface
> is enough).
> 
> So if you want to implement dependencies betweeen userspace and
> kernelspace because your architecture has them builtin, it's ok, but
> they should not be forced upon those arch that don't need them.

Sure, if your platform core is not going to give the user any control over
the runtime PM, it should be able to tell the PM core about that, in which case
the top-level runtime PM interface should be inactive.

This is one of the reasons why I think that /sys/power/state should cover
system sleep states only.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[David Brownell]

On Sunday 24 June 2007, Rafael J. Wysocki wrote:
> On Sunday, 24 June 2007 02:28, Alan Stern wrote:
> > On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> > 
> > > > Right now the states we have are On, Standby, and Suspend, and the CPU
> > > > runs only in the On state.  But on some platforms there could be
> > > > multiple states in which the CPU is able to run, albeit with degraded 
> > > > performance.

That starts to get into those cpufreq/"operating point" discussions".


> > > I wouldn't call those system sleep states.  For example, ACPI defines system
> > > sleep states as the states in which no instructions are executed by any CPUs
> > > and I think that's reasonable.

Its Embedded Controller surely executes instructions!


> > > Moreover, the ACPI spec insists that transitions between different sleep
> > > states should be made through the On state.

Not that the world should feel compelled to restrict itself to what
ACPI envisions of course...


> > Okay.  But on non-ACPI systems, do we want to restrict the 
> > /sys/power/state interface to sleep states?  How then should the user 
> > tell the system to go to a low-performance run state?  Or should that 
> > be handled automatically within the kernel?
> 
> I think that the /sys/power/state interface should be restricted to system
> sleep states only and we should introduce another one for handling non-sleep
> low-power states.

Then /sys/power/state would more accurately be named something
like /sys/power/sleep_state, right?

The core reason it would make sense to have such a bifurcation is
IMO that those lower power operating states mostly shouldn't involve
walking the device tree and suspending everything.  They involve some
set of hardware (possibly including CPUs) entering low power states.
But not *every* bit of hardware.

Another aspect here is what idle state is used.  Sometimes that
can be all but indistinguishable from a "sleep" state.


> IMHO, the kernel can automatically transition to non-sleep low-power states,
> but the users should be able to define the conditions for that.  Also, the user
> should be able to force the transition if necessary/desired.

There are several ways those lowpower run states can be entered.
But I'm not keen on userspace being able to "force" transitions
like that.

IMO it's strongly preferable if the platform code -- perhaps its
custom idle loop -- inventories the system regularly to see if
it's eligible to enter one of the states.

As a rule, if certain resources are in use that means related
power states can't be entered.  If they're in use, then userspace
trying to force a transition means breaking something -- else
those resources would stay used!

The similarity with cpufreq is obvious:  cpufreq inventories the
CPU usage regularly to see if the CPU should be revved up or down.

Now, inputs to that inventory can come from userspace ... there's
information about upcoming usage that the kernel could otherwise
only guess at.  User exiting the video playback app, vs just pausing
it, might suggest more aggressive savings are appropriate; etc.
Userspace can also provide tuning parameters.  I have no problems
with that model.

- Dave

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Pavel Machek]

Hi!

> > Right now the states we have are On, Standby, and Suspend, and the CPU
> > runs only in the On state.  But on some platforms there could be
> > multiple states in which the CPU is able to run, albeit with degraded 
> > performance.
> 
> I wouldn't call those system sleep states.  For example, ACPI defines system
> sleep states as the states in which no instructions are executed by any CPUs
> and I think that's reasonable.

Well, in some cases, we have 200MHz CPU running at 30kHz. ...that's so
slow that it is pretty similar to ACPI sleep state.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Dmitry Krivoschekov]

Pavel Machek wrote:
>>> Right now the states we have are On, Standby, and Suspend, and the CPU
>>> runs only in the On state.  But on some platforms there could be
>>> multiple states in which the CPU is able to run, albeit with degraded 
>>> performance.
>> I wouldn't call those system sleep states.  For example, ACPI defines system
>> sleep states as the states in which no instructions are executed by any CPUs
>> and I think that's reasonable.
>
> Well, in some cases, we have 200MHz CPU running at 30kHz. ...that's so
> slow that it is pretty similar to ACPI sleep state.
> 									
Pavel,

let's do not mix the things. Rafael gave exact meaning of a sleep state --
"no instructions are executed by any CPUs", that, I assume, can be
interpreted
as "processor does not do any effective job". It does not mean if any clock
frequency supplied or not. In some cases the frequency supplied to speed up
the wakeup process, but, the processor can't operate on this frequency,
so this is a sleep state. But, if the processor can do some job,
even if a frequency very very slow, then this is an active mode.


Regards,
Dmitry

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Pavel Machek]

Hi!

> >>> Right now the states we have are On, Standby, and Suspend, and the CPU
> >>> runs only in the On state.  But on some platforms there could be
> >>> multiple states in which the CPU is able to run, albeit with degraded 
> >>> performance.
> >> I wouldn't call those system sleep states.  For example, ACPI defines system
> >> sleep states as the states in which no instructions are executed by any CPUs
> >> and I think that's reasonable.
> >
> > Well, in some cases, we have 200MHz CPU running at 30kHz. ...that's so
> > slow that it is pretty similar to ACPI sleep state.
> > 									
> Pavel,
> 
> let's do not mix the things. Rafael gave exact meaning of a sleep state --
> "no instructions are executed by any CPUs", that, I assume, can be
> interpreted
> as "processor does not do any effective job". It does not mean if any clock
> frequency supplied or not. In some cases the frequency supplied to speed up
> the wakeup process, but, the processor can't operate on this frequency,
> so this is a sleep state. But, if the processor can do some job,
> even if a frequency very very slow, then this is an active mode.

Well, I'm trying to say that cpu (samsung arm in openmoko?) may be
able to execute instructions at very very low speed, but that the
speed is so low, that we should treat it as a sleep state.

I mean -- you can't use cpufreq to automatically chose between 32kHz
and 200MHz.

...but I guess it is all not too important...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

On Monday, 25 June 2007 02:01, David Brownell wrote:
> On Sunday 24 June 2007, Rafael J. Wysocki wrote:
> > On Sunday, 24 June 2007 02:28, Alan Stern wrote:
> > > On Sun, 24 Jun 2007, Rafael J. Wysocki wrote:
> > > 
> > > > > Right now the states we have are On, Standby, and Suspend, and the CPU
> > > > > runs only in the On state.  But on some platforms there could be
> > > > > multiple states in which the CPU is able to run, albeit with degraded 
> > > > > performance.
> 
> That starts to get into those cpufreq/"operating point" discussions".
> 
> 
> > > > I wouldn't call those system sleep states.  For example, ACPI defines system
> > > > sleep states as the states in which no instructions are executed by any CPUs
> > > > and I think that's reasonable.
> 
> Its Embedded Controller surely executes instructions!

Oh, I don't think that they regard the EC as a CPU, but never mind. :-)

> > > > Moreover, the ACPI spec insists that transitions between different sleep
> > > > states should be made through the On state.
> 
> Not that the world should feel compelled to restrict itself to what
> ACPI envisions of course...

Sure, but if ACPI does something in a reasonable way, then why not?
 
> > > Okay.  But on non-ACPI systems, do we want to restrict the 
> > > /sys/power/state interface to sleep states?  How then should the user 
> > > tell the system to go to a low-performance run state?  Or should that 
> > > be handled automatically within the kernel?
> > 
> > I think that the /sys/power/state interface should be restricted to system
> > sleep states only and we should introduce another one for handling non-sleep
> > low-power states.
> 
> Then /sys/power/state would more accurately be named something
> like /sys/power/sleep_state, right?

Yes, but there's user land that uses this interface.  Let's not confuse it.

> The core reason it would make sense to have such a bifurcation is
> IMO that those lower power operating states mostly shouldn't involve
> walking the device tree and suspending everything.  They involve some
> set of hardware (possibly including CPUs) entering low power states.
> But not *every* bit of hardware.
> 
> Another aspect here is what idle state is used.  Sometimes that
> can be all but indistinguishable from a "sleep" state.

Yes.  I think we can define a 'system sleep state' as a state that requires
the main code path in kernel/power/main.c (starting from enter_state()) to be
executed in order for the system to enter it.

> > IMHO, the kernel can automatically transition to non-sleep low-power states,
> > but the users should be able to define the conditions for that.  Also, the user
> > should be able to force the transition if necessary/desired.
> 
> There are several ways those lowpower run states can be entered.
> But I'm not keen on userspace being able to "force" transitions
> like that.
> 
> IMO it's strongly preferable if the platform code -- perhaps its
> custom idle loop -- inventories the system regularly to see if
> it's eligible to enter one of the states.
> 
> As a rule, if certain resources are in use that means related
> power states can't be entered.  If they're in use, then userspace
> trying to force a transition means breaking something -- else
> those resources would stay used!
> 
> The similarity with cpufreq is obvious:  cpufreq inventories the
> CPU usage regularly to see if the CPU should be revved up or down.
> 
> Now, inputs to that inventory can come from userspace ... there's
> information about upcoming usage that the kernel could otherwise
> only guess at.  User exiting the video playback app, vs just pausing
> it, might suggest more aggressive savings are appropriate; etc.
> Userspace can also provide tuning parameters.  I have no problems
> with that model.

OK

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [RFC][PATCH -mm] PM: Introduce set_target method in pm_ops

[Rafael J. Wysocki]

Hi,

On Monday, 25 June 2007 21:28, Pavel Machek wrote:
> Hi!
> 
> > >>> Right now the states we have are On, Standby, and Suspend, and the CPU
> > >>> runs only in the On state.  But on some platforms there could be
> > >>> multiple states in which the CPU is able to run, albeit with degraded 
> > >>> performance.
> > >> I wouldn't call those system sleep states.  For example, ACPI defines system
> > >> sleep states as the states in which no instructions are executed by any CPUs
> > >> and I think that's reasonable.
> > >
> > > Well, in some cases, we have 200MHz CPU running at 30kHz. ...that's so
> > > slow that it is pretty similar to ACPI sleep state.
> > > 									
> > Pavel,
> > 
> > let's do not mix the things. Rafael gave exact meaning of a sleep state --
> > "no instructions are executed by any CPUs", that, I assume, can be
> > interpreted
> > as "processor does not do any effective job". It does not mean if any clock
> > frequency supplied or not. In some cases the frequency supplied to speed up
> > the wakeup process, but, the processor can't operate on this frequency,
> > so this is a sleep state. But, if the processor can do some job,
> > even if a frequency very very slow, then this is an active mode.
> 
> Well, I'm trying to say that cpu (samsung arm in openmoko?) may be
> able to execute instructions at very very low speed, but that the
> speed is so low, that we should treat it as a sleep state.
> 
> I mean -- you can't use cpufreq to automatically chose between 32kHz
> and 200MHz.
> 
> ...but I guess it is all not too important...

Well, as I've just written to Dave, I think that we can define a 'system sleep
state' as a state that requires the main code path in kernel/power/main.c
(starting from enter_state()) to be executed in order for the system to enter
it.  Arguably, in such a state no useful work will be done by the system.

The other states can be regarded as 'running' or 'On' states.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth