[PATCH 0/7] convert semaphore to mutex in struct class

[PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

Convert semaphore to mutex in struct class.
All the patches in this series should be applyed simultaneously

toc:
---
1-driver-core-struct-class-convert-semaphore-to-mutex.patch
2-i2c-struct-class-convert-semaphore-to-mutex.patch
3-ieee1394-struct-class-convert-semaphore-to-mutex.patch
4-power-struct-class-convert-semaphore-to-mutex.patch
5-rtc-struct-class-convert-semaphore-to-mutex.patch
6-scsi-struct-class-convert-semaphore-to-mutex.patch
7-spi-struct-class-convert-semaphore-to-mutex.patch

Summary diffstat:
---
drivers/base/class.c              |   22 ++++++++++----------
drivers/base/core.c               |   13 +++++-------
drivers/i2c/i2c-core.c            |    9 +++-----
drivers/ieee1394/nodemgr.c        |   40 +++++++++++++++++++-------------------
drivers/power/apm_power.c         |    6 ++---
drivers/power/power_supply_core.c |    8 +++----
drivers/rtc/interface.c           |    4 +--
drivers/scsi/hosts.c              |    4 +--
drivers/spi/spi.c                 |    4 +--
include/linux/device.h            |    3 +-
10 files changed, 56 insertions(+), 57 deletions(-)

One lockdep warning detected as following, thus use mutex_lock_nested with SINGLE_DEPTH_NESTING in class_device_add

Jan  3 10:45:15 darkstar kernel: =============================================
Jan  3 10:45:15 darkstar kernel: [ INFO: possible recursive locking detected ]
Jan  3 10:45:15 darkstar kernel: 2.6.24-rc6-mm1-mutex #1
Jan  3 10:45:15 darkstar kernel: ---------------------------------------------
Jan  3 10:45:15 darkstar kernel: modprobe/2130 is trying to acquire lock:
Jan  3 10:45:15 darkstar kernel:  (&cls->mutex){--..}, at: [<c02d4ee0>] class_device_add+0x140/0x240
Jan  3 10:45:15 darkstar kernel:
Jan  3 10:45:15 darkstar kernel: but task is already holding lock:
Jan  3 10:45:15 darkstar kernel:  (&cls->mutex){--..}, at: [<c02d52c3>] class_interface_register+0x43/0xf0
Jan  3 10:45:15 darkstar kernel:
Jan  3 10:45:15 darkstar kernel: other info that might help us debug this:
Jan  3 10:45:15 darkstar kernel: 1 lock held by modprobe/2130:
Jan  3 10:45:15 darkstar kernel:  #0:  (&cls->mutex){--..}, at: [<c02d52c3>] class_interface_register+0x43/0xf0
Jan  3 10:45:15 darkstar kernel:
Jan  3 10:45:15 darkstar kernel: stack backtrace:
Jan  3 10:45:15 darkstar kernel: Pid: 2130, comm: modprobe Not tainted 2.6.24-rc6-mm1-mutex #1
Jan  3 10:45:15 darkstar kernel:  [<c0104d2a>] show_trace_log_lvl+0x1a/0x30
Jan  3 10:45:15 darkstar kernel:  [<c0104d52>] show_trace+0x12/0x20
Jan  3 10:45:15 darkstar kernel:  [<c0104ecd>] dump_stack+0x6d/0x80
Jan  3 10:45:15 darkstar kernel:  [<c0152867>] print_deadlock_bug+0xc7/0xe0
Jan  3 10:45:15 darkstar kernel:  [<c01528ec>] check_deadlock+0x6c/0x80
Jan  3 10:45:15 darkstar kernel:  [<c0152d5c>] validate_chain+0x14c/0x370
Jan  3 10:45:15 darkstar kernel:  [<c01548b0>] __lock_acquire+0x1c0/0x7e0
Jan  3 10:45:15 darkstar kernel:  [<c0155599>] lock_acquire+0x79/0xb0
Jan  3 10:45:15 darkstar kernel:  [<c04252cc>] mutex_lock_nested+0x8c/0x300
Jan  3 10:45:15 darkstar kernel:  [<c02d4ee0>] class_device_add+0x140/0x240
Jan  3 10:45:15 darkstar kernel:  [<c02d4ff2>] class_device_register+0x12/0x20
Jan  3 10:45:15 darkstar kernel:  [<c02d509a>] class_device_create+0x9a/0xb0
Jan  3 10:45:15 darkstar kernel:  [<f886225c>] sg_add+0x12c/0x200 [sg]
Jan  3 10:45:15 darkstar kernel:  [<c02d5359>] class_interface_register+0xd9/0xf0
Jan  3 10:45:15 darkstar kernel:  [<c031242f>] scsi_register_interface+0xf/0x20
Jan  3 10:45:15 darkstar kernel:  [<f882b082>] init_sg+0x82/0xbc [sg]
Jan  3 10:45:15 darkstar kernel:  [<c015e4fa>] sys_init_module+0xea/0x130
Jan  3 10:45:15 darkstar kernel:  [<c0103f42>] syscall_call+0x7/0xb
Jan  3 10:45:15 darkstar kernel:  =======================

If there's anything missed please help to point out, thanks.

Regards
dave

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 01:50:20PM +0800, Dave Young wrote:
> Convert semaphore to mutex in struct class.
...
> One lockdep warning detected as following, thus use mutex_lock_nested with SINGLE_DEPTH_NESTING in class_device_add
> 
> Jan  3 10:45:15 darkstar kernel: =============================================
> Jan  3 10:45:15 darkstar kernel: [ INFO: possible recursive locking detected ]
> Jan  3 10:45:15 darkstar kernel: 2.6.24-rc6-mm1-mutex #1
> Jan  3 10:45:15 darkstar kernel: ---------------------------------------------
...
> If there's anything missed please help to point out, thanks.

Dave, IMHO it's not 'the right' way to do it: from this and earlier
discussions it seems there could be many more warnings like this one;
lockdep simply always turns itself off after first one. So, merging
your patches like this would effectively turn off lockdep for all
other places as well, maybe for a long time.

I'd suggest to try first to do it with some wrappers around mutexes,
which simply omit lockdep verification, and later try to replace them
one by one, after checking and testing there are no such warnings
anymore (which would often need some additional annotations about
nesting and probably some changes in lockdep too).

Thanks,
Jarek P.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 08:06:09AM +0100, Jarek Poplawski wrote:
> On Thu, Jan 03, 2008 at 01:50:20PM +0800, Dave Young wrote:
> > Convert semaphore to mutex in struct class.
> ...
> > One lockdep warning detected as following, thus use mutex_lock_nested with SINGLE_DEPTH_NESTING in class_device_add
> > 
> > Jan  3 10:45:15 darkstar kernel: =============================================
> > Jan  3 10:45:15 darkstar kernel: [ INFO: possible recursive locking detected ]
> > Jan  3 10:45:15 darkstar kernel: 2.6.24-rc6-mm1-mutex #1
> > Jan  3 10:45:15 darkstar kernel: ---------------------------------------------
> ...
> > If there's anything missed please help to point out, thanks.
> 
> Dave, IMHO it's not 'the right' way to do it: [...]

OOPS! (I was sleeping...) Unless it has turned out it's not so hard
here, and you are quite sure there should be no more warnings after
this one nesting annotation - then of course, this is the right way!

Sorry (?)
Jarek P.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 3, 2008 3:24 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Thu, Jan 03, 2008 at 08:06:09AM +0100, Jarek Poplawski wrote:
> > On Thu, Jan 03, 2008 at 01:50:20PM +0800, Dave Young wrote:
> > > Convert semaphore to mutex in struct class.
> > ...
> > > One lockdep warning detected as following, thus use mutex_lock_nested with SINGLE_DEPTH_NESTING in class_device_add
> > >
> > > Jan  3 10:45:15 darkstar kernel: =============================================
> > > Jan  3 10:45:15 darkstar kernel: [ INFO: possible recursive locking detected ]
> > > Jan  3 10:45:15 darkstar kernel: 2.6.24-rc6-mm1-mutex #1
> > > Jan  3 10:45:15 darkstar kernel: ---------------------------------------------
> > ...
> > > If there's anything missed please help to point out, thanks.
> >
> > Dave, IMHO it's not 'the right' way to do it: [...]
>
> OOPS! (I was sleeping...) Unless it has turned out it's not so hard
> here, and you are quite sure there should be no more warnings after
> this one nesting annotation - then of course, this is the right way!

Thanks ;)
I don't know if there's other possible warning places with this mutex
or not,  if you have any ideas about this, please tell me.

>
> Sorry (?)
> Jarek P.
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 03:21:36PM +0800, Dave Young wrote:
...
> I don't know if there's other possible warning places with this mutex
> or not,  if you have any ideas about this, please tell me.

I think lockdep is just to tell such things. So, the question is, how
much it was tested already, because if there are many warnings
reported e.g. after merging to -mm, then this could be better to re-do
it this other way... But, I hope this will not be necessary.

Jarek P.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Stefan Richter]

Dave Young wrote:
> Convert semaphore to mutex in struct class.
> All the patches in this series should be applyed simultaneously

Therefore you eventually need to repost it as a single patch.  It can't
go into one of the maintainer trees otherwise, because they only take
fully bijectable patches.  (Kernel must build and run at any point in
between any patch series.)

> toc:
> ---
> 1-driver-core-struct-class-convert-semaphore-to-mutex.patch
> 2-i2c-struct-class-convert-semaphore-to-mutex.patch
> 3-ieee1394-struct-class-convert-semaphore-to-mutex.patch
> 4-power-struct-class-convert-semaphore-to-mutex.patch
> 5-rtc-struct-class-convert-semaphore-to-mutex.patch
> 6-scsi-struct-class-convert-semaphore-to-mutex.patch
> 7-spi-struct-class-convert-semaphore-to-mutex.patch

I was going to test it at runtime on top of 2.6.24-rc6, but the first
and second patch depend on stuff in -mm.  So, my laziness wins for now,
as -mm is not my cup of tea.

About your changelog:

	"xyz: convert semaphore to mutex in struct class

	Use mutex instead of semaphore in struct class.

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

You don't need the second line because it says the same as the first
line.  Either kill it, or replace it by an explanation _why_ the
semaphore is to be replaced by mutex.  (I guess you do it because they
are lighter-weight, both in semantics and in implementation, and because
there are better facilities to debug mutexes...)
-- 
Stefan Richter
-=====-==--- ---= --==-
http://arcgraph.de/sr/

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Sun, Jan 06, 2008 at 07:41:29PM +0100, Stefan Richter wrote:
> Dave Young wrote:
> > Convert semaphore to mutex in struct class.
> > All the patches in this series should be applyed simultaneously
> 
> Therefore you eventually need to repost it as a single patch.  It can't
> go into one of the maintainer trees otherwise, because they only take
> fully bijectable patches.  (Kernel must build and run at any point in
> between any patch series.)
> 
> > toc:
> > ---
> > 1-driver-core-struct-class-convert-semaphore-to-mutex.patch
> > 2-i2c-struct-class-convert-semaphore-to-mutex.patch
> > 3-ieee1394-struct-class-convert-semaphore-to-mutex.patch
> > 4-power-struct-class-convert-semaphore-to-mutex.patch
> > 5-rtc-struct-class-convert-semaphore-to-mutex.patch
> > 6-scsi-struct-class-convert-semaphore-to-mutex.patch
> > 7-spi-struct-class-convert-semaphore-to-mutex.patch
> 
> I was going to test it at runtime on top of 2.6.24-rc6, but the first
> and second patch depend on stuff in -mm.  So, my laziness wins for now,
> as -mm is not my cup of tea.
> 
> About your changelog:
> 
> 	"xyz: convert semaphore to mutex in struct class
> 
> 	Use mutex instead of semaphore in struct class.
> 
> 	Signed-off-by: Dave Young <hidave.darkstar@gmail.com>"
> 
> You don't need the second line because it says the same as the first
> line.  Either kill it, or replace it by an explanation _why_ the
> semaphore is to be replaced by mutex.  (I guess you do it because they
> are lighter-weight, both in semantics and in implementation, and because
> there are better facilities to debug mutexes...)

Thanks for your comment, I rewrite it for 2.6.24-rc7 as a all-in-one patch, please see following. Drop i2c maintainer and list in cc because there's no changes about i2c in this patch:

Convert semaphore to mutex in struct class

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

---
drivers/base/class.c              |   22 ++++++++++----------
drivers/base/core.c               |   18 +++++++----------
drivers/ieee1394/nodemgr.c        |   40 +++++++++++++++++++-------------------
drivers/power/apm_power.c         |    6 ++---
drivers/power/power_supply_core.c |    8 +++----
drivers/rtc/interface.c           |    4 +--
drivers/scsi/hosts.c              |    4 +--
drivers/spi/spi.c                 |    4 +--
include/linux/device.h            |    3 +-
9 files changed, 54 insertions(+), 55 deletions(-)

diff -upr a/linux-2.6.24-rc7/drivers/base/class.c linux-2.6.24-rc7/drivers/base/class.c
--- a/linux-2.6.24-rc7/drivers/base/class.c	2008-01-07 08:56:05.000000000 +0800
+++ linux-2.6.24-rc7/drivers/base/class.c	2008-01-07 09:12:49.000000000 +0800
@@ -144,7 +144,7 @@ int class_register(struct class * cls)
 	INIT_LIST_HEAD(&cls->devices);
 	INIT_LIST_HEAD(&cls->interfaces);
 	kset_init(&cls->class_dirs);
-	init_MUTEX(&cls->sem);
+	mutex_init(&cls->mutex);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -617,13 +617,13 @@ int class_device_add(struct class_device
 	kobject_uevent(&class_dev->kobj, KOBJ_ADD);
 
 	/* notify any interfaces this device is now here */
-	down(&parent_class->sem);
+	mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
 	list_add_tail(&class_dev->node, &parent_class->children);
 	list_for_each_entry(class_intf, &parent_class->interfaces, node) {
 		if (class_intf->add)
 			class_intf->add(class_dev, class_intf);
 	}
-	up(&parent_class->sem);
+	mutex_unlock(&parent_class->mutex);
 
 	goto out1;
 
@@ -725,12 +725,12 @@ void class_device_del(struct class_devic
 	struct class_interface *class_intf;
 
 	if (parent_class) {
-		down(&parent_class->sem);
+		mutex_lock(&parent_class->mutex);
 		list_del_init(&class_dev->node);
 		list_for_each_entry(class_intf, &parent_class->interfaces, node)
 			if (class_intf->remove)
 				class_intf->remove(class_dev, class_intf);
-		up(&parent_class->sem);
+		mutex_unlock(&parent_class->mutex);
 	}
 
 	if (class_dev->dev) {
@@ -772,14 +772,14 @@ void class_device_destroy(struct class *
 	struct class_device *class_dev = NULL;
 	struct class_device *class_dev_tmp;
 
-	down(&cls->sem);
+	mutex_lock(&cls->mutex);
 	list_for_each_entry(class_dev_tmp, &cls->children, node) {
 		if (class_dev_tmp->devt == devt) {
 			class_dev = class_dev_tmp;
 			break;
 		}
 	}
-	up(&cls->sem);
+	mutex_unlock(&cls->mutex);
 
 	if (class_dev)
 		class_device_unregister(class_dev);
@@ -812,7 +812,7 @@ int class_interface_register(struct clas
 	if (!parent)
 		return -EINVAL;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_add_tail(&class_intf->node, &parent->interfaces);
 	if (class_intf->add) {
 		list_for_each_entry(class_dev, &parent->children, node)
@@ -822,7 +822,7 @@ int class_interface_register(struct clas
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->add_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	return 0;
 }
@@ -836,7 +836,7 @@ void class_interface_unregister(struct c
 	if (!parent)
 		return;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_del_init(&class_intf->node);
 	if (class_intf->remove) {
 		list_for_each_entry(class_dev, &parent->children, node)
@@ -846,7 +846,7 @@ void class_interface_unregister(struct c
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->remove_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	class_put(parent);
 }
diff -upr a/linux-2.6.24-rc7/drivers/base/core.c linux-2.6.24-rc7/drivers/base/core.c
--- a/linux-2.6.24-rc7/drivers/base/core.c	2008-01-07 08:56:05.000000000 +0800
+++ linux-2.6.24-rc7/drivers/base/core.c	2008-01-07 09:39:11.000000000 +0800
@@ -19,8 +19,6 @@
 #include <linux/kdev_t.h>
 #include <linux/notifier.h>
 
-#include <asm/semaphore.h>
-
 #include "base.h"
 #include "power/power.h"
 
@@ -783,7 +781,7 @@ int device_add(struct device *dev)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
 	if (dev->class) {
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* tie the class to the device */
 		list_add_tail(&dev->node, &dev->class->devices);
 
@@ -791,7 +789,7 @@ int device_add(struct device *dev)
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->add_dev)
 				class_intf->add_dev(dev, class_intf);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
  Done:
 	put_device(dev);
@@ -928,14 +926,14 @@ void device_del(struct device * dev)
 			sysfs_remove_link(&dev->kobj, "device");
 		}
 
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* notify any interfaces that the device is now gone */
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->remove_dev)
 				class_intf->remove_dev(dev, class_intf);
 		/* remove the device from the class list */
 		list_del_init(&dev->node);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 
 		/* If we live in a parent class-directory, unreference it */
 		if (dev->kobj.parent->kset == &dev->class->class_dirs) {
@@ -946,7 +944,7 @@ void device_del(struct device * dev)
 			 * if we are the last child of our class, delete
 			 * our class-directory at this parent
 			 */
-			down(&dev->class->sem);
+			mutex_lock(&dev->class->mutex);
 			list_for_each_entry(d, &dev->class->devices, node) {
 				if (d == dev)
 					continue;
@@ -959,7 +957,7 @@ void device_del(struct device * dev)
 				kobject_del(dev->kobj.parent);
 
 			kobject_put(dev->kobj.parent);
-			up(&dev->class->sem);
+			mutex_unlock(&dev->class->mutex);
 		}
 	}
 	device_remove_file(dev, &uevent_attr);
@@ -1168,14 +1166,14 @@ void device_destroy(struct class *class,
 	struct device *dev = NULL;
 	struct device *dev_tmp;
 
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev_tmp, &class->devices, node) {
 		if (dev_tmp->devt == devt) {
 			dev = dev_tmp;
 			break;
 		}
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	if (dev)
 		device_unregister(dev);
diff -upr a/linux-2.6.24-rc7/drivers/ieee1394/nodemgr.c linux-2.6.24-rc7/drivers/ieee1394/nodemgr.c
--- a/linux-2.6.24-rc7/drivers/ieee1394/nodemgr.c	2008-01-07 08:56:06.000000000 +0800
+++ linux-2.6.24-rc7/drivers/ieee1394/nodemgr.c	2008-01-07 09:26:47.000000000 +0800
@@ -733,16 +733,16 @@ static void nodemgr_remove_uds(struct no
 	struct unit_directory *tmp, *ud;
 
 	/* Iteration over nodemgr_ud_class.devices has to be protected by
-	 * nodemgr_ud_class.sem, but device_unregister() will eventually
-	 * take nodemgr_ud_class.sem too. Therefore pick out one ud at a time,
-	 * release the semaphore, and then unregister the ud. Since this code
+	 * nodemgr_ud_class.mutex, but device_unregister() will eventually
+	 * take nodemgr_ud_class.mutex too. Therefore pick out one ud at a time,
+	 * unlock the mutex, and then unregister the ud. Since this code
 	 * may be called from other contexts besides the knodemgrds, protect the
-	 * gap after release of the semaphore by nodemgr_serialize_remove_uds.
+	 * gap after unlock of the mutex by nodemgr_serialize_remove_uds.
 	 */
 	mutex_lock(&nodemgr_serialize_remove_uds);
 	for (;;) {
 		ud = NULL;
-		down(&nodemgr_ud_class.sem);
+		mutex_lock(&nodemgr_ud_class.mutex);
 		list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
 			tmp = container_of(dev, struct unit_directory,
 					   unit_dev);
@@ -751,7 +751,7 @@ static void nodemgr_remove_uds(struct no
 				break;
 			}
 		}
-		up(&nodemgr_ud_class.sem);
+		mutex_unlock(&nodemgr_ud_class.mutex);
 		if (ud == NULL)
 			break;
 		device_unregister(&ud->unit_dev);
@@ -888,7 +888,7 @@ static struct node_entry *find_entry_by_
 	struct device *dev;
 	struct node_entry *ne, *ret_ne = NULL;
 
-	down(&nodemgr_ne_class.sem);
+	mutex_lock(&nodemgr_ne_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
 		ne = container_of(dev, struct node_entry, node_dev);
 
@@ -897,7 +897,7 @@ static struct node_entry *find_entry_by_
 			break;
 		}
 	}
-	up(&nodemgr_ne_class.sem);
+	mutex_unlock(&nodemgr_ne_class.mutex);
 
 	return ret_ne;
 }
@@ -909,7 +909,7 @@ static struct node_entry *find_entry_by_
 	struct device *dev;
 	struct node_entry *ne, *ret_ne = NULL;
 
-	down(&nodemgr_ne_class.sem);
+	mutex_lock(&nodemgr_ne_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
 		ne = container_of(dev, struct node_entry, node_dev);
 
@@ -918,7 +918,7 @@ static struct node_entry *find_entry_by_
 			break;
 		}
 	}
-	up(&nodemgr_ne_class.sem);
+	mutex_unlock(&nodemgr_ne_class.mutex);
 
 	return ret_ne;
 }
@@ -1384,7 +1384,7 @@ static void nodemgr_suspend_ne(struct no
 	ne->in_limbo = 1;
 	WARN_ON(device_create_file(&ne->device, &dev_attr_ne_in_limbo));
 
-	down(&nodemgr_ud_class.sem);
+	mutex_lock(&nodemgr_ud_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
 		ud = container_of(dev, struct unit_directory, unit_dev);
 		if (ud->ne != ne)
@@ -1404,7 +1404,7 @@ static void nodemgr_suspend_ne(struct no
 			device_release_driver(&ud->device);
 		put_driver(drv);
 	}
-	up(&nodemgr_ud_class.sem);
+	mutex_unlock(&nodemgr_ud_class.mutex);
 }
 
 
@@ -1417,7 +1417,7 @@ static void nodemgr_resume_ne(struct nod
 	ne->in_limbo = 0;
 	device_remove_file(&ne->device, &dev_attr_ne_in_limbo);
 
-	down(&nodemgr_ud_class.sem);
+	mutex_lock(&nodemgr_ud_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
 		ud = container_of(dev, struct unit_directory, unit_dev);
 		if (ud->ne != ne)
@@ -1434,7 +1434,7 @@ static void nodemgr_resume_ne(struct nod
 		}
 		put_driver(drv);
 	}
-	up(&nodemgr_ud_class.sem);
+	mutex_unlock(&nodemgr_ud_class.mutex);
 
 	HPSB_DEBUG("Node resumed: ID:BUS[" NODE_BUS_FMT "]  GUID[%016Lx]",
 		   NODE_BUS_ARGS(ne->host, ne->nodeid), (unsigned long long)ne->guid);
@@ -1449,7 +1449,7 @@ static void nodemgr_update_pdrv(struct n
 	struct hpsb_protocol_driver *pdrv;
 	int error;
 
-	down(&nodemgr_ud_class.sem);
+	mutex_lock(&nodemgr_ud_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ud_class.devices, node) {
 		ud = container_of(dev, struct unit_directory, unit_dev);
 		if (ud->ne != ne)
@@ -1470,7 +1470,7 @@ static void nodemgr_update_pdrv(struct n
 			device_release_driver(&ud->device);
 		put_driver(drv);
 	}
-	up(&nodemgr_ud_class.sem);
+	mutex_unlock(&nodemgr_ud_class.mutex);
 }
 
 
@@ -1545,7 +1545,7 @@ static void nodemgr_node_probe(struct ho
 	 * while probes are time-consuming. (Well, those probes need some
 	 * improvement...) */
 
-	down(&nodemgr_ne_class.sem);
+	mutex_lock(&nodemgr_ne_class.mutex);
 	list_for_each_entry(dev, &nodemgr_ne_class.devices, node) {
 		ne = container_of(dev, struct node_entry, node_dev);
 		if (!ne->needs_probe)
@@ -1556,7 +1556,7 @@ static void nodemgr_node_probe(struct ho
 		if (ne->needs_probe)
 			nodemgr_probe_ne(hi, ne, generation);
 	}
-	up(&nodemgr_ne_class.sem);
+	mutex_unlock(&nodemgr_ne_class.mutex);
 
 
 	/* If we had a bus reset while we were scanning the bus, it is
@@ -1775,14 +1775,14 @@ int nodemgr_for_each_host(void *data, in
 	struct hpsb_host *host;
 	int error = 0;
 
-	down(&hpsb_host_class.sem);
+	mutex_lock(&hpsb_host_class.mutex);
 	list_for_each_entry(dev, &hpsb_host_class.devices, node) {
 		host = container_of(dev, struct hpsb_host, host_dev);
 
 		if ((error = cb(host, data)))
 			break;
 	}
-	up(&hpsb_host_class.sem);
+	mutex_unlock(&hpsb_host_class.mutex);
 
 	return error;
 }
diff -upr a/linux-2.6.24-rc7/drivers/power/apm_power.c linux-2.6.24-rc7/drivers/power/apm_power.c
--- a/linux-2.6.24-rc7/drivers/power/apm_power.c	2008-01-07 08:56:08.000000000 +0800
+++ linux-2.6.24-rc7/drivers/power/apm_power.c	2008-01-07 09:27:05.000000000 +0800
@@ -207,10 +207,10 @@ static void apm_battery_apm_get_power_st
 	union power_supply_propval status;
 	union power_supply_propval capacity, time_to_full, time_to_empty;
 
-	down(&power_supply_class->sem);
+	mutex_lock(&power_supply_class->mutex);
 	find_main_battery();
 	if (!main_battery) {
-		up(&power_supply_class->sem);
+		mutex_unlock(&power_supply_class->mutex);
 		return;
 	}
 
@@ -278,7 +278,7 @@ static void apm_battery_apm_get_power_st
 		}
 	}
 
-	up(&power_supply_class->sem);
+	mutex_unlock(&power_supply_class->mutex);
 }
 
 static int __init apm_battery_init(void)
diff -upr a/linux-2.6.24-rc7/drivers/power/power_supply_core.c linux-2.6.24-rc7/drivers/power/power_supply_core.c
--- a/linux-2.6.24-rc7/drivers/power/power_supply_core.c	2007-10-10 04:31:38.000000000 +0800
+++ linux-2.6.24-rc7/drivers/power/power_supply_core.c	2008-01-07 09:27:05.000000000 +0800
@@ -31,7 +31,7 @@ static void power_supply_changed_work(st
 	for (i = 0; i < psy->num_supplicants; i++) {
 		struct device *dev;
 
-		down(&power_supply_class->sem);
+		mutex_lock(&power_supply_class->mutex);
 		list_for_each_entry(dev, &power_supply_class->devices, node) {
 			struct power_supply *pst = dev_get_drvdata(dev);
 
@@ -40,7 +40,7 @@ static void power_supply_changed_work(st
 					pst->external_power_changed(pst);
 			}
 		}
-		up(&power_supply_class->sem);
+		mutex_unlock(&power_supply_class->mutex);
 	}
 
 	power_supply_update_leds(psy);
@@ -60,7 +60,7 @@ int power_supply_am_i_supplied(struct po
 	union power_supply_propval ret = {0,};
 	struct device *dev;
 
-	down(&power_supply_class->sem);
+	mutex_lock(&power_supply_class->mutex);
 	list_for_each_entry(dev, &power_supply_class->devices, node) {
 		struct power_supply *epsy = dev_get_drvdata(dev);
 		int i;
@@ -76,7 +76,7 @@ int power_supply_am_i_supplied(struct po
 		}
 	}
 out:
-	up(&power_supply_class->sem);
+	mutex_unlock(&power_supply_class->mutex);
 
 	dev_dbg(psy->dev, "%s %d\n", __FUNCTION__, ret.intval);
 
diff -upr a/linux-2.6.24-rc7/drivers/rtc/interface.c linux-2.6.24-rc7/drivers/rtc/interface.c
--- a/linux-2.6.24-rc7/drivers/rtc/interface.c	2008-01-07 08:56:08.000000000 +0800
+++ linux-2.6.24-rc7/drivers/rtc/interface.c	2008-01-07 09:27:11.000000000 +0800
@@ -256,7 +256,7 @@ struct rtc_device *rtc_class_open(char *
 	struct device *dev;
 	struct rtc_device *rtc = NULL;
 
-	down(&rtc_class->sem);
+	mutex_lock(&rtc_class->mutex);
 	list_for_each_entry(dev, &rtc_class->devices, node) {
 		if (strncmp(dev->bus_id, name, BUS_ID_SIZE) == 0) {
 			dev = get_device(dev);
@@ -272,7 +272,7 @@ struct rtc_device *rtc_class_open(char *
 			rtc = NULL;
 		}
 	}
-	up(&rtc_class->sem);
+	mutex_unlock(&rtc_class->mutex);
 
 	return rtc;
 }
diff -upr a/linux-2.6.24-rc7/drivers/scsi/hosts.c linux-2.6.24-rc7/drivers/scsi/hosts.c
--- a/linux-2.6.24-rc7/drivers/scsi/hosts.c	2008-01-07 08:56:09.000000000 +0800
+++ linux-2.6.24-rc7/drivers/scsi/hosts.c	2008-01-07 09:25:42.000000000 +0800
@@ -443,7 +443,7 @@ struct Scsi_Host *scsi_host_lookup(unsig
 	struct class_device *cdev;
 	struct Scsi_Host *shost = ERR_PTR(-ENXIO), *p;
 
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(cdev, &class->children, node) {
 		p = class_to_shost(cdev);
 		if (p->host_no == hostnum) {
@@ -451,7 +451,7 @@ struct Scsi_Host *scsi_host_lookup(unsig
 			break;
 		}
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return shost;
 }
diff -upr a/linux-2.6.24-rc7/drivers/spi/spi.c linux-2.6.24-rc7/drivers/spi/spi.c
--- a/linux-2.6.24-rc7/drivers/spi/spi.c	2008-01-07 08:56:09.000000000 +0800
+++ linux-2.6.24-rc7/drivers/spi/spi.c	2008-01-07 09:26:30.000000000 +0800
@@ -501,7 +501,7 @@ struct spi_master *spi_busnum_to_master(
 	struct spi_master	*master = NULL;
 	struct spi_master	*m;
 
-	down(&spi_master_class.sem);
+	mutex_lock(&spi_master_class.mutex);
 	list_for_each_entry(dev, &spi_master_class.children, node) {
 		m = container_of(dev, struct spi_master, dev);
 		if (m->bus_num == bus_num) {
@@ -509,7 +509,7 @@ struct spi_master *spi_busnum_to_master(
 			break;
 		}
 	}
-	up(&spi_master_class.sem);
+	mutex_unlock(&spi_master_class.mutex);
 	return master;
 }
 EXPORT_SYMBOL_GPL(spi_busnum_to_master);
diff -upr a/linux-2.6.24-rc7/include/linux/device.h linux-2.6.24-rc7/include/linux/device.h
--- a/linux-2.6.24-rc7/include/linux/device.h	2008-01-07 08:56:11.000000000 +0800
+++ linux-2.6.24-rc7/include/linux/device.h	2008-01-07 09:19:29.000000000 +0800
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/mutex.h>
 #include <asm/semaphore.h>
 #include <asm/atomic.h>
 #include <asm/device.h>
@@ -180,7 +181,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 mutex		mutex;	/* locks both the children and interfaces lists */
 
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Greg KH]

On Mon, Jan 07, 2008 at 10:09:44AM +0800, Dave Young wrote:
> 
> Thanks for your comment, I rewrite it for 2.6.24-rc7 as a all-in-one
> patch, please see following. Drop i2c maintainer and list in cc
> because there's no changes about i2c in this patch:
> 
> Convert semaphore to mutex in struct class

Most of the non-driver core code should be converted to not use the
lock in the class at all.  They should use a local lock instead.

So how about a patch series that removes the usage of the current
semaphore from the different drivers, and then, a single patch that
converts the semaphore to mutex in the class code, it should not touch
anything outside of the drivers/base/ directory (not including the
driver.h file.)

Sound reasonable?

thanks,

greg k-h

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[David Brownell]

On Monday 07 January 2008, Greg KH wrote:
> Most of the non-driver core code should be converted to not use the
> lock in the class at all.  They should use a local lock instead.

Or better yet, that yet-to-be-written class_for_each_instance()
iterator ... :)

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Stefan Richter]

David Brownell wrote:
> On Monday 07 January 2008, Greg KH wrote:
>> Most of the non-driver core code should be converted to not use the
>> lock in the class at all.  They should use a local lock instead.
> 
> Or better yet, that yet-to-be-written class_for_each_instance()
> iterator ... :)

By far most of the usages of class.semaphore or class.mutex in drivers
are to protect the class.devices list.  The only¹ right thing to do
there is to keep using the class.{semaphore,mutex}.  The more elegant
variation of this would be David's class_for_each_instance() iterator
which would allow us to hide the locking details from the drivers.

-------
¹)  Well, another correct thing to do would be to not take any locks or
mutexes in the driver core but instead let the drivers do the necessary
serialization between calls like class_device_add() and the likes and
list iterations.  But this would complicate the API because of the
additional locking requirements, and hence would invariably result in
buggy usages of the API.
-- 
Stefan Richter
-=====-==--- ---= --===
http://arcgraph.de/sr/

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Jarek Poplawski]

On Mon, Jan 07, 2008 at 02:23:33PM +0100, Stefan Richter wrote:
> David Brownell wrote:
> > On Monday 07 January 2008, Greg KH wrote:
> >> Most of the non-driver core code should be converted to not use the
> >> lock in the class at all.  They should use a local lock instead.
> > 
> > Or better yet, that yet-to-be-written class_for_each_instance()
> > iterator ... :)
> 
> By far most of the usages of class.semaphore or class.mutex in drivers
> are to protect the class.devices list.  The only? right thing to do
> there is to keep using the class.{semaphore,mutex}.  The more elegant
> variation of this would be David's class_for_each_instance() iterator
> which would allow us to hide the locking details from the drivers.
> 
> -------
> ?)  Well, another correct thing to do would be to not take any locks or
> mutexes in the driver core but instead let the drivers do the necessary
> serialization between calls like class_device_add() and the likes and
> list iterations.  But this would complicate the API because of the
> additional locking requirements, and hence would invariably result in
> buggy usages of the API.
> -- 

I hope I'm wrong, but IMHO it should be safer not to mix such changes,
so do the mutexes first or delay them until the end. Otherwise some
false blaming seems almost inevitable.

Regards,
Jarek P.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Stefan Richter]

Jarek Poplawski wrote:
>> David Brownell wrote:
>> > On Monday 07 January 2008, Greg KH wrote:
>> >> Most of the non-driver core code should be converted to not use the
>> >> lock in the class at all.  They should use a local lock instead.
>> > 
>> > Or better yet, that yet-to-be-written class_for_each_instance()
>> > iterator ... :)
...
> I hope I'm wrong, but IMHO it should be safer not to mix such changes,
> so do the mutexes first or delay them until the end. Otherwise some
> false blaming seems almost inevitable.

I agree.  Sem2mutex conversion should not be mixed with API conversion,
even if one or both seem trivial.
-- 
Stefan Richter
-=====-==--- ---= --===
http://arcgraph.de/sr/

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Greg KH]

On Mon, Jan 07, 2008 at 02:23:33PM +0100, Stefan Richter wrote:
> David Brownell wrote:
> > On Monday 07 January 2008, Greg KH wrote:
> >> Most of the non-driver core code should be converted to not use the
> >> lock in the class at all.  They should use a local lock instead.
> > 
> > Or better yet, that yet-to-be-written class_for_each_instance()
> > iterator ... :)
> 
> By far most of the usages of class.semaphore or class.mutex in drivers
> are to protect the class.devices list.  The only? right thing to do
> there is to keep using the class.{semaphore,mutex}.  The more elegant
> variation of this would be David's class_for_each_instance() iterator
> which would allow us to hide the locking details from the drivers.

If such functionality is needed, fine, I have no objection to that
change to move the locking logic into the driver core.

thanks,

greg k-h

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Stefan Richter]

Greg KH wrote:
> On Mon, Jan 07, 2008 at 02:23:33PM +0100, Stefan Richter wrote:
>> David Brownell wrote:
>> > On Monday 07 January 2008, Greg KH wrote:
>> >> Most of the non-driver core code should be converted to not use the
>> >> lock in the class at all.  They should use a local lock instead.
>> > 
>> > Or better yet, that yet-to-be-written class_for_each_instance()
>> > iterator ... :)
>> 
>> By far most of the usages of class.semaphore or class.mutex in drivers
>> are to protect the class.devices list.  The only? right thing to do
>> there is to keep using the class.{semaphore,mutex}.  The more elegant
>> variation of this would be David's class_for_each_instance() iterator
>> which would allow us to hide the locking details from the drivers.
> 
> If such functionality is needed, fine, I have no objection to that
> change to move the locking logic into the driver core.

It's already in the driver core to the most part.  It remains to be seen
what is less complicated in the end:  Transparent mutex-protected list
accesses provided by driver core (requires the iterator), or all the
necessary locking done by the drivers themselves (requires some more
lock-taking but perhaps fewer lock instances overall in the drivers, and
respective redefinitions and documentation of the driver core API).

Semi off-topic:  What about struct device.sem?  Is there any chance to
rip this out of the driver core and let drivers serialize everything?  I
suppose not...
-- 
Stefan Richter
-=====-==--- ---= --===
http://arcgraph.de/sr/

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Greg KH]

On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> It's already in the driver core to the most part.  It remains to be seen
> what is less complicated in the end:  Transparent mutex-protected list
> accesses provided by driver core (requires the iterator), or all the
> necessary locking done by the drivers themselves (requires some more
> lock-taking but perhaps fewer lock instances overall in the drivers, and
> respective redefinitions and documentation of the driver core API).

I favor changing the driver core api and doing this kind of thing there.
It keeps the drivers simpler and should hopefully make their lives
easier.

> Semi off-topic:  What about struct device.sem?  Is there any chance to
> rip this out of the driver core and let drivers serialize everything?  I
> suppose not...

See the previous long threads about this very topic, that is what caused
this class.sem patches :)

thanks,

greg k-h

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > It's already in the driver core to the most part.  It remains to be seen
> > what is less complicated in the end:  Transparent mutex-protected list
> > accesses provided by driver core (requires the iterator), or all the
> > necessary locking done by the drivers themselves (requires some more
> > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > respective redefinitions and documentation of the driver core API).
>
> I favor changing the driver core api and doing this kind of thing there.
> It keeps the drivers simpler and should hopefully make their lives
> easier.

What about this?

#define class_for_each_dev(pos, head, member) \
        for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
s = list_entry((head)->next, typeof(*pos), member); \
        prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
(container_of(head, struct class, devices))->mutex), 0); \
        pos = list_entry(pos->member.next, typeof(*pos), member))

>
> > Semi off-topic:  What about struct device.sem?  Is there any chance to
> > rip this out of the driver core and let drivers serialize everything?  I
> > suppose not...
>
> See the previous long threads about this very topic, that is what caused
> this class.sem patches :)
>
> thanks,
>
> greg k-h
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Greg KH]

On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > It's already in the driver core to the most part.  It remains to be seen
> > > what is less complicated in the end:  Transparent mutex-protected list
> > > accesses provided by driver core (requires the iterator), or all the
> > > necessary locking done by the drivers themselves (requires some more
> > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > respective redefinitions and documentation of the driver core API).
> >
> > I favor changing the driver core api and doing this kind of thing there.
> > It keeps the drivers simpler and should hopefully make their lives
> > easier.
> 
> What about this?
> 
> #define class_for_each_dev(pos, head, member) \
>         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> s = list_entry((head)->next, typeof(*pos), member); \
>         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> (container_of(head, struct class, devices))->mutex), 0); \
>         pos = list_entry(pos->member.next, typeof(*pos), member))

Eeek, just make the thing a function please, where you pass the iterator
function in, like the driver core has (driver_for_each_device)

thanks,

greg k-h

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 9, 2008 6:48 AM, Greg KH <gregkh@suse.de> wrote:
> On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > > It's already in the driver core to the most part.  It remains to be seen
> > > > what is less complicated in the end:  Transparent mutex-protected list
> > > > accesses provided by driver core (requires the iterator), or all the
> > > > necessary locking done by the drivers themselves (requires some more
> > > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > > respective redefinitions and documentation of the driver core API).
> > >
> > > I favor changing the driver core api and doing this kind of thing there.
> > > It keeps the drivers simpler and should hopefully make their lives
> > > easier.
> >
> > What about this?
> >
> > #define class_for_each_dev(pos, head, member) \
> >         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> > s = list_entry((head)->next, typeof(*pos), member); \
> >         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> > (container_of(head, struct class, devices))->mutex), 0); \
> >         pos = list_entry(pos->member.next, typeof(*pos), member))
>
I'm wrong, it's same as before indeed.

> Eeek, just make the thing a function please, where you pass the iterator
> function in, like the driver core has (driver_for_each_device)

Ok, so need a new member of knode_class, I will update the patch later.
Thanks.

>
> thanks,
>
> greg k-h
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> On Jan 9, 2008 6:48 AM, Greg KH <gregkh@suse.de> wrote:
> > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > > > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > > > It's already in the driver core to the most part.  It remains to be seen
> > > > > what is less complicated in the end:  Transparent mutex-protected list
> > > > > accesses provided by driver core (requires the iterator), or all the
> > > > > necessary locking done by the drivers themselves (requires some more
> > > > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > > > respective redefinitions and documentation of the driver core API).
> > > >
> > > > I favor changing the driver core api and doing this kind of thing there.
> > > > It keeps the drivers simpler and should hopefully make their lives
> > > > easier.
> > >
> > > What about this?
> > >
> > > #define class_for_each_dev(pos, head, member) \
> > >         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> > > s = list_entry((head)->next, typeof(*pos), member); \
> > >         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> > > (container_of(head, struct class, devices))->mutex), 0); \
> > >         pos = list_entry(pos->member.next, typeof(*pos), member))
> >
> I'm wrong, it's same as before indeed.
> 
> > Eeek, just make the thing a function please, where you pass the iterator
> > function in, like the driver core has (driver_for_each_device)
> 
> Ok, so need a new member of knode_class, I will update the patch later.
> Thanks.

Withdraw my post, sorry :)

For now the mutex patch, I will only use the mutex to lock the devices list and write an iterater function. 
Most of the iterating is for finding some device in the list, so maybe need a match function just like drivers do?

Regards
dave
> 
> >
> > thanks,
> >
> > greg k-h
> >

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 9, 2008 2:13 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>
> On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > On Jan 9, 2008 6:48 AM, Greg KH <gregkh@suse.de> wrote:
> > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > > > > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > > > > It's already in the driver core to the most part.  It remains to be seen
> > > > > > what is less complicated in the end:  Transparent mutex-protected list
> > > > > > accesses provided by driver core (requires the iterator), or all the
> > > > > > necessary locking done by the drivers themselves (requires some more
> > > > > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > > > > respective redefinitions and documentation of the driver core API).
> > > > >
> > > > > I favor changing the driver core api and doing this kind of thing there.
> > > > > It keeps the drivers simpler and should hopefully make their lives
> > > > > easier.
> > > >
> > > > What about this?
> > > >
> > > > #define class_for_each_dev(pos, head, member) \
> > > >         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> > > > s = list_entry((head)->next, typeof(*pos), member); \
> > > >         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> > > > (container_of(head, struct class, devices))->mutex), 0); \
> > > >         pos = list_entry(pos->member.next, typeof(*pos), member))
> > >
> > I'm wrong, it's same as before indeed.
> >
> > > Eeek, just make the thing a function please, where you pass the iterator
> > > function in, like the driver core has (driver_for_each_device)
> >
> > Ok, so need a new member of knode_class, I will update the patch later.
> > Thanks.
>
> Withdraw my post, sorry :)
>
> For now the mutex patch, I will only use the mutex to lock the devices list and write an iterater function.
> Most of the iterating is for finding some device in the list, so maybe need a match function just like drivers do?
>

Drop one more mail address of David Brownell in cc list.
Sorry for this, david

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 9, 2008 2:37 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>
> On Jan 9, 2008 2:13 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> >
> > On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > > On Jan 9, 2008 6:48 AM, Greg KH <gregkh@suse.de> wrote:
> > > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > > On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > > > > > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > > > > > It's already in the driver core to the most part.  It remains to be seen
> > > > > > > what is less complicated in the end:  Transparent mutex-protected list
> > > > > > > accesses provided by driver core (requires the iterator), or all the
> > > > > > > necessary locking done by the drivers themselves (requires some more
> > > > > > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > > > > > respective redefinitions and documentation of the driver core API).
> > > > > >
> > > > > > I favor changing the driver core api and doing this kind of thing there.
> > > > > > It keeps the drivers simpler and should hopefully make their lives
> > > > > > easier.
> > > > >
> > > > > What about this?
> > > > >
> > > > > #define class_for_each_dev(pos, head, member) \
> > > > >         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> > > > > s = list_entry((head)->next, typeof(*pos), member); \
> > > > >         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> > > > > (container_of(head, struct class, devices))->mutex), 0); \
> > > > >         pos = list_entry(pos->member.next, typeof(*pos), member))
> > > >
> > > I'm wrong, it's same as before indeed.
> > >
> > > > Eeek, just make the thing a function please, where you pass the iterator
> > > > function in, like the driver core has (driver_for_each_device)
> > >
> > > Ok, so need a new member of knode_class, I will update the patch later.
> > > Thanks.
> >
> > Withdraw my post, sorry :)
> >
> > For now the mutex patch, I will only use the mutex to lock the devices list and write an iterater function.
> > Most of the iterating is for finding some device in the list, so maybe need a match function just like drivers do?
> >
>
> Drop one more mail address of David Brownell in cc list.
> Sorry for this, david
>
gmail web client make me crazy.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Wed, Jan 09, 2008 at 02:39:23PM +0800, Dave Young wrote:
> On Jan 9, 2008 2:37 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> >
> > On Jan 9, 2008 2:13 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> > >
> > > On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > > > On Jan 9, 2008 6:48 AM, Greg KH <gregkh@suse.de> wrote:
> > > > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > > > On Jan 8, 2008 1:20 AM, Greg KH <gregkh@suse.de> wrote:
> > > > > > > On Mon, Jan 07, 2008 at 06:13:37PM +0100, Stefan Richter wrote:
> > > > > > > > It's already in the driver core to the most part.  It remains to be seen
> > > > > > > > what is less complicated in the end:  Transparent mutex-protected list
> > > > > > > > accesses provided by driver core (requires the iterator), or all the
> > > > > > > > necessary locking done by the drivers themselves (requires some more
> > > > > > > > lock-taking but perhaps fewer lock instances overall in the drivers, and
> > > > > > > > respective redefinitions and documentation of the driver core API).
> > > > > > >
> > > > > > > I favor changing the driver core api and doing this kind of thing there.
> > > > > > > It keeps the drivers simpler and should hopefully make their lives
> > > > > > > easier.
> > > > > >
> > > > > > What about this?
> > > > > >
> > > > > > #define class_for_each_dev(pos, head, member) \
> > > > > >         for (mutex_lock(&(container_of(head, struct class, devices))->mutex), po
> > > > > > s = list_entry((head)->next, typeof(*pos), member); \
> > > > > >         prefetch(pos->member.next), &pos->member != (head) ? 1 : (mutex_unlock(&
> > > > > > (container_of(head, struct class, devices))->mutex), 0); \
> > > > > >         pos = list_entry(pos->member.next, typeof(*pos), member))
> > > > >
> > > > I'm wrong, it's same as before indeed.
> > > >
> > > > > Eeek, just make the thing a function please, where you pass the iterator
> > > > > function in, like the driver core has (driver_for_each_device)
> > > >
> > > > Ok, so need a new member of knode_class, I will update the patch later.
> > > > Thanks.
> > >
> > > Withdraw my post, sorry :)
> > >
> > > For now the mutex patch, I will only use the mutex to lock the devices list and write an iterater function.
> > > Most of the iterating is for finding some device in the list, so maybe need a match function just like drivers do?
> > >
> >
> > Drop one more mail address of David Brownell in cc list.
> > Sorry for this, david
> >
> gmail web client make me crazy.
Hi,
The patches are done on my side, please help to check. 

This is the first one of the series about driver core changes. 
If this one is accepted and there's no other problem I will post the others for maintainer's review (they need your comment and help because I don't know well about the specific driver logic).

Thanks a lot in advance.
---

1. convert class semaphore to mutex.
2. add class iterater functions to encapsulate the detail of class devices/children list iterating :
	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   |   98 +++++++++++++++++++++++++++++++++++++++++++------
 drivers/base/core.c    |   18 ++++-----
 include/linux/device.h |   11 +++++
 3 files changed, 105 insertions(+), 22 deletions(-)

diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c	2008-01-10 17:17:11.000000000 +0800
+++ linux.new/drivers/base/class.c	2008-01-10 17:17:11.000000000 +0800
@@ -144,7 +144,7 @@ int class_register(struct class * cls)
 	INIT_LIST_HEAD(&cls->devices);
 	INIT_LIST_HEAD(&cls->interfaces);
 	kset_init(&cls->class_dirs);
-	init_MUTEX(&cls->sem);
+	mutex_init(&cls->mutex);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -617,13 +617,13 @@ int class_device_add(struct class_device
 	kobject_uevent(&class_dev->kobj, KOBJ_ADD);
 
 	/* notify any interfaces this device is now here */
-	down(&parent_class->sem);
+	mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
 	list_add_tail(&class_dev->node, &parent_class->children);
 	list_for_each_entry(class_intf, &parent_class->interfaces, node) {
 		if (class_intf->add)
 			class_intf->add(class_dev, class_intf);
 	}
-	up(&parent_class->sem);
+	mutex_unlock(&parent_class->mutex);
 
 	goto out1;
 
@@ -725,12 +725,12 @@ void class_device_del(struct class_devic
 	struct class_interface *class_intf;
 
 	if (parent_class) {
-		down(&parent_class->sem);
+		mutex_lock(&parent_class->mutex);
 		list_del_init(&class_dev->node);
 		list_for_each_entry(class_intf, &parent_class->interfaces, node)
 			if (class_intf->remove)
 				class_intf->remove(class_dev, class_intf);
-		up(&parent_class->sem);
+		mutex_unlock(&parent_class->mutex);
 	}
 
 	if (class_dev->dev) {
@@ -772,14 +772,14 @@ void class_device_destroy(struct class *
 	struct class_device *class_dev = NULL;
 	struct class_device *class_dev_tmp;
 
-	down(&cls->sem);
+	mutex_lock(&cls->mutex);
 	list_for_each_entry(class_dev_tmp, &cls->children, node) {
 		if (class_dev_tmp->devt == devt) {
 			class_dev = class_dev_tmp;
 			break;
 		}
 	}
-	up(&cls->sem);
+	mutex_unlock(&cls->mutex);
 
 	if (class_dev)
 		class_device_unregister(class_dev);
@@ -812,7 +812,7 @@ int class_interface_register(struct clas
 	if (!parent)
 		return -EINVAL;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_add_tail(&class_intf->node, &parent->interfaces);
 	if (class_intf->add) {
 		list_for_each_entry(class_dev, &parent->children, node)
@@ -822,11 +822,87 @@ int class_interface_register(struct clas
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->add_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	return 0;
 }
 
+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;
+	mutex_lock(&class->mutex);
+	list_for_each_entry(dev, &class->devices, node) {
+		error = fn(dev, data);
+		if (error)
+			break;
+	}
+	mutex_unlock(&class->mutex);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_device);
+
+struct device *class_find_device(struct class *class, void *data,
+				   int (*match)(struct device *, void *))
+{
+	struct device *dev;
+
+	if (!class)
+		return NULL;
+
+	mutex_lock(&class->mutex);
+	list_for_each_entry(dev, &class->devices, node)
+		if (match(dev, data) && get_device(dev))
+			break;
+	mutex_unlock(&class->mutex);
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(class_find_device);
+
+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;
+	mutex_lock(&class->mutex);
+	list_for_each_entry(dev, &class->children, node) {
+		error = fn(dev, data);
+		if (error)
+			break;
+	}
+	mutex_unlock(&class->mutex);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_for_each_child);
+
+struct class_device *class_find_child(struct class *class, void *data,
+				   int (*match)(struct class_device *, void *))
+{
+	struct class_device *dev;
+
+	if (!class)
+		return NULL;
+
+	mutex_lock(&class->mutex);
+	list_for_each_entry(dev, &class->children, node)
+		if (match(dev, data) && class_device_get(dev))
+			break;
+	mutex_unlock(&class->mutex);
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(class_find_child);
+
 void class_interface_unregister(struct class_interface *class_intf)
 {
 	struct class * parent = class_intf->class;
@@ -836,7 +912,7 @@ void class_interface_unregister(struct c
 	if (!parent)
 		return;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_del_init(&class_intf->node);
 	if (class_intf->remove) {
 		list_for_each_entry(class_dev, &parent->children, node)
@@ -846,7 +922,7 @@ void class_interface_unregister(struct c
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->remove_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	class_put(parent);
 }
diff -upr linux/drivers/base/core.c linux.new/drivers/base/core.c
--- linux/drivers/base/core.c	2008-01-10 17:17:11.000000000 +0800
+++ linux.new/drivers/base/core.c	2008-01-10 17:17:11.000000000 +0800
@@ -19,8 +19,6 @@
 #include <linux/kdev_t.h>
 #include <linux/notifier.h>
 
-#include <asm/semaphore.h>
-
 #include "base.h"
 #include "power/power.h"
 
@@ -783,7 +781,7 @@ int device_add(struct device *dev)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
 	if (dev->class) {
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* tie the class to the device */
 		list_add_tail(&dev->node, &dev->class->devices);
 
@@ -791,7 +789,7 @@ int device_add(struct device *dev)
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->add_dev)
 				class_intf->add_dev(dev, class_intf);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
  Done:
 	put_device(dev);
@@ -928,14 +926,14 @@ void device_del(struct device * dev)
 			sysfs_remove_link(&dev->kobj, "device");
 		}
 
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* notify any interfaces that the device is now gone */
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->remove_dev)
 				class_intf->remove_dev(dev, class_intf);
 		/* remove the device from the class list */
 		list_del_init(&dev->node);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 
 		/* If we live in a parent class-directory, unreference it */
 		if (dev->kobj.parent->kset == &dev->class->class_dirs) {
@@ -946,7 +944,7 @@ void device_del(struct device * dev)
 			 * if we are the last child of our class, delete
 			 * our class-directory at this parent
 			 */
-			down(&dev->class->sem);
+			mutex_lock(&dev->class->mutex);
 			list_for_each_entry(d, &dev->class->devices, node) {
 				if (d == dev)
 					continue;
@@ -959,7 +957,7 @@ void device_del(struct device * dev)
 				kobject_del(dev->kobj.parent);
 
 			kobject_put(dev->kobj.parent);
-			up(&dev->class->sem);
+			mutex_unlock(&dev->class->mutex);
 		}
 	}
 	device_remove_file(dev, &uevent_attr);
@@ -1168,14 +1166,14 @@ void device_destroy(struct class *class,
 	struct device *dev = NULL;
 	struct device *dev_tmp;
 
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev_tmp, &class->devices, node) {
 		if (dev_tmp->devt == devt) {
 			dev = dev_tmp;
 			break;
 		}
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	if (dev)
 		device_unregister(dev);
diff -upr linux/include/linux/device.h linux.new/include/linux/device.h
--- linux/include/linux/device.h	2008-01-10 17:17:11.000000000 +0800
+++ linux.new/include/linux/device.h	2008-01-10 17:17:12.000000000 +0800
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/mutex.h>
 #include <asm/semaphore.h>
 #include <asm/atomic.h>
 #include <asm/device.h>
@@ -180,7 +181,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 mutex		mutex;
 
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;
@@ -197,6 +198,14 @@ struct class {
 	int	(*resume)(struct device *);
 };
 
+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 *));
 extern int __must_check class_register(struct class *);
 extern void class_unregister(struct class *);
 

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Stefan Richter]

Dave Young wrote:
> This is the first one of the series about driver core changes.

Please always provide kerneldoc comments when you add new API elements;
here: exported functions.

It's unfortunate that the driver core's API isn't fully documented yet,
and you shouldn't make it worse.

That's only my personal opinion as one API user though.  But others
might agree.  Among else, things worth documenting are return values
after errors, side effects (!), constraints on the calling context if
there are any special constraints.

I assume you didn't write documentation yet because you need general
feedback first.

...
> +struct device *class_find_device(struct class *class, void *data,
> +				   int (*match)(struct device *, void *))
> +{
> +	struct device *dev;
> +
> +	if (!class)
> +		return NULL;
> +
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->devices, node)
> +		if (match(dev, data) && get_device(dev))
> +			break;
> +	mutex_unlock(&class->mutex);
> +
> +	return dev;
> +}

What is returned if there was no match?
What if there was a match but get_ failed?

...
> +struct class_device *class_find_child(struct class *class, void *data,
> +				   int (*match)(struct class_device *, void *))
> +{
...
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->children, node)
> +		if (match(dev, data) && class_device_get(dev))
> +			break;
> +	mutex_unlock(&class->mutex);
> +
> +	return dev;
> +}

Here too?
-- 
Stefan Richter
-=====-==--- ---= -=-=-
http://arcgraph.de/sr/

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 10, 2008 8:34 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Dave Young wrote:
> > This is the first one of the series about driver core changes.
>
> Please always provide kerneldoc comments when you add new API elements;
> here: exported functions.
>
> It's unfortunate that the driver core's API isn't fully documented yet,
> and you shouldn't make it worse.
>
> That's only my personal opinion as one API user though.  But others
> might agree.  Among else, things worth documenting are return values
> after errors, side effects (!), constraints on the calling context if
> there are any special constraints.
>
> I assume you didn't write documentation yet because you need general
> feedback first.

Yes, I did not. Thanks for pointing out, I will do.
>
> ...
> > +struct device *class_find_device(struct class *class, void *data,
> > +                                int (*match)(struct device *, void *))
> > +{
> > +     struct device *dev;
> > +
> > +     if (!class)
> > +             return NULL;
> > +
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->devices, node)
> > +             if (match(dev, data) && get_device(dev))
> > +                     break;
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return dev;
> > +}
>
> What is returned if there was no match?
> What if there was a match but get_ failed?

Will fix it.
>
> ...
> > +struct class_device *class_find_child(struct class *class, void *data,
> > +                                int (*match)(struct class_device *, void *))
> > +{
> ...
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->children, node)
> > +             if (match(dev, data) && class_device_get(dev))
> > +                     break;
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return dev;
> > +}
>
> Here too?

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

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Cornelia Huck]

On Thu, 10 Jan 2008 17:48:43 +0800,
Dave Young <hidave.darkstar@gmail.com> wrote:

Please add a kerneldoc comment for each of the new interfaces.

> +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;
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->devices, node) {
> +		error = fn(dev, data);

Hm, the equivalent _for_each_device() functions all elevate the
device's refcount while calling fn(). I wonder whether we want this
here as well?

> +		if (error)
> +			break;
> +	}
> +	mutex_unlock(&class->mutex);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(class_for_each_device);
> +
> +struct device *class_find_device(struct class *class, void *data,
> +				   int (*match)(struct device *, void *))
> +{
> +	struct device *dev;
> +
> +	if (!class)
> +		return NULL;
> +
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->devices, node)
> +		if (match(dev, data) && get_device(dev))

First get the reference and then drop it if the match function returns
0 to make this analogous to the other _find_device() functions?

> +			break;
> +	mutex_unlock(&class->mutex);
> +
> +	return dev;
> +}
> +EXPORT_SYMBOL_GPL(class_find_device);
> +
> +int class_for_each_child(struct class *class, void *data,
> +			   int (*fn)(struct class_device *, void *))

Haven't looked at the callers, but isn't it better to convert them to
use struct device instead so we don't need to introduce new
class_device api?

> +{
> +	struct class_device *dev;
> +	int error = 0;
> +
> +	if (!class)
> +		return -EINVAL;
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->children, node) {
> +		error = fn(dev, data);

Same comment as above concerning reference counts.

> +		if (error)
> +			break;
> +	}
> +	mutex_unlock(&class->mutex);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(class_for_each_child);
> +
> +struct class_device *class_find_child(struct class *class, void *data,
> +				   int (*match)(struct class_device *, void *))
> +{
> +	struct class_device *dev;
> +
> +	if (!class)
> +		return NULL;
> +
> +	mutex_lock(&class->mutex);
> +	list_for_each_entry(dev, &class->children, node)
> +		if (match(dev, data) && class_device_get(dev))

And here.

> +			break;
> +	mutex_unlock(&class->mutex);
> +
> +	return dev;
> +}
> +EXPORT_SYMBOL_GPL(class_find_child);
> +

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 10, 2008 9:23 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Thu, 10 Jan 2008 17:48:43 +0800,
> Dave Young <hidave.darkstar@gmail.com> wrote:
>
> Please add a kerneldoc comment for each of the new interfaces.
Will do.
>
> > +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;
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->devices, node) {
> > +             error = fn(dev, data);
>
> Hm, the equivalent _for_each_device() functions all elevate the
> device's refcount while calling fn(). I wonder whether we want this
> here as well?

Thanks for comment.
Hm, I'm not sure about this. Greg, what's your opinion?

>
> > +             if (error)
> > +                     break;
> > +     }
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return error;
> > +}
> > +EXPORT_SYMBOL_GPL(class_for_each_device);
> > +
> > +struct device *class_find_device(struct class *class, void *data,
> > +                                int (*match)(struct device *, void *))
> > +{
> > +     struct device *dev;
> > +
> > +     if (!class)
> > +             return NULL;
> > +
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->devices, node)
> > +             if (match(dev, data) && get_device(dev))
>
> First get the reference and then drop it if the match function returns
> 0 to make this analogous to the other _find_device() functions?

It's just like other _find_device() functions. Are these more get/put
really needed?

>
> > +                     break;
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return dev;
> > +}
> > +EXPORT_SYMBOL_GPL(class_find_device);
> > +
> > +int class_for_each_child(struct class *class, void *data,
> > +                        int (*fn)(struct class_device *, void *))
>
> Haven't looked at the callers, but isn't it better to convert them to
> use struct device instead so we don't need to introduce new
> class_device api?

The drivers/scsi/hosts.c need it.

>
> > +{
> > +     struct class_device *dev;
> > +     int error = 0;
> > +
> > +     if (!class)
> > +             return -EINVAL;
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->children, node) {
> > +             error = fn(dev, data);
>
> Same comment as above concerning reference counts.
>
> > +             if (error)
> > +                     break;
> > +     }
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return error;
> > +}
> > +EXPORT_SYMBOL_GPL(class_for_each_child);
> > +
> > +struct class_device *class_find_child(struct class *class, void *data,
> > +                                int (*match)(struct class_device *, void *))
> > +{
> > +     struct class_device *dev;
> > +
> > +     if (!class)
> > +             return NULL;
> > +
> > +     mutex_lock(&class->mutex);
> > +     list_for_each_entry(dev, &class->children, node)
> > +             if (match(dev, data) && class_device_get(dev))
>
> And here.
>
>
> > +                     break;
> > +     mutex_unlock(&class->mutex);
> > +
> > +     return dev;
> > +}
> > +EXPORT_SYMBOL_GPL(class_find_child);
> > +
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Cornelia Huck]

On Fri, 11 Jan 2008 10:33:16 +0800,
"Dave Young" <hidave.darkstar@gmail.com> wrote:


> > > +struct device *class_find_device(struct class *class, void *data,
> > > +                                int (*match)(struct device *, void *))
> > > +{
> > > +     struct device *dev;
> > > +
> > > +     if (!class)
> > > +             return NULL;
> > > +
> > > +     mutex_lock(&class->mutex);
> > > +     list_for_each_entry(dev, &class->devices, node)
> > > +             if (match(dev, data) && get_device(dev))
> >
> > First get the reference and then drop it if the match function returns
> > 0 to make this analogous to the other _find_device() functions?
> 
> It's just like other _find_device() functions. Are these more get/put
> really needed?

The other _find_device() functions operate on klists, which means that
there is an implicit get while the element is handled. This function
operates on a normal list, which means that getting/putting the
reference looks a bit different if we want the same effect.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 11, 2008 4:23 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Fri, 11 Jan 2008 10:33:16 +0800,
> "Dave Young" <hidave.darkstar@gmail.com> wrote:
>
>
> > > > +struct device *class_find_device(struct class *class, void *data,
> > > > +                                int (*match)(struct device *, void *))
> > > > +{
> > > > +     struct device *dev;
> > > > +
> > > > +     if (!class)
> > > > +             return NULL;
> > > > +
> > > > +     mutex_lock(&class->mutex);
> > > > +     list_for_each_entry(dev, &class->devices, node)
> > > > +             if (match(dev, data) && get_device(dev))
> > >
> > > First get the reference and then drop it if the match function returns
> > > 0 to make this analogous to the other _find_device() functions?
> >
> > It's just like other _find_device() functions. Are these more get/put
> > really needed?
>
> The other _find_device() functions operate on klists, which means that
> there is an implicit get while the element is handled. This function
> operates on a normal list, which means that getting/putting the
> reference looks a bit different if we want the same effect.
>

Hi, you are right. Will be fixed.

Thanks.

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Alan Stern]

On Thu, 10 Jan 2008, Dave Young wrote:

> Hi,
> The patches are done on my side, please help to check. 
> 
> This is the first one of the series about driver core changes. 
> If this one is accepted and there's no other problem I will post the others for maintainer's review (they need your comment and help because I don't know well about the specific driver logic).

Before spending too much time on all these conversions, shouldn't you 
first determine whether they will work with the lockdep checker?  In 
one of your earlier messages there was a clear indication that they 
will not.

Alan Stern

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 10, 2008 11:41 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 10 Jan 2008, Dave Young wrote:
>
> > Hi,
> > The patches are done on my side, please help to check.
> >
> > This is the first one of the series about driver core changes.
> > If this one is accepted and there's no other problem I will post the others for maintainer's review (they need your comment and help because I don't know well about the specific driver logic).
>
> Before spending too much time on all these conversions, shouldn't you
> first determine whether they will work with the lockdep checker?  In
> one of your earlier messages there was a clear indication that they
> will not.
Thanks.

I managed to build and boot ok with lockdep config options except the
apm_power which need to be cross compiled.

But not sure whether there's something which are not hit yet.
>
> Alan Stern
>
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Greg KH]

On Thu, Jan 10, 2008 at 05:48:43PM +0800, Dave Young wrote:
> The patches are done on my side, please help to check. 

Along with all of the other comments from people, I have a few.

> This is the first one of the series about driver core changes. 
> If this one is accepted and there's no other problem I will post the others for maintainer's review (they need your comment and help because I don't know well about the specific driver logic).
> 
> Thanks a lot in advance.
> ---
> 
> 1. convert class semaphore to mutex.
> 2. add class iterater functions to encapsulate the detail of class devices/children list iterating :
> 	class_for_each_device
> 	class_find_device
> 	class_for_each_child
> 	class_find_child

No, please create 1 patch per type-of-change.

So in this case you would have a series of patches:
	1) add the class iterator functions
	2-n) convert the existing places in the kernel using the
	     class->semaphore to use the new iterator functions
	n+1) convert class semaphore to mutex, which should only touch
	     the driver core

That way everything builds along the way, and it's easy to understand
and review.

Oh, and please start a new thread when you create a new patch like this
so it doesn't get burried in people's inboxes...

thanks,

greg k-h

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 11, 2008 2:39 AM, Greg KH <gregkh@suse.de> wrote:
> On Thu, Jan 10, 2008 at 05:48:43PM +0800, Dave Young wrote:
> > The patches are done on my side, please help to check.
>
> Along with all of the other comments from people, I have a few.
>
> > This is the first one of the series about driver core changes.
> > If this one is accepted and there's no other problem I will post the others for maintainer's review (they need your comment and help because I don't know well about the specific driver logic).
> >
> > Thanks a lot in advance.
> > ---
> >
> > 1. convert class semaphore to mutex.
> > 2. add class iterater functions to encapsulate the detail of class devices/children list iterating :
> >       class_for_each_device
> >       class_find_device
> >       class_for_each_child
> >       class_find_child
>
> No, please create 1 patch per type-of-change.
>
> So in this case you would have a series of patches:
>         1) add the class iterator functions
>         2-n) convert the existing places in the kernel using the
>              class->semaphore to use the new iterator functions
>         n+1) convert class semaphore to mutex, which should only touch
>              the driver core

Thanks,  you are quite right. I will do it when I update the patch next time.

>
> That way everything builds along the way, and it's easy to understand
> and review.
>
> Oh, and please start a new thread when you create a new patch like this
> so it doesn't get burried in people's inboxes...

Will do when the new patch available.

>
> thanks,
>
> greg k-h
>

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Alan Stern]

On Mon, 7 Jan 2008, Stefan Richter wrote:

> Semi off-topic:  What about struct device.sem?  Is there any chance to
> rip this out of the driver core and let drivers serialize everything?  I
> suppose not...

There isn't.  The core uses that lock.

Alan Stern

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Dave Young]

On Jan 7, 2008 4:45 PM, Greg KH <gregkh@suse.de> wrote:
> On Mon, Jan 07, 2008 at 10:09:44AM +0800, Dave Young wrote:
> >
> > Thanks for your comment, I rewrite it for 2.6.24-rc7 as a all-in-one
> > patch, please see following. Drop i2c maintainer and list in cc
> > because there's no changes about i2c in this patch:
> >
> > Convert semaphore to mutex in struct class
>
> Most of the non-driver core code should be converted to not use the
> lock in the class at all.  They should use a local lock instead.
>
> So how about a patch series that removes the usage of the current
> semaphore from the different drivers, and then, a single patch that
> converts the semaphore to mutex in the class code, it should not touch
> anything outside of the drivers/base/ directory (not including the
> driver.h file.)
>
> Sound reasonable?

looks like a better aproach, let me take a look ...

>
> thanks,
>
> greg k-h
>