[RFC Add no_suspend attribute V2] Let the driver know if it's in use

[RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

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
and sometimes this is apparent only at the application level.
The patch add the no_suspend attribute and it can be used by the
the drivers to stop power down during suspend.

Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
Cc: "Alan Stern" <stern@rowland.harvard.edu>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: "Pavel Mackek" <pavel@ucw.cz>
Cc: "Len Brown" <lenb@kernel.org>

---
 drivers/base/power/main.c  |   54 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/power.h |    5 ++++
 include/linux/device.h     |    1 +
 include/linux/pm.h         |    3 ++
 4 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 69b4ddb..baae309 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -65,6 +65,45 @@ void device_pm_unlock(void)
 }
 
 /**
+ *	device_set_no_suspend_enable - Mark the device as used by userspace
+ *	application
+ */
+void device_set_no_suspend_enable(struct device *dev, bool enable)
+{
+	struct device *next;
+
+	mutex_lock(&dpm_list_mtx);
+
+	/* the new status is equal the old one */
+	if (dev->power.no_suspend == !!enable)
+		goto out;
+
+	/* change the device status */
+	dev->power.no_suspend = !!enable;
+	if (dev->power.no_suspend)
+		dev->power.subtree_no_suspend = 0;
+
+	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
+		/* 
+		 * exit if we find a node with the same parent of the start
+		 * device
+		 */
+		if (dev->parent && next->parent == dev->parent)
+			break;
+
+		if (next->parent) {
+			/* Propagate the status */
+			next->power.subtree_no_suspend =
+				device_no_suspend_enable(next->parent);
+		}
+	}
+out:
+	mutex_unlock(&dpm_list_mtx);
+	return;
+}
+EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
+
+/**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
  */
@@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
+			/* if the parent has suspend disable, propagate it
+			 * to the new child */
+			dev->power.subtree_no_suspend = 1;
+		}
 	} else if (transition_started) {
 		/*
 		 * We refuse to register parentless devices while a PM
@@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
 		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
 	}
 
-	list_add_tail(&dev->power.entry, &dpm_list);
+	if (dev->parent) {
+		/*
+		 * if the device has a parent insert just before it.
+		 */
+		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
+	}
+	else
+		list_add_tail(&dev->power.entry, &dpm_list);
+
 	mutex_unlock(&dpm_list_mtx);
 }
 
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index c7cb4fc..a997b20 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -3,6 +3,11 @@ static inline void device_pm_init(struct device *dev)
 	dev->power.status = DPM_ON;
 }
 
+static inline int device_no_suspend_enable(struct device *dev)
+{
+	return dev->power.no_suspend || dev->power.subtree_no_suspend;
+}
+
 #ifdef CONFIG_PM_SLEEP
 
 /*
diff --git a/include/linux/device.h b/include/linux/device.h
index 2918c0e..74ca60d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -496,6 +496,7 @@ extern struct device *device_find_child(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,
 		       enum dpm_order dpm_order);
+extern void device_set_no_suspend_enable(struct device *dev, bool enable);
 
 /*
  * Root device objects for grouping under /sys/devices
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 1d4e2d2..e664d63 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		no_suspend:1;
+	unsigned		subtree_no_suspend:1;
+
 	enum dpm_state		status;		/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	struct list_head	entry;
-- 
1.5.6.5

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Randy Dunlap]

Michael Trimarchi wrote:
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 69b4ddb..baae309 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -65,6 +65,45 @@ void device_pm_unlock(void)
>  }
>  
>  /**
> + *	device_set_no_suspend_enable - Mark the device as used by userspace
> + *	application
> + */
> +void device_set_no_suspend_enable(struct device *dev, bool enable)
> +{

Small nit:  "/**" means "this is kernel-doc notation", but the kernel-doc
notation is not correct, so please either (a) make it correct (preferred)
or (b) don't use "/**" there.

For (a), it should look more like:

/**
 * device_set_no_suspend_enable - mark the device as used by userspace application
 * @dev: the target device
 * @enable: true if the device should be marked as "no_suspend"

Please correct these as needed, especially the @enable flag.  It seems
a bit like a double negative, unfortunately.

Thanks.

-- 
~Randy

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Hi,

Randy Dunlap wrote:
> Michael Trimarchi wrote:
>   
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 69b4ddb..baae309 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -65,6 +65,45 @@ void device_pm_unlock(void)
>>  }
>>  
>>  /**
>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
>> + *	application
>> + */
>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
>> +{
>>     
>
> Small nit:  "/**" means "this is kernel-doc notation", but the kernel-doc
> notation is not correct, so please either (a) make it correct (preferred)
> or (b) don't use "/**" there.
>
> For (a), it should look more like:
>
> /**
>  * device_set_no_suspend_enable - mark the device as used by userspace application
>  * @dev: the target device
>  * @enable: true if the device should be marked as "no_suspend"
>
> Please correct these as needed, especially the @enable flag.  It seems
> a bit like a double negative, unfortunately.
>
>   
Thanks

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Hi

I add all in cc because mutt give me some trouble :(
Michael Trimarchi wrote:
> 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
> and sometimes this is apparent only at the application level.
> The patch add the no_suspend attribute and it can be used by the
> the drivers to stop power down during suspend.
>
> Signed-off-by: Michael Trimarchi <trimarchi@gandalf.sssup.it>
> Cc: "Alan Stern" <stern@rowland.harvard.edu>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: "Pavel Mackek" <pavel@ucw.cz>
> Cc: "Len Brown" <lenb@kernel.org>
>
> ---
>  drivers/base/power/main.c  |   54 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/base/power/power.h |    5 ++++
>  include/linux/device.h     |    1 +
>  include/linux/pm.h         |    3 ++
>  4 files changed, 62 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 69b4ddb..baae309 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -65,6 +65,45 @@ void device_pm_unlock(void)
>  }
>  
>  /**
> + *	device_set_no_suspend_enable - Mark the device as used by userspace
> + *	application
> + */
> +void device_set_no_suspend_enable(struct device *dev, bool enable)
> +{
> +	struct device *next;
> +
> +	mutex_lock(&dpm_list_mtx);
> +
> +	/* the new status is equal the old one */
> +	if (dev->power.no_suspend == !!enable)
> +		goto out;
> +
> +	/* change the device status */
> +	dev->power.no_suspend = !!enable;
> +	if (dev->power.no_suspend)
> +		dev->power.subtree_no_suspend = 0;
>   
I find a bug here, i will fix.
It can be ok the rest of the code?
> +
> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
> +		/* 
> +		 * exit if we find a node with the same parent of the start
> +		 * device
> +		 */
> +		if (dev->parent && next->parent == dev->parent)
> +			break;
> +
> +		if (next->parent) {
> +			/* Propagate the status */
> +			next->power.subtree_no_suspend =
> +				device_no_suspend_enable(next->parent);
> +		}
> +	}
> +out:
> +	mutex_unlock(&dpm_list_mtx);
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
> +
> +/**
>   *	device_pm_add - add a device to the list of active devices
>   *	@dev:	Device to be added to the list
>   */
> @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
> +			/* if the parent has suspend disable, propagate it
> +			 * to the new child */
> +			dev->power.subtree_no_suspend = 1;
> +		}
>  	} else if (transition_started) {
>  		/*
>  		 * We refuse to register parentless devices while a PM
> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
>  	}
>  
> -	list_add_tail(&dev->power.entry, &dpm_list);
> +	if (dev->parent) {
> +		/*
> +		 * if the device has a parent insert just before it.
> +		 */
> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
> +	}
> +	else
> +		list_add_tail(&dev->power.entry, &dpm_list);
> +
>  	mutex_unlock(&dpm_list_mtx);
>  }
>  
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index c7cb4fc..a997b20 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -3,6 +3,11 @@ static inline void device_pm_init(struct device *dev)
>  	dev->power.status = DPM_ON;
>  }
>  
> +static inline int device_no_suspend_enable(struct device *dev)
> +{
> +	return dev->power.no_suspend || dev->power.subtree_no_suspend;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  /*
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 2918c0e..74ca60d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -496,6 +496,7 @@ extern struct device *device_find_child(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,
>  		       enum dpm_order dpm_order);
> +extern void device_set_no_suspend_enable(struct device *dev, bool enable);
>  
>  /*
>   * Root device objects for grouping under /sys/devices
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 1d4e2d2..e664d63 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		no_suspend:1;
> +	unsigned		subtree_no_suspend:1;
> +
>  	enum dpm_state		status;		/* Owned by the PM core */
>  #ifdef	CONFIG_PM_SLEEP
>  	struct list_head	entry;
>   

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Greg KH]

On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
> >  /**
> > + *	device_set_no_suspend_enable - Mark the device as used by userspace
> > + *	application
> > + */

This is not proper kernel-doc, please fix this up.

And "no_suspend_enable" is ackward, drop the "enable" part?


> > +void device_set_no_suspend_enable(struct device *dev, bool enable)
> > +{
> > +	struct device *next;
> > +
> > +	mutex_lock(&dpm_list_mtx);
> > +
> > +	/* the new status is equal the old one */
> > +	if (dev->power.no_suspend == !!enable)
> > +		goto out;
> > +
> > +	/* change the device status */
> > +	dev->power.no_suspend = !!enable;
> > +	if (dev->power.no_suspend)
> > +		dev->power.subtree_no_suspend = 0;
> >   
> I find a bug here, i will fix.
> It can be ok the rest of the code?
> > +
> > +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
> > +		/* 
> > +		 * exit if we find a node with the same parent of the start
> > +		 * device
> > +		 */
> > +		if (dev->parent && next->parent == dev->parent)
> > +			break;
> > +
> > +		if (next->parent) {
> > +			/* Propagate the status */
> > +			next->power.subtree_no_suspend =
> > +				device_no_suspend_enable(next->parent);
> > +		}
> > +	}
> > +out:
> > +	mutex_unlock(&dpm_list_mtx);
> > +	return;
> > +}
> > +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
> > +
> > +/**
> >   *	device_pm_add - add a device to the list of active devices
> >   *	@dev:	Device to be added to the list
> >   */
> > @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
> > +			/* if the parent has suspend disable, propagate it
> > +			 * to the new child */
> > +			dev->power.subtree_no_suspend = 1;
> > +		}
> >  	} else if (transition_started) {
> >  		/*
> >  		 * We refuse to register parentless devices while a PM
> > @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
> >  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
> >  	}
> >  
> > -	list_add_tail(&dev->power.entry, &dpm_list);
> > +	if (dev->parent) {
> > +		/*
> > +		 * if the device has a parent insert just before it.
> > +		 */
> > +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
> > +	}
> > +	else
> > +		list_add_tail(&dev->power.entry, &dpm_list);
> > +

Why are you changing the ordering for when we add devices to the list?
This seems like you are adding stuff now in backwards order, why make
this change?

confused,

greg k-h

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Hi
Greg KH wrote:
> On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
>   
>>>  /**
>>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
>>> + *	application
>>> + */
>>>       
>
> This is not proper kernel-doc, please fix this up.
>
> And "no_suspend_enable" is ackward, drop the "enable" part?
>
>   
Ok
>   
>>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
>>> +{
>>> +	struct device *next;
>>> +
>>> +	mutex_lock(&dpm_list_mtx);
>>> +
>>> +	/* the new status is equal the old one */
>>> +	if (dev->power.no_suspend == !!enable)
>>> +		goto out;
>>> +
>>> +	/* change the device status */
>>> +	dev->power.no_suspend = !!enable;
>>> +	if (dev->power.no_suspend)
>>> +		dev->power.subtree_no_suspend = 0;
>>>   
>>>       
>> I find a bug here, i will fix.
>> It can be ok the rest of the code?
>>     
>>> +
>>> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
>>> +		/* 
>>> +		 * exit if we find a node with the same parent of the start
>>> +		 * device
>>> +		 */
>>> +		if (dev->parent && next->parent == dev->parent)
>>> +			break;
>>> +
>>> +		if (next->parent) {
>>> +			/* Propagate the status */
>>> +			next->power.subtree_no_suspend =
>>> +				device_no_suspend_enable(next->parent);
>>> +		}
>>> +	}
>>> +out:
>>> +	mutex_unlock(&dpm_list_mtx);
>>> +	return;
>>> +}
>>> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
>>> +
>>> +/**
>>>   *	device_pm_add - add a device to the list of active devices
>>>   *	@dev:	Device to be added to the list
>>>   */
>>> @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
>>> +			/* if the parent has suspend disable, propagate it
>>> +			 * to the new child */
>>> +			dev->power.subtree_no_suspend = 1;
>>> +		}
>>>  	} else if (transition_started) {
>>>  		/*
>>>  		 * We refuse to register parentless devices while a PM
>>> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
>>>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
>>>  	}
>>>  
>>> -	list_add_tail(&dev->power.entry, &dpm_list);
>>> +	if (dev->parent) {
>>> +		/*
>>> +		 * if the device has a parent insert just before it.
>>> +		 */
>>> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
>>> +	}
>>> +	else
>>> +		list_add_tail(&dev->power.entry, &dpm_list);
>>> +
>>>       
>
> Why are you changing the ordering for when we add devices to the list?
> This seems like you are adding stuff now in backwards order, why make
> this change?
>   
The import thing is that a child is before the parent. This is put all 
the child before the
parent and just next to it. So

A3A2A1AB3B3B2B1

It is correct? This give the possiblity to dump the graph better and it 
doesn't change anything
in the correctness of the list
Michael
> confused,
>
> greg k-h
>
>   

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Greg KH wrote:
> On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
>   
>>>  /**
>>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
>>> + *	application
>>> + */
>>>       
>
> This is not proper kernel-doc, please fix this up.
>
> And "no_suspend_enable" is ackward, drop the "enable" part?
>
>
>   
>>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
>>> +{
>>> +	struct device *next;
>>> +
>>> +	mutex_lock(&dpm_list_mtx);
>>> +
>>> +	/* the new status is equal the old one */
>>> +	if (dev->power.no_suspend == !!enable)
>>> +		goto out;
>>> +
>>> +	/* change the device status */
>>> +	dev->power.no_suspend = !!enable;
>>> +	if (dev->power.no_suspend)
>>> +		dev->power.subtree_no_suspend = 0;
>>>   
>>>       
>> I find a bug here, i will fix.
>> It can be ok the rest of the code?
>>     
>>> +
>>> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
>>> +		/* 
>>> +		 * exit if we find a node with the same parent of the start
>>> +		 * device
>>> +		 */
>>> +		if (dev->parent && next->parent == dev->parent)
>>> +			break;
>>> +
>>> +		if (next->parent) {
>>> +			/* Propagate the status */
>>> +			next->power.subtree_no_suspend =
>>> +				device_no_suspend_enable(next->parent);
>>> +		}
>>> +	}
>>> +out:
>>> +	mutex_unlock(&dpm_list_mtx);
>>> +	return;
>>> +}
>>> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
>>> +
>>> +/**
>>>   *	device_pm_add - add a device to the list of active devices
>>>   *	@dev:	Device to be added to the list
>>>   */
>>> @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
>>> +			/* if the parent has suspend disable, propagate it
>>> +			 * to the new child */
>>> +			dev->power.subtree_no_suspend = 1;
>>> +		}
>>>  	} else if (transition_started) {
>>>  		/*
>>>  		 * We refuse to register parentless devices while a PM
>>> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
>>>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
>>>  	}
>>>  
>>> -	list_add_tail(&dev->power.entry, &dpm_list);
>>> +	if (dev->parent) {
>>> +		/*
>>> +		 * if the device has a parent insert just before it.
>>> +		 */
>>> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
>>> +	}
>>> +	else
>>> +		list_add_tail(&dev->power.entry, &dpm_list);
>>> +
>>>       
>
> Why are you changing the ordering for when we add devices to the list?
> This seems like you are adding stuff now in backwards order, why make
> this change?
>   
Sorry you are right the children must be discovere after and not  before 
parent.
I was convinced that children go to suspend before parent.
Michael
> confused,
>
> greg k-h
>
>   

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Rafael J. Wysocki]

On Saturday 25 April 2009, Michael Trimarchi wrote:
> Greg KH wrote:
> > On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
> >   
> >>>  /**
> >>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
> >>> + *	application
> >>> + */
> >>>       
> >
> > This is not proper kernel-doc, please fix this up.
> >
> > And "no_suspend_enable" is ackward, drop the "enable" part?
> >
> >
> >   
> >>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
> >>> +{
> >>> +	struct device *next;
> >>> +
> >>> +	mutex_lock(&dpm_list_mtx);
> >>> +
> >>> +	/* the new status is equal the old one */
> >>> +	if (dev->power.no_suspend == !!enable)
> >>> +		goto out;
> >>> +
> >>> +	/* change the device status */
> >>> +	dev->power.no_suspend = !!enable;
> >>> +	if (dev->power.no_suspend)
> >>> +		dev->power.subtree_no_suspend = 0;
> >>>   
> >>>       
> >> I find a bug here, i will fix.
> >> It can be ok the rest of the code?
> >>     
> >>> +
> >>> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
> >>> +		/* 
> >>> +		 * exit if we find a node with the same parent of the start
> >>> +		 * device
> >>> +		 */
> >>> +		if (dev->parent && next->parent == dev->parent)
> >>> +			break;
> >>> +
> >>> +		if (next->parent) {
> >>> +			/* Propagate the status */
> >>> +			next->power.subtree_no_suspend =
> >>> +				device_no_suspend_enable(next->parent);
> >>> +		}
> >>> +	}
> >>> +out:
> >>> +	mutex_unlock(&dpm_list_mtx);
> >>> +	return;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
> >>> +
> >>> +/**
> >>>   *	device_pm_add - add a device to the list of active devices
> >>>   *	@dev:	Device to be added to the list
> >>>   */
> >>> @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
> >>> +			/* if the parent has suspend disable, propagate it
> >>> +			 * to the new child */
> >>> +			dev->power.subtree_no_suspend = 1;
> >>> +		}
> >>>  	} else if (transition_started) {
> >>>  		/*
> >>>  		 * We refuse to register parentless devices while a PM
> >>> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
> >>>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
> >>>  	}
> >>>  
> >>> -	list_add_tail(&dev->power.entry, &dpm_list);
> >>> +	if (dev->parent) {
> >>> +		/*
> >>> +		 * if the device has a parent insert just before it.
> >>> +		 */
> >>> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
> >>> +	}
> >>> +	else
> >>> +		list_add_tail(&dev->power.entry, &dpm_list);
> >>> +
> >>>       
> >
> > Why are you changing the ordering for when we add devices to the list?
> > This seems like you are adding stuff now in backwards order, why make
> > this change?
> >   
> Sorry you are right the children must be discovere after and not  before 
> parent.
> I was convinced that children go to suspend before parent.

They generally do, but that's not precise.

More precisely, there are three walks of dpm_list during suspend.  First,
dpm_prepare() walks the list in the straight order, so if the parents are
in the list before their children, they will be visited first.  This is the
phase you can use to propagate your "no_suspend" flags from the parents to the
children, IMO.

Then, dpm_suspend() and device_power_down() walk the list in the reverse order,
and that's where the device drivers' suspend callbacks are executed.

Thanks,
Rafael

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Saturday 25 April 2009, Michael Trimarchi wrote:
>   
>> Greg KH wrote:
>>     
>>> On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
>>>   
>>>       
>>>>>  /**
>>>>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
>>>>> + *	application
>>>>> + */
>>>>>       
>>>>>           
>>> This is not proper kernel-doc, please fix this up.
>>>
>>> And "no_suspend_enable" is ackward, drop the "enable" part?
>>>
>>>
>>>   
>>>       
>>>>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
>>>>> +{
>>>>> +	struct device *next;
>>>>> +
>>>>> +	mutex_lock(&dpm_list_mtx);
>>>>> +
>>>>> +	/* the new status is equal the old one */
>>>>> +	if (dev->power.no_suspend == !!enable)
>>>>> +		goto out;
>>>>> +
>>>>> +	/* change the device status */
>>>>> +	dev->power.no_suspend = !!enable;
>>>>> +	if (dev->power.no_suspend)
>>>>> +		dev->power.subtree_no_suspend = 0;
>>>>>   
>>>>>       
>>>>>           
>>>> I find a bug here, i will fix.
>>>> It can be ok the rest of the code?
>>>>     
>>>>         
>>>>> +
>>>>> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
>>>>> +		/* 
>>>>> +		 * exit if we find a node with the same parent of the start
>>>>> +		 * device
>>>>> +		 */
>>>>> +		if (dev->parent && next->parent == dev->parent)
>>>>> +			break;
>>>>> +
>>>>> +		if (next->parent) {
>>>>> +			/* Propagate the status */
>>>>> +			next->power.subtree_no_suspend =
>>>>> +				device_no_suspend_enable(next->parent);
>>>>> +		}
>>>>> +	}
>>>>> +out:
>>>>> +	mutex_unlock(&dpm_list_mtx);
>>>>> +	return;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
>>>>> +
>>>>> +/**
>>>>>   *	device_pm_add - add a device to the list of active devices
>>>>>   *	@dev:	Device to be added to the list
>>>>>   */
>>>>> @@ -78,6 +117,11 @@ 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_no_suspend_enable(dev->parent)) {
>>>>> +			/* if the parent has suspend disable, propagate it
>>>>> +			 * to the new child */
>>>>> +			dev->power.subtree_no_suspend = 1;
>>>>> +		}
>>>>>  	} else if (transition_started) {
>>>>>  		/*
>>>>>  		 * We refuse to register parentless devices while a PM
>>>>> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
>>>>>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
>>>>>  	}
>>>>>  
>>>>> -	list_add_tail(&dev->power.entry, &dpm_list);
>>>>> +	if (dev->parent) {
>>>>> +		/*
>>>>> +		 * if the device has a parent insert just before it.
>>>>> +		 */
>>>>> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
>>>>>           
Anyway, I revert this beacuse is wrong but is it correct to use here the
list_add instead the list_add_tail, it put the child node
after the parent stricly and don't create hole in subtree visit. Now I 
see only a dump
reason for debugging and a simple exit in flag update.
The important thing is that the children follow the parent,
and in this way is like visiting the tree. Nothing change for me because 
is difficult
to isolate only a subtree, if you have a list, but I'm sure that if I 
find a subtree with
the same parent, my subtree is finish, and if someone add a new device, 
the system put
in the correct position.
>>>>> +	}
>>>>> +	else
>>>>> +		list_add_tail(&dev->power.entry, &dpm_list);
>>>>> +
>>>>>       
>>>>>           
>>> Why are you changing the ordering for when we add devices to the list?
>>> This seems like you are adding stuff now in backwards order, why make
>>> this change?
>>>   
>>>       
>> Sorry you are right the children must be discovere after and not  before 
>> parent.
>> I was convinced that children go to suspend before parent.
>>     
>
> They generally do, but that's not precise.
>
> More precisely, there are three walks of dpm_list during suspend.  First,
> dpm_prepare() walks the list in the straight order, so if the parents are
> in the list before their children, they will be visited first.  This is the
> phase you can use to propagate your "no_suspend" flags from the parents to the
> children, IMO.
>
> Then, dpm_suspend() and device_power_down() walk the list in the reverse order,
> and that's where the device drivers' suspend callbacks are executed.
>   
Ok it's clear... Thank's Rafael for the support. You give me a big help 
when you invite
me to check the code again. Now I try to produce a new patch.
> Thanks,
> Rafael
>
>   
Thanks.
Michael

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Alan Stern]

On Sat, 25 Apr 2009, Michael Trimarchi wrote:

> Anyway, I revert this beacuse is wrong but is it correct to use here the
> list_add instead the list_add_tail, it put the child node
> after the parent stricly and don't create hole in subtree visit. Now I 
> see only a dump
> reason for debugging and a simple exit in flag update.
> The important thing is that the children follow the parent,
> and in this way is like visiting the tree.

That's not necessarily true.  There may be dependencies between devices 
that aren't expressed by the parent-child relationship.

For example, device B might require device A even though A isn't a 
parent (or ancestor) of B.  The way dpm_list works now this is okay, 
because devices are added in order of discovery.  That is, if B depends 
on A then A must have been discovered before B, so A will come first on 
the list.

If you change things by adding children directly after their parent 
then you will mess this up.  For example, if B's parent was discovered 
before A, then adding B directly after its parent would mean putting B 
before A on the list.  Then you'd run into trouble during a system 
suspend, because A would be suspended before B and that would prevent B 
from working properly.

> Nothing change for me because 
> is difficult
> to isolate only a subtree, if you have a list, but I'm sure that if I 
> find a subtree with
> the same parent, my subtree is finish, and if someone add a new device, 
> the system put
> in the correct position.

It is _not_ difficult to isolate a subtree using a list.  For example:

	mutex_lock(&dpm_mutex);
	topdev->in_subtree = 1;
	list_for_each_entry(dpm_list, dev, power.entry) {
		if (dev->parent && dev->parent->in_subtree)
			dev->in_subtree = 1;
	}
	list_for_each_entry(dpm_list, dev, power.entry) {
		if (dev->in_subtree) {
			dev->in_subtree = 0;
			dev->dont_suspend = 1;
		}
	}
	mutex_unlock(&dpm_mutex);

This will set the dont_suspend flag for all devices in the subtree 
starting at topdev.

Alan Stern

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Alan Stern wrote:
> On Sat, 25 Apr 2009, Michael Trimarchi wrote:
>
>   
>> Anyway, I revert this beacuse is wrong but is it correct to use here the
>> list_add instead the list_add_tail, it put the child node
>> after the parent stricly and don't create hole in subtree visit. Now I 
>> see only a dump
>> reason for debugging and a simple exit in flag update.
>> The important thing is that the children follow the parent,
>> and in this way is like visiting the tree.
>>     
>
> That's not necessarily true.  There may be dependencies between devices 
> that aren't expressed by the parent-child relationship.
>
> For example, device B might require device A even though A isn't a 
> parent (or ancestor) of B.  The way dpm_list works now this is okay, 
> because devices are added in order of discovery.  That is, if B depends 
> on A then A must have been discovered before B, so A will come first on 
> the list.
>   
Ok, so the sysfs don't contain all the dependenceis  but dmp_list contains
the exaclty suspend order of the device
> If you change things by adding children directly after their parent 
> then you will mess this up.  For example, if B's parent was discovered 
> before A, then adding B directly after its parent would mean putting B 
> before A on the list.  Then you'd run into trouble during a system 
> suspend, because A would be suspended before B and that would prevent B 
> from working properly.
>   
Ok, I understand, so how to know the devices dependences? There is a way 
to express
this relation? Because if it is not possible a single flag can be used
>   
>> Nothing change for me because 
>> is difficult
>> to isolate only a subtree, if you have a list, but I'm sure that if I 
>> find a subtree with
>> the same parent, my subtree is finish, and if someone add a new device, 
>> the system put
>> in the correct position.
>>     
>
> It is _not_ difficult to isolate a subtree using a list.  For example:
>
> 	mutex_lock(&dpm_mutex);
> 	topdev->in_subtree = 1;
> 	list_for_each_entry(dpm_list, dev, power.entry) {
> 		if (dev->parent && dev->parent->in_subtree)
> 			dev->in_subtree = 1;
> 	}
> 	list_for_each_entry(dpm_list, dev, power.entry) {
> 		if (dev->in_subtree) {
> 			dev->in_subtree = 0;
> 			dev->dont_suspend = 1;
> 		}
> 	}
> 	mutex_unlock(&dpm_mutex);
>
> This will set the dont_suspend flag for all devices in the subtree 
> starting at topdev.
>   
Yes this is possible, but you walk all list twice.
Regards Michael

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Alan Stern]

On Sat, 25 Apr 2009, Michael Trimarchi wrote:

> Ok, I understand, so how to know the devices dependences? There is a way 
> to express
> this relation? Because if it is not possible a single flag can be used

Nobody knows all the device dependencies.  That's why we're afraid that 
if you change the list, something will break.

> > This will set the dont_suspend flag for all devices in the subtree 
> > starting at topdev.
> >   
> Yes this is possible, but you walk all list twice.

So what?  Walking the list is very quick, and you're not on a 
time-critical path.

Alan Stern

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Hi,
Alan Stern wrote:
> On Sat, 25 Apr 2009, Michael Trimarchi wrote:
>
>   
>> Ok, I understand, so how to know the devices dependences? There is a way 
>> to express
>> this relation? Because if it is not possible a single flag can be used
>>     
>
> Nobody knows all the device dependencies.  That's why we're afraid that 
> if you change the list, something will break.
>
>   
But this happen for broken device? The list is a sufficient condition 
for correct
suspend phase but is not necesarry, because all the dependence are not 
registerd.
But just to understand: What are the typical situation, because I 
suppose that
we are not talking about broken device.
>>> This will set the dont_suspend flag for all devices in the subtree 
>>> starting at topdev.
>>>   
>>>       
>> Yes this is possible, but you walk all list twice.
>>     
>
> So what?  Walking the list is very quick, and you're not on a 
> time-critical path.
>
>   
Yes, you are right, I will submit a new patch with all the necessary fixes
> Alan Stern
>
>
>   
Thanks

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Alan Stern]

On Fri, 1 May 2009, Michael Trimarchi wrote:

> > Nobody knows all the device dependencies.  That's why we're afraid that 
> > if you change the list, something will break.
> >
> >   
> But this happen for broken device? The list is a sufficient condition 
> for correct
> suspend phase but is not necesarry, because all the dependence are not 
> registerd.
> But just to understand: What are the typical situation, because I 
> suppose that
> we are not talking about broken device.

No, we're talking about normal, working devices.

I don't know that any situation is "typical", but here is one example.
In USB, a high-speed EHCI controller is accompanied by one or more
full/low-speed companion controllers (UHCI or OHCI).  They are
implemented as different PCI functions on the same PCI card, so they
are siblings in the device tree -- neither is a parent or a child of
the other.  During resume from hibernation, it is important that the
companion controllers be resumed _before_ the EHCI controller;
otherwise EHCI port handoffs won't work (you can't hand off a port to a
companion controller that hasn't been resumed yet).  It does work now,
because the companion controllers are registered before the EHCI
controller and so they come earlier in the dpm_list.

Does that give you an idea of the potential problems?  There may be 
other similar issues that nobody has ever run across and nobody knows 
about, simply because we have never changed the order of devices on the 
dpm_list.

Alan Stern

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Hi all,

Alan Stern wrote:
> On Sat, 25 Apr 2009, Michael Trimarchi wrote:
>
>   
>> Anyway, I revert this beacuse is wrong but is it correct to use here the
>> list_add instead the list_add_tail, it put the child node
>> after the parent stricly and don't create hole in subtree visit. Now I 
>> see only a dump
>> reason for debugging and a simple exit in flag update.
>> The important thing is that the children follow the parent,
>> and in this way is like visiting the tree.
>>     
>
> That's not necessarily true.  There may be dependencies between devices 
> that aren't expressed by the parent-child relationship.
>
> For example, device B might require device A even though A isn't a 
> parent (or ancestor) of B.  The way dpm_list works now this is okay, 
> because devices are added in order of discovery.  That is, if B depends 
> on A then A must have been discovered before B, so A will come first on 
> the list.
>
> If you change things by adding children directly after their parent 
> then you will mess this up.  For example, if B's parent was discovered 
> before A, then adding B directly after its parent would mean putting B 
> before A on the list.  Then you'd run into trouble during a system 
> suspend, because A would be suspended before B and that would prevent B 
> from working properly.
>
>   
>> Nothing change for me because 
>> is difficult
>> to isolate only a subtree, if you have a list, but I'm sure that if I 
>> find a subtree with
>> the same parent, my subtree is finish, and if someone add a new device, 
>> the system put
>> in the correct position.
>>     
>
> It is _not_ difficult to isolate a subtree using a list.  For example:
>
> 	mutex_lock(&dpm_mutex);
> 	topdev->in_subtree = 1;
> 	list_for_each_entry(dpm_list, dev, power.entry) {
> 		if (dev->parent && dev->parent->in_subtree)
> 			dev->in_subtree = 1;
> 	}
> 	list_for_each_entry(dpm_list, dev, power.entry) {
> 		if (dev->in_subtree) {
> 			dev->in_subtree = 0;
> 			dev->dont_suspend = 1;
> 		}
> 	}
> 	mutex_unlock(&dpm_mutex);
>
> This will set the dont_suspend flag for all devices in the subtree 
> starting at topdev.
>
> Alan Stern
>   
I follow the discussion around the new runtime support and I would like 
to know if I can
add to the new interface the no_suspend flag or maybe now there is a 
better solution.

Michael
>
>
>   

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Rafael J. Wysocki]

On Thursday 23 July 2009, Michael Trimarchi wrote:
> Hi all,
> 
> Alan Stern wrote:
> > On Sat, 25 Apr 2009, Michael Trimarchi wrote:
> >
> >   
> >> Anyway, I revert this beacuse is wrong but is it correct to use here the
> >> list_add instead the list_add_tail, it put the child node
> >> after the parent stricly and don't create hole in subtree visit. Now I 
> >> see only a dump
> >> reason for debugging and a simple exit in flag update.
> >> The important thing is that the children follow the parent,
> >> and in this way is like visiting the tree.
> >>     
> >
> > That's not necessarily true.  There may be dependencies between devices 
> > that aren't expressed by the parent-child relationship.
> >
> > For example, device B might require device A even though A isn't a 
> > parent (or ancestor) of B.  The way dpm_list works now this is okay, 
> > because devices are added in order of discovery.  That is, if B depends 
> > on A then A must have been discovered before B, so A will come first on 
> > the list.
> >
> > If you change things by adding children directly after their parent 
> > then you will mess this up.  For example, if B's parent was discovered 
> > before A, then adding B directly after its parent would mean putting B 
> > before A on the list.  Then you'd run into trouble during a system 
> > suspend, because A would be suspended before B and that would prevent B 
> > from working properly.
> >
> >   
> >> Nothing change for me because 
> >> is difficult
> >> to isolate only a subtree, if you have a list, but I'm sure that if I 
> >> find a subtree with
> >> the same parent, my subtree is finish, and if someone add a new device, 
> >> the system put
> >> in the correct position.
> >>     
> >
> > It is _not_ difficult to isolate a subtree using a list.  For example:
> >
> > 	mutex_lock(&dpm_mutex);
> > 	topdev->in_subtree = 1;
> > 	list_for_each_entry(dpm_list, dev, power.entry) {
> > 		if (dev->parent && dev->parent->in_subtree)
> > 			dev->in_subtree = 1;
> > 	}
> > 	list_for_each_entry(dpm_list, dev, power.entry) {
> > 		if (dev->in_subtree) {
> > 			dev->in_subtree = 0;
> > 			dev->dont_suspend = 1;
> > 		}
> > 	}
> > 	mutex_unlock(&dpm_mutex);
> >
> > This will set the dont_suspend flag for all devices in the subtree 
> > starting at topdev.
> >
> > Alan Stern
> >   
> I follow the discussion around the new runtime support and I would like 
> to know if I can
> add to the new interface the no_suspend flag or maybe now there is a 
> better solution.

Hmm.  Do I remember correctly that the no_suspend flag is for preventing the
system-wide suspend in case some devices are in use?

Rafael

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Alan Stern]

On Thu, 23 Jul 2009, Rafael J. Wysocki wrote:

> > I follow the discussion around the new runtime support and I would like 
> > to know if I can
> > add to the new interface the no_suspend flag or maybe now there is a 
> > better solution.
> 
> Hmm.  Do I remember correctly that the no_suspend flag is for preventing the
> system-wide suspend in case some devices are in use?

As I recall, the no_suspend flag was used to mark a few devices which
should not be powered down during an otherwise normal system sleep.

For example, you might want to suspend the computer while leaving a
built-in DVB tuner on because the tuner is attached to an external TV
set.

Alan Stern

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi]

Alan Stern wrote:
> On Thu, 23 Jul 2009, Rafael J. Wysocki wrote:
>
>   
>>> I follow the discussion around the new runtime support and I would like 
>>> to know if I can
>>> add to the new interface the no_suspend flag or maybe now there is a 
>>> better solution.
>>>       
>> Hmm.  Do I remember correctly that the no_suspend flag is for preventing the
>> system-wide suspend in case some devices are in use?
>>     
>
> As I recall, the no_suspend flag was used to mark a few devices which
> should not be powered down during an otherwise normal system sleep.
>
> For example, you might want to suspend the computer while leaving a
> built-in DVB tuner on because the tuner is attached to an external TV
> set.
>
> Alan Stern
>   
Is it ok for you if a prepare only the patch the add the flag and let 
the user space
and the driver to use it?

Michael
>
>   

Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Rafael J. Wysocki]

On Thursday 23 July 2009, Michael Trimarchi wrote:
> Alan Stern wrote:
> > On Thu, 23 Jul 2009, Rafael J. Wysocki wrote:
> >
> >   
> >>> I follow the discussion around the new runtime support and I would like 
> >>> to know if I can
> >>> add to the new interface the no_suspend flag or maybe now there is a 
> >>> better solution.
> >>>       
> >> Hmm.  Do I remember correctly that the no_suspend flag is for preventing the
> >> system-wide suspend in case some devices are in use?
> >>     
> >
> > As I recall, the no_suspend flag was used to mark a few devices which
> > should not be powered down during an otherwise normal system sleep.
> >
> > For example, you might want to suspend the computer while leaving a
> > built-in DVB tuner on because the tuner is attached to an external TV
> > set.
> >
> > Alan Stern
> >   
> Is it ok for you if a prepare only the patch the add the flag and let 
> the user space
> and the driver to use it?

It seems unrelated to the run-time framework, so I think we can add that flag.

Please post the patch.

Best,
Rafael