Re: [PATCH 0/7] convert semaphore to mutex in struct class
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.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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.0 +0800
+++ linux.new/drivers/base/class.c 2008-01-10 17:17:11.0 +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
Re: [PATCH 0/7] convert semaphore to mutex in 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/
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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);
+
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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/
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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);
+
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 ... :) - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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/
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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/ - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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/
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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.0
+0800
+++ linux-2.6.24-rc7/drivers/base/class.c 2008-01-07 09:12:49.0
+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
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] convert semaphore to mutex in struct class
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. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
