Skip to contentSkip to conversation

s390: klist bus_find_device & driver_find_device callback.

PATCH In review Martin Schwidefsky 5 participants 9 messages Jun 21, 2005 – Jun 22, 2005

Subsystems (3)
s390 Driver core s390 common I/O layer
lore
Share & tools
lore

Martin Schwidefsky

author
#0 lore raw

PATCH 1/16 s390: klist bus_find_device & driver_find_device callback.

[patch 1/16] s390: klist bus_find_device & driver_find_device callback.

From: Cornelia Huck <[email protected]>

Add bus_find_device() and driver_find_device() which allow a callback for each device in the bus's resp. the driver's klist.

Signed-off-by Martin Schwidefsky <[email protected]>

diffstat:

 drivers/base/bus.c     |   31 +++++++++++++++++++++++++++++++
 drivers/base/driver.c  |   34 ++++++++++++++++++++++++++++++++++
 include/linux/device.h |    7 +++++++
 3 files changed, 72 insertions(+)
drivers/base/bus.c +31 -0
diff -urpN linux-2.6/drivers/base/bus.c linux-2.6-patched/drivers/base/bus.c
@@ -177,6 +177,37 @@ int bus_for_each_dev(struct bus_type * b
 	return error;
 }
 
+/**
+ * bus_find_device - device iterator for locating a particular device.
+ * @bus: bus type
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the bus_for_each_dev() function above, but it
+ * returns a pointer to a device that is 'found', as determined
+ * by the @match callback. The callback should return a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * bus_find_device(struct bus_type * bus, struct device * start,
+				void * data, int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!bus)
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
 
 
 static struct device_driver * next_driver(struct klist_iter * i)
drivers/base/driver.c +34 -0
diff -urpN linux-2.6/drivers/base/driver.c linux-2.6-patched/drivers/base/driver.c
@@ -56,6 +56,40 @@ EXPORT_SYMBOL_GPL(driver_for_each_device
 
 
 /**
+ * driver_find_device - device iterator for locating a particular device.
+ * @driver: The device's driver
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the driver_for_each_device() function above, but it
+ * returns a pointer to a device that is 'found', as determined
+ * by the @match callback. The callback should return a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * driver_find_device(struct device_driver *drv,
+				   struct device * start, void * data,
+				   int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!drv)
+		return NULL;
+
+	klist_iter_init_node(&drv->klist_devices, &i,
+			     (start ? &start->knode_driver : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
+
+
+/**
  *	driver_create_file - create sysfs file for driver.
  *	@drv:	driver.
  *	@attr:	driver attribute descriptor.
include/linux/device.h +7 -0
diff -urpN linux-2.6/include/linux/device.h linux-2.6-patched/include/linux/device.h
@@ -81,6 +81,9 @@ extern struct bus_type * find_bus(char *
 int bus_for_each_dev(struct bus_type * bus, struct device * start, void * data,
 		     int (*fn)(struct device *, void *));
 
+struct device *  bus_find_device(struct bus_type * bus, struct device * start,
+				 void * data, int (*match)(struct device *, void *));
+
 int bus_for_each_drv(struct bus_type * bus, struct device_driver * start, 
 		     void * data, int (*fn)(struct device_driver *, void *));
 

@@ -143,6 +146,10 @@ extern void driver_remove_file(struct de
 extern int driver_for_each_device(struct device_driver * drv, struct device * start,
 				  void * data, int (*fn)(struct device *, void *));
 
+struct device *  driver_find_device(struct device_driver * drv,
+				    struct device * start, void * data,
+				    int (*match)(struct device *, void *));
+
 
 /*
  * device classes
#1 lore raw
Quoted 43 lines · Martin Schwidefsky

[patch 1/16] s390: klist bus_find_device & driver_find_device callback.

From: Cornelia Huck <[email protected]>

Add bus_find_device() and driver_find_device() which allow a callback for each device in the bus's resp. the driver's klist.

Signed-off-by Martin Schwidefsky <[email protected]>

diffstat:

 drivers/base/bus.c     |   31 +++++++++++++++++++++++++++++++
 drivers/base/driver.c  |   34 ++++++++++++++++++++++++++++++++++
 include/linux/device.h |    7 +++++++
 3 files changed, 72 insertions(+)

diff -urpN linux-2.6/drivers/base/bus.c linux-2.6-patched/drivers/base/bus.c

drivers/base/bus.c +21 -0
@@ -177,6 +177,37 @@ int bus_for_each_dev(struct bus_type * b
 	return error;
 }
 
+/**
+ * bus_find_device - device iterator for locating a particular device.
+ * @bus: bus type
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the bus_for_each_dev() function above, but it
+ * returns a pointer to a device that is 'found', as determined
+ * by the @match callback. The callback should return a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * bus_find_device(struct bus_type * bus, struct device * start,
+				void * data, int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!bus)

drivers/base/bus.c int bus_for_each_dev(struct bus_type * b · line 210

Quoted 10 lines
drivers/base/bus.c +10 -0
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}

What's wrong with just using bus_for_each_dev() instead? You have to supply a "match" type function anyway, so the caller doesn't have an easier time using this function instead.

You also don't increment the reference properly when you return the pointer, so you better document that... :(

In short, I don't think this is needed at all, as it's an almost identical copy of bus_for_each_dev().

Quoted 28 lines

diff -urpN linux-2.6/drivers/base/driver.c linux-2.6-patched/drivers/base/driver.c

drivers/base/driver.c +21 -0
@@ -56,6 +56,40 @@ EXPORT_SYMBOL_GPL(driver_for_each_device
 
 
 /**
+ * driver_find_device - device iterator for locating a particular device.
+ * @driver: The device's driver
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the driver_for_each_device() function above, but it
+ * returns a pointer to a device that is 'found', as determined
+ * by the @match callback. The callback should return a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * driver_find_device(struct device_driver *drv,
+				   struct device * start, void * data,
+				   int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!drv)

drivers/base/driver.c EXPORT_SYMBOL_GPL(driver_for_each_device · line 89

Quoted 10 lines
drivers/base/driver.c +10 -0
+		return NULL;
+
+	klist_iter_init_node(&drv->klist_devices, &i,
+			     (start ? &start->knode_driver : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}

Same comment as above, I don't think this function is necessary.

thanks,

greg k-h

#2 lore raw

Greg KH <[email protected]> wrote on 22.06.2005 08:26:27:

Quoted 3 lines

What's wrong with just using bus_for_each_dev() instead? You have to supply a "match" type function anyway, so the caller doesn't have an easier time using this function instead.

Maybe it's just too early in the morning, but I don't see how I could achive what I want to do with bus_for_each_dev(). The idea behind bus_find_device() is to scan the bus for a device matching some criterium and to return a pointer to it with which the caller can continue to work. bus_for_each_dev() calls the match function for every device until we abort, but I don't see how I can grab a reference to a specific device for later use.

Quoted 2 lines

You also don't increment the reference properly when you return the pointer, so you better document that... :(

You're right, this should be done in the base code and not by the caller...

Quoted 2 lines

In short, I don't think this is needed at all, as it's an almost identical copy of bus_for_each_dev().

It looks similar, yes, but they are for different purposes:
- bus_for_each_dev(): do something for each device (as specified in the callback function)
- bus_find_device(): get a reference to a specific device for later use (as matched in the callback function)

Quoted 1 line

Same comment as above, I don't think this function is necessary.

Same comment from me, I think I need this interface for drivers as well.

Quoted 3 lines

thanks,

greg k-h

Regards,
Cornelia

#3 lore raw
Quoted 6 lines · Cornelia Huck

Greg KH <[email protected]> wrote on 22.06.2005 08:26:27:

Quoted 3 lines

What's wrong with just using bus_for_each_dev() instead? You have to supply a "match" type function anyway, so the caller doesn't have an easier time using this function instead.

Quoted 7 lines

Maybe it's just too early in the morning, but I don't see how I could achive what I want to do with bus_for_each_dev(). The idea behind bus_find_device() is to scan the bus for a device matching some criterium and to return a pointer to it with which the caller can continue to work. bus_for_each_dev() calls the match function for every device until we abort, but I don't see how I can grab a reference to a specific device for later use.

Ah, now I get it. "later use" is the key point here. I was thinking you could do whatever you want within the callback. But if you want to do something later on with this pointer, that would be very tough.

Hm, I could use this kind of function, as I had to jump through a few hoops when iterating over all devices on a bus, when I just wanted to find a specific device (it involved creating a temp structure on the stack and doing my logic in the callback function itself, for details see
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/driver-bind.patch
for a patch that adds manual binding of drivers to devices from userspace through sysfs. With this function it should get even smaller.)

Quoted 5 lines
Quoted 2 lines

You also don't increment the reference properly when you return the pointer, so you better document that... :(

You're right, this should be done in the base code and not by the caller...

Care to fix this up and resend it?

Sorry for the misunderstanding.

thanks,

greg k-h

#4 lore raw

PATCH 1/2 s390: klist bus_find_device & driver_find_device callback.

[patch 1/2] s390: klist bus_find_device & driver_find_device callback.

Add bus_find_device() and driver_find_device() which allow searching for a device in the bus's resp. the driver's klist and obtain a reference on it.

This version now increases the reference count for the device to be returned. Also, the interface has been exported to GPL modules and the comments have been improved.

Signed-off-by Cornelia Huck <[email protected]>
 drivers/base/bus.c     |   33 +++++++++++++++++++++++++++++++++
 drivers/base/driver.c  |   35 +++++++++++++++++++++++++++++++++++
 include/linux/device.h |    7 +++++++
 3 files changed, 75 insertions(+), 0 deletions(-)
drivers/base/bus.c +33 -0
@@ -177,6 +177,38 @@ int bus_for_each_dev(struct bus_type * b
 	return error;
 }
 
+/**
+ * bus_find_device - device iterator for locating a particular device.
+ * @bus: bus type
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the bus_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 a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * bus_find_device(struct bus_type * bus,
+				struct device * start, void * data,
+				int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!bus)
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data) && get_device(dev))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
 
 
 static struct device_driver * next_driver(struct klist_iter * i)

@@ -558,6 +590,7 @@ int __init buses_init(void)
 
 
 EXPORT_SYMBOL_GPL(bus_for_each_dev);
+EXPORT_SYMBOL_GPL(bus_find_device);
 EXPORT_SYMBOL_GPL(bus_for_each_drv);
 
 EXPORT_SYMBOL_GPL(bus_add_device);
drivers/base/driver.c +35 -0
@@ -56,6 +56,41 @@ EXPORT_SYMBOL_GPL(driver_for_each_device
 
 
 /**
+ * driver_find_device - device iterator for locating a particular device.
+ * @driver: The device's driver
+ * @start: Device to begin with
+ * @data: Data to pass to match function
+ * @match: Callback function to check device
+ *
+ * This is similar to the driver_for_each_device() 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 a bool - 0 if
+ * the device doesn't match and 1 if it does.
+ * The function will return if a device is found.
+ */
+
+struct device * driver_find_device(struct device_driver *drv,
+				   struct device * start, void * data,
+				   int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!drv)
+		return NULL;
+
+	klist_iter_init_node(&drv->klist_devices, &i,
+			     (start ? &start->knode_driver : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data) && get_device(dev))
+			break;
+	klist_iter_exit(&i);
+	return dev;
+}
+
+EXPORT_SYMBOL_GPL(driver_find_device);
+
+/**
  *	driver_create_file - create sysfs file for driver.
  *	@drv:	driver.
  *	@attr:	driver attribute descriptor.
include/linux/device.h +7 -0
@@ -81,6 +81,9 @@ extern struct bus_type * find_bus(char *
 int bus_for_each_dev(struct bus_type * bus, struct device * start, void * data,
 		     int (*fn)(struct device *, void *));
 
+struct device *  bus_find_device(struct bus_type * bus, struct device * start,
+				 void * data, int (*match)(struct device *, void *));
+
 int bus_for_each_drv(struct bus_type * bus, struct device_driver * start, 
 		     void * data, int (*fn)(struct device_driver *, void *));
 

@@ -143,6 +146,10 @@ extern void driver_remove_file(struct de
 extern int driver_for_each_device(struct device_driver * drv, struct device * start,
 				  void * data, int (*fn)(struct device *, void *));
 
+struct device *  driver_find_device(struct device_driver * drv,
+				    struct device * start, void * data,
+				    int (*match)(struct device *, void *));
+
 
 /*
  * device classes
#5 lore raw

PATCH 2/2 s390: use klist in cio.

In reply to Greg KH

[patch 2/2] s390: use klist in cio.

Convert the common I/O layer to use the klist interfaces.

This patch has been adapted from the previous version to the changed interface semantics. Also, gcc 4.0 compile warnings have been removed.

Signed-off-by Cornelia Huck <[email protected]>
 drivers/s390/cio/ccwgroup.c |   30 +++++----------
 drivers/s390/cio/css.c      |   34 +++++++----------
 drivers/s390/cio/device.c   |   84 ++++++++++++++++++++++---------------------
 3 files changed, 66 insertions(+), 82 deletions(-)
drivers/s390/cio/ccwgroup.c +9 -21
@@ -403,34 +403,22 @@ ccwgroup_driver_register (struct ccwgrou
 	return driver_register(&cdriver->driver);
 }
 
-static inline struct device *
-__get_next_ccwgroup_device(struct device_driver *drv)
+static int
+__ccwgroup_driver_unregister_device(struct device *dev, void *data)
 {
-	struct device *dev, *d;
-
-	down_read(&drv->bus->subsys.rwsem);
-	dev = NULL;
-	list_for_each_entry(d, &drv->devices, driver_list) {
-		dev = get_device(d);
-		if (dev)
-			break;
-	}
-	up_read(&drv->bus->subsys.rwsem);
-	return dev;
+	__ccwgroup_remove_symlinks(to_ccwgroupdev(dev));
+	device_unregister(dev);
+	put_device(dev);
+	return 0;
 }
 
 void
 ccwgroup_driver_unregister (struct ccwgroup_driver *cdriver)
 {
-	struct device *dev;
-
 	/* We don't want ccwgroup devices to live longer than their driver. */
 	get_driver(&cdriver->driver);
-	while ((dev = __get_next_ccwgroup_device(&cdriver->driver))) {
-		__ccwgroup_remove_symlinks(to_ccwgroupdev(dev));
-		device_unregister(dev);
-		put_device(dev);
-	};
+	driver_for_each_device(&cdriver->driver, NULL, NULL,
+			       __ccwgroup_driver_unregister_device);
 	put_driver(&cdriver->driver);
 	driver_unregister(&cdriver->driver);
 }

@@ -449,7 +437,7 @@ __ccwgroup_get_gdev_by_cdev(struct ccw_d
 	if (cdev->dev.driver_data) {
 		gdev = (struct ccwgroup_device *)cdev->dev.driver_data;
 		if (get_device(&gdev->dev)) {
-			if (!list_empty(&gdev->dev.node))
+			if (klist_node_attached(&gdev->dev.knode_bus))
 				return gdev;
 			put_device(&gdev->dev);
 		}
drivers/s390/cio/css.c +14 -20
@@ -128,34 +128,28 @@ css_probe_device(int irq)
 	return ret;
 }
 
+static int
+check_subchannel(struct device * dev, void * data)
+{
+	struct subchannel *sch;
+	int irq = (unsigned long)data;
+
+	sch = to_subchannel(dev);
+	return (sch->irq == irq);
+}
+
 struct subchannel *
 get_subchannel_by_schid(int irq)
 {
-	struct subchannel *sch;
-	struct list_head *entry;
 	struct device *dev;
 
-	if (!get_bus(&css_bus_type))
-		return NULL;
-	down_read(&css_bus_type.subsys.rwsem);
-	sch = NULL;
-	list_for_each(entry, &css_bus_type.devices.list) {
-		dev = get_device(container_of(entry,
-					      struct device, bus_list));
-		if (!dev)
-			continue;
-		sch = to_subchannel(dev);
-		if (sch->irq == irq)
-			break;
-		put_device(dev);
-		sch = NULL;
-	}
-	up_read(&css_bus_type.subsys.rwsem);
-	put_bus(&css_bus_type);
+	dev = bus_find_device(&css_bus_type, NULL,
+			      (void *)(unsigned long)irq, check_subchannel);
 
-	return sch;
+	return dev ? to_subchannel(dev) : NULL;
 }
 
+
 static inline int
 css_get_subchannel_status(struct subchannel *sch, int schid)
 {
drivers/s390/cio/device.c +43 -41

116 lines not on this page · raw message

#6 lore raw
In reply to Greg KH
Quoted 1 line · On Wed, 22 Jun 2005 01:14:12 -0700 Greg KH <[email protected]> wrote:

Care to fix this up and resend it?

Yes, patches are on the way that supersede patches 1/16 and 2/16 from Martin's batch. The other patches are not affected by the change.

Quoted 2 lines

Sorry for the misunderstanding.

I should have written a better description :)

Quoted 4 lines

thanks,

greg k-h

Regards,
Cornelia

#7 lore raw
In reply to Martin Schwidefsky
Quoted 6 lines
drivers/base/bus.c +6 -0
+struct device * bus_find_device(struct bus_type * bus, struct device * start,
+				void * data, int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+

drivers/base/bus.c int bus_for_each_dev(struct bus_type * b · line 209

Quoted 10 lines
drivers/base/bus.c +10 -0
+	if (!bus)
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;

does the klist magic somehow grab a reference for you?

#8 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 10 lines
drivers/base/bus.c +10 -0
+struct device * bus_find_device(struct bus_type * bus, struct device * start,
+				void * data, int (*match)(struct device *, void *))
+{
+	struct klist_iter i;
+	struct device * dev;
+
+	if (!bus)
+		return NULL;
+
+	klist_iter_init_node(&bus->klist_devices, &i,
Quoted 8 lines

drivers/base/bus.c int bus_for_each_dev(struct bus_type * b · line 209

Quoted 6 lines
drivers/base/bus.c +6 -0
+			     (start ? &start->knode_bus : NULL));
+	while ((dev = next_device(&i)))
+		if (match(dev, data))
+			break;
+	klist_iter_exit(&i);
+	return dev;

does the klist magic somehow grab a reference for you?

No, see my previous comment about the need for this to be fixed for these two functions.

thanks,

greg k-h