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

[RFC Add in_use attribute] 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 patch add the in_use attribute, that it can be used by the
the drivers to avoid 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>

---
diff --git a/drivers/base/core.c b/drivers/base/core.c
index e73c92d..d67043b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
 }
 
 /**
+ * device_visit_subtree - device subtree iterator.
+ * @root: root struct device.
+ * @data: data for the callback.
+ * @fn: function to be called for each device.
+ *
+ * Iterate the @parent's subtree devices, and call @fn for each,
+ * passing it @data.
+ *
+ */
+void device_visit_subtree(struct device *root, void *data,
+			  int (*fn)(struct device *dev, void *data))
+{
+	struct klist_iter i;
+	struct device *parent = root;
+	struct device *child = NULL;
+	int error;
+
+	klist_iter_init(&parent->p->klist_children, &i);
+move_down:
+	error = fn(parent, data);
+	if (error && parent != root)
+		goto move_up;
+
+	pr_debug("device: '%s': %s\n", dev_name(parent), __func__);
+
+	child = next_device(&i);
+	if (child) {
+		parent = child;
+		goto move_down;
+	}
+move_up:
+	klist_iter_exit(&i);
+	if (parent != root) {
+		klist_iter_init_node(&parent->parent->p->klist_children, &i,
+				     &parent->p->knode_parent);
+		parent = next_device(&i);
+		if (parent)
+			goto move_down;
+		klist_iter_exit(&i);
+	}
+}
+
+/**
  * device_for_each_child - device child iterator.
  * @parent: parent struct device.
  * @data: data for the callback.
@@ -1207,6 +1250,7 @@ int __init devices_init(void)
 	return -ENOMEM;
 }
 
+EXPORT_SYMBOL_GPL(device_visit_subtree);
 EXPORT_SYMBOL_GPL(device_for_each_child);
 EXPORT_SYMBOL_GPL(device_find_child);
 
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 69b4ddb..00ad150 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -64,6 +64,45 @@ void device_pm_unlock(void)
 	mutex_unlock(&dpm_list_mtx);
 }
 
+int device_set_may_inuse_enable(struct device *dev, void *data)
+{
+	pr_debug("PM: Device change in use status: %s\n", dev_name(dev));
+
+	/* if the device is suspend the subtree is in may_suspend status */
+	if (dev->power.is_inuse)
+		goto out;
+
+	dev->power.may_inuse = (unsigned int)data;
+	return 0;
+out:
+	/* cut the entire subtree */
+	return 1;
+}
+
+/**
+ *	device_set_inuse_enable - Mark the device as used by userspace
+ *	application
+ */
+int device_set_inuse_enable(struct device *dev, int enable)
+{
+	mutex_lock(&dpm_list_mtx);
+
+	/* the new status is equal the old one */
+	if (dev->power.is_inuse == enable)
+		goto out;
+
+	dev->power.is_inuse = enable;
+
+	/* Update device children to set the in use status */
+	device_visit_subtree(dev, (void *)enable,
+				device_set_may_inuse_enable);
+
+out:
+	mutex_unlock(&dpm_list_mtx);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_set_inuse_enable);
+
 /**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
@@ -78,6 +117,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_is_inuse(dev->parent)) {
+			mutex_unlock(&dpm_list_mtx);
+			/* if the parent has suspend disable, propagate it
+			 * to the new child */
+			device_set_may_inuse_enable(dev, (void *)1);
+			mutex_lock(&dpm_list_mtx);
+		}
 	} else if (transition_started) {
 		/*
 		 * We refuse to register parentless devices while a PM
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index c7cb4fc..e7d21bb 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_is_inuse(struct device *dev)
+{
+	return dev->power.is_inuse || dev->power.may_inuse;
+}
+
 #ifdef CONFIG_PM_SLEEP
 
 /*
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 596aeec..45d7f60 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 inuse_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%s\n", device_is_inuse(dev)
+		? enabled : disabled);
+}
+
+static ssize_t
+inuse_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_inuse_enable(dev, 1);
+	else if (len == sizeof disabled - 1
+			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
+		device_set_inuse_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(in_use, 0644, inuse_show, inuse_store);
 
 static struct attribute * power_attrs[] = {
 	&dev_attr_wakeup.attr,
+	&dev_attr_in_use.attr,
 	NULL,
 };
 static struct attribute_group pm_attr_group = {
diff --git a/include/linux/device.h b/include/linux/device.h
index 2918c0e..84a2bab 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -496,6 +496,9 @@ 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 int device_set_inuse_enable(struct device *dev, int enable);
+extern void device_visit_subtree(struct device *root, void *data,
+				 int (*fn)(struct device *dev, void *data));
 
 /*
  * Root device objects for grouping under /sys/devices
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 1d4e2d2..85f3fb2 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		is_inuse:1;
+	unsigned		may_inuse:1;
+
 	enum dpm_state		status;		/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	struct list_head	entry;

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

[Michael Trimarchi]

Hi,

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 patch add the in_use attribute, that it can be used by the
> the drivers to avoid 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>
>
>   
I made a mistake to send the email, because I missed all the involved
people. This patch change the previus one "[linux-pm] [RFC Disable 
suspend on a
specific device] This is a little change in linux power scheme".

Regards
Michael

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

[Rafael J. Wysocki]

On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> the drivers to avoid power down during suspend.

OK, so the idea is that in_use will be set by the user space for devices that
shouldn't be suspended.  Is this correct?

Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
may_inuse is supposed to mean that we can set in_use for this device, I'd call
it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
unset it if it is going to react to 'in_use'.

> 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>
> 
> ---
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index e73c92d..d67043b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
>  }
>  
>  /**
> + * device_visit_subtree - device subtree iterator.
> + * @root: root struct device.
> + * @data: data for the callback.
> + * @fn: function to be called for each device.
> + *
> + * Iterate the @parent's subtree devices, and call @fn for each,
> + * passing it @data.
> + *
> + */

Hmm, I'm not sure ig Greg is going to like it.

> +void device_visit_subtree(struct device *root, void *data,
> +			  int (*fn)(struct device *dev, void *data))
> +{
> +	struct klist_iter i;
> +	struct device *parent = root;

I'd call it 'current' or 'cur';

> +	struct device *child = NULL;
> +	int error;
> +
> +	klist_iter_init(&parent->p->klist_children, &i);
> +move_down:
> +	error = fn(parent, data);
> +	if (error && parent != root)

Shouldn't the iteration break on error?

> +		goto move_up;
> +
> +	pr_debug("device: '%s': %s\n", dev_name(parent), __func__);
> +
> +	child = next_device(&i);
> +	if (child) {
> +		parent = child;
> +		goto move_down;
> +	}
> +move_up:
> +	klist_iter_exit(&i);
> +	if (parent != root) {
> +		klist_iter_init_node(&parent->parent->p->klist_children, &i,
> +				     &parent->p->knode_parent);
> +		parent = next_device(&i);
> +		if (parent)
> +			goto move_down;
> +		klist_iter_exit(&i);
> +	}

Please find a way to reduce the number of gotos in this function.

Besides, I'm not sure if it's really necessary.  What's wrong with using
simply device_for_each_child() instead?

> +}
> +
> +/**
>   * device_for_each_child - device child iterator.
>   * @parent: parent struct device.
>   * @data: data for the callback.
> @@ -1207,6 +1250,7 @@ int __init devices_init(void)
>  	return -ENOMEM;
>  }
>  
> +EXPORT_SYMBOL_GPL(device_visit_subtree);
>  EXPORT_SYMBOL_GPL(device_for_each_child);
>  EXPORT_SYMBOL_GPL(device_find_child);
>  
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 69b4ddb..00ad150 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -64,6 +64,45 @@ void device_pm_unlock(void)
>  	mutex_unlock(&dpm_list_mtx);
>  }
>  
> +int device_set_may_inuse_enable(struct device *dev, void *data)

What exactly is the purpose of this function?

> +{
> +	pr_debug("PM: Device change in use status: %s\n", dev_name(dev));
> +
> +	/* if the device is suspend the subtree is in may_suspend status */
> +	if (dev->power.is_inuse)
> +		goto out;

   return 1; ?

> +
> +	dev->power.may_inuse = (unsigned int)data;

Can this conversion be avoided?

> +	return 0;
> +out:
> +	/* cut the entire subtree */
> +	return 1;
> +}
> +
> +/**
> + *	device_set_inuse_enable - Mark the device as used by userspace
> + *	application
> + */
> +int device_set_inuse_enable(struct device *dev, int enable)

We have bool for things like 'enable'.

> +{
> +	mutex_lock(&dpm_list_mtx);
> +
> +	/* the new status is equal the old one */
> +	if (dev->power.is_inuse == enable)
> +		goto out;
> +
> +	dev->power.is_inuse = enable;
> +
> +	/* Update device children to set the in use status */
> +	device_visit_subtree(dev, (void *)enable,
> +				device_set_may_inuse_enable);

Why not do:

    if (dev->power.in_use != enable) {
         dev->power.in_use = enable;
         device_visit_subtree(dev, (void *)enable, device_set_may_inuse_enable);
    }

Also, I think this 'enable' conversion isn't really necessary.  You can use two
separate helper functions for setting and unsetting and pass NULL as the second
argument.

> +
> +out:
> +	mutex_unlock(&dpm_list_mtx);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(device_set_inuse_enable);
> +
>  /**
>   *	device_pm_add - add a device to the list of active devices
>   *	@dev:	Device to be added to the list
> @@ -78,6 +117,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_is_inuse(dev->parent)) {
> +			mutex_unlock(&dpm_list_mtx);
> +			/* if the parent has suspend disable, propagate it
> +			 * to the new child */
> +			device_set_may_inuse_enable(dev, (void *)1);

The conversion is just terrible.  I'd very much prefer it to be
device_set_in_use_possible_enable(dev, true).

> +			mutex_lock(&dpm_list_mtx);
> +		}
>  	} else if (transition_started) {
>  		/*
>  		 * We refuse to register parentless devices while a PM
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index c7cb4fc..e7d21bb 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_is_inuse(struct device *dev)
> +{
> +	return dev->power.is_inuse || dev->power.may_inuse;
> +}

OK, so what's the meaning of is_inuse and may_inuse?

>  #ifdef CONFIG_PM_SLEEP
>  
>  /*
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 596aeec..45d7f60 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 inuse_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +	return sprintf(buf, "%s\n", device_is_inuse(dev)
> +		? enabled : disabled);
> +}
> +
> +static ssize_t
> +inuse_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_inuse_enable(dev, 1);
> +	else if (len == sizeof disabled - 1
> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
> +		device_set_inuse_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(in_use, 0644, inuse_show, inuse_store);
>  
>  static struct attribute * power_attrs[] = {
>  	&dev_attr_wakeup.attr,
> +	&dev_attr_in_use.attr,
>  	NULL,
>  };
>  static struct attribute_group pm_attr_group = {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 2918c0e..84a2bab 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -496,6 +496,9 @@ 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 int device_set_inuse_enable(struct device *dev, int enable);
> +extern void device_visit_subtree(struct device *root, void *data,
> +				 int (*fn)(struct device *dev, void *data));
>  
>  /*
>   * Root device objects for grouping under /sys/devices
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 1d4e2d2..85f3fb2 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		is_inuse:1;
> +	unsigned		may_inuse:1;
> +
>  	enum dpm_state		status;		/* Owned by the PM core */
>  #ifdef	CONFIG_PM_SLEEP
>  	struct list_head	entry;

Thanks,
Rafael

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

[Alan Stern]

On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:

> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > the drivers to avoid power down during suspend.
> 
> OK, so the idea is that in_use will be set by the user space for devices that
> shouldn't be suspended.  Is this correct?
> 
> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
> may_inuse is supposed to mean that we can set in_use for this device, I'd call
> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
> unset it if it is going to react to 'in_use'.

I don't see why two separate flags are needed.  Why can't there be just 
one?

Also, I don't see why the in_use flag has to propagate down to all the 
descendant devices when it is set.  Why not let userspace be 
responsible for that?

Finally, I don't like either name very much.  This flag is supposed to
indicate that the device is being used in a mode that can run by itself
even when the rest of the system is suspended.  Calling it "in_use" 
doesn't express the crucial fact that the device is self-sufficient.

Alan Stern

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

[Greg KH]

I seem to not have seen the original post about this, thanks Alan for
adding me to the cc:

On Mon, Apr 20, 2009 at 06:11:18PM -0400, Alan Stern wrote:
> On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:
> 
> > On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > > the drivers to avoid power down during suspend.

I'm confused, why would a driver not know if it was in use or not?
Actually, how would it not know already by virtue of what is happening
within it (io in flight, buttons being pushed, dma streaming, etc.)?

> > OK, so the idea is that in_use will be set by the user space for devices that
> > shouldn't be suspended.  Is this correct?

So userspace knows better than the kernel as to if a specific driver is
being used at the moment?  Why is this so?

confused,

greg k-h

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

[Rafael J. Wysocki]

On Tuesday 21 April 2009, Greg KH wrote:
> 
> I seem to not have seen the original post about this, thanks Alan for
> adding me to the cc:
> 
> On Mon, Apr 20, 2009 at 06:11:18PM -0400, Alan Stern wrote:
> > On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:
> > 
> > > On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > > > the drivers to avoid power down during suspend.
> 
> I'm confused, why would a driver not know if it was in use or not?
> Actually, how would it not know already by virtue of what is happening
> within it (io in flight, buttons being pushed, dma streaming, etc.)?
> 
> > > OK, so the idea is that in_use will be set by the user space for devices that
> > > shouldn't be suspended.  Is this correct?
> 
> So userspace knows better than the kernel as to if a specific driver is
> being used at the moment?  Why is this so?

The name of the flag is not the best one. :-)

The flag is supposed to mean "don't suspend this device during system-wide
suspend, because it's being used for something you may be unaware of".
AFAICS.

Best,
Rafael

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

[Greg KH]

On Tue, Apr 21, 2009 at 08:33:01PM +0200, Rafael J. Wysocki wrote:
> On Tuesday 21 April 2009, Greg KH wrote:
> > 
> > I seem to not have seen the original post about this, thanks Alan for
> > adding me to the cc:
> > 
> > On Mon, Apr 20, 2009 at 06:11:18PM -0400, Alan Stern wrote:
> > > On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:
> > > 
> > > > On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > > > > the drivers to avoid power down during suspend.
> > 
> > I'm confused, why would a driver not know if it was in use or not?
> > Actually, how would it not know already by virtue of what is happening
> > within it (io in flight, buttons being pushed, dma streaming, etc.)?
> > 
> > > > OK, so the idea is that in_use will be set by the user space for devices that
> > > > shouldn't be suspended.  Is this correct?
> > 
> > So userspace knows better than the kernel as to if a specific driver is
> > being used at the moment?  Why is this so?
> 
> The name of the flag is not the best one. :-)
> 
> The flag is supposed to mean "don't suspend this device during system-wide
> suspend, because it's being used for something you may be unaware of".
> AFAICS.

Then it needs to be changed, as it is not obvious at all what is going
on here.  Your "no_suspend" suggestion would be good.

thanks,

greg k-h

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

[Michael Trimarchi]

Alan Stern wrote:
> On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:
>
>   
>> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>>> the drivers to avoid power down during suspend.
>>>       
>> OK, so the idea is that in_use will be set by the user space for devices that
>> shouldn't be suspended.  Is this correct?
>>
>> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
>> may_inuse is supposed to mean that we can set in_use for this device, I'd call
>> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
>> unset it if it is going to react to 'in_use'.
>>     
>
> I don't see why two separate flags are needed.  Why can't there be just 
> one?
>
> Also, I don't see why the in_use flag has to propagate down to all the 
> descendant devices when it is set.  Why not let userspace be 
> responsible for that?
>   
Yes it is possible to leave the possibility to the userspace. With this 
patch
the userspace can deselect/select a tree in one write and eventually 
reanable subpart. This
was the old design choice to use kernel to track dependences.
> Finally, I don't like either name very much.  This flag is supposed to
> indicate that the device is being used in a mode that can run by itself
> even when the rest of the system is suspended.  Calling it "in_use" 
> doesn't express the crucial fact that the device is self-sufficient.
>   
Ok I will change
> Alan Stern
>
>
>   
Michael

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

[Rafael J. Wysocki]

On Tuesday 21 April 2009, Alan Stern wrote:
> On Mon, 20 Apr 2009, Rafael J. Wysocki wrote:
> 
> > On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > > the drivers to avoid power down during suspend.
> > 
> > OK, so the idea is that in_use will be set by the user space for devices that
> > shouldn't be suspended.  Is this correct?
> > 
> > Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
> > may_inuse is supposed to mean that we can set in_use for this device, I'd call
> > it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
> > unset it if it is going to react to 'in_use'.
> 
> I don't see why two separate flags are needed.  Why can't there be just 
> one?

There could be one, but since the core would handle the sysfs attribute
associated with it, the second flag would be useful to indicate to the core
whether or not the attribute should be available at all.

> Also, I don't see why the in_use flag has to propagate down to all the 
> descendant devices when it is set.  Why not let userspace be 
> responsible for that?

Agreed.

Michael, would it be a problem if the user space were responsible for
setting the flag for all devices that shouldn't be suspended?

> Finally, I don't like either name very much.  This flag is supposed to
> indicate that the device is being used in a mode that can run by itself
> even when the rest of the system is suspended.  Calling it "in_use" 
> doesn't express the crucial fact that the device is self-sufficient.

You're rignt, it would be better to call it something like 'no_suspend'.

Thanks,
Rafael

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

[Greg KH]

On Mon, Apr 20, 2009 at 11:54:04PM +0200, Rafael J. Wysocki wrote:
> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> > the drivers to avoid power down during suspend.
> 
> OK, so the idea is that in_use will be set by the user space for devices that
> shouldn't be suspended.  Is this correct?

If so, why?  Why would you suspend anything then?  Why not just have
userspace suspend the devices it wants to suspend and leave the ones it
thinks is "in_use" alone?

> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
> may_inuse is supposed to mean that we can set in_use for this device, I'd call
> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
> unset it if it is going to react to 'in_use'.
> 
> > 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>
> > 
> > ---
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index e73c92d..d67043b 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
> >  }
> >  
> >  /**
> > + * device_visit_subtree - device subtree iterator.
> > + * @root: root struct device.
> > + * @data: data for the callback.
> > + * @fn: function to be called for each device.
> > + *
> > + * Iterate the @parent's subtree devices, and call @fn for each,
> > + * passing it @data.
> > + *
> > + */
> 
> Hmm, I'm not sure ig Greg is going to like it.

I have the same big question you do:

> Besides, I'm not sure if it's really necessary.  What's wrong with using
> simply device_for_each_child() instead?

Exactly, what are you trying to do that differs from
device_for_each_child()?

> > @@ -1207,6 +1250,7 @@ int __init devices_init(void)
> >  	return -ENOMEM;
> >  }
> >  
> > +EXPORT_SYMBOL_GPL(device_visit_subtree);

I see you didn't run your patch through scripts/checkpatch.pl :)

Please do so in the future.

thanks,

greg k-h

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

[Michael Trimarchi]

Hi,

Greg KH wrote:
> On Mon, Apr 20, 2009 at 11:54:04PM +0200, Rafael J. Wysocki wrote:
>   
>> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>>> the drivers to avoid power down during suspend.
>>>       
>> OK, so the idea is that in_use will be set by the user space for devices that
>> shouldn't be suspended.  Is this correct?
>>     
>
> If so, why?  Why would you suspend anything then?  Why not just have
> userspace suspend the devices it wants to suspend and leave the ones it
> thinks is "in_use" alone?
>
>   
Because it the previus thread the idea is that the driver should use 
this flag

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


>> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
>> may_inuse is supposed to mean that we can set in_use for this device, I'd call
>> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
>> unset it if it is going to react to 'in_use'.
>>
>>     
>>> 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>
>>>
>>> ---
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index e73c92d..d67043b 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
>>>  }
>>>  
>>>  /**
>>> + * device_visit_subtree - device subtree iterator.
>>> + * @root: root struct device.
>>> + * @data: data for the callback.
>>> + * @fn: function to be called for each device.
>>> + *
>>> + * Iterate the @parent's subtree devices, and call @fn for each,
>>> + * passing it @data.
>>> + *
>>> + */
>>>       
>> Hmm, I'm not sure ig Greg is going to like it.
>>     
>
> I have the same big question you do:
>
>   
>> Besides, I'm not sure if it's really necessary.  What's wrong with using
>> simply device_for_each_child() instead?
>>     
>
> Exactly, what are you trying to do that differs from
> device_for_each_child()?
>   
Is device for each child use to visist the first level of the tree?
>   
>>> @@ -1207,6 +1250,7 @@ int __init devices_init(void)
>>>  	return -ENOMEM;
>>>  }
>>>  
>>> +EXPORT_SYMBOL_GPL(device_visit_subtree);
>>>       
>
> I see you didn't run your patch through scripts/checkpatch.pl :)
>   
I run the checkpatch but I find that the exported symbol are there so I 
add the new one.
> Please do so in the future.
>
> thanks,
>
> greg k-h
>
>   
Michael

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

[Greg KH]

On Tue, Apr 21, 2009 at 07:08:44AM +0200, Michael Trimarchi wrote:
> Hi,
> 
> Greg KH wrote:
> > On Mon, Apr 20, 2009 at 11:54:04PM +0200, Rafael J. Wysocki wrote:
> >   
> >> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> >>> the drivers to avoid power down during suspend.
> >>>       
> >> OK, so the idea is that in_use will be set by the user space for devices that
> >> shouldn't be suspended.  Is this correct?
> >>     
> >
> > If so, why?  Why would you suspend anything then?  Why not just have
> > userspace suspend the devices it wants to suspend and leave the ones it
> > thinks is "in_use" alone?
> >
> >   
> Because it the previus thread the idea is that the driver should use 
> this flag
> 
> "[linux-pm] [RFC Disable suspend on a
> specific device] This is a little change in linux power scheme".

I wasn't involved in that thread, and am not on linux-pm, care to
summarize why this change is now recommended?

> > Exactly, what are you trying to do that differs from
> > device_for_each_child()?
> >   
> Is device for each child use to visist the first level of the tree?

Have you tried it?

thanks,

greg k-h

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

[Michael Trimarchi]

Hi

Greg KH wrote:
> On Tue, Apr 21, 2009 at 07:08:44AM +0200, Michael Trimarchi wrote:
>   
>> Hi,
>>
>> Greg KH wrote:
>>     
>>> On Mon, Apr 20, 2009 at 11:54:04PM +0200, Rafael J. Wysocki wrote:
>>>   
>>>       
>>>> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>>>>> the drivers to avoid power down during suspend.
>>>>>       
>>>>>           
>>>> OK, so the idea is that in_use will be set by the user space for devices that
>>>> shouldn't be suspended.  Is this correct?
>>>>     
>>>>         
>>> If so, why?  Why would you suspend anything then?  Why not just have
>>> userspace suspend the devices it wants to suspend and leave the ones it
>>> thinks is "in_use" alone?
>>>
>>>   
>>>       
>> Because it the previus thread the idea is that the driver should use 
>> this flag
>>
>> "[linux-pm] [RFC Disable suspend on a
>> specific device] This is a little change in linux power scheme".
>>     
>
> I wasn't involved in that thread, and am not on linux-pm, care to
> summarize why this change is now recommended?
>   
Sorry, I try to summurize:

There are embededd system that has some devices used by the cpu and other
unit. So in linux suspend the cpu, call the suspend to all the devices
and can power-off devices that are used by other subsystem. The first
patch that I send add an attribute and avoid suspend on devices that are flagged
and use the pm subsystem do skip suspend on that device. 
This second patch let to device this possibility.


>   
>>> Exactly, what are you trying to do that differs from
>>> device_for_each_child()?
>>>   
>>>       
>> Is device for each child use to visist the first level of the tree?
>>     
>
> Have you tried it?
>   
No, I take a look at the code.

int device_for_each_child(struct device *parent, void *data,
                          int (*fn)(struct device *dev, void *data))
{
        struct klist_iter i;
        struct device *child;
        int error = 0;

        klist_iter_init(&parent->p->klist_children, &i);

I was thinking that the klist_children is the fist_level one children, 
so each time
a device is registerd it add the link to the parent.

        while ((child = next_device(&i)) && !error)
                error = fn(child, data);
        klist_iter_exit(&i);

So if I want to walk the tree I must visit the children of the any 
child. Sorry,
maybe I miss somenthing.
> thanks,
>
> greg k-h
>
>   
Michael

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

[Greg KH]

On Tue, Apr 21, 2009 at 08:43:28AM +0200, Michael Trimarchi wrote:
> >>> Exactly, what are you trying to do that differs from
> >>> device_for_each_child()?
> >>>   
> >>>       
> >> Is device for each child use to visist the first level of the tree?
> >>     
> >
> > Have you tried it?
> >   
> No, I take a look at the code.
> 
> int device_for_each_child(struct device *parent, void *data,
>                           int (*fn)(struct device *dev, void *data))
> {
>         struct klist_iter i;
>         struct device *child;
>         int error = 0;
> 
>         klist_iter_init(&parent->p->klist_children, &i);
> 
> I was thinking that the klist_children is the fist_level one children, 
> so each time
> a device is registerd it add the link to the parent.

Yes it does.

But you have to start at some device, right?  So you don't need to
iterate over it, just go from there on down if needed.

So I don't see why this helper function is needed at all yet, shouldn't
we be doing the check within the normal suspend device walk of the tree?

thanks,

greg k-h

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

[Michael Trimarchi]

Greg KH wrote:
> On Tue, Apr 21, 2009 at 08:43:28AM +0200, Michael Trimarchi wrote:
>   
>>>>> Exactly, what are you trying to do that differs from
>>>>> device_for_each_child()?
>>>>>   
>>>>>       
>>>>>           
>>>> Is device for each child use to visist the first level of the tree?
>>>>     
>>>>         
>>> Have you tried it?
>>>   
>>>       
>> No, I take a look at the code.
>>
>> int device_for_each_child(struct device *parent, void *data,
>>                           int (*fn)(struct device *dev, void *data))
>> {
>>         struct klist_iter i;
>>         struct device *child;
>>         int error = 0;
>>
>>         klist_iter_init(&parent->p->klist_children, &i);
>>
>> I was thinking that the klist_children is the fist_level one children, 
>> so each time
>> a device is registerd it add the link to the parent.
>>     
>
> Yes it does.
>
> But you have to start at some device, right?  So you don't need to
> iterate over it, just go from there on down if needed.
>   
I start for a device, go down until I find a leaf or find that the 
subtree is marked.
Mark the leaf and go up and take the next node like the walk_tg_tree. 
The difference is
that I skip subtree if they are mark in_use.
> So I don't see why this helper function is needed at all yet, shouldn't
> we be doing the check within the normal suspend device walk of the tree?
>   
Sorry but here I need some help here. Where is the walk of the device 
tree during suspend?
> thanks,
>
> greg k-h
>
>   
Michael

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

[Rafael J. Wysocki]

On Thursday 23 April 2009, Michael Trimarchi wrote:
> Greg KH wrote:
> > On Tue, Apr 21, 2009 at 08:43:28AM +0200, Michael Trimarchi wrote:
> >   
> >>>>> Exactly, what are you trying to do that differs from
> >>>>> device_for_each_child()?
> >>>>>   
> >>>>>       
> >>>>>           
> >>>> Is device for each child use to visist the first level of the tree?
> >>>>     
> >>>>         
> >>> Have you tried it?
> >>>   
> >>>       
> >> No, I take a look at the code.
> >>
> >> int device_for_each_child(struct device *parent, void *data,
> >>                           int (*fn)(struct device *dev, void *data))
> >> {
> >>         struct klist_iter i;
> >>         struct device *child;
> >>         int error = 0;
> >>
> >>         klist_iter_init(&parent->p->klist_children, &i);
> >>
> >> I was thinking that the klist_children is the fist_level one children, 
> >> so each time
> >> a device is registerd it add the link to the parent.
> >>     
> >
> > Yes it does.
> >
> > But you have to start at some device, right?  So you don't need to
> > iterate over it, just go from there on down if needed.
> >   
> I start for a device, go down until I find a leaf or find that the 
> subtree is marked.
> Mark the leaf and go up and take the next node like the walk_tg_tree. 
> The difference is
> that I skip subtree if they are mark in_use.
> > So I don't see why this helper function is needed at all yet, shouldn't
> > we be doing the check within the normal suspend device walk of the tree?
> >   
> Sorry but here I need some help here. Where is the walk of the device 
> tree during suspend?

In drivers/base/power/main.c there are functions like dpm_suspend(), for one
example, that walk the device tree, but they do it in a simplified way, using a
list prepared specifically for this purpose (so the walk is in fact linear).

Thanks,
Rafael

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

[Michael Trimarchi]

Hi,

Rafael J. Wysocki wrote:
> On Thursday 23 April 2009, Michael Trimarchi wrote:
>   
>> Greg KH wrote:
>>     
>>> On Tue, Apr 21, 2009 at 08:43:28AM +0200, Michael Trimarchi wrote:
>>>   
>>>       
>>>>>>> Exactly, what are you trying to do that differs from
>>>>>>> device_for_each_child()?
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Is device for each child use to visist the first level of the tree?
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Have you tried it?
>>>>>   
>>>>>       
>>>>>           
>>>> No, I take a look at the code.
>>>>
>>>> int device_for_each_child(struct device *parent, void *data,
>>>>                           int (*fn)(struct device *dev, void *data))
>>>> {
>>>>         struct klist_iter i;
>>>>         struct device *child;
>>>>         int error = 0;
>>>>
>>>>         klist_iter_init(&parent->p->klist_children, &i);
>>>>
>>>> I was thinking that the klist_children is the fist_level one children, 
>>>> so each time
>>>> a device is registerd it add the link to the parent.
>>>>     
>>>>         
>>> Yes it does.
>>>
>>> But you have to start at some device, right?  So you don't need to
>>> iterate over it, just go from there on down if needed.
>>>   
>>>       
>> I start for a device, go down until I find a leaf or find that the 
>> subtree is marked.
>> Mark the leaf and go up and take the next node like the walk_tg_tree. 
>> The difference is
>> that I skip subtree if they are mark in_use.
>>     
>>> So I don't see why this helper function is needed at all yet, shouldn't
>>> we be doing the check within the normal suspend device walk of the tree?
>>>   
>>>       
>> Sorry but here I need some help here. Where is the walk of the device 
>> tree during suspend?
>>     
>
>   

> In drivers/base/power/main.c there are functions like dpm_suspend(), for one
> example, that walk the device tree, but they do it in a simplified way, using a
> list prepared specifically for this purpose (so the walk is in fact linear).
>   
I don't want add a new list and
If I use the dpm_list a must go back to the root for each element using 
the parent and check
the flag because the list maybe don't mantein the parent relation.

example:
s(A) is son of A ss(A) etc

A-B-s(B)-s(B)-s(A)-ss(B)-ss(A)

I don't know if B the second element has child of B I must look it's 
parent and so on.
So the complexty increase.

The suspend order is correct but the computation time is worse. Maybe I 
can move
the code outside the base/core.c and put it in the power/main.c but I 
don't have
the next_device function and other to move up and down. I put there that 
function
because I have all the necessary code.

Michael

> Thanks,
> Rafael
>
>   

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

[Rafael J. Wysocki]

On Thursday 23 April 2009, Michael Trimarchi wrote:
> Hi,
> 
> Rafael J. Wysocki wrote:
> > On Thursday 23 April 2009, Michael Trimarchi wrote:
> >   
> >> Greg KH wrote:
> >>     
> >>> On Tue, Apr 21, 2009 at 08:43:28AM +0200, Michael Trimarchi wrote:
> >>>   
> >>>       
> >>>>>>> Exactly, what are you trying to do that differs from
> >>>>>>> device_for_each_child()?
> >>>>>>>   
> >>>>>>>       
> >>>>>>>           
> >>>>>>>               
> >>>>>> Is device for each child use to visist the first level of the tree?
> >>>>>>     
> >>>>>>         
> >>>>>>             
> >>>>> Have you tried it?
> >>>>>   
> >>>>>       
> >>>>>           
> >>>> No, I take a look at the code.
> >>>>
> >>>> int device_for_each_child(struct device *parent, void *data,
> >>>>                           int (*fn)(struct device *dev, void *data))
> >>>> {
> >>>>         struct klist_iter i;
> >>>>         struct device *child;
> >>>>         int error = 0;
> >>>>
> >>>>         klist_iter_init(&parent->p->klist_children, &i);
> >>>>
> >>>> I was thinking that the klist_children is the fist_level one children, 
> >>>> so each time
> >>>> a device is registerd it add the link to the parent.
> >>>>     
> >>>>         
> >>> Yes it does.
> >>>
> >>> But you have to start at some device, right?  So you don't need to
> >>> iterate over it, just go from there on down if needed.
> >>>   
> >>>       
> >> I start for a device, go down until I find a leaf or find that the 
> >> subtree is marked.
> >> Mark the leaf and go up and take the next node like the walk_tg_tree. 
> >> The difference is
> >> that I skip subtree if they are mark in_use.
> >>     
> >>> So I don't see why this helper function is needed at all yet, shouldn't
> >>> we be doing the check within the normal suspend device walk of the tree?
> >>>   
> >>>       
> >> Sorry but here I need some help here. Where is the walk of the device 
> >> tree during suspend?
> >>     
> >
> >   
> 
> > In drivers/base/power/main.c there are functions like dpm_suspend(), for one
> > example, that walk the device tree, but they do it in a simplified way, using a
> > list prepared specifically for this purpose (so the walk is in fact linear).
> >   
> I don't want add a new list and
> If I use the dpm_list a must go back to the root for each element using 
> the parent and check
> the flag because the list maybe don't mantein the parent relation.

It does maintain the parent relation because of the way it is created.  Devices
are added to the list in the order of discovery and obviously the parents are
discovered before their children (there are a few exceptions involving a
reordering of the list, but I don't think they are relevant for your case).

> example:
> s(A) is son of A ss(A) etc
> 
> A-B-s(B)-s(B)-s(A)-ss(B)-ss(A)
> 
> I don't know if B the second element has child of B I must look it's 
> parent and so on.
> So the complexty increase.
> 
> The suspend order is correct but the computation time is worse. Maybe I 
> can move
> the code outside the base/core.c and put it in the power/main.c but I 
> don't have
> the next_device function and other to move up and down. I put there that 
> function
> because I have all the necessary code.

Please have a deeper look into the existing code before you start adding new
things.  It's a good chance the code already does some of the things you
need, although you may want to modify it slightly here and there.

Thanks,
Rafael

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

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>> the drivers to avoid power down during suspend.
>>     
>
> OK, so the idea is that in_use will be set by the user space for devices that
> shouldn't be suspended.  Is this correct?
>
> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
> may_inuse is supposed to mean that we can set in_use for this device, I'd call
> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
> unset it if it is going to react to 'in_use'.
>   
is_inuse is set for the device. The may_inuse is automatically setting 
for the child
device. This is done for automatically propagate the dependency
>   
>> 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>
>>
>> ---
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index e73c92d..d67043b 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
>>  }
>>  
>>  /**
>> + * device_visit_subtree - device subtree iterator.
>> + * @root: root struct device.
>> + * @data: data for the callback.
>> + * @fn: function to be called for each device.
>> + *
>> + * Iterate the @parent's subtree devices, and call @fn for each,
>> + * passing it @data.
>> + *
>> + */
>>     
>
> Hmm, I'm not sure ig Greg is going to like it.
>
>   
This function walk the tree of devices following the dependences in 
iterative mode.
>> +void device_visit_subtree(struct device *root, void *data,
>> +			  int (*fn)(struct device *dev, void *data))
>> +{
>> +	struct klist_iter i;
>> +	struct device *parent = root;
>>     
>
> I'd call it 'current' or 'cur';
>
>   
ok
>> +	struct device *child = NULL;
>> +	int error;
>> +
>> +	klist_iter_init(&parent->p->klist_children, &i);
>> +move_down:
>> +	error = fn(parent, data);
>> +	if (error && parent != root)
>>     
>
> Shouldn't the iteration break on error?
>
>   
The iteration don't break on error because, the return just said that the
subtree is just enable
>> +		goto move_up;
>> +
>> +	pr_debug("device: '%s': %s\n", dev_name(parent), __func__);
>> +
>> +	child = next_device(&i);
>> +	if (child) {
>> +		parent = child;
>> +		goto move_down;
>> +	}
>> +move_up:
>> +	klist_iter_exit(&i);
>> +	if (parent != root) {
>> +		klist_iter_init_node(&parent->parent->p->klist_children, &i,
>> +				     &parent->p->knode_parent);
>> +		parent = next_device(&i);
>> +		if (parent)
>> +			goto move_down;
>> +		klist_iter_exit(&i);
>> +	}
>>     
>
> Please find a way to reduce the number of gotos in this function.
>
> Besides, I'm not sure if it's really necessary.  What's wrong with using
> simply device_for_each_child() instead?
>   
Just to have an iterative function
>   
>> +}
>> +
>> +/**
>>   * device_for_each_child - device child iterator.
>>   * @parent: parent struct device.
>>   * @data: data for the callback.
>> @@ -1207,6 +1250,7 @@ int __init devices_init(void)
>>  	return -ENOMEM;
>>  }
>>  
>> +EXPORT_SYMBOL_GPL(device_visit_subtree);
>>  EXPORT_SYMBOL_GPL(device_for_each_child);
>>  EXPORT_SYMBOL_GPL(device_find_child);
>>  
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 69b4ddb..00ad150 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -64,6 +64,45 @@ void device_pm_unlock(void)
>>  	mutex_unlock(&dpm_list_mtx);
>>  }
>>  
>> +int device_set_may_inuse_enable(struct device *dev, void *data)
>>     
>
> What exactly is the purpose of this function?
>
>   
This function said that the parent is used by a driver
>> +{
>> +	pr_debug("PM: Device change in use status: %s\n", dev_name(dev));
>> +
>> +	/* if the device is suspend the subtree is in may_suspend status */
>> +	if (dev->power.is_inuse)
>> +		goto out;
>>     
>
>    return 1; ?
>
>   
>> +
>> +	dev->power.may_inuse = (unsigned int)data;
>>     
>
> Can this conversion be avoided?
>
>   
>> +	return 0;
>> +out:
>> +	/* cut the entire subtree */
>> +	return 1;
>> +}
>> +
>> +/**
>> + *	device_set_inuse_enable - Mark the device as used by userspace
>> + *	application
>> + */
>> +int device_set_inuse_enable(struct device *dev, int enable)
>>     
>
> We have bool for things like 'enable'.
>
>   
ok
>> +{
>> +	mutex_lock(&dpm_list_mtx);
>> +
>> +	/* the new status is equal the old one */
>> +	if (dev->power.is_inuse == enable)
>> +		goto out;
>> +
>> +	dev->power.is_inuse = enable;
>> +
>> +	/* Update device children to set the in use status */
>> +	device_visit_subtree(dev, (void *)enable,
>> +				device_set_may_inuse_enable);
>>     
>
> Why not do:
>
>     if (dev->power.in_use != enable) {
>          dev->power.in_use = enable;
>          device_visit_subtree(dev, (void *)enable, device_set_may_inuse_enable);
>     }
>
> Also, I think this 'enable' conversion isn't really necessary.  You can use two
> separate helper functions for setting and unsetting and pass NULL as the second
> argument.
>
>   
ok
>> +
>> +out:
>> +	mutex_unlock(&dpm_list_mtx);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(device_set_inuse_enable);
>> +
>>  /**
>>   *	device_pm_add - add a device to the list of active devices
>>   *	@dev:	Device to be added to the list
>> @@ -78,6 +117,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_is_inuse(dev->parent)) {
>> +			mutex_unlock(&dpm_list_mtx);
>> +			/* if the parent has suspend disable, propagate it
>> +			 * to the new child */
>> +			device_set_may_inuse_enable(dev, (void *)1);
>>     
>
> The conversion is just terrible.  I'd very much prefer it to be
> device_set_in_use_possible_enable(dev, true).
>
>   
ok
>> +			mutex_lock(&dpm_list_mtx);
>> +		}
>>  	} else if (transition_started) {
>>  		/*
>>  		 * We refuse to register parentless devices while a PM
>> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
>> index c7cb4fc..e7d21bb 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_is_inuse(struct device *dev)
>> +{
>> +	return dev->power.is_inuse || dev->power.may_inuse;
>> +}
>>     
>
> OK, so what's the meaning of is_inuse and may_inuse?
>
>   
Maybe the idea is if the parent is in_use the child are may_inuse so 
they are potentialy in
use. The user can disable a tree and after reanable a child.
>>  #ifdef CONFIG_PM_SLEEP
>>  
>>  /*
>> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
>> index 596aeec..45d7f60 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 inuse_show(struct device *dev, struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", device_is_inuse(dev)
>> +		? enabled : disabled);
>> +}
>> +
>> +static ssize_t
>> +inuse_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_inuse_enable(dev, 1);
>> +	else if (len == sizeof disabled - 1
>> +			&& strncmp(buf, disabled, sizeof disabled - 1) == 0)
>> +		device_set_inuse_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(in_use, 0644, inuse_show, inuse_store);
>>  
>>  static struct attribute * power_attrs[] = {
>>  	&dev_attr_wakeup.attr,
>> +	&dev_attr_in_use.attr,
>>  	NULL,
>>  };
>>  static struct attribute_group pm_attr_group = {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 2918c0e..84a2bab 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -496,6 +496,9 @@ 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 int device_set_inuse_enable(struct device *dev, int enable);
>> +extern void device_visit_subtree(struct device *root, void *data,
>> +				 int (*fn)(struct device *dev, void *data));
>>  
>>  /*
>>   * Root device objects for grouping under /sys/devices
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index 1d4e2d2..85f3fb2 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		is_inuse:1;
>> +	unsigned		may_inuse:1;
>> +
>>  	enum dpm_state		status;		/* Owned by the PM core */
>>  #ifdef	CONFIG_PM_SLEEP
>>  	struct list_head	entry;
>>     
>
> Thanks,
> Rafael
>
>   
Michael

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

[Rafael J. Wysocki]

On Tuesday 21 April 2009, Michael Trimarchi wrote:
> Rafael J. Wysocki wrote:
> > On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
> >> the drivers to avoid power down during suspend.
> >>     
> >
> > OK, so the idea is that in_use will be set by the user space for devices that
> > shouldn't be suspended.  Is this correct?
> >
> > Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
> > may_inuse is supposed to mean that we can set in_use for this device, I'd call
> > it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
> > unset it if it is going to react to 'in_use'.
> >   
> is_inuse is set for the device. The may_inuse is automatically setting 
> for the child
> device. This is done for automatically propagate the dependency

I see.  I'd call it differently, then.

Besides, is it really always the case that setting the flag for one device
implies that the entire subtree below it should have the flag set?  IOW,
there may be some devices in the subtree that we may want to suspend anyway,
I think.

> >> 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>
> >>
> >> ---
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index e73c92d..d67043b 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
> >>  }
> >>  
> >>  /**
> >> + * device_visit_subtree - device subtree iterator.
> >> + * @root: root struct device.
> >> + * @data: data for the callback.
> >> + * @fn: function to be called for each device.
> >> + *
> >> + * Iterate the @parent's subtree devices, and call @fn for each,
> >> + * passing it @data.
> >> + *
> >> + */
> >>     
> >
> > Hmm, I'm not sure ig Greg is going to like it.
> >
> >   
> This function walk the tree of devices following the dependences in 
> iterative mode.

Yes, it does, but the implementation is not the cleanest one IMO.

> >> +void device_visit_subtree(struct device *root, void *data,
> >> +			  int (*fn)(struct device *dev, void *data))
> >> +{
> >> +	struct klist_iter i;
> >> +	struct device *parent = root;
> >>     
> >
> > I'd call it 'current' or 'cur';
> >
> >   
> ok
> >> +	struct device *child = NULL;
> >> +	int error;
> >> +
> >> +	klist_iter_init(&parent->p->klist_children, &i);
> >> +move_down:
> >> +	error = fn(parent, data);
> >> +	if (error && parent != root)
> >>     
> >
> > Shouldn't the iteration break on error?
> >
> >   
> The iteration don't break on error because, the return just said that the
> subtree is just enable

You're assuming that _your_ function will be the only one called via this one,
but in that case why do you introduce a generic low level helper?

> >> +		goto move_up;
> >> +
> >> +	pr_debug("device: '%s': %s\n", dev_name(parent), __func__);
> >> +
> >> +	child = next_device(&i);
> >> +	if (child) {
> >> +		parent = child;
> >> +		goto move_down;
> >> +	}
> >> +move_up:
> >> +	klist_iter_exit(&i);
> >> +	if (parent != root) {
> >> +		klist_iter_init_node(&parent->parent->p->klist_children, &i,
> >> +				     &parent->p->knode_parent);
> >> +		parent = next_device(&i);
> >> +		if (parent)
> >> +			goto move_down;
> >> +		klist_iter_exit(&i);
> >> +	}
> >>     
> >
> > Please find a way to reduce the number of gotos in this function.
> >
> > Besides, I'm not sure if it's really necessary.  What's wrong with using
> > simply device_for_each_child() instead?
> >   
> Just to have an iterative function

Care to elaborate?

> >   
> >> +}
> >> +
> >> +/**
> >>   * device_for_each_child - device child iterator.
> >>   * @parent: parent struct device.
> >>   * @data: data for the callback.
> >> @@ -1207,6 +1250,7 @@ int __init devices_init(void)
> >>  	return -ENOMEM;
> >>  }
> >>  
> >> +EXPORT_SYMBOL_GPL(device_visit_subtree);
> >>  EXPORT_SYMBOL_GPL(device_for_each_child);
> >>  EXPORT_SYMBOL_GPL(device_find_child);
> >>  
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index 69b4ddb..00ad150 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -64,6 +64,45 @@ void device_pm_unlock(void)
> >>  	mutex_unlock(&dpm_list_mtx);
> >>  }
> >>  
> >> +int device_set_may_inuse_enable(struct device *dev, void *data)
> >>     
> >
> > What exactly is the purpose of this function?
> >
> >   
> This function said that the parent is used by a driver
> >> +{
> >> +	pr_debug("PM: Device change in use status: %s\n", dev_name(dev));
> >> +
> >> +	/* if the device is suspend the subtree is in may_suspend status */
> >> +	if (dev->power.is_inuse)
> >> +		goto out;
> >>     
> >
> >    return 1; ?
> >
> >   
> >> +
> >> +	dev->power.may_inuse = (unsigned int)data;
> >>     
> >
> > Can this conversion be avoided?
> >
> >   
> >> +	return 0;
> >> +out:
> >> +	/* cut the entire subtree */
> >> +	return 1;
> >> +}
> >> +
> >> +/**
> >> + *	device_set_inuse_enable - Mark the device as used by userspace
> >> + *	application
> >> + */
> >> +int device_set_inuse_enable(struct device *dev, int enable)
> >>     
> >
> > We have bool for things like 'enable'.
> >
> >   
> ok
> >> +{
> >> +	mutex_lock(&dpm_list_mtx);
> >> +
> >> +	/* the new status is equal the old one */
> >> +	if (dev->power.is_inuse == enable)
> >> +		goto out;
> >> +
> >> +	dev->power.is_inuse = enable;
> >> +
> >> +	/* Update device children to set the in use status */
> >> +	device_visit_subtree(dev, (void *)enable,
> >> +				device_set_may_inuse_enable);
> >>     
> >
> > Why not do:
> >
> >     if (dev->power.in_use != enable) {
> >          dev->power.in_use = enable;
> >          device_visit_subtree(dev, (void *)enable, device_set_may_inuse_enable);
> >     }
> >
> > Also, I think this 'enable' conversion isn't really necessary.  You can use two
> > separate helper functions for setting and unsetting and pass NULL as the second
> > argument.
> >
> >   
> ok
> >> +
> >> +out:
> >> +	mutex_unlock(&dpm_list_mtx);
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(device_set_inuse_enable);
> >> +
> >>  /**
> >>   *	device_pm_add - add a device to the list of active devices
> >>   *	@dev:	Device to be added to the list
> >> @@ -78,6 +117,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_is_inuse(dev->parent)) {
> >> +			mutex_unlock(&dpm_list_mtx);
> >> +			/* if the parent has suspend disable, propagate it
> >> +			 * to the new child */
> >> +			device_set_may_inuse_enable(dev, (void *)1);
> >>     
> >
> > The conversion is just terrible.  I'd very much prefer it to be
> > device_set_in_use_possible_enable(dev, true).
> >
> >   
> ok
> >> +			mutex_lock(&dpm_list_mtx);
> >> +		}
> >>  	} else if (transition_started) {
> >>  		/*
> >>  		 * We refuse to register parentless devices while a PM
> >> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> >> index c7cb4fc..e7d21bb 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_is_inuse(struct device *dev)
> >> +{
> >> +	return dev->power.is_inuse || dev->power.may_inuse;
> >> +}
> >>     
> >
> > OK, so what's the meaning of is_inuse and may_inuse?
> >
> >   
> Maybe the idea is if the parent is in_use the child are may_inuse so 
> they are potentialy in
> use. The user can disable a tree and after reanable a child.

So I'd call the flag subtree_in_use or better subtree_no_suspend, then.

Moreover, you don't really have to propagate the no_suspend bit down the
device tree when the flag is set for a device.  You can simply modify the
prepare phase of suspend to check if the current device's parent has
no_suspend or subtree_no_suspend set and to set that for the current device
if so (or clear otherwise).

Thanks,
Rafael

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

[Michael Trimarchi]

Hi,

Rafael J. Wysocki wrote:
> On Tuesday 21 April 2009, Michael Trimarchi wrote:
>   
>> Rafael J. Wysocki wrote:
>>     
>>> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>>>> the drivers to avoid power down during suspend.
>>>>     
>>>>         
>>> OK, so the idea is that in_use will be set by the user space for devices that
>>> shouldn't be suspended.  Is this correct?
>>>
>>> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
>>> may_inuse is supposed to mean that we can set in_use for this device, I'd call
>>> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
>>> unset it if it is going to react to 'in_use'.
>>>
>>>       
>> is_inuse is set for the device. The may_inuse is automatically setting 
>> for the child
>> device. This is done for automatically propagate the dependency
>>     
>
> I see.  I'd call it differently, then.
>
> Besides, is it really always the case that setting the flag for one device
> implies that the entire subtree below it should have the flag set?  IOW,
> there may be some devices in the subtree that we may want to suspend anyway,
> I think.
>   
There are two possible scenario here:
- propagate to the subtree for each device that its parent is in use
- leave to the user space marking the use/in_use status (i will change 
the name)

I don't see problem to write a function in userspace instead on kernel 
space.
We left the decision to suspend/not suspend to the driver. Is it correct?

So basically, just add the flag and set it in device structure and don't 
skip the
device during the suspend phase. This is can be a simple and clean solution.


Michael

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

[Michael Trimarchi]

Rafael J. Wysocki wrote:
> On Tuesday 21 April 2009, Michael Trimarchi wrote:
>   
>> Rafael J. Wysocki wrote:
>>     
>>> On Thursday 16 April 2009, 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 patch add the in_use attribute, that it can be used by the
>>>> the drivers to avoid power down during suspend.
>>>>     
>>>>         
>>> OK, so the idea is that in_use will be set by the user space for devices that
>>> shouldn't be suspended.  Is this correct?
>>>
>>> Assuming it is, I'd call the flag 'in_use' rather than 'is_inuse'.  Also, if
>>> may_inuse is supposed to mean that we can set in_use for this device, I'd call
>>> it 'in_use_valid', I'd make it be unset by default and I'd allow the driver to
>>> unset it if it is going to react to 'in_use'.
>>>   
>>>       
>> is_inuse is set for the device. The may_inuse is automatically setting 
>> for the child
>> device. This is done for automatically propagate the dependency
>>     
>
> I see.  I'd call it differently, then.
>
> Besides, is it really always the case that setting the flag for one device
> implies that the entire subtree below it should have the flag set?  IOW,
> there may be some devices in the subtree that we may want to suspend anyway,
> I think.
>
>   
>>>> 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>
>>>>
>>>> ---
>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>> index e73c92d..d67043b 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -1124,6 +1124,49 @@ static struct device *next_device(struct klist_iter *i)
>>>>  }
>>>>  
>>>>  /**
>>>> + * device_visit_subtree - device subtree iterator.
>>>> + * @root: root struct device.
>>>> + * @data: data for the callback.
>>>> + * @fn: function to be called for each device.
>>>> + *
>>>> + * Iterate the @parent's subtree devices, and call @fn for each,
>>>> + * passing it @data.
>>>> + *
>>>> + */
>>>>     
>>>>         
>>> Hmm, I'm not sure ig Greg is going to like it.
>>>
>>>   
>>>       
>> This function walk the tree of devices following the dependences in 
>> iterative mode.
>>     
>
> Yes, it does, but the implementation is not the cleanest one IMO.
>
>   
>>>> +void device_visit_subtree(struct device *root, void *data,
>>>> +			  int (*fn)(struct device *dev, void *data))
>>>> +{
>>>> +	struct klist_iter i;
>>>> +	struct device *parent = root;
>>>>     
>>>>         
>>> I'd call it 'current' or 'cur';
>>>
>>>   
>>>       
>> ok
>>     
>>>> +	struct device *child = NULL;
>>>> +	int error;
>>>> +
>>>> +	klist_iter_init(&parent->p->klist_children, &i);
>>>> +move_down:
>>>> +	error = fn(parent, data);
>>>> +	if (error && parent != root)
>>>>     
>>>>         
>>> Shouldn't the iteration break on error?
>>>
>>>   
>>>       
>> The iteration don't break on error because, the return just said that the
>> subtree is just enable
>>     
>
> You're assuming that _your_ function will be the only one called via this one,
> but in that case why do you introduce a generic low level helper?
>
>   
>>>> +		goto move_up;
>>>> +
>>>> +	pr_debug("device: '%s': %s\n", dev_name(parent), __func__);
>>>> +
>>>> +	child = next_device(&i);
>>>> +	if (child) {
>>>> +		parent = child;
>>>> +		goto move_down;
>>>> +	}
>>>> +move_up:
>>>> +	klist_iter_exit(&i);
>>>> +	if (parent != root) {
>>>> +		klist_iter_init_node(&parent->parent->p->klist_children, &i,
>>>> +				     &parent->p->knode_parent);
>>>> +		parent = next_device(&i);
>>>> +		if (parent)
>>>> +			goto move_down;
>>>> +		klist_iter_exit(&i);
>>>> +	}
>>>>     
>>>>         
>>> Please find a way to reduce the number of gotos in this function.
>>>
>>> Besides, I'm not sure if it's really necessary.  What's wrong with using
>>> simply device_for_each_child() instead?
>>>   
>>>       
>> Just to have an iterative function
>>     
>
> Care to elaborate?
>
>   
>>>   
>>>       
>>>> +}
>>>> +
>>>> +/**
>>>>   * device_for_each_child - device child iterator.
>>>>   * @parent: parent struct device.
>>>>   * @data: data for the callback.
>>>> @@ -1207,6 +1250,7 @@ int __init devices_init(void)
>>>>  	return -ENOMEM;
>>>>  }
>>>>  
>>>> +EXPORT_SYMBOL_GPL(device_visit_subtree);
>>>>  EXPORT_SYMBOL_GPL(device_for_each_child);
>>>>  EXPORT_SYMBOL_GPL(device_find_child);
>>>>  
>>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>>> index 69b4ddb..00ad150 100644
>>>> --- a/drivers/base/power/main.c
>>>> +++ b/drivers/base/power/main.c
>>>> @@ -64,6 +64,45 @@ void device_pm_unlock(void)
>>>>  	mutex_unlock(&dpm_list_mtx);
>>>>  }
>>>>  
>>>> +int device_set_may_inuse_enable(struct device *dev, void *data)
>>>>     
>>>>         
>>> What exactly is the purpose of this function?
>>>
>>>   
>>>       
>> This function said that the parent is used by a driver
>>     
>>>> +{
>>>> +	pr_debug("PM: Device change in use status: %s\n", dev_name(dev));
>>>> +
>>>> +	/* if the device is suspend the subtree is in may_suspend status */
>>>> +	if (dev->power.is_inuse)
>>>> +		goto out;
>>>>     
>>>>         
>>>    return 1; ?
>>>
>>>   
>>>       
>>>> +
>>>> +	dev->power.may_inuse = (unsigned int)data;
>>>>     
>>>>         
>>> Can this conversion be avoided?
>>>
>>>   
>>>       
>>>> +	return 0;
>>>> +out:
>>>> +	/* cut the entire subtree */
>>>> +	return 1;
>>>> +}
>>>> +
>>>> +/**
>>>> + *	device_set_inuse_enable - Mark the device as used by userspace
>>>> + *	application
>>>> + */
>>>> +int device_set_inuse_enable(struct device *dev, int enable)
>>>>     
>>>>         
>>> We have bool for things like 'enable'.
>>>
>>>   
>>>       
>> ok
>>     
>>>> +{
>>>> +	mutex_lock(&dpm_list_mtx);
>>>> +
>>>> +	/* the new status is equal the old one */
>>>> +	if (dev->power.is_inuse == enable)
>>>> +		goto out;
>>>> +
>>>> +	dev->power.is_inuse = enable;
>>>> +
>>>> +	/* Update device children to set the in use status */
>>>> +	device_visit_subtree(dev, (void *)enable,
>>>> +				device_set_may_inuse_enable);
>>>>     
>>>>         
>>> Why not do:
>>>
>>>     if (dev->power.in_use != enable) {
>>>          dev->power.in_use = enable;
>>>          device_visit_subtree(dev, (void *)enable, device_set_may_inuse_enable);
>>>     }
>>>
>>> Also, I think this 'enable' conversion isn't really necessary.  You can use two
>>> separate helper functions for setting and unsetting and pass NULL as the second
>>> argument.
>>>
>>>   
>>>       
>> ok
>>     
>>>> +
>>>> +out:
>>>> +	mutex_unlock(&dpm_list_mtx);
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(device_set_inuse_enable);
>>>> +
>>>>  /**
>>>>   *	device_pm_add - add a device to the list of active devices
>>>>   *	@dev:	Device to be added to the list
>>>> @@ -78,6 +117,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_is_inuse(dev->parent)) {
>>>> +			mutex_unlock(&dpm_list_mtx);
>>>> +			/* if the parent has suspend disable, propagate it
>>>> +			 * to the new child */
>>>> +			device_set_may_inuse_enable(dev, (void *)1);
>>>>     
>>>>         
>>> The conversion is just terrible.  I'd very much prefer it to be
>>> device_set_in_use_possible_enable(dev, true).
>>>
>>>   
>>>       
>> ok
>>     
>>>> +			mutex_lock(&dpm_list_mtx);
>>>> +		}
>>>>  	} else if (transition_started) {
>>>>  		/*
>>>>  		 * We refuse to register parentless devices while a PM
>>>> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
>>>> index c7cb4fc..e7d21bb 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_is_inuse(struct device *dev)
>>>> +{
>>>> +	return dev->power.is_inuse || dev->power.may_inuse;
>>>> +}
>>>>     
>>>>         
>>> OK, so what's the meaning of is_inuse and may_inuse?
>>>
>>>   
>>>       
>> Maybe the idea is if the parent is in_use the child are may_inuse so 
>> they are potentialy in
>> use. The user can disable a tree and after reanable a child.
>>     
>
> So I'd call the flag subtree_in_use or better subtree_no_suspend, then.
>   
If you say just subtree is in use, you have this case:

A in_use ---> A1 may_inuse----->A4 may_inuse---- A6 no_in_use
|              |
\----> A2      \ --A7 may_inuse ---- A5 no_in_use
|
\----> A3

The user do echo "enabled" > in_use for device A

A1, A2, A4, A6, A7, A5 go in may_inuse state.

The user space can check that the device is in_use. it does't know is for
an in_use or may_inuse condition but it doesnt metter because the user space
can change for example the A5 and A6 and give the graph above. This is 
the recursion
issue.

> Moreover, you don't really have to propagate the no_suspend bit down the
> device tree when the flag is set for a device.  You can simply modify the
> prepare phase of suspend to check if the current device's parent has
> no_suspend or subtree_no_suspend set and to set that for the current device
> if so (or clear otherwise).
>   
If I understand we don't want that this flag change the pm transition.
> Thanks,
> Rafael
>
>   
Michael

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

[Rafael J. Wysocki]

On Thursday 23 April 2009, Michael Trimarchi wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday 21 April 2009, Michael Trimarchi wrote:
> >   
> >> Rafael J. Wysocki wrote:
> >>     
> >>> On Thursday 16 April 2009, Michael Trimarchi wrote:
> >>>   
> >> Maybe the idea is if the parent is in_use the child are may_inuse so 
> >> they are potentialy in
> >> use. The user can disable a tree and after reanable a child.
> >>     
> >
> > So I'd call the flag subtree_in_use or better subtree_no_suspend, then.
> >   
> If you say just subtree is in use, you have this case:

I didn't mean that.  The suggestion was only to change the _names_ of the
flags, so that 'no_suspend' means "leave this device alone during suspend" and
'subtree_no_suspend' means "you may want to leave this device alone during
suspend, because one of the devices it depends on has no_suspend set".  This
still is a per-device flag, though, so the user space may be allowed to unset
it if there's an interface for that.

> A in_use ---> A1 may_inuse----->A4 may_inuse---- A6 no_in_use
> |              |
> \----> A2      \ --A7 may_inuse ---- A5 no_in_use
> |
> \----> A3
> 
> The user do echo "enabled" > in_use for device A
> 
> A1, A2, A4, A6, A7, A5 go in may_inuse state.
> 
> The user space can check that the device is in_use. it does't know is for
> an in_use or may_inuse condition but it doesnt metter because the user space
> can change for example the A5 and A6 and give the graph above. This is 
> the recursion
> issue.
>
> > Moreover, you don't really have to propagate the no_suspend bit down the
> > device tree when the flag is set for a device.  You can simply modify the
> > prepare phase of suspend to check if the current device's parent has
> > no_suspend or subtree_no_suspend set and to set that for the current device
> > if so (or clear otherwise).
> >   
> If I understand we don't want that this flag change the pm transition.

The idea was that if you wanted to propagate 'no_suspend' down the device
tree from given parent, then you could use the existing suspend code for
that without adding any new code walking the device tree.

Still, if you want the user space to be able to allow the device to suspend
even if, for example, its parent has 'no_suspend' set, then IMO the best
approach would be to require the user space to set 'no_suspend' directly for
each device it doesn't want to suspend, regardless of the dependencies
between the devices.

Thanks,
Rafael