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
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.
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.
On Jan 3, 2008 3:24 PM, Jarek Poplawski <[email protected]> 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.
>
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.
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 <[email protected]>"
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/
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 <[email protected]>"
>
> 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 <[email protected]>
---
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;
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
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 ... :)
On Jan 7, 2008 4:45 PM, Greg KH <[email protected]> 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
>
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/
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.
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
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/
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/
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
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
On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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
>
On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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
On Jan 9, 2008 6:48 AM, Greg KH <[email protected]> wrote:
> On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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
>
On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> On Jan 9, 2008 6:48 AM, Greg KH <[email protected]> wrote:
> > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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
> >
On Jan 9, 2008 2:13 PM, Dave Young <[email protected]> wrote:
>
> On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > On Jan 9, 2008 6:48 AM, Greg KH <[email protected]> wrote:
> > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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
On Jan 9, 2008 2:37 PM, Dave Young <[email protected]> wrote:
>
> On Jan 9, 2008 2:13 PM, Dave Young <[email protected]> wrote:
> >
> > On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > > On Jan 9, 2008 6:48 AM, Greg KH <[email protected]> wrote:
> > > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > > On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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.
On Wed, Jan 09, 2008 at 02:39:23PM +0800, Dave Young wrote:
> On Jan 9, 2008 2:37 PM, Dave Young <[email protected]> wrote:
> >
> > On Jan 9, 2008 2:13 PM, Dave Young <[email protected]> wrote:
> > >
> > > On Wed, Jan 09, 2008 at 09:32:48AM +0800, Dave Young wrote:
> > > > On Jan 9, 2008 6:48 AM, Greg KH <[email protected]> wrote:
> > > > > On Tue, Jan 08, 2008 at 03:05:10PM +0800, Dave Young wrote:
> > > > > > On Jan 8, 2008 1:20 AM, Greg KH <[email protected]> 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 <[email protected]>
---
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 *);
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/
On Thu, 10 Jan 2008 17:48:43 +0800,
Dave Young <[email protected]> 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);
> +
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
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
On Jan 10, 2008 8:34 PM, Stefan Richter <[email protected]> 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/
>
On Jan 10, 2008 9:23 PM, Cornelia Huck <[email protected]> wrote:
> On Thu, 10 Jan 2008 17:48:43 +0800,
> Dave Young <[email protected]> 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);
> > +
>
On Jan 10, 2008 11:41 PM, Alan Stern <[email protected]> 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
>
>
On Jan 11, 2008 2:39 AM, Greg KH <[email protected]> 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
>
On Fri, 11 Jan 2008 10:33:16 +0800,
"Dave Young" <[email protected]> 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.
On Jan 11, 2008 4:23 PM, Cornelia Huck <[email protected]> wrote:
> On Fri, 11 Jan 2008 10:33:16 +0800,
> "Dave Young" <[email protected]> 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.