[PATCH 1/7] driver-core : add class iteration api

[PATCH 1/7] driver-core : add class iteration api

[Dave Young]

Add the following class iteration functions for driver use:
class_for_each_device
class_find_device
class_for_each_child
class_find_child

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    8 ++
 2 files changed, 167 insertions(+)

diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c	2008-01-12 14:42:24.000000000 +0800
+++ linux.new/drivers/base/class.c	2008-01-12 14:42:24.000000000 +0800
@@ -798,6 +798,165 @@ void class_device_put(struct class_devic
 		kobject_put(&class_dev->kobj);
 }
 
+/**
+ *	class_for_each_device - device iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each device
+ *
+ *	Iterate over @class's list of devices, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_device(struct class *class, void *data,
+			   int (*fn)(struct device *, void *))
+{
+	struct device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			error = fn(dev, data);
+			put_device(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_device);
+
+/**
+ * class_find_device - device iterator for locating a particular device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check device
+ *
+ * This is similar to the class_for_each_dev() function above, but it
+ * returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+ */
+struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *))
+{
+	struct device *dev;
+	int error = 1;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				error = 0;
+				break;
+			} else
+				put_device(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	if (error)
+		return NULL;
+	return dev;
+}
+EXPORT_SYMBOL_GPL(class_find_device);
+
+/**
+ *	class_for_each_child - class child iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each child of the class
+ *
+ *	Iterate over @class's list of children, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			error = fn(dev, data);
+			class_device_put(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_child);
+
+/**
+ * class_find_child - device iterator for locating a particular class_device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check class_device
+ *
+ * This is similar to the class_for_each_child() function above, but it
+ * returns a reference to a class_device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the class_device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more class_devices.
+ */
+struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int error = 1;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				error = 0;
+				break;
+			} else
+				class_device_put(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	if (error)
+		return NULL;
+	return dev;
+}
+EXPORT_SYMBOL_GPL(class_find_child);
 
 int class_interface_register(struct class_interface *class_intf)
 {
diff -upr linux/include/linux/device.h linux.new/include/linux/device.h
--- linux/include/linux/device.h	2008-01-12 14:42:24.000000000 +0800
+++ linux.new/include/linux/device.h	2008-01-12 14:42:24.000000000 +0800
@@ -199,6 +199,14 @@ struct class {
 
 extern int __must_check class_register(struct class *);
 extern void class_unregister(struct class *);
+extern int class_for_each_device(struct class *class, void *data,
+				int (*fn)(struct device *dev, void *data));
+extern struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *));
+extern int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *));
+extern struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *));
 
 
 struct class_attribute {

Re: [PATCH 1/7] driver-core : add class iteration api

[Stefan Richter]

Dave Young wrote:
> Add the following class iteration functions for driver use:

Thanks Dave.  I will check the ieee1394 part in detail later.

...
> +/**
> + * class_find_device - device iterator for locating a particular device
> + * @class: the class we're iterating
> + * @data: data for the match function
> + * @match: function to check device
> + *
> + * This is similar to the class_for_each_dev() function above, but it
> + * returns a reference to a device that is 'found' for later use, as
> + * determined by the @match callback.

Maybe add "Drop the reference with put_device() after use." for the
really slow driver programmers like me?

> + *
> + * The callback should return 0 if the device doesn't match and non-zero
> + * if it does.  If the callback returns non-zero, this function will
> + * return to the caller and not iterate over any more devices.
> + */
> +struct device *class_find_device(struct class *class, void *data,
> +				   int (*match)(struct device *, void *))
> +{

A general comment on the linux/device.h API (not a direct comment on
your patch):

The match argument in bus_find_device(), driver_find_device(),
device_find_child(), class_find_device(), class_find_child() could be
changed to

	bool (*match)(struct device *, void *)).

Then the semantics are IMO a little bit clearer.  Ditto for the
dr_match_t type and the struct bus_type.match member.

I don't know though whether the churn of doing such a change everywhere
would be justified by the result.


A comment on patch 2/7...6/7:

You can bring most or all of the various __match implementations into a
slightly terser but IMO easy to read form, like this:

 static int __match_ne(struct device *dev, void *data)
 {
 	struct unit_directory *ud;
 	struct node_entry *ne = (struct node_entry *)data;

 	ud = container_of(dev, struct unit_directory, unit_dev);
-	if (ud->ne == ne)
-		return 1;
-	return 0;
+	return ud->ne == ne;
 }

Here it is also easy to see that readability would improve if the return
type was bool rather than int.
-- 
Stefan Richter
-=====-==--- ---= -==--
http://arcgraph.de/sr/

Re: [PATCH 1/7] driver-core : add class iteration api

[Jarek Poplawski]

On Sat, Jan 12, 2008 at 05:47:54PM +0800, Dave Young wrote:
> Add the following class iteration functions for driver use:
> class_for_each_device
> class_find_device
> class_for_each_child
> class_find_child
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
> 
> ---
>  drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |    8 ++
>  2 files changed, 167 insertions(+)
> 
> diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
> --- linux/drivers/base/class.c	2008-01-12 14:42:24.000000000 +0800
> +++ linux.new/drivers/base/class.c	2008-01-12 14:42:24.000000000 +0800
> @@ -798,6 +798,165 @@ void class_device_put(struct class_devic
>  		kobject_put(&class_dev->kobj);
>  }
>  
> +/**
> + *	class_for_each_device - device iterator
> + *	@class:	the class we're iterating
> + *	@data: data for the callback
> + *	@fn: function to be called for each device
> + *
> + *	Iterate over @class's list of devices, and call @fn for each,
> + *	passing it @data.
> + *
> + *	We check the return of @fn each time. If it returns anything
> + *	other than 0, we break out and return that value.
> + */
> +int class_for_each_device(struct class *class, void *data,
> +			   int (*fn)(struct device *, void *))
> +{
> +	struct device *dev;
> +	int error = 0;
> +
> +	if (!class)
> +		return -EINVAL;
> +	down(&class->sem);
> +	list_for_each_entry(dev, &class->devices, node) {

Probably some tiny oversight, but I see this comment to struct class
doesn't mention devices list, so maybe this needs to be updated BTW?:

(from include/linux/device.h)
"        struct semaphore        sem;    /* locks both the children and interfaces lists */"

Regards,
Jarek P.

Re: [PATCH 1/7] driver-core : add class iteration api

[Dave Young]

On Jan 12, 2008 6:50 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Dave Young wrote:
> > Add the following class iteration functions for driver use:
>
> Thanks Dave.  I will check the ieee1394 part in detail later.
>
> ...
> > +/**
> > + * class_find_device - device iterator for locating a particular device
> > + * @class: the class we're iterating
> > + * @data: data for the match function
> > + * @match: function to check device
> > + *
> > + * This is similar to the class_for_each_dev() function above, but it
> > + * returns a reference to a device that is 'found' for later use, as
> > + * determined by the @match callback.
>
> Maybe add "Drop the reference with put_device() after use." for the
> really slow driver programmers like me?

Sounds good, thanks.

>
> > + *
> > + * The callback should return 0 if the device doesn't match and non-zero
> > + * if it does.  If the callback returns non-zero, this function will
> > + * return to the caller and not iterate over any more devices.
> > + */
> > +struct device *class_find_device(struct class *class, void *data,
> > +                                int (*match)(struct device *, void *))
> > +{
>
> A general comment on the linux/device.h API (not a direct comment on
> your patch):
>
> The match argument in bus_find_device(), driver_find_device(),
> device_find_child(), class_find_device(), class_find_child() could be
> changed to
>
>         bool (*match)(struct device *, void *)).
>
> Then the semantics are IMO a little bit clearer.  Ditto for the
> dr_match_t type and the struct bus_type.match member.

Yes, from semantics side it's better.
But IMHO int is good as well, although it need a little bit more
understanding of the api.

>
> I don't know though whether the churn of doing such a change everywhere
> would be justified by the result.
>
>
> A comment on patch 2/7...6/7:
>
> You can bring most or all of the various __match implementations into a
> slightly terser but IMO easy to read form, like this:
>
>  static int __match_ne(struct device *dev, void *data)
>  {
>         struct unit_directory *ud;
>         struct node_entry *ne = (struct node_entry *)data;
>
>         ud = container_of(dev, struct unit_directory, unit_dev);
> -       if (ud->ne == ne)
> -               return 1;
> -       return 0;
> +       return ud->ne == ne;
>  }
>
> Here it is also easy to see that readability would improve if the return
> type was bool rather than int.

Ok, thanks.

> --
> Stefan Richter
> -=====-==--- ---= -==--
> http://arcgraph.de/sr/
>

Re: [PATCH 1/7] driver-core : add class iteration api

[Dave Young]

On Jan 13, 2008 4:11 AM, Jarek Poplawski <jarkao2@gmail.com> wrote:
>
> On Sat, Jan 12, 2008 at 05:47:54PM +0800, Dave Young wrote:
> > Add the following class iteration functions for driver use:
> > class_for_each_device
> > class_find_device
> > class_for_each_child
> > class_find_child
> >
> > Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> >
> > ---
> >  drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/device.h |    8 ++
> >  2 files changed, 167 insertions(+)
> >
> > diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
> > --- linux/drivers/base/class.c        2008-01-12 14:42:24.000000000 +0800
> > +++ linux.new/drivers/base/class.c    2008-01-12 14:42:24.000000000 +0800
> > @@ -798,6 +798,165 @@ void class_device_put(struct class_devic
> >               kobject_put(&class_dev->kobj);
> >  }
> >
> > +/**
> > + *   class_for_each_device - device iterator
> > + *   @class: the class we're iterating
> > + *   @data: data for the callback
> > + *   @fn: function to be called for each device
> > + *
> > + *   Iterate over @class's list of devices, and call @fn for each,
> > + *   passing it @data.
> > + *
> > + *   We check the return of @fn each time. If it returns anything
> > + *   other than 0, we break out and return that value.
> > + */
> > +int class_for_each_device(struct class *class, void *data,
> > +                        int (*fn)(struct device *, void *))
> > +{
> > +     struct device *dev;
> > +     int error = 0;
> > +
> > +     if (!class)
> > +             return -EINVAL;
> > +     down(&class->sem);
> > +     list_for_each_entry(dev, &class->devices, node) {
>
> Probably some tiny oversight, but I see this comment to struct class
> doesn't mention devices list, so maybe this needs to be updated BTW?:
>
> (from include/linux/device.h)
> "        struct semaphore        sem;    /* locks both the children and interfaces lists */"

Sorry for my lazy, I think so too.
IMHO, it should be updated after the comments.

>
> Regards,
> Jarek P.
>

Re: [PATCH 1/7] driver-core : add class iteration api

[Jarek Poplawski]

On Mon, Jan 14, 2008 at 09:36:04AM +0800, Dave Young wrote:
> On Jan 13, 2008 4:11 AM, Jarek Poplawski <jarkao2@gmail.com> wrote:
...
> > Probably some tiny oversight, but I see this comment to struct class
> > doesn't mention devices list, so maybe this needs to be updated BTW?:
> >
> > (from include/linux/device.h)
> > "        struct semaphore        sem;    /* locks both the children and interfaces lists */"
> 
> Sorry for my lazy, I think so too.
> IMHO, it should be updated after the comments.

As a matter of fact, only later I've found this question is 'fixed' in
7/7. But, actually, I was more concerned if this patch changed the
'official' policy of this sem (then it would be nice to mention about
this in a changelog) or this comment was simply incomplete.

Thanks,
Jarek P.

Re: [PATCH 1/7] driver-core : add class iteration api

[Dave Young]

On Jan 14, 2008 2:58 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Mon, Jan 14, 2008 at 09:36:04AM +0800, Dave Young wrote:
> > On Jan 13, 2008 4:11 AM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> ...
> > > Probably some tiny oversight, but I see this comment to struct class
> > > doesn't mention devices list, so maybe this needs to be updated BTW?:
> > >
> > > (from include/linux/device.h)
> > > "        struct semaphore        sem;    /* locks both the children and interfaces lists */"
> >
> > Sorry for my lazy, I think so too.
> > IMHO, it should be updated after the comments.
>
> As a matter of fact, only later I've found this question is 'fixed' in
> 7/7. But, actually, I was more concerned if this patch changed the
> 'official' policy of this sem (then it would be nice to mention about
> this in a changelog) or this comment was simply incomplete.

It was just "killed" because I thought it's not true.

I means I will update the correct comment after comments.

>
> Thanks,
> Jarek P.
>

Re: [PATCH 1/7] driver-core : add class iteration api

[Cornelia Huck]

On Sat, 12 Jan 2008 17:47:54 +0800,
Dave Young <hidave.darkstar@gmail.com> wrote:

Minor style suggestion (same for class_find_child):

> +struct device *class_find_device(struct class *class, void *data,
> +				   int (*match)(struct device *, void *))
> +{
> +	struct device *dev;
> +	int error = 1;

How about using inverse logic here (e.g., start with int found = 0)...

> +
> +	if (!class)
> +		return NULL;
> +
> +	down(&class->sem);
> +	list_for_each_entry(dev, &class->devices, node) {
> +		dev = get_device(dev);
> +		if (dev) {
> +			if (match(dev, data)) {
> +				error = 0;

...and set found = 1 here...

> +				break;
> +			} else
> +				put_device(dev);
> +		} else
> +			break;
> +	}
> +	up(&class->sem);
> +
> +	if (error)
> +		return NULL;
> +	return dev;

...and do
	return found ? dev : NULL;
in the end?

Especially since not finding the device is not really an error.

> +}

Otherwise this looks fine to me.

Re: [PATCH 1/7] driver-core : add class iteration api

[Dave Young]

On Jan 14, 2008 8:13 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Sat, 12 Jan 2008 17:47:54 +0800,
> Dave Young <hidave.darkstar@gmail.com> wrote:
>
> Minor style suggestion (same for class_find_child):
>
> > +struct device *class_find_device(struct class *class, void *data,
> > +                                int (*match)(struct device *, void *))
> > +{
> > +     struct device *dev;
> > +     int error = 1;
>
> How about using inverse logic here (e.g., start with int found = 0)...

Sounds good, will do. Thanks.

>
> > +
> > +     if (!class)
> > +             return NULL;
> > +
> > +     down(&class->sem);
> > +     list_for_each_entry(dev, &class->devices, node) {
> > +             dev = get_device(dev);
> > +             if (dev) {
> > +                     if (match(dev, data)) {
> > +                             error = 0;
>
> ...and set found = 1 here...
>
> > +                             break;
> > +                     } else
> > +                             put_device(dev);
> > +             } else
> > +                     break;
> > +     }
> > +     up(&class->sem);
> > +
> > +     if (error)
> > +             return NULL;
> > +     return dev;
>
> ...and do
>         return found ? dev : NULL;
> in the end?
>
> Especially since not finding the device is not really an error.
>
> > +}
>
> Otherwise this looks fine to me.
>

Re: [PATCH 1/7] driver-core : add class iteration api

[Dave Young]

Add the following class iteration functions for driver use:
class_for_each_device
class_find_device
class_for_each_child
class_find_child

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   11 ++-
 2 files changed, 168 insertions(+), 2 deletions(-)

diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c	2008-01-15 11:12:29.000000000 +0800
+++ linux.new/drivers/base/class.c	2008-01-15 11:12:29.000000000 +0800
@@ -798,6 +798,165 @@ void class_device_put(struct class_devic
 		kobject_put(&class_dev->kobj);
 }
 
+/**
+ *	class_for_each_device - device iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each device
+ *
+ *	Iterate over @class's list of devices, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_device(struct class *class, void *data,
+			   int (*fn)(struct device *, void *))
+{
+	struct device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			error = fn(dev, data);
+			put_device(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_device);
+
+/**
+ * class_find_device - device iterator for locating a particular device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check device
+ *
+ * This is similar to the class_for_each_dev() function above, but it
+ * returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+
+ * Note, you will need to drop the reference with put_device() after use.
+ */
+struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *))
+{
+	struct device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				put_device(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_device);
+
+/**
+ *	class_for_each_child - class child iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each child of the class
+ *
+ *	Iterate over @class's list of children, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			error = fn(dev, data);
+			class_device_put(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_child);
+
+/**
+ * class_find_child - device iterator for locating a particular class_device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check class_device
+ *
+ * This is similar to the class_for_each_child() function above, but it
+ * returns a reference to a class_device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the class_device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more class_devices.
+
+ * Note, you will need to drop the reference with class_device_put() after use.
+ */
+struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				class_device_put(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_child);
 
 int class_interface_register(struct class_interface *class_intf)
 {
diff -upr linux/include/linux/device.h linux.new/include/linux/device.h
--- linux/include/linux/device.h	2008-01-15 11:12:29.000000000 +0800
+++ linux.new/include/linux/device.h	2008-01-15 11:12:29.000000000 +0800
@@ -180,8 +180,7 @@ struct class {
 	struct list_head	devices;
 	struct list_head	interfaces;
 	struct kset		class_dirs;
-	struct semaphore	sem;	/* locks both the children and interfaces lists */
-
+	struct semaphore	sem; /* locks children, devices, interfaces */
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;
 	struct device_attribute		* dev_attrs;
@@ -199,6 +198,14 @@ struct class {
 
 extern int __must_check class_register(struct class *);
 extern void class_unregister(struct class *);
+extern int class_for_each_device(struct class *class, void *data,
+				int (*fn)(struct device *dev, void *data));
+extern struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *));
+extern int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *));
+extern struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *));
 
 
 struct class_attribute {

Re: [PATCH 1/7] driver-core : add class iteration api

[Cornelia Huck]

On Tue, 15 Jan 2008 17:13:22 +0800,
Dave Young <hidave.darkstar@gmail.com> wrote:

> 
> Add the following class iteration functions for driver use:
> class_for_each_device
> class_find_device
> class_for_each_child
> class_find_child
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

> 
> ---
>  drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |   11 ++-
>  2 files changed, 168 insertions(+), 2 deletions(-)

[PATCH 1/6] driver-core : add class iteration api

[Dave Young]

Add the following class iteration functions for driver use:
class_for_each_device
class_find_device
class_for_each_child
class_find_child

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 drivers/base/class.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   11 ++-
 2 files changed, 168 insertions(+), 2 deletions(-)

diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c	2008-01-15 11:12:29.000000000 +0800
+++ linux.new/drivers/base/class.c	2008-01-15 11:12:29.000000000 +0800
@@ -798,6 +798,165 @@ void class_device_put(struct class_devic
 		kobject_put(&class_dev->kobj);
 }
 
+/**
+ *	class_for_each_device - device iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each device
+ *
+ *	Iterate over @class's list of devices, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_device(struct class *class, void *data,
+			   int (*fn)(struct device *, void *))
+{
+	struct device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			error = fn(dev, data);
+			put_device(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_device);
+
+/**
+ * class_find_device - device iterator for locating a particular device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check device
+ *
+ * This is similar to the class_for_each_dev() function above, but it
+ * returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+
+ * Note, you will need to drop the reference with put_device() after use.
+ */
+struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *))
+{
+	struct device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				put_device(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_device);
+
+/**
+ *	class_for_each_child - class child iterator
+ *	@class:	the class we're iterating
+ *	@data: data for the callback
+ *	@fn: function to be called for each child of the class
+ *
+ *	Iterate over @class's list of children, and call @fn for each,
+ *	passing it @data.
+ *
+ *	We check the return of @fn each time. If it returns anything
+ *	other than 0, we break out and return that value.
+ */
+int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			error = fn(dev, data);
+			class_device_put(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_child);
+
+/**
+ * class_find_child - device iterator for locating a particular class_device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check class_device
+ *
+ * This is similar to the class_for_each_child() function above, but it
+ * returns a reference to a class_device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the class_device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more class_devices.
+
+ * Note, you will need to drop the reference with class_device_put() after use.
+ */
+struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				class_device_put(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_child);
 
 int class_interface_register(struct class_interface *class_intf)
 {
diff -upr linux/include/linux/device.h linux.new/include/linux/device.h
--- linux/include/linux/device.h	2008-01-15 11:12:29.000000000 +0800
+++ linux.new/include/linux/device.h	2008-01-15 11:12:29.000000000 +0800
@@ -180,8 +180,7 @@ struct class {
 	struct list_head	devices;
 	struct list_head	interfaces;
 	struct kset		class_dirs;
-	struct semaphore	sem;	/* locks both the children and interfaces lists */
-
+	struct semaphore	sem; /* locks children, devices, interfaces */
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;
 	struct device_attribute		* dev_attrs;
@@ -199,6 +198,14 @@ struct class {
 
 extern int __must_check class_register(struct class *);
 extern void class_unregister(struct class *);
+extern int class_for_each_device(struct class *class, void *data,
+				int (*fn)(struct device *dev, void *data));
+extern struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *));
+extern int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *));
+extern struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *));
 
 
 struct class_attribute {

Re: [PATCH 1/6] driver-core : add class iteration api

[Dave Young]

On Jan 22, 2008 1:54 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>
> Add the following class iteration functions for driver use:
> class_for_each_device
> class_find_device
> class_for_each_child
> class_find_child
>
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH 1/6] driver-core : add class iteration api

[David Brownell]

On Monday 21 January 2008, Dave Young wrote:
>  
> +/**
> + *	class_for_each_device - device iterator
> + *	@class:	the class we're iterating
> + *	@data: data for the callback
> + *	@fn: function to be called for each device
> + *
> + *	Iterate over @class's list of devices, and call @fn for each,
> + *	passing it @data.
> + *
> + *	We check the return of @fn each time. If it returns anything
> + *	other than 0, we break out and return that value.

I have a suggestion for better documentation, which
applies to all these utilities:


> + */
> +int class_for_each_device(struct class *class, void *data,
> +			   int (*fn)(struct device *, void *))
> +{
> +	struct device *dev;
> +	int error = 0;
> +
> +	if (!class)
> +		return -EINVAL;
> +	down(&class->sem);
> +	list_for_each_entry(dev, &class->devices, node) {
> +		dev = get_device(dev);
> +		if (dev) {
> +			error = fn(dev, data);

This is called with class->sem held.  So fn() has a
constraint to not re-acquire that ... else it'd be
self-deadlocking.  I'd like to see docs at least
mention that; calls to add or remove class members
would be verboten, for example, which isn't an issue
with most other driver model iterators.


> +			put_device(dev);
> +		} else
> +			error = -ENODEV;
> +		if (error)
> +			break;
> +	}
> +	up(&class->sem);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(class_for_each_device);

Re: [PATCH 1/6] driver-core : add class iteration api

[Dave Young]

On Jan 22, 2008 2:24 PM, David Brownell <david-b@pacbell.net> wrote:
> On Monday 21 January 2008, Dave Young wrote:
> >
> > +/**
> > + *   class_for_each_device - device iterator
> > + *   @class: the class we're iterating
> > + *   @data: data for the callback
> > + *   @fn: function to be called for each device
> > + *
> > + *   Iterate over @class's list of devices, and call @fn for each,
> > + *   passing it @data.
> > + *
> > + *   We check the return of @fn each time. If it returns anything
> > + *   other than 0, we break out and return that value.
>
> I have a suggestion for better documentation, which
> applies to all these utilities:
>
>
> > + */
> > +int class_for_each_device(struct class *class, void *data,
> > +                        int (*fn)(struct device *, void *))
> > +{
> > +     struct device *dev;
> > +     int error = 0;
> > +
> > +     if (!class)
> > +             return -EINVAL;
> > +     down(&class->sem);
> > +     list_for_each_entry(dev, &class->devices, node) {
> > +             dev = get_device(dev);
> > +             if (dev) {
> > +                     error = fn(dev, data);
>
> This is called with class->sem held.  So fn() has a
> constraint to not re-acquire that ... else it'd be
> self-deadlocking.  I'd like to see docs at least
> mention that; calls to add or remove class members
> would be verboten, for example, which isn't an issue
> with most other driver model iterators.

Very good comment, thanks david.  I will update after a while.

>
>
>
> > +                     put_device(dev);
> > +             } else
> > +                     error = -ENODEV;
> > +             if (error)
> > +                     break;
> > +     }
> > +     up(&class->sem);
> > +
> > +     return error;
> > +}
> > +EXPORT_SYMBOL_GPL(class_for_each_device);
>

Re: [PATCH 1/6] driver-core : add class iteration api

[Dave Young]

On Mon, Jan 21, 2008 at 10:24:17PM -0800, David Brownell wrote:
> On Monday 21 January 2008, Dave Young wrote:
> >  
> > +/**
> > + *	class_for_each_device - device iterator
> > + *	@class:	the class we're iterating
> > + *	@data: data for the callback
> > + *	@fn: function to be called for each device
> > + *
> > + *	Iterate over @class's list of devices, and call @fn for each,
> > + *	passing it @data.
> > + *
> > + *	We check the return of @fn each time. If it returns anything
> > + *	other than 0, we break out and return that value.
> 
> I have a suggestion for better documentation, which
> applies to all these utilities:
> 
> 
> > + */
> > +int class_for_each_device(struct class *class, void *data,
> > +			   int (*fn)(struct device *, void *))
> > +{
> > +	struct device *dev;
> > +	int error = 0;
> > +
> > +	if (!class)
> > +		return -EINVAL;
> > +	down(&class->sem);
> > +	list_for_each_entry(dev, &class->devices, node) {
> > +		dev = get_device(dev);
> > +		if (dev) {
> > +			error = fn(dev, data);
> 
> This is called with class->sem held.  So fn() has a
> constraint to not re-acquire that ... else it'd be
> self-deadlocking.  I'd like to see docs at least
> mention that; calls to add or remove class members
> would be verboten, for example, which isn't an issue
> with most other driver model iterators.
> 
> 
> > +			put_device(dev);
> > +		} else
> > +			error = -ENODEV;
> > +		if (error)
> > +			break;
> > +	}
> > +	up(&class->sem);
> > +
> > +	return error;
> > +}
> > +EXPORT_SYMBOL_GPL(class_for_each_device);

Update kerneldoc as david brownell's sugestion.
Is it right for me add Cornelia Huck's ack after this change?
---

Add the following class iteration functions for driver use:
class_for_each_device
class_find_device
class_for_each_child
class_find_child

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 drivers/base/class.c   |  175 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   11 ++-
 2 files changed, 184 insertions(+), 2 deletions(-)

diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c	2008-01-22 15:06:55.000000000 +0800
+++ linux.new/drivers/base/class.c	2008-01-22 15:06:55.000000000 +0800
@@ -798,6 +798,181 @@ void class_device_put(struct class_devic
 		kobject_put(&class_dev->kobj);
 }
 
+/**
+ * class_for_each_device - device iterator
+ * @class: the class we're iterating
+ * @data: data for the callback
+ * @fn: function to be called for each device
+ *
+ * Iterate over @class's list of devices, and call @fn for each,
+ * passing it @data.
+ *
+ * We check the return of @fn each time. If it returns anything
+ * other than 0, we break out and return that value.
+ *
+ * Note, we hold class->sem in this function, so it can not be
+ * re-acquired in @fn, otherwise it will self-deadlocking. For
+ * example, calls to add or remove class members would be verboten.
+ */
+int class_for_each_device(struct class *class, void *data,
+			   int (*fn)(struct device *, void *))
+{
+	struct device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			error = fn(dev, data);
+			put_device(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_device);
+
+/**
+ * class_find_device - device iterator for locating a particular device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check device
+ *
+ * This is similar to the class_for_each_dev() function above, but it
+ * returns a reference to a device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more devices.
+
+ * Note, you will need to drop the reference with put_device() after use.
+ *
+ * We hold class->sem in this function, so it can not be
+ * re-acquired in @match, otherwise it will self-deadlocking. For
+ * example, calls to add or remove class members would be verboten.
+ */
+struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *))
+{
+	struct device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->devices, node) {
+		dev = get_device(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				put_device(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_device);
+
+/**
+ * class_for_each_child - class child iterator
+ * @class: the class we're iterating
+ * @data: data for the callback
+ * @fn: function to be called for each child of the class
+ *
+ * Iterate over @class's list of children, and call @fn for each,
+ * passing it @data.
+ *
+ * We check the return of @fn each time. If it returns anything
+ * other than 0, we break out and return that value.
+ *
+ * Note, we hold class->sem in this function, so it can not be
+ * re-acquired in @fn, otherwise it will self-deadlocking. For
+ * example, calls to add or remove class members would be verboten.
+ */
+int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int error = 0;
+
+	if (!class)
+		return -EINVAL;
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			error = fn(dev, data);
+			class_device_put(dev);
+		} else
+			error = -ENODEV;
+		if (error)
+			break;
+	}
+	up(&class->sem);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_child);
+
+/**
+ * class_find_child - device iterator for locating a particular class_device
+ * @class: the class we're iterating
+ * @data: data for the match function
+ * @match: function to check class_device
+ *
+ * This is similar to the class_for_each_child() function above, but it
+ * returns a reference to a class_device that is 'found' for later use, as
+ * determined by the @match callback.
+ *
+ * The callback should return 0 if the class_device doesn't match and non-zero
+ * if it does.  If the callback returns non-zero, this function will
+ * return to the caller and not iterate over any more class_devices.
+ *
+ * Note, you will need to drop the reference with class_device_put() after use.
+ *
+ * We hold class->sem in this function, so it can not be
+ * re-acquired in @match, otherwise it will self-deadlocking. For
+ * example, calls to add or remove class members would be verboten.
+ */
+struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *))
+{
+	struct class_device *dev;
+	int found = 0;
+
+	if (!class)
+		return NULL;
+
+	down(&class->sem);
+	list_for_each_entry(dev, &class->children, node) {
+		dev = class_device_get(dev);
+		if (dev) {
+			if (match(dev, data)) {
+				found = 1;
+				break;
+			} else
+				class_device_put(dev);
+		} else
+			break;
+	}
+	up(&class->sem);
+
+	return found ? dev : NULL;
+}
+EXPORT_SYMBOL_GPL(class_find_child);
 
 int class_interface_register(struct class_interface *class_intf)
 {
diff -upr linux/include/linux/device.h linux.new/include/linux/device.h
--- linux/include/linux/device.h	2008-01-22 15:06:55.000000000 +0800
+++ linux.new/include/linux/device.h	2008-01-22 15:06:55.000000000 +0800
@@ -180,8 +180,7 @@ struct class {
 	struct list_head	devices;
 	struct list_head	interfaces;
 	struct kset		class_dirs;
-	struct semaphore	sem;	/* locks both the children and interfaces lists */
-
+	struct semaphore	sem; /* locks children, devices, interfaces */
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;
 	struct device_attribute		* dev_attrs;
@@ -199,6 +198,14 @@ struct class {
 
 extern int __must_check class_register(struct class *);
 extern void class_unregister(struct class *);
+extern int class_for_each_device(struct class *class, void *data,
+				int (*fn)(struct device *dev, void *data));
+extern struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *));
+extern int class_for_each_child(struct class *class, void *data,
+			   int (*fn)(struct class_device *, void *));
+extern struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *));
 
 
 struct class_attribute {

Re: [PATCH 1/6] driver-core : add class iteration api

[Cornelia Huck]

On Tue, 22 Jan 2008 15:27:08 +0800,
Dave Young <hidave.darkstar@gmail.com> wrote:

> On Mon, Jan 21, 2008 at 10:24:17PM -0800, David Brownell wrote:
> > This is called with class->sem held.  So fn() has a
> > constraint to not re-acquire that ... else it'd be
> > self-deadlocking.  I'd like to see docs at least
> > mention that; calls to add or remove class members
> > would be verboten, for example, which isn't an issue
> > with most other driver model iterators.

Indeed, it's a good idea to point this out.

> Update kerneldoc as david brownell's sugestion.
> Is it right for me add Cornelia Huck's ack after this change?

Fine with me.

> ---
> 
> Add the following class iteration functions for driver use:
> class_for_each_device
> class_find_device
> class_for_each_child
> class_find_child
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> ---
>  drivers/base/class.c   |  175 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |   11 ++-
>  2 files changed, 184 insertions(+), 2 deletions(-)

Re: [PATCH 1/6] driver-core : add class iteration api

[Greg KH]

On Tue, Jan 22, 2008 at 03:27:08PM +0800, Dave Young wrote:
> 
> Add the following class iteration functions for driver use:
> class_for_each_device
> class_find_device
> class_for_each_child
> class_find_child

As class_for_each_child() is not used by anyone in this patch series,
and we want to heavily discourage the use of class_device (only scsi and
IB are the last remaining users), I'll cut out this portion of the
patch.

Any objection?

thanks,

greg k-h

Re: [PATCH 1/6] driver-core : add class iteration api

[Dave Young]

On Jan 23, 2008 6:25 AM, Greg KH <greg@kroah.com> wrote:
> On Tue, Jan 22, 2008 at 03:27:08PM +0800, Dave Young wrote:
> >
> > Add the following class iteration functions for driver use:
> > class_for_each_device
> > class_find_device
> > class_for_each_child
> > class_find_child
>
> As class_for_each_child() is not used by anyone in this patch series,
> and we want to heavily discourage the use of class_device (only scsi and
> IB are the last remaining users), I'll cut out this portion of the
> patch.
>
> Any objection?

Looks good to me.

>
> thanks,
>
> greg k-h
>