Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Pavel Machek]

Hi!

> > I think this is a rather fundamental issue and it requires some more thought.
> >
> > What platform is your device based on, BTW?

> I'm working on an openmoko freerunner gta02 and do some work on android 
> framework.
> It's a smartphone
> http://wiki.openmoko.org/wiki/Main_Page
> 
> The bluetooth device for example in the Freerunner has a direct I2S connection to
> the WM8753.  Audio from a bluetooth headset is decoded by and sent digitially 
> to the WM8753 which does the digital to analogue conversion and 
> routes it out via the appropriate outputs.
> Analogue problem has the ti-caplyso when the audio is routed for a phone call.

I still believe it should be done in the driver... at least
today. Perhaps driver should just keep i2s/bluetooth powered up when
it is in use.

Perhaps we need new state 'sleep but keep working' for cases like
that; echo mem > state should force machine to drop calls etc. There
should be another value where stuff that is in use keeps being powered
up; I also suspect that that's what android wants to use. 

> Is the correct answer to you question?
 
Yes :-).

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

This is a little rework of linux power scheme. TODO:
- remove recursive call
- clean-up code

Avoid suspend of a device that is connected with other coprocessor
like GSM chip.

Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>

---
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 3098c46..2a6ddb3 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -63,6 +63,40 @@ void device_pm_unlock(void)
 	mutex_unlock(&dpm_list_mtx);
 }
 
+int device_set_may_suspend_enable(struct device *dev, void *data)
+{
+	/* if the device is suspend the subtree is in may_suspend status */
+	if (!dev->power.can_suspend)
+		goto out;
+
+	dev->power.may_suspend = (unsigned int)data;
+	device_for_each_child(dev, data,
+			      device_set_may_suspend_enable);
+out:
+	return 0;
+}
+
+/**
+ *	device_set_suspend_enable - enable/disable device to suspend
+ */
+int device_set_suspend_enable(struct device *dev, int enable)
+{
+	mutex_lock(&dpm_list_mtx);
+
+	if (dev->power.can_suspend == !!enable)
+		goto out;
+
+	/* Update device children to avoid suspend */
+	device_for_each_child(dev, (void *)!!enable,
+			      device_set_may_suspend_enable);
+
+	dev->power.can_suspend = !!enable;
+out:
+	mutex_unlock(&dpm_list_mtx);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_set_suspend_enable);
+
 /**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
@@ -77,6 +111,13 @@ void device_pm_add(struct device *dev)
 		if (dev->parent->power.status >= DPM_SUSPENDING)
 			dev_warn(dev, "parent %s should not be sleeping\n",
 				 dev_name(dev->parent));
+		if (!device_can_suspend(dev->parent)) {
+			mutex_unlock(&dpm_list_mtx);
+			/* if the parent has suspend disable, propagate it
+			 * to the new child */
+			device_set_may_suspend_enable(dev, 0);
+			mutex_lock(&dpm_list_mtx);
+		}
 	} else if (transition_started) {
 		/*
 		 * We refuse to register parentless devices while a PM
@@ -120,13 +161,13 @@ static int pm_op(struct device *dev, struct dev_pm_ops *ops,
 	switch (state.event) {
 #ifdef CONFIG_SUSPEND
 	case PM_EVENT_SUSPEND:
-		if (ops->suspend) {
+		if (device_can_suspend(dev) && ops->suspend) {
 			error = ops->suspend(dev);
 			suspend_report_result(ops->suspend, error);
 		}
 		break;
 	case PM_EVENT_RESUME:
-		if (ops->resume) {
+		if (device_can_suspend(dev) && ops->resume) {
 			error = ops->resume(dev);
 			suspend_report_result(ops->resume, error);
 		}
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 41f51fa..1197b13 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -1,6 +1,13 @@
 static inline void device_pm_init(struct device *dev)
 {
 	dev->power.status = DPM_ON;
+	dev->power.can_suspend = 1;
+	dev->power.may_suspend = 1;
+}
+
+static inline int device_can_suspend(struct device *dev)
+{
+	return (dev->power.can_suspend && dev->power.may_suspend);
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 596aeec..25236aa 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -43,6 +43,34 @@
 static const char enabled[] = "enabled";
 static const char disabled[] = "disabled";
 
+static ssize_t suspend_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%s\n", device_can_suspend(dev)
+		? enabled : disabled);
+}
+
+static ssize_t
+suspend_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t n)
+{
+	char *cp;
+	int len = n;
+
+	cp = memchr(buf, '\n', n);
+	if (cp)
+		len = cp - buf;
+	if (len == sizeof enabled - 1
+			&& strncmp(buf, enabled, sizeof enabled - 1) == 0)
+		device_set_suspend_enable(dev, 1);
+	else if (len == sizeof disabled - 1
+			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
+		device_set_suspend_enable(dev, 0);
+	else
+		return -EINVAL;
+	return n;
+}
+
 static ssize_t
 wake_show(struct device * dev, struct device_attribute *attr, char * buf)
 {
@@ -76,10 +104,11 @@ wake_store(struct device * dev, struct device_attribute *attr,
 }
 
 static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
-
+static DEVICE_ATTR(suspend, 0644, suspend_show, suspend_store);
 
 static struct attribute * power_attrs[] = {
 	&dev_attr_wakeup.attr,
+	&dev_attr_suspend.attr,
 	NULL,
 };
 static struct attribute_group pm_attr_group = {
diff --git a/include/linux/device.h b/include/linux/device.h
index fdb073b..dc9f242 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -487,6 +487,7 @@ extern struct device *device_find_child(struct device *dev, void *data,
 				int (*match)(struct device *dev, void *data));
 extern int device_rename(struct device *dev, char *new_name);
 extern int device_move(struct device *dev, struct device *new_parent);
+extern int device_set_suspend_enable(struct device *dev, int enable);
 
 /*
  * Root device objects for grouping under /sys/devices
diff --git a/include/linux/pm.h b/include/linux/pm.h
index de2e0a8..7586a90 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -319,6 +319,9 @@ struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
 	unsigned		should_wakeup:1;
+	unsigned		can_suspend:1;
+	unsigned		may_suspend:1;
+
 	enum dpm_state		status;		/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	struct list_head	entry;

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Tuesday 07 April 2009, Michael Trimarchi wrote:
> This is a little rework of linux power scheme. TODO:
> - remove recursive call
> - clean-up code
> 
> Avoid suspend of a device that is connected with other coprocessor
> like GSM chip.

Well, can you please tell us a bit about the motivation for this work?

I don't like the approach, but I'd like to know what you're trying to achieve.

Rafael


> Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
> 
> ---
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3098c46..2a6ddb3 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -63,6 +63,40 @@ void device_pm_unlock(void)
>  	mutex_unlock(&dpm_list_mtx);
>  }
>  
> +int device_set_may_suspend_enable(struct device *dev, void *data)
> +{
> +	/* if the device is suspend the subtree is in may_suspend status */
> +	if (!dev->power.can_suspend)
> +		goto out;
> +
> +	dev->power.may_suspend = (unsigned int)data;
> +	device_for_each_child(dev, data,
> +			      device_set_may_suspend_enable);
> +out:
> +	return 0;
> +}
> +
> +/**
> + *	device_set_suspend_enable - enable/disable device to suspend
> + */
> +int device_set_suspend_enable(struct device *dev, int enable)
> +{
> +	mutex_lock(&dpm_list_mtx);
> +
> +	if (dev->power.can_suspend == !!enable)
> +		goto out;
> +
> +	/* Update device children to avoid suspend */
> +	device_for_each_child(dev, (void *)!!enable,
> +			      device_set_may_suspend_enable);
> +
> +	dev->power.can_suspend = !!enable;
> +out:
> +	mutex_unlock(&dpm_list_mtx);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(device_set_suspend_enable);
> +
>  /**
>   *	device_pm_add - add a device to the list of active devices
>   *	@dev:	Device to be added to the list
> @@ -77,6 +111,13 @@ void device_pm_add(struct device *dev)
>  		if (dev->parent->power.status >= DPM_SUSPENDING)
>  			dev_warn(dev, "parent %s should not be sleeping\n",
>  				 dev_name(dev->parent));
> +		if (!device_can_suspend(dev->parent)) {
> +			mutex_unlock(&dpm_list_mtx);
> +			/* if the parent has suspend disable, propagate it
> +			 * to the new child */
> +			device_set_may_suspend_enable(dev, 0);
> +			mutex_lock(&dpm_list_mtx);
> +		}
>  	} else if (transition_started) {
>  		/*
>  		 * We refuse to register parentless devices while a PM
> @@ -120,13 +161,13 @@ static int pm_op(struct device *dev, struct dev_pm_ops *ops,
>  	switch (state.event) {
>  #ifdef CONFIG_SUSPEND
>  	case PM_EVENT_SUSPEND:
> -		if (ops->suspend) {
> +		if (device_can_suspend(dev) && ops->suspend) {
>  			error = ops->suspend(dev);
>  			suspend_report_result(ops->suspend, error);
>  		}
>  		break;
>  	case PM_EVENT_RESUME:
> -		if (ops->resume) {
> +		if (device_can_suspend(dev) && ops->resume) {
>  			error = ops->resume(dev);
>  			suspend_report_result(ops->resume, error);
>  		}
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 41f51fa..1197b13 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -1,6 +1,13 @@
>  static inline void device_pm_init(struct device *dev)
>  {
>  	dev->power.status = DPM_ON;
> +	dev->power.can_suspend = 1;
> +	dev->power.may_suspend = 1;
> +}
> +
> +static inline int device_can_suspend(struct device *dev)
> +{
> +	return (dev->power.can_suspend && dev->power.may_suspend);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 596aeec..25236aa 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -43,6 +43,34 @@
>  static const char enabled[] = "enabled";
>  static const char disabled[] = "disabled";
>  
> +static ssize_t suspend_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	return sprintf(buf, "%s\n", device_can_suspend(dev)
> +		? enabled : disabled);
> +}
> +
> +static ssize_t
> +suspend_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t n)
> +{
> +	char *cp;
> +	int len = n;
> +
> +	cp = memchr(buf, '\n', n);
> +	if (cp)
> +		len = cp - buf;
> +	if (len == sizeof enabled - 1
> +			&& strncmp(buf, enabled, sizeof enabled - 1) == 0)
> +		device_set_suspend_enable(dev, 1);
> +	else if (len == sizeof disabled - 1
> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
> +		device_set_suspend_enable(dev, 0);
> +	else
> +		return -EINVAL;
> +	return n;
> +}
> +
>  static ssize_t
>  wake_show(struct device * dev, struct device_attribute *attr, char * buf)
>  {
> @@ -76,10 +104,11 @@ wake_store(struct device * dev, struct device_attribute *attr,
>  }
>  
>  static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
> -
> +static DEVICE_ATTR(suspend, 0644, suspend_show, suspend_store);
>  
>  static struct attribute * power_attrs[] = {
>  	&dev_attr_wakeup.attr,
> +	&dev_attr_suspend.attr,
>  	NULL,
>  };
>  static struct attribute_group pm_attr_group = {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index fdb073b..dc9f242 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -487,6 +487,7 @@ extern struct device *device_find_child(struct device *dev, void *data,
>  				int (*match)(struct device *dev, void *data));
>  extern int device_rename(struct device *dev, char *new_name);
>  extern int device_move(struct device *dev, struct device *new_parent);
> +extern int device_set_suspend_enable(struct device *dev, int enable);
>  
>  /*
>   * Root device objects for grouping under /sys/devices
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index de2e0a8..7586a90 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -319,6 +319,9 @@ struct dev_pm_info {
>  	pm_message_t		power_state;
>  	unsigned		can_wakeup:1;
>  	unsigned		should_wakeup:1;
> +	unsigned		can_suspend:1;
> +	unsigned		may_suspend:1;
> +
>  	enum dpm_state		status;		/* Owned by the PM core */
>  #ifdef	CONFIG_PM_SLEEP
>  	struct list_head	entry;

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Hi,

Rafael J. Wysocki wrote:
> On Tuesday 07 April 2009, Michael Trimarchi wrote:
>   
>> This is a little rework of linux power scheme. TODO:
>> - remove recursive call
>> - clean-up code
>>
>> Avoid suspend of a device that is connected with other coprocessor
>> like GSM chip.
>>     
>
> Well, can you please tell us a bit about the motivation for this work?
>
> I don't like the approach, but I'd like to know what you're trying to achieve.
>   
I works on an hardware that have the gsm connect to wm8753 and the 
bluetooth subsystem
too direct connected. So I would like that a phone call for example 
remains on during
system suspend. With this approch I can disable suspend of an entire 
subtree of device,
that can be used by other hardware component. This change avoid any 
specific change to the
driver.

Michael
> Rafael
>
>
>   
>> Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
>>
>> ---
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 3098c46..2a6ddb3 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -63,6 +63,40 @@ void device_pm_unlock(void)
>>  	mutex_unlock(&dpm_list_mtx);
>>  }
>>  
>> +int device_set_may_suspend_enable(struct device *dev, void *data)
>> +{
>> +	/* if the device is suspend the subtree is in may_suspend status */
>> +	if (!dev->power.can_suspend)
>> +		goto out;
>> +
>> +	dev->power.may_suspend = (unsigned int)data;
>> +	device_for_each_child(dev, data,
>> +			      device_set_may_suspend_enable);
>> +out:
>> +	return 0;
>> +}
>> +
>> +/**
>> + *	device_set_suspend_enable - enable/disable device to suspend
>> + */
>> +int device_set_suspend_enable(struct device *dev, int enable)
>> +{
>> +	mutex_lock(&dpm_list_mtx);
>> +
>> +	if (dev->power.can_suspend == !!enable)
>> +		goto out;
>> +
>> +	/* Update device children to avoid suspend */
>> +	device_for_each_child(dev, (void *)!!enable,
>> +			      device_set_may_suspend_enable);
>> +
>> +	dev->power.can_suspend = !!enable;
>> +out:
>> +	mutex_unlock(&dpm_list_mtx);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(device_set_suspend_enable);
>> +
>>  /**
>>   *	device_pm_add - add a device to the list of active devices
>>   *	@dev:	Device to be added to the list
>> @@ -77,6 +111,13 @@ void device_pm_add(struct device *dev)
>>  		if (dev->parent->power.status >= DPM_SUSPENDING)
>>  			dev_warn(dev, "parent %s should not be sleeping\n",
>>  				 dev_name(dev->parent));
>> +		if (!device_can_suspend(dev->parent)) {
>> +			mutex_unlock(&dpm_list_mtx);
>> +			/* if the parent has suspend disable, propagate it
>> +			 * to the new child */
>> +			device_set_may_suspend_enable(dev, 0);
>> +			mutex_lock(&dpm_list_mtx);
>> +		}
>>  	} else if (transition_started) {
>>  		/*
>>  		 * We refuse to register parentless devices while a PM
>> @@ -120,13 +161,13 @@ static int pm_op(struct device *dev, struct dev_pm_ops *ops,
>>  	switch (state.event) {
>>  #ifdef CONFIG_SUSPEND
>>  	case PM_EVENT_SUSPEND:
>> -		if (ops->suspend) {
>> +		if (device_can_suspend(dev) && ops->suspend) {
>>  			error = ops->suspend(dev);
>>  			suspend_report_result(ops->suspend, error);
>>  		}
>>  		break;
>>  	case PM_EVENT_RESUME:
>> -		if (ops->resume) {
>> +		if (device_can_suspend(dev) && ops->resume) {
>>  			error = ops->resume(dev);
>>  			suspend_report_result(ops->resume, error);
>>  		}
>> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
>> index 41f51fa..1197b13 100644
>> --- a/drivers/base/power/power.h
>> +++ b/drivers/base/power/power.h
>> @@ -1,6 +1,13 @@
>>  static inline void device_pm_init(struct device *dev)
>>  {
>>  	dev->power.status = DPM_ON;
>> +	dev->power.can_suspend = 1;
>> +	dev->power.may_suspend = 1;
>> +}
>> +
>> +static inline int device_can_suspend(struct device *dev)
>> +{
>> +	return (dev->power.can_suspend && dev->power.may_suspend);
>>  }
>>  
>>  #ifdef CONFIG_PM_SLEEP
>> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>> index 596aeec..25236aa 100644
>> --- a/drivers/base/power/sysfs.c
>> +++ b/drivers/base/power/sysfs.c
>> @@ -43,6 +43,34 @@
>>  static const char enabled[] = "enabled";
>>  static const char disabled[] = "disabled";
>>  
>> +static ssize_t suspend_show(struct device *dev, struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", device_can_suspend(dev)
>> +		? enabled : disabled);
>> +}
>> +
>> +static ssize_t
>> +suspend_store(struct device *dev, struct device_attribute *attr,
>> +		const char *buf, size_t n)
>> +{
>> +	char *cp;
>> +	int len = n;
>> +
>> +	cp = memchr(buf, '\n', n);
>> +	if (cp)
>> +		len = cp - buf;
>> +	if (len == sizeof enabled - 1
>> +			&& strncmp(buf, enabled, sizeof enabled - 1) == 0)
>> +		device_set_suspend_enable(dev, 1);
>> +	else if (len == sizeof disabled - 1
>> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
>> +		device_set_suspend_enable(dev, 0);
>> +	else
>> +		return -EINVAL;
>> +	return n;
>> +}
>> +
>>  static ssize_t
>>  wake_show(struct device * dev, struct device_attribute *attr, char * buf)
>>  {
>> @@ -76,10 +104,11 @@ wake_store(struct device * dev, struct device_attribute *attr,
>>  }
>>  
>>  static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
>> -
>> +static DEVICE_ATTR(suspend, 0644, suspend_show, suspend_store);
>>  
>>  static struct attribute * power_attrs[] = {
>>  	&dev_attr_wakeup.attr,
>> +	&dev_attr_suspend.attr,
>>  	NULL,
>>  };
>>  static struct attribute_group pm_attr_group = {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index fdb073b..dc9f242 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -487,6 +487,7 @@ extern struct device *device_find_child(struct device *dev, void *data,
>>  				int (*match)(struct device *dev, void *data));
>>  extern int device_rename(struct device *dev, char *new_name);
>>  extern int device_move(struct device *dev, struct device *new_parent);
>> +extern int device_set_suspend_enable(struct device *dev, int enable);
>>  
>>  /*
>>   * Root device objects for grouping under /sys/devices
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index de2e0a8..7586a90 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -319,6 +319,9 @@ struct dev_pm_info {
>>  	pm_message_t		power_state;
>>  	unsigned		can_wakeup:1;
>>  	unsigned		should_wakeup:1;
>> +	unsigned		can_suspend:1;
>> +	unsigned		may_suspend:1;
>> +
>>  	enum dpm_state		status;		/* Owned by the PM core */
>>  #ifdef	CONFIG_PM_SLEEP
>>  	struct list_head	entry;
>>     
>
>   

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Tuesday 07 April 2009, Michael Trimarchi wrote:
> Hi,
> 
> Rafael J. Wysocki wrote:
> > On Tuesday 07 April 2009, Michael Trimarchi wrote:
> >   
> >> This is a little rework of linux power scheme. TODO:
> >> - remove recursive call
> >> - clean-up code
> >>
> >> Avoid suspend of a device that is connected with other coprocessor
> >> like GSM chip.
> >>     
> >
> > Well, can you please tell us a bit about the motivation for this work?
> >
> > I don't like the approach, but I'd like to know what you're trying to achieve.
> >   
> I works on an hardware that have the gsm connect to wm8753 and the 
> bluetooth subsystem
> too direct connected. So I would like that a phone call for example 
> remains on during
> system suspend. With this approch I can disable suspend of an entire 
> subtree of device,
> that can be used by other hardware component. This change avoid any 
> specific change to the
> driver.

Why do you want to avoid changing the driver, actually?

Rafael

   
> >> Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
> >>
> >> ---
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index 3098c46..2a6ddb3 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -63,6 +63,40 @@ void device_pm_unlock(void)
> >>  	mutex_unlock(&dpm_list_mtx);
> >>  }
> >>  
> >> +int device_set_may_suspend_enable(struct device *dev, void *data)
> >> +{
> >> +	/* if the device is suspend the subtree is in may_suspend status */
> >> +	if (!dev->power.can_suspend)
> >> +		goto out;
> >> +
> >> +	dev->power.may_suspend = (unsigned int)data;
> >> +	device_for_each_child(dev, data,
> >> +			      device_set_may_suspend_enable);
> >> +out:
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + *	device_set_suspend_enable - enable/disable device to suspend
> >> + */
> >> +int device_set_suspend_enable(struct device *dev, int enable)
> >> +{
> >> +	mutex_lock(&dpm_list_mtx);
> >> +
> >> +	if (dev->power.can_suspend == !!enable)
> >> +		goto out;
> >> +
> >> +	/* Update device children to avoid suspend */
> >> +	device_for_each_child(dev, (void *)!!enable,
> >> +			      device_set_may_suspend_enable);
> >> +
> >> +	dev->power.can_suspend = !!enable;
> >> +out:
> >> +	mutex_unlock(&dpm_list_mtx);
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(device_set_suspend_enable);
> >> +
> >>  /**
> >>   *	device_pm_add - add a device to the list of active devices
> >>   *	@dev:	Device to be added to the list
> >> @@ -77,6 +111,13 @@ void device_pm_add(struct device *dev)
> >>  		if (dev->parent->power.status >= DPM_SUSPENDING)
> >>  			dev_warn(dev, "parent %s should not be sleeping\n",
> >>  				 dev_name(dev->parent));
> >> +		if (!device_can_suspend(dev->parent)) {
> >> +			mutex_unlock(&dpm_list_mtx);
> >> +			/* if the parent has suspend disable, propagate it
> >> +			 * to the new child */
> >> +			device_set_may_suspend_enable(dev, 0);
> >> +			mutex_lock(&dpm_list_mtx);
> >> +		}
> >>  	} else if (transition_started) {
> >>  		/*
> >>  		 * We refuse to register parentless devices while a PM
> >> @@ -120,13 +161,13 @@ static int pm_op(struct device *dev, struct dev_pm_ops *ops,
> >>  	switch (state.event) {
> >>  #ifdef CONFIG_SUSPEND
> >>  	case PM_EVENT_SUSPEND:
> >> -		if (ops->suspend) {
> >> +		if (device_can_suspend(dev) && ops->suspend) {
> >>  			error = ops->suspend(dev);
> >>  			suspend_report_result(ops->suspend, error);
> >>  		}
> >>  		break;
> >>  	case PM_EVENT_RESUME:
> >> -		if (ops->resume) {
> >> +		if (device_can_suspend(dev) && ops->resume) {
> >>  			error = ops->resume(dev);
> >>  			suspend_report_result(ops->resume, error);
> >>  		}
> >> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> >> index 41f51fa..1197b13 100644
> >> --- a/drivers/base/power/power.h
> >> +++ b/drivers/base/power/power.h
> >> @@ -1,6 +1,13 @@
> >>  static inline void device_pm_init(struct device *dev)
> >>  {
> >>  	dev->power.status = DPM_ON;
> >> +	dev->power.can_suspend = 1;
> >> +	dev->power.may_suspend = 1;
> >> +}
> >> +
> >> +static inline int device_can_suspend(struct device *dev)
> >> +{
> >> +	return (dev->power.can_suspend && dev->power.may_suspend);
> >>  }
> >>  
> >>  #ifdef CONFIG_PM_SLEEP
> >> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> >> index 596aeec..25236aa 100644
> >> --- a/drivers/base/power/sysfs.c
> >> +++ b/drivers/base/power/sysfs.c
> >> @@ -43,6 +43,34 @@
> >>  static const char enabled[] = "enabled";
> >>  static const char disabled[] = "disabled";
> >>  
> >> +static ssize_t suspend_show(struct device *dev, struct device_attribute *attr,
> >> +				char *buf)
> >> +{
> >> +	return sprintf(buf, "%s\n", device_can_suspend(dev)
> >> +		? enabled : disabled);
> >> +}
> >> +
> >> +static ssize_t
> >> +suspend_store(struct device *dev, struct device_attribute *attr,
> >> +		const char *buf, size_t n)
> >> +{
> >> +	char *cp;
> >> +	int len = n;
> >> +
> >> +	cp = memchr(buf, '\n', n);
> >> +	if (cp)
> >> +		len = cp - buf;
> >> +	if (len == sizeof enabled - 1
> >> +			&& strncmp(buf, enabled, sizeof enabled - 1) == 0)
> >> +		device_set_suspend_enable(dev, 1);
> >> +	else if (len == sizeof disabled - 1
> >> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
> >> +		device_set_suspend_enable(dev, 0);
> >> +	else
> >> +		return -EINVAL;
> >> +	return n;
> >> +}
> >> +
> >>  static ssize_t
> >>  wake_show(struct device * dev, struct device_attribute *attr, char * buf)
> >>  {
> >> @@ -76,10 +104,11 @@ wake_store(struct device * dev, struct device_attribute *attr,
> >>  }
> >>  
> >>  static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
> >> -
> >> +static DEVICE_ATTR(suspend, 0644, suspend_show, suspend_store);
> >>  
> >>  static struct attribute * power_attrs[] = {
> >>  	&dev_attr_wakeup.attr,
> >> +	&dev_attr_suspend.attr,
> >>  	NULL,
> >>  };
> >>  static struct attribute_group pm_attr_group = {
> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index fdb073b..dc9f242 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -487,6 +487,7 @@ extern struct device *device_find_child(struct device *dev, void *data,
> >>  				int (*match)(struct device *dev, void *data));
> >>  extern int device_rename(struct device *dev, char *new_name);
> >>  extern int device_move(struct device *dev, struct device *new_parent);
> >> +extern int device_set_suspend_enable(struct device *dev, int enable);
> >>  
> >>  /*
> >>   * Root device objects for grouping under /sys/devices
> >> diff --git a/include/linux/pm.h b/include/linux/pm.h
> >> index de2e0a8..7586a90 100644
> >> --- a/include/linux/pm.h
> >> +++ b/include/linux/pm.h
> >> @@ -319,6 +319,9 @@ struct dev_pm_info {
> >>  	pm_message_t		power_state;
> >>  	unsigned		can_wakeup:1;
> >>  	unsigned		should_wakeup:1;
> >> +	unsigned		can_suspend:1;
> >> +	unsigned		may_suspend:1;
> >> +
> >>  	enum dpm_state		status;		/* Owned by the PM core */
> >>  #ifdef	CONFIG_PM_SLEEP
> >>  	struct list_head	entry;

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Tuesday 07 April 2009, Michael Trimarchi wrote:
>   
>> Hi,
>>
>> Rafael J. Wysocki wrote:
>>     
>>> On Tuesday 07 April 2009, Michael Trimarchi wrote:
>>>   
>>>       
>>>> This is a little rework of linux power scheme. TODO:
>>>> - remove recursive call
>>>> - clean-up code
>>>>
>>>> Avoid suspend of a device that is connected with other coprocessor
>>>> like GSM chip.
>>>>     
>>>>         
>>> Well, can you please tell us a bit about the motivation for this work?
>>>
>>> I don't like the approach, but I'd like to know what you're trying to achieve.
>>>   
>>>       
>> I works on an hardware that have the gsm connect to wm8753 and the 
>> bluetooth subsystem
>> too direct connected. So I would like that a phone call for example 
>> remains on during
>> system suspend. With this approch I can disable suspend of an entire 
>> subtree of device,
>> that can be used by other hardware component. This change avoid any 
>> specific change to the
>> driver.
>>     
>
> Why do you want to avoid changing the driver, actually?
>
> Rafael
>
>   
Because, the driver may be connected to other device and you must change 
the entire
set. This simple modification can help to share devices on an embedded 
board and control
suspend/resume enable from user space. I don't know if it can be usefull 
on broken board/device too. Do you see any possible risk with this 
change? When I write it I try to have a simple
modification on linux-pm part.

Michael
>    
>   
>>>> Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
>>>>
>>>> ---
>>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>>> index 3098c46..2a6ddb3 100644
>>>> --- a/drivers/base/power/main.c
>>>> +++ b/drivers/base/power/main.c
>>>> @@ -63,6 +63,40 @@ void device_pm_unlock(void)
>>>>  	mutex_unlock(&dpm_list_mtx);
>>>>  }
>>>>  
>>>> +int device_set_may_suspend_enable(struct device *dev, void *data)
>>>> +{
>>>> +	/* if the device is suspend the subtree is in may_suspend status */
>>>> +	if (!dev->power.can_suspend)
>>>> +		goto out;
>>>> +
>>>> +	dev->power.may_suspend = (unsigned int)data;
>>>> +	device_for_each_child(dev, data,
>>>> +			      device_set_may_suspend_enable);
>>>> +out:
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + *	device_set_suspend_enable - enable/disable device to suspend
>>>> + */
>>>> +int device_set_suspend_enable(struct device *dev, int enable)
>>>> +{
>>>> +	mutex_lock(&dpm_list_mtx);
>>>> +
>>>> +	if (dev->power.can_suspend == !!enable)
>>>> +		goto out;
>>>> +
>>>> +	/* Update device children to avoid suspend */
>>>> +	device_for_each_child(dev, (void *)!!enable,
>>>> +			      device_set_may_suspend_enable);
>>>> +
>>>> +	dev->power.can_suspend = !!enable;
>>>> +out:
>>>> +	mutex_unlock(&dpm_list_mtx);
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(device_set_suspend_enable);
>>>> +
>>>>  /**
>>>>   *	device_pm_add - add a device to the list of active devices
>>>>   *	@dev:	Device to be added to the list
>>>> @@ -77,6 +111,13 @@ void device_pm_add(struct device *dev)
>>>>  		if (dev->parent->power.status >= DPM_SUSPENDING)
>>>>  			dev_warn(dev, "parent %s should not be sleeping\n",
>>>>  				 dev_name(dev->parent));
>>>> +		if (!device_can_suspend(dev->parent)) {
>>>> +			mutex_unlock(&dpm_list_mtx);
>>>> +			/* if the parent has suspend disable, propagate it
>>>> +			 * to the new child */
>>>> +			device_set_may_suspend_enable(dev, 0);
>>>> +			mutex_lock(&dpm_list_mtx);
>>>> +		}
>>>>  	} else if (transition_started) {
>>>>  		/*
>>>>  		 * We refuse to register parentless devices while a PM
>>>> @@ -120,13 +161,13 @@ static int pm_op(struct device *dev, struct dev_pm_ops *ops,
>>>>  	switch (state.event) {
>>>>  #ifdef CONFIG_SUSPEND
>>>>  	case PM_EVENT_SUSPEND:
>>>> -		if (ops->suspend) {
>>>> +		if (device_can_suspend(dev) && ops->suspend) {
>>>>  			error = ops->suspend(dev);
>>>>  			suspend_report_result(ops->suspend, error);
>>>>  		}
>>>>  		break;
>>>>  	case PM_EVENT_RESUME:
>>>> -		if (ops->resume) {
>>>> +		if (device_can_suspend(dev) && ops->resume) {
>>>>  			error = ops->resume(dev);
>>>>  			suspend_report_result(ops->resume, error);
>>>>  		}
>>>> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
>>>> index 41f51fa..1197b13 100644
>>>> --- a/drivers/base/power/power.h
>>>> +++ b/drivers/base/power/power.h
>>>> @@ -1,6 +1,13 @@
>>>>  static inline void device_pm_init(struct device *dev)
>>>>  {
>>>>  	dev->power.status = DPM_ON;
>>>> +	dev->power.can_suspend = 1;
>>>> +	dev->power.may_suspend = 1;
>>>> +}
>>>> +
>>>> +static inline int device_can_suspend(struct device *dev)
>>>> +{
>>>> +	return (dev->power.can_suspend && dev->power.may_suspend);
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_PM_SLEEP
>>>> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>>>> index 596aeec..25236aa 100644
>>>> --- a/drivers/base/power/sysfs.c
>>>> +++ b/drivers/base/power/sysfs.c
>>>> @@ -43,6 +43,34 @@
>>>>  static const char enabled[] = "enabled";
>>>>  static const char disabled[] = "disabled";
>>>>  
>>>> +static ssize_t suspend_show(struct device *dev, struct device_attribute *attr,
>>>> +				char *buf)
>>>> +{
>>>> +	return sprintf(buf, "%s\n", device_can_suspend(dev)
>>>> +		? enabled : disabled);
>>>> +}
>>>> +
>>>> +static ssize_t
>>>> +suspend_store(struct device *dev, struct device_attribute *attr,
>>>> +		const char *buf, size_t n)
>>>> +{
>>>> +	char *cp;
>>>> +	int len = n;
>>>> +
>>>> +	cp = memchr(buf, '\n', n);
>>>> +	if (cp)
>>>> +		len = cp - buf;
>>>> +	if (len == sizeof enabled - 1
>>>> +			&& strncmp(buf, enabled, sizeof enabled - 1) == 0)
>>>> +		device_set_suspend_enable(dev, 1);
>>>> +	else if (len == sizeof disabled - 1
>>>> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
>>>> +		device_set_suspend_enable(dev, 0);
>>>> +	else
>>>> +		return -EINVAL;
>>>> +	return n;
>>>> +}
>>>> +
>>>>  static ssize_t
>>>>  wake_show(struct device * dev, struct device_attribute *attr, char * buf)
>>>>  {
>>>> @@ -76,10 +104,11 @@ wake_store(struct device * dev, struct device_attribute *attr,
>>>>  }
>>>>  
>>>>  static DEVICE_ATTR(wakeup, 0644, wake_show, wake_store);
>>>> -
>>>> +static DEVICE_ATTR(suspend, 0644, suspend_show, suspend_store);
>>>>  
>>>>  static struct attribute * power_attrs[] = {
>>>>  	&dev_attr_wakeup.attr,
>>>> +	&dev_attr_suspend.attr,
>>>>  	NULL,
>>>>  };
>>>>  static struct attribute_group pm_attr_group = {
>>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>>> index fdb073b..dc9f242 100644
>>>> --- a/include/linux/device.h
>>>> +++ b/include/linux/device.h
>>>> @@ -487,6 +487,7 @@ extern struct device *device_find_child(struct device *dev, void *data,
>>>>  				int (*match)(struct device *dev, void *data));
>>>>  extern int device_rename(struct device *dev, char *new_name);
>>>>  extern int device_move(struct device *dev, struct device *new_parent);
>>>> +extern int device_set_suspend_enable(struct device *dev, int enable);
>>>>  
>>>>  /*
>>>>   * Root device objects for grouping under /sys/devices
>>>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>>>> index de2e0a8..7586a90 100644
>>>> --- a/include/linux/pm.h
>>>> +++ b/include/linux/pm.h
>>>> @@ -319,6 +319,9 @@ struct dev_pm_info {
>>>>  	pm_message_t		power_state;
>>>>  	unsigned		can_wakeup:1;
>>>>  	unsigned		should_wakeup:1;
>>>> +	unsigned		can_suspend:1;
>>>> +	unsigned		may_suspend:1;
>>>> +
>>>>  	enum dpm_state		status;		/* Owned by the PM core */
>>>>  #ifdef	CONFIG_PM_SLEEP
>>>>  	struct list_head	entry;
>>>>         
>
>   

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Pavel Machek]

Hi!

>>> I works on an hardware that have the gsm connect to wm8753 and the  
>>> bluetooth subsystem
>>> too direct connected. So I would like that a phone call for example  
>>> remains on during
>>> system suspend. With this approch I can disable suspend of an entire  
>>> subtree of device,
>>> that can be used by other hardware component. This change avoid any  
>>> specific change to the
>>> driver.
>>
>> Why do you want to avoid changing the driver, actually?
>>   
> Because, the driver may be connected to other device and you must change  
> the entire
> set. This simple modification can help to share devices on an embedded  
> board and control
> suspend/resume enable from user space. I don't know if it can be usefull  
> on broken board/device too. Do you see any possible risk with this  
> change? 

Yes. Most devices (on a PC for example) will not like this being
enabled, right? So you are basically introducing a "break me" sysfs
variable.

I guess you should audit the drivers you are interested in to see if
they can survive with the hardware alive while the rest of system is
suspended, then add the option to the ones that can.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Tuesday 07 April 2009, Michael Trimarchi wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday 07 April 2009, Michael Trimarchi wrote:
> >   
> >> Hi,
> >>
> >> Rafael J. Wysocki wrote:
> >>     
> >>> On Tuesday 07 April 2009, Michael Trimarchi wrote:
> >>>   
> >>>       
> >>>> This is a little rework of linux power scheme. TODO:
> >>>> - remove recursive call
> >>>> - clean-up code
> >>>>
> >>>> Avoid suspend of a device that is connected with other coprocessor
> >>>> like GSM chip.
> >>>>     
> >>>>         
> >>> Well, can you please tell us a bit about the motivation for this work?
> >>>
> >>> I don't like the approach, but I'd like to know what you're trying to achieve.
> >>>   
> >>>       
> >> I works on an hardware that have the gsm connect to wm8753 and the 
> >> bluetooth subsystem
> >> too direct connected. So I would like that a phone call for example 
> >> remains on during
> >> system suspend. With this approch I can disable suspend of an entire 
> >> subtree of device,
> >> that can be used by other hardware component. This change avoid any 
> >> specific change to the
> >> driver.
> >>     
> >
> > Why do you want to avoid changing the driver, actually?
> >
> > Rafael
> >
> >   
> Because, the driver may be connected to other device and you must change 
> the entire
> set. This simple modification can help to share devices on an embedded 
> board and control
> suspend/resume enable from user space. I don't know if it can be usefull 
> on broken board/device too. Do you see any possible risk with this 
> change?

Yes, a (rather high) risk of abuse.

> When I write it I try to have a simple modification on linux-pm part.

So, basically, you'd like to introduce an interface allowing the user space to
tell the kernel which devices not to suspend, because there may be some
dependencies between devices that the kernel presumably doesn't know of.

Well, what about teaching these dependencies to the kernel, so that it
can take them into account by itself?

Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Alan Stern]

On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:

> > >> I works on an hardware that have the gsm connect to wm8753 and the 
> > >> bluetooth subsystem
> > >> too direct connected. So I would like that a phone call for example 
> > >> remains on during
> > >> system suspend. With this approch I can disable suspend of an entire 
> > >> subtree of device,
> > >> that can be used by other hardware component. This change avoid any 
> > >> specific change to the
> > >> driver.
> > >>     
> > >
> > > Why do you want to avoid changing the driver, actually?
> > >
> > > Rafael
> > >
> > >   
> > Because, the driver may be connected to other device and you must change 
> > the entire
> > set. This simple modification can help to share devices on an embedded 
> > board and control
> > suspend/resume enable from user space. I don't know if it can be usefull 
> > on broken board/device too. Do you see any possible risk with this 
> > change?
> 
> Yes, a (rather high) risk of abuse.
> 
> > When I write it I try to have a simple modification on linux-pm part.
> 
> So, basically, you'd like to introduce an interface allowing the user space to
> tell the kernel which devices not to suspend, because there may be some
> dependencies between devices that the kernel presumably doesn't know of.
> 
> Well, what about teaching these dependencies to the kernel, so that it
> can take them into account by itself?

Isn't that exactly what the patch already does?  It lets the kernel
know that when the "don't-suspend" flag is set for a device then the
entire sub-tree rooted at that device should remain unsuspended.

Alan Stern

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Pavel Machek]

On Tue 2009-04-07 17:31:13, Alan Stern wrote:
> On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:
> 
> > > >> I works on an hardware that have the gsm connect to wm8753 and the 
> > > >> bluetooth subsystem
> > > >> too direct connected. So I would like that a phone call for example 
> > > >> remains on during
> > > >> system suspend. With this approch I can disable suspend of an entire 
> > > >> subtree of device,
> > > >> that can be used by other hardware component. This change avoid any 
> > > >> specific change to the
> > > >> driver.
> > > >>     
> > > >
> > > > Why do you want to avoid changing the driver, actually?
> > > >
> > > > Rafael
> > > >
> > > >   
> > > Because, the driver may be connected to other device and you must change 
> > > the entire
> > > set. This simple modification can help to share devices on an embedded 
> > > board and control
> > > suspend/resume enable from user space. I don't know if it can be usefull 
> > > on broken board/device too. Do you see any possible risk with this 
> > > change?
> > 
> > Yes, a (rather high) risk of abuse.
> > 
> > > When I write it I try to have a simple modification on linux-pm part.
> > 
> > So, basically, you'd like to introduce an interface allowing the user space to
> > tell the kernel which devices not to suspend, because there may be some
> > dependencies between devices that the kernel presumably doesn't know of.
> > 
> > Well, what about teaching these dependencies to the kernel, so that it
> > can take them into account by itself?
> 
> Isn't that exactly what the patch already does?  It lets the kernel
> know that when the "don't-suspend" flag is set for a device then the
> entire sub-tree rooted at that device should remain unsuspended.

Well.... userspace should not have to decide this. If userspace tells
kernel not to suspend video card (on PC/ACPI), then we either honour
the request, or violate ACPI spec (and probably break suspend).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Tuesday 07 April 2009, Alan Stern wrote:
> On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:
> 
> > > >> I works on an hardware that have the gsm connect to wm8753 and the 
> > > >> bluetooth subsystem
> > > >> too direct connected. So I would like that a phone call for example 
> > > >> remains on during
> > > >> system suspend. With this approch I can disable suspend of an entire 
> > > >> subtree of device,
> > > >> that can be used by other hardware component. This change avoid any 
> > > >> specific change to the
> > > >> driver.
> > > >>     
> > > >
> > > > Why do you want to avoid changing the driver, actually?
> > > >
> > > > Rafael
> > > >
> > > >   
> > > Because, the driver may be connected to other device and you must change 
> > > the entire
> > > set. This simple modification can help to share devices on an embedded 
> > > board and control
> > > suspend/resume enable from user space. I don't know if it can be usefull 
> > > on broken board/device too. Do you see any possible risk with this 
> > > change?
> > 
> > Yes, a (rather high) risk of abuse.
> > 
> > > When I write it I try to have a simple modification on linux-pm part.
> > 
> > So, basically, you'd like to introduce an interface allowing the user space to
> > tell the kernel which devices not to suspend, because there may be some
> > dependencies between devices that the kernel presumably doesn't know of.
> > 
> > Well, what about teaching these dependencies to the kernel, so that it
> > can take them into account by itself?
> 
> Isn't that exactly what the patch already does?  It lets the kernel
> know that when the "don't-suspend" flag is set for a device then the
> entire sub-tree rooted at that device should remain unsuspended.

Well, in fact I wanted to know your opinion about this patch. :-)

IMO, there is a danger that suspend will break or even the machine will crash
if this is used for a wrong device.  I don't think it's generally safe to add
switches like this on the core level, because the core doesn't really know
which drivers are prepared for it.

Thanks,
Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Nigel Cunningham]

Hi.

On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
> Well.... userspace should not have to decide this. If userspace tells
> kernel not to suspend video card (on PC/ACPI), then we either honour
> the request, or violate ACPI spec (and probably break suspend).

What about the cases where the ACPI spec is irrelevant? (As I understand
it, not all embedded boards use ACPI). Would this be a good approach in
those cases? If so, perhaps the trick would be to make the functionality
depend on !CONFIG_ACPI?

Regards,

Nigel

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Hi,

Nigel Cunningham wrote:
> Hi.
>
> On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
>> Well.... userspace should not have to decide this. If userspace tells
>> kernel not to suspend video card (on PC/ACPI), then we either honour
>> the request, or violate ACPI spec (and probably break suspend).
>
> What about the cases where the ACPI spec is irrelevant? (As I understand
> it, not all embedded boards use ACPI). Would this be a good approach in
> those cases? If so, perhaps the trick would be to make the functionality
> depend on !CONFIG_ACPI?
It can be an option, or just add only in embedded configuration where is 
not ACPI configured.
The dependences is allready provided by the kernel. The default is to 
have suspend enabled.
The user level access it's needed because the kernel does't exacly know 
when the device
must remain on/off during suspend. This api change can't cover any 
possible scenario but
introduce a flexbility scheame in suspend process. Avoid suspend in some 
device can be obtain
looking at dependece too? I don't know exacly if the acpi capapiblity 
can be seen throw the
link to a bus or a specific class, but we can limit it to the platform 
device instead
all device.

Michael
>
> Regards,
>
> Nigel
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Wednesday 08 April 2009, Michael Trimarchi wrote:
> Hi,
> 
> Nigel Cunningham wrote:
> > Hi.
> >
> > On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
> >> Well.... userspace should not have to decide this. If userspace tells
> >> kernel not to suspend video card (on PC/ACPI), then we either honour
> >> the request, or violate ACPI spec (and probably break suspend).
> >
> > What about the cases where the ACPI spec is irrelevant? (As I understand
> > it, not all embedded boards use ACPI). Would this be a good approach in
> > those cases? If so, perhaps the trick would be to make the functionality
> > depend on !CONFIG_ACPI?
> It can be an option, or just add only in embedded configuration where is 
> not ACPI configured.
> The dependences is allready provided by the kernel. The default is to 
> have suspend enabled.
> The user level access it's needed because the kernel does't exacly know 
> when the device must remain on/off during suspend.

So, who exactly is going to have that information?  Does it depend on a user
decision on something else?  If something else, then what?

> This api change can't cover any possible scenario but
> introduce a flexbility scheame in suspend process. Avoid suspend in some 
> device can be obtain
> looking at dependece too? I don't know exacly if the acpi capapiblity 
> can be seen throw the
> link to a bus or a specific class, but we can limit it to the platform 
> device instead all device.

If I understand it correctly, the behavior you'd like to obtain is quite
similar to the one of wake-up devices that usually also need to remain
powered (at least to some extent) during suspend.  This, however, is handled
by the drivers of that devices, in their suspend callbacks.

How is your device different from the other wake-up devices?

Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Wednesday 08 April 2009, Michael Trimarchi wrote:
>   
>> Hi,
>>
>> Nigel Cunningham wrote:
>>     
>>> Hi.
>>>
>>> On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
>>>       
>>>> Well.... userspace should not have to decide this. If userspace tells
>>>> kernel not to suspend video card (on PC/ACPI), then we either honour
>>>> the request, or violate ACPI spec (and probably break suspend).
>>>>         
>>> What about the cases where the ACPI spec is irrelevant? (As I understand
>>> it, not all embedded boards use ACPI). Would this be a good approach in
>>> those cases? If so, perhaps the trick would be to make the functionality
>>> depend on !CONFIG_ACPI?
>>>       
>> It can be an option, or just add only in embedded configuration where is 
>> not ACPI configured.
>> The dependences is allready provided by the kernel. The default is to 
>> have suspend enabled.
>> The user level access it's needed because the kernel does't exacly know 
>> when the device must remain on/off during suspend.
>>     
>
> So, who exactly is going to have that information?  Does it depend on a user
> decision on something else?  If something else, then what?
>   
An example:

- a user has a blootooth handset and make a call so the application 
framework
know that that device are reserverd or blocked and are usefull for maintains
the call on. but linux can suspend without problem. So the user level
echo "disabled" for exaple to the 
/sys/devices/platform/soc-audio/power/disabled
and avoid suspending of audio devices and subdevice like aplifier, 
switch , etc. This
increase life battery of system, because permits a partial suspend.
>   
>> This api change can't cover any possible scenario but
>> introduce a flexbility scheame in suspend process. Avoid suspend in some 
>> device can be obtain
>> looking at dependece too? I don't know exacly if the acpi capapiblity 
>> can be seen throw the
>> link to a bus or a specific class, but we can limit it to the platform 
>> device instead all device.
>>     
>
> If I understand it correctly, the behavior you'd like to obtain is quite
> similar to the one of wake-up devices that usually also need to remain
> powered (at least to some extent) during suspend.  This, however, is handled
> by the drivers of that devices, in their suspend callbacks.
>
> How is your device different from the other wake-up devices?
>   
The difference from the wakeup device it's if I remember that they are 
suspendend
but ready to wakeup on external input. I want that device remain on, so 
I think that is
a little different beahvior. It's like to have a PM_DEVICE configuration 
dynamic instead
of static.
> Rafael
>
>   
Michael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Wednesday 08 April 2009, Michael Trimarchi wrote:
> Rafael J. Wysocki wrote:
> > On Wednesday 08 April 2009, Michael Trimarchi wrote:
> >   
> >> Hi,
> >>
> >> Nigel Cunningham wrote:
> >>     
> >>> Hi.
> >>>
> >>> On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
> >>>       
> >>>> Well.... userspace should not have to decide this. If userspace tells
> >>>> kernel not to suspend video card (on PC/ACPI), then we either honour
> >>>> the request, or violate ACPI spec (and probably break suspend).
> >>>>         
> >>> What about the cases where the ACPI spec is irrelevant? (As I understand
> >>> it, not all embedded boards use ACPI). Would this be a good approach in
> >>> those cases? If so, perhaps the trick would be to make the functionality
> >>> depend on !CONFIG_ACPI?
> >>>       
> >> It can be an option, or just add only in embedded configuration where is 
> >> not ACPI configured.
> >> The dependences is allready provided by the kernel. The default is to 
> >> have suspend enabled.
> >> The user level access it's needed because the kernel does't exacly know 
> >> when the device must remain on/off during suspend.
> >>     
> >
> > So, who exactly is going to have that information?  Does it depend on a user
> > decision on something else?  If something else, then what?
> >   
> An example:
> 
> - a user has a blootooth handset and make a call so the application 
> framework
> know that that device are reserverd or blocked and are usefull for maintains
> the call on. but linux can suspend without problem. So the user level
> echo "disabled" for exaple to the 
> /sys/devices/platform/soc-audio/power/disabled
> and avoid suspending of audio devices and subdevice like aplifier, 
> switch , etc. This
> increase life battery of system, because permits a partial suspend.
> >   
> >> This api change can't cover any possible scenario but
> >> introduce a flexbility scheame in suspend process. Avoid suspend in some 
> >> device can be obtain
> >> looking at dependece too? I don't know exacly if the acpi capapiblity 
> >> can be seen throw the
> >> link to a bus or a specific class, but we can limit it to the platform 
> >> device instead all device.
> >>     
> >
> > If I understand it correctly, the behavior you'd like to obtain is quite
> > similar to the one of wake-up devices that usually also need to remain
> > powered (at least to some extent) during suspend.  This, however, is handled
> > by the drivers of that devices, in their suspend callbacks.
> >
> > How is your device different from the other wake-up devices?
> >   
> The difference from the wakeup device it's if I remember that they are 
> suspendend
> but ready to wakeup on external input. I want that device remain on, so 
> I think that is
> a little different beahvior. It's like to have a PM_DEVICE configuration 
> dynamic instead of static.

OK, thanks.

I think this is a rather fundamental issue and it requires some more thought.

What platform is your device based on, BTW?

Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Wednesday 08 April 2009, Michael Trimarchi wrote:
>   
>> Rafael J. Wysocki wrote:
>>     
>>> On Wednesday 08 April 2009, Michael Trimarchi wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> Nigel Cunningham wrote:
>>>>     
>>>>         
>>>>> Hi.
>>>>>
>>>>> On Tue, 2009-04-07 at 23:38 +0200, Pavel Machek wrote:
>>>>>       
>>>>>           
>>>>>> Well.... userspace should not have to decide this. If userspace tells
>>>>>> kernel not to suspend video card (on PC/ACPI), then we either honour
>>>>>> the request, or violate ACPI spec (and probably break suspend).
>>>>>>         
>>>>>>             
>>>>> What about the cases where the ACPI spec is irrelevant? (As I understand
>>>>> it, not all embedded boards use ACPI). Would this be a good approach in
>>>>> those cases? If so, perhaps the trick would be to make the functionality
>>>>> depend on !CONFIG_ACPI?
>>>>>       
>>>>>           
>>>> It can be an option, or just add only in embedded configuration where is 
>>>> not ACPI configured.
>>>> The dependences is allready provided by the kernel. The default is to 
>>>> have suspend enabled.
>>>> The user level access it's needed because the kernel does't exacly know 
>>>> when the device must remain on/off during suspend.
>>>>     
>>>>         
>>> So, who exactly is going to have that information?  Does it depend on a user
>>> decision on something else?  If something else, then what?
>>>   
>>>       
>> An example:
>>
>> - a user has a blootooth handset and make a call so the application 
>> framework
>> know that that device are reserverd or blocked and are usefull for maintains
>> the call on. but linux can suspend without problem. So the user level
>> echo "disabled" for exaple to the 
>> /sys/devices/platform/soc-audio/power/disabled
>> and avoid suspending of audio devices and subdevice like aplifier, 
>> switch , etc. This
>> increase life battery of system, because permits a partial suspend.
>>     
>>>   
>>>       
>>>> This api change can't cover any possible scenario but
>>>> introduce a flexbility scheame in suspend process. Avoid suspend in some 
>>>> device can be obtain
>>>> looking at dependece too? I don't know exacly if the acpi capapiblity 
>>>> can be seen throw the
>>>> link to a bus or a specific class, but we can limit it to the platform 
>>>> device instead all device.
>>>>     
>>>>         
>>> If I understand it correctly, the behavior you'd like to obtain is quite
>>> similar to the one of wake-up devices that usually also need to remain
>>> powered (at least to some extent) during suspend.  This, however, is handled
>>> by the drivers of that devices, in their suspend callbacks.
>>>
>>> How is your device different from the other wake-up devices?
>>>   
>>>       
>> The difference from the wakeup device it's if I remember that they are 
>> suspendend
>> but ready to wakeup on external input. I want that device remain on, so 
>> I think that is
>> a little different beahvior. It's like to have a PM_DEVICE configuration 
>> dynamic instead of static.
>>     
>
> OK, thanks.
>
> I think this is a rather fundamental issue and it requires some more thought.
>
> What platform is your device based on, BTW?
>
> Rafael
>
>   
I'm working on an openmoko freerunner gta02 and do some work on android 
framework.
It's a smartphone
http://wiki.openmoko.org/wiki/Main_Page

The bluetooth device for example in the Freerunner has a direct I2S connection to
the WM8753.  Audio from a bluetooth headset is decoded by and sent digitially 
to the WM8753 which does the digital to analogue conversion and 
routes it out via the appropriate outputs.
Analogue problem has the ti-caplyso when the audio is routed for a phone call.

Is the correct answer to you question?

Michael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Mark Brown]

On Wed, Apr 08, 2009 at 10:34:56AM +0200, Rafael J. Wysocki wrote:

> I think this is a rather fundamental issue and it requires some more thought.

> What platform is your device based on, BTW?

FWIW this is an issue for a very large proportion of modern mobile phone
platforms - when on a call they can often suspend the bulk of the system
while leaving the audio path used for the call alive (normally some
combination of the telephony modem, audio CODEC and bluetooth chipset)
and wake up if the user presses a button or similar.

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Mark Brown]

On Tue, Apr 07, 2009 at 11:40:53PM +0200, Rafael J. Wysocki wrote:

> IMO, there is a danger that suspend will break or even the machine will crash
> if this is used for a wrong device.  I don't think it's generally safe to add
> switches like this on the core level, because the core doesn't really know
> which drivers are prepared for it.

OTOH it would be good to have a standard way of exposing this behaviour
to user space when it can be supported so that applications have some
degree of abstraction when working with these systems and drivers don't
have to keep on cooking up their own methods of exposing it.

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Igor Stoppa]

On Wed, 2009-04-08 at 13:42 +0200, ext Mark Brown wrote:
> On Wed, Apr 08, 2009 at 10:34:56AM +0200, Rafael J. Wysocki wrote:
> 
> > I think this is a rather fundamental issue and it requires some more thought.
> 
> > What platform is your device based on, BTW?
> 
> FWIW this is an issue for a very large proportion of modern mobile phone
> platforms - when on a call they can often suspend the bulk of the system
> while leaving the audio path used for the call alive (normally some
> combination of the telephony modem, audio CODEC and bluetooth chipset)
> and wake up if the user presses a button or similar.

That might be the outcome, but it should be simply because those parts
which are not in use are idle and therefore can enter spontaneously into
a low power state.

-- 

Cheers, Igor

---

Igor Stoppa
Maemo Software - Nokia Devices R&D - Helsinki

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Igor Stoppa]

On Wed, 2009-04-08 at 13:53 +0200, ext Mark Brown wrote:
> On Tue, Apr 07, 2009 at 11:40:53PM +0200, Rafael J. Wysocki wrote:
> 
> > IMO, there is a danger that suspend will break or even the machine will crash
> > if this is used for a wrong device.  I don't think it's generally safe to add
> > switches like this on the core level, because the core doesn't really know
> > which drivers are prepared for it.
> 
> OTOH it would be good to have a standard way of exposing this behaviour
> to user space when it can be supported so that applications have some
> degree of abstraction when working with these systems and drivers don't
> have to keep on cooking up their own methods of exposing it.

Don't rely on this method and instead implement automatic power saving
when entering runtime idle.

-- 

Cheers, Igor

---

Igor Stoppa
Maemo Software - Nokia Devices R&D - Helsinki

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Mark Brown]

On Wed, Apr 08, 2009 at 07:44:01PM +0300, Igor Stoppa wrote:

> That might be the outcome, but it should be simply because those parts
> which are not in use are idle and therefore can enter spontaneously into
> a low power state.

You might want to read the Android wakelock thread here - this comes
from Android and has been gone over in some detail in the threads
generated from their wakelock stuff.  Essentially what they are doing is
exactly what you suggest, they've identified that suspend is a low power
state and make every effort to enter it as often as possible, including
when on a call.  Clearly there are some issues with this approach but
the Android guys did feel that entering suspend was worthwhile for them,
especially since their users are able to download applications to the
phone.

Besides, no matter how low the power drain from your CPU when fully
gated and at the lowest DVFS setting it's still possible to get a lower
power drain since the CPU is going to be burning something and the
regulators providing the supply to it are going to be burning something.
In most cases it's going to be peanuts - it certainly will be in the
phone use case where you've got radios going.

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Igor Stoppa]

Hi,
On Wed, 2009-04-08 at 20:23 +0200, ext Mark Brown wrote:

> You might want to read the Android wakelock thread here - this comes
> from Android and has been gone over in some detail in the threads
> generated from their wakelock stuff.  

I did follow it. Didn't really like that either.

> Essentially what they are doing is
> exactly what you suggest, they've identified that suspend is a low power
> state and make every effort to enter it as often as possible, including
> when on a call. 

I'm not suggesting that. Suspend is inadequate for _real_ runtime power
management. Certainly it is not seamless.
Of course a smarter approach depends on the HW supporting certain
features, but if the HW is not there, I'd rather consider the suspend
based approach a workaround, rather than a real solution.

>  Clearly there are some issues with this approach but
> the Android guys did feel that entering suspend was worthwhile for them,
> especially since their users are able to download applications to the
> phone.

I don't think this is anyhow different from the Nokia Internet Tablets.

> Besides, no matter how low the power drain from your CPU when fully
> gated and at the lowest DVFS setting it's still possible to get a lower
> power drain since the CPU is going to be burning something and the
> regulators providing the supply to it are going to be burning something.

If you have a proper SoC, it can enter clock stop, lower voltage and
possibly even OFF mode. DVFS is by no means the way to save power in
general. It just addresses certain cases where the latency requirements
are too strict to afford entering really deep power saving states.
Otherwise it would mostly be better to run at max speed and do race to
OFF.

Regulators should have the options to be switched off if they are not
needed to preserve states, or enter a low power mode when only minimal
(non functional current) is needed.

> In most cases it's going to be peanuts - it certainly will be in the
> phone use case where you've got radios going.

I know that, but for this, suspend is still a poor man's power
management solution.
Of course the HW needs to be up to the intended task.

-- 

Cheers, Igor

---

Igor Stoppa
Maemo Software - Nokia Devices R&D - Helsinki

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Alan Stern]

On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:

> Well, in fact I wanted to know your opinion about this patch. :-)

Clearly this patch isn't appropriate for regular desktop or laptop 
systems.  I'm not so sure it's the best approach for embedded systems 
either.

Part of the problem is the set of devices which would remain 
unsuspended: the device for which the flag is set plus everything below 
it in the device tree.  This goes against the way the kernel has 
behaved up to now, which is that a device may not be suspended before 
all its children are suspended.

In addition, the patch appears to ignore issues involving clock and
voltage domains.  These things often are not reflected directly in the 
structure of the device tree.

At a more fundamental level, this change points out a real weakness in
the way suspend is currently implemented.  From the PM core's point of
view, system suspend involves two main activities:

	Telling drivers to stop using their devices, and

	Turning off (or reducing) power to the devices.

The PM framework does not treat these separately; a single suspend
method call is used for both purposes.  But more and more we are seeing
that they should be, especially on non-ACPI systems.  This patch is, in
a roundabout way, an attempt to do so.

Part of the problem is that people tend to think of "suspend" as
meaning "suspend the system".  However a much more flexible -- dare I
say more valid? -- point of view is "suspend the CPUs and at the same
time remove (or reduce) power for devices that will no longer need it".  
In other words, system suspend really is just a kind of runtime
suspend, in which the devices being suspended are the CPUs and the
sysdevs.

Obviously this is an oversimplification, but I think it's a useful 
approach.

Just think about it.  Suppose every driver supported autosuspend.  
When a driver received a notification that the CPU was going to be
suspended, it would know that its device wasn't going to need power
(since the device can't do anything useful without the driver telling
it what to do) and so it would automatically power the device down,
while also arranging not to access the device any more.  Thus the
suspend method calls would really exist only to let drivers know that
their code was going to stop running (since the CPU was about to stop
all activity); the device-power management part would merely be a side
effect.

And then, of course, drivers on embedded systems would be smart enough 
to know that some of the devices _should_ remain powered up, because 
they could still be useful even when the CPU wasn't running.  The only 
obstacle is letting the drivers know when their devices actually _are_ 
in use -- sometimes this is apparent only at the application level.

So the patch should be rewritten.  Change the name of the new attribute
to something like "autonomous" or "in_use", and don't make the PM core 
skip devices when the attribute is set.  Instead, change the relevant 
drivers.  Their suspend methods should arrange for the driver to stop 
using the device, but if the attribute is set then the device should 
not be powered down.

Alan Stern

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Alan Stern wrote:
> On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:
>
>   
>> Well, in fact I wanted to know your opinion about this patch. :-)
>>     
>
> Clearly this patch isn't appropriate for regular desktop or laptop 
> systems.  I'm not so sure it's the best approach for embedded systems 
> either.
>
> Part of the problem is the set of devices which would remain 
> unsuspended: the device for which the flag is set plus everything below 
> it in the device tree.  This goes against the way the kernel has 
> behaved up to now, which is that a device may not be suspended before 
> all its children are suspended.
>
> In addition, the patch appears to ignore issues involving clock and
> voltage domains.  These things often are not reflected directly in the 
> structure of the device tree.
>
> At a more fundamental level, this change points out a real weakness in
> the way suspend is currently implemented.  From the PM core's point of
> view, system suspend involves two main activities:
>
> 	Telling drivers to stop using their devices, and
>
> 	Turning off (or reducing) power to the devices.
>
> The PM framework does not treat these separately; a single suspend
> method call is used for both purposes.  But more and more we are seeing
> that they should be, especially on non-ACPI systems.  This patch is, in
> a roundabout way, an attempt to do so.
>
> Part of the problem is that people tend to think of "suspend" as
> meaning "suspend the system".  However a much more flexible -- dare I
> say more valid? -- point of view is "suspend the CPUs and at the same
> time remove (or reduce) power for devices that will no longer need it".  
> In other words, system suspend really is just a kind of runtime
> suspend, in which the devices being suspended are the CPUs and the
> sysdevs.
>
> Obviously this is an oversimplification, but I think it's a useful 
> approach.
>
> Just think about it.  Suppose every driver supported autosuspend.  
> When a driver received a notification that the CPU was going to be
> suspended, it would know that its device wasn't going to need power
> (since the device can't do anything useful without the driver telling
> it what to do) and so it would automatically power the device down,
> while also arranging not to access the device any more.  Thus the
> suspend method calls would really exist only to let drivers know that
> their code was going to stop running (since the CPU was about to stop
> all activity); the device-power management part would merely be a side
> effect.
>
> And then, of course, drivers on embedded systems would be smart enough 
> to know that some of the devices _should_ remain powered up, because 
> they could still be useful even when the CPU wasn't running.  The only 
> obstacle is letting the drivers know when their devices actually _are_ 
> in use -- sometimes this is apparent only at the application level.
>
> So the patch should be rewritten.  Change the name of the new attribute
> to something like "autonomous" or "in_use", and don't make the PM core 
> skip devices when the attribute is set.  Instead, change the relevant 
> drivers.  Their suspend methods should arrange for the driver to stop 
> using the device, but if the attribute is set then the device should 
> not be powered down.
>
> Alan Stern
>
>
>   
Ok I will provide a new patch with this approch.

Regards Michael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Wednesday 08 April 2009, Alan Stern wrote:
> On Tue, 7 Apr 2009, Rafael J. Wysocki wrote:
> 
> > Well, in fact I wanted to know your opinion about this patch. :-)
> 
> Clearly this patch isn't appropriate for regular desktop or laptop 
> systems.  I'm not so sure it's the best approach for embedded systems 
> either.
> 
> Part of the problem is the set of devices which would remain 
> unsuspended: the device for which the flag is set plus everything below 
> it in the device tree.  This goes against the way the kernel has 
> behaved up to now, which is that a device may not be suspended before 
> all its children are suspended.
> 
> In addition, the patch appears to ignore issues involving clock and
> voltage domains.  These things often are not reflected directly in the 
> structure of the device tree.
> 
> At a more fundamental level, this change points out a real weakness in
> the way suspend is currently implemented.  From the PM core's point of
> view, system suspend involves two main activities:
> 
> 	Telling drivers to stop using their devices, and
> 
> 	Turning off (or reducing) power to the devices.
> 
> The PM framework does not treat these separately; a single suspend
> method call is used for both purposes.  But more and more we are seeing
> that they should be, especially on non-ACPI systems.  This patch is, in
> a roundabout way, an attempt to do so.

Well, with the recent changes of the PM framework that have just gone into
.30-rc1 the "late" suspend call may in fact be regarded as a "turn off" or
"power down" one, while the "regular" suspend callback has become a "stop using
the device" one.

> Part of the problem is that people tend to think of "suspend" as
> meaning "suspend the system".  However a much more flexible -- dare I
> say more valid? -- point of view is "suspend the CPUs and at the same
> time remove (or reduce) power for devices that will no longer need it".  
> In other words, system suspend really is just a kind of runtime
> suspend, in which the devices being suspended are the CPUs and the
> sysdevs.
> 
> Obviously this is an oversimplification, but I think it's a useful 
> approach.

Well, unfortunately ACPI makes the distinction between suspending devices
in order to put the system into a sleep state and suspending devices at run
time (ACPI requires us to specify the target sleep state of the whole system in
advance and presumably the outcome of some AML routines used for suspending
devices may depend on this).  That's why the people who work primarily on ACPI
systems regard suspend as meaning "suspend the system".

> Just think about it.  Suppose every driver supported autosuspend.  
> When a driver received a notification that the CPU was going to be
> suspended, it would know that its device wasn't going to need power
> (since the device can't do anything useful without the driver telling
> it what to do) and so it would automatically power the device down,
> while also arranging not to access the device any more.  Thus the
> suspend method calls would really exist only to let drivers know that
> their code was going to stop running (since the CPU was about to stop
> all activity); the device-power management part would merely be a side
> effect.

Yes, I think this is the situation we should be targeting, but we seem to be
very far from it at the moment. :-)

> And then, of course, drivers on embedded systems would be smart enough 
> to know that some of the devices _should_ remain powered up, because 
> they could still be useful even when the CPU wasn't running.  The only 
> obstacle is letting the drivers know when their devices actually _are_ 
> in use -- sometimes this is apparent only at the application level.
> 
> So the patch should be rewritten.  Change the name of the new attribute
> to something like "autonomous" or "in_use", and don't make the PM core 
> skip devices when the attribute is set.  Instead, change the relevant 
> drivers.  Their suspend methods should arrange for the driver to stop 
> using the device, but if the attribute is set then the device should 
> not be powered down.

Good idea IMO.

Thanks a lot for the excellent comment. :-)

Best,
Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Mark Brown]

On Wed, Apr 08, 2009 at 10:53:07PM +0300, Igor Stoppa wrote:
> On Wed, 2009-04-08 at 20:23 +0200, ext Mark Brown wrote:

> > Essentially what they are doing is
> > exactly what you suggest, they've identified that suspend is a low power
> > state and make every effort to enter it as often as possible, including
> > when on a call. 

> I'm not suggesting that. Suspend is inadequate for _real_ runtime power
> management. Certainly it is not seamless.
> Of course a smarter approach depends on the HW supporting certain
> features, but if the HW is not there, I'd rather consider the suspend
> based approach a workaround, rather than a real solution.

Oh sure, but in principle it's exactly the same idea and you wouldn't
want to use the Android approach without also trying to drive the system
power down as much as possible while active.  Either way you're trying
to drive the system to the lowest power state automatically.

I'm not saying that it's the most lovely idea ever but there does seem
to be a use case, more in terms of stopping user space than anything
kernel side.

> >  Clearly there are some issues with this approach but
> > the Android guys did feel that entering suspend was worthwhile for them,
> > especially since their users are able to download applications to the
> > phone.

> I don't think this is anyhow different from the Nokia Internet Tablets.

Technically there's no difference.  The differences sound like they are
more on the support side and probably reflect the differences in
distribution chain, support model and expected user base for the
products.

> > Besides, no matter how low the power drain from your CPU when fully
> > gated and at the lowest DVFS setting it's still possible to get a lower

> If you have a proper SoC, it can enter clock stop, lower voltage and

...

> Regulators should have the options to be switched off if they are not
> needed to preserve states, or enter a low power mode when only minimal
> (non functional current) is needed.

Right, the issue is that the system also needs to be able to arrange to
power down the supplies to the SoC for maximum power savings.  Good on
chip power management is much more common than the ability to do that
without entering suspend.  Like I say, the savings from this are going
to be very small anywy so it's probably not worth worrying too much about.

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Alan Stern]

On Wed, 8 Apr 2009, Rafael J. Wysocki wrote:

> > the way suspend is currently implemented.  From the PM core's point of
> > view, system suspend involves two main activities:
> > 
> > 	Telling drivers to stop using their devices, and
> > 
> > 	Turning off (or reducing) power to the devices.
> > 
> > The PM framework does not treat these separately; a single suspend
> > method call is used for both purposes.  But more and more we are seeing
> > that they should be, especially on non-ACPI systems.  This patch is, in
> > a roundabout way, an attempt to do so.
> 
> Well, with the recent changes of the PM framework that have just gone into
> .30-rc1 the "late" suspend call may in fact be regarded as a "turn off" or
> "power down" one, while the "regular" suspend callback has become a "stop using
> the device" one.

Sort of, but that's not the real difference between suspend and 
suspend_late.  The real difference has to do with whether or not 
interrupts are enabled.

Still, if drivers begin to adopt this approach then it is a step in the 
right direction.

> > Part of the problem is that people tend to think of "suspend" as
> > meaning "suspend the system".  However a much more flexible -- dare I
> > say more valid? -- point of view is "suspend the CPUs and at the same
> > time remove (or reduce) power for devices that will no longer need it".  
> > In other words, system suspend really is just a kind of runtime
> > suspend, in which the devices being suspended are the CPUs and the
> > sysdevs.
> > 
> > Obviously this is an oversimplification, but I think it's a useful 
> > approach.
> 
> Well, unfortunately ACPI makes the distinction between suspending devices
> in order to put the system into a sleep state and suspending devices at run
> time (ACPI requires us to specify the target sleep state of the whole system in
> advance and presumably the outcome of some AML routines used for suspending
> devices may depend on this).  That's why the people who work primarily on ACPI
> systems regard suspend as meaning "suspend the system".

Just because ACPI has this requirement, that doesn't mean drivers have
to be designed around it.  We should be able to write a runtime-suspend
routine that does the right thing even when a system-suspend transition
is underway.

BTW, how does ACPI formally handle the case where the system is about
to go to sleep and some devices are already runtime-suspended?  Does it
require that the devices be resumed first so that they can be suspended
again the "right" way?

> > Just think about it.  Suppose every driver supported autosuspend.  
> > When a driver received a notification that the CPU was going to be
> > suspended, it would know that its device wasn't going to need power
> > (since the device can't do anything useful without the driver telling
> > it what to do) and so it would automatically power the device down,
> > while also arranging not to access the device any more.  Thus the
> > suspend method calls would really exist only to let drivers know that
> > their code was going to stop running (since the CPU was about to stop
> > all activity); the device-power management part would merely be a side
> > effect.
> 
> Yes, I think this is the situation we should be targeting, but we seem to be
> very far from it at the moment. :-)

True...  But let's not lose hope!  :-)

Alan Stern

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Rafael J. Wysocki]

On Thursday 09 April 2009, Alan Stern wrote:
> On Wed, 8 Apr 2009, Rafael J. Wysocki wrote:
> 
> > > the way suspend is currently implemented.  From the PM core's point of
> > > view, system suspend involves two main activities:
> > > 
> > > 	Telling drivers to stop using their devices, and
> > > 
> > > 	Turning off (or reducing) power to the devices.
> > > 
> > > The PM framework does not treat these separately; a single suspend
> > > method call is used for both purposes.  But more and more we are seeing
> > > that they should be, especially on non-ACPI systems.  This patch is, in
> > > a roundabout way, an attempt to do so.
> > 
> > Well, with the recent changes of the PM framework that have just gone into
> > .30-rc1 the "late" suspend call may in fact be regarded as a "turn off" or
> > "power down" one, while the "regular" suspend callback has become a "stop using
> > the device" one.
> 
> Sort of, but that's not the real difference between suspend and 
> suspend_late.  The real difference has to do with whether or not 
> interrupts are enabled.

Yes, but also the powering down/up should be moved to the late suspend/early
resume callbacks IMO.  Moreover, some PCI drivers may just let the core do
the power state changes which are then going to take place in the late/early
phases of suspend/resume.

> Still, if drivers begin to adopt this approach then it is a step in the 
> right direction.

Agreed.

> > > Part of the problem is that people tend to think of "suspend" as
> > > meaning "suspend the system".  However a much more flexible -- dare I
> > > say more valid? -- point of view is "suspend the CPUs and at the same
> > > time remove (or reduce) power for devices that will no longer need it".  
> > > In other words, system suspend really is just a kind of runtime
> > > suspend, in which the devices being suspended are the CPUs and the
> > > sysdevs.
> > > 
> > > Obviously this is an oversimplification, but I think it's a useful 
> > > approach.
> > 
> > Well, unfortunately ACPI makes the distinction between suspending devices
> > in order to put the system into a sleep state and suspending devices at run
> > time (ACPI requires us to specify the target sleep state of the whole system in
> > advance and presumably the outcome of some AML routines used for suspending
> > devices may depend on this).  That's why the people who work primarily on ACPI
> > systems regard suspend as meaning "suspend the system".
> 
> Just because ACPI has this requirement, that doesn't mean drivers have
> to be designed around it.  We should be able to write a runtime-suspend
> routine that does the right thing even when a system-suspend transition
> is underway.
> 
> BTW, how does ACPI formally handle the case where the system is about
> to go to sleep and some devices are already runtime-suspended?  Does it
> require that the devices be resumed first so that they can be suspended
> again the "right" way?

This, I must admit, is unclear to me, but I can imagine a situation in which
some extra preparations of the platform are needed for waking up the system
from a sleep state.  In such a case, I think, the wake-up device in question
should better be put into D0 before it can be prepared for the system suspend.

Thanks,
Rafael

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Pavel Machek]

On Wed 2009-04-08 12:53:30, Mark Brown wrote:
> On Tue, Apr 07, 2009 at 11:40:53PM +0200, Rafael J. Wysocki wrote:
> 
> > IMO, there is a danger that suspend will break or even the machine will crash
> > if this is used for a wrong device.  I don't think it's generally safe to add
> > switches like this on the core level, because the core doesn't really know
> > which drivers are prepared for it.
> 
> OTOH it would be good to have a standard way of exposing this behaviour
> to user space when it can be supported so that applications have some
> degree of abstraction when working with these systems and drivers don't
> have to keep on cooking up their own methods of exposing it.

Well, "standard way of exposing the behaviour" is quite useless if it
leads to kernel crashes, right? Those drivers that can do it can do it
themselves. And yes, they should all use the same interface.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Mark Brown]

On Tue, Apr 07, 2009 at 10:06:11AM +0200, Pavel Machek wrote:

> > the WM8753.  Audio from a bluetooth headset is decoded by and sent digitially 
> > to the WM8753 which does the digital to analogue conversion and 
> > routes it out via the appropriate outputs.
> > Analogue problem has the ti-caplyso when the audio is routed for a phone call.

> I still believe it should be done in the driver... at least
> today. Perhaps driver should just keep i2s/bluetooth powered up when
> it is in use.

The driver needs some way to figure out what it's supposed to do - it's
not immediately obvious without some policy information if suspend means
to suspend everything or if things that can keep working should do so.
The natural assumption for most systems would be that suspend means
suspend everything so that's what ASoC (the embedded audio subsystem
within ALSA) is doing.

Note also that there are multiple drivers involved in this decision.
The audio subsystem is separate from the bluetooth subsystem (which is
mostly managed from user space rather than by a driver) and the audio
subsystem is itself decomposed with three separate drivers in play,
though there is also an entry point for the subsystem which orchestrates
the audio side.  Keeping the audio live if the bluetooth suspends isn't
too useful.

> Perhaps we need new state 'sleep but keep working' for cases like
> that; echo mem > state should force machine to drop calls etc. There
> should be another value where stuff that is in use keeps being powered
> up; I also suspect that that's what android wants to use. 

That kind of devolves towards always driving to the lowest power
consumption (which we want to do anyway).  I suspect setting it system
wide would end up being too coarse grained.

Re: [RFC Disable suspend on a specific device] This is a little change in linux power scheme

[Michael Trimarchi]

Hi,

Mark Brown wrote:
> On Tue, Apr 07, 2009 at 10:06:11AM +0200, Pavel Machek wrote:
>
>   
>>> the WM8753.  Audio from a bluetooth headset is decoded by and sent digitially 
>>> to the WM8753 which does the digital to analogue conversion and 
>>> routes it out via the appropriate outputs.
>>> Analogue problem has the ti-caplyso when the audio is routed for a phone call.
>>>       
>
>   
>> I still believe it should be done in the driver... at least
>> today. Perhaps driver should just keep i2s/bluetooth powered up when
>> it is in use.
>>     
>
> The driver needs some way to figure out what it's supposed to do - it's
> not immediately obvious without some policy information if suspend means
> to suspend everything or if things that can keep working should do so.
> The natural assumption for most systems would be that suspend means
> suspend everything so that's what ASoC (the embedded audio subsystem
> within ALSA) is doing.
>
> Note also that there are multiple drivers involved in this decision.
> The audio subsystem is separate from the bluetooth subsystem (which is
> mostly managed from user space rather than by a driver) and the audio
> subsystem is itself decomposed with three separate drivers in play,
> though there is also an entry point for the subsystem which orchestrates
> the audio side.  Keeping the audio live if the bluetooth suspends isn't
> too useful.
>
>   
I sent a new patches moving the decision to the driver and implement a flag
that notify the driver is in_use by userspace application as proposed by 
Alan Stern
during the previus discussion.

Michael