Convert the class semaphore to mutex.
Signed-off-by: Dave Young <[email protected]>
---
drivers/base/class.c | 38 +++++++++++++++++++-------------------
drivers/base/core.c | 18 ++++++++----------
include/linux/device.h | 3 ++-
3 files changed, 29 insertions(+), 30 deletions(-)
diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c 2008-01-12 16:11:07.000000000 +0800
+++ linux.new/drivers/base/class.c 2008-01-12 16:11:08.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);
@@ -818,7 +818,7 @@ int class_for_each_device(struct class *
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -829,7 +829,7 @@ int class_for_each_device(struct class *
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -858,7 +858,7 @@ struct device *class_find_device(struct
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -870,7 +870,7 @@ struct device *class_find_device(struct
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
if (error)
return NULL;
@@ -898,7 +898,7 @@ int class_for_each_child(struct class *c
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -909,7 +909,7 @@ int class_for_each_child(struct class *c
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -938,7 +938,7 @@ struct class_device *class_find_child(st
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -950,7 +950,7 @@ struct class_device *class_find_child(st
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
if (error)
return NULL;
@@ -971,7 +971,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)
@@ -981,7 +981,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;
}
@@ -995,7 +995,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)
@@ -1005,7 +1005,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-12 16:11:07.000000000 +0800
+++ linux.new/drivers/base/core.c 2008-01-12 16:11:08.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-12 16:11:07.000000000 +0800
+++ linux.new/include/linux/device.h 2008-01-12 16:11:08.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;
Convert the class semaphore to mutex.
Signed-off-by: Dave Young <[email protected]>
---
drivers/base/class.c | 38 +++++++++++++++++++-------------------
drivers/base/core.c | 18 ++++++++----------
include/linux/device.h | 3 ++-
3 files changed, 29 insertions(+), 30 deletions(-)
diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
--- linux/drivers/base/class.c 2008-01-15 14:04:26.000000000 +0800
+++ linux.new/drivers/base/class.c 2008-01-15 14:04:26.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);
@@ -818,7 +818,7 @@ int class_for_each_device(struct class *
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -829,7 +829,7 @@ int class_for_each_device(struct class *
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -860,7 +860,7 @@ struct device *class_find_device(struct
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -872,7 +872,7 @@ struct device *class_find_device(struct
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -898,7 +898,7 @@ int class_for_each_child(struct class *c
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -909,7 +909,7 @@ int class_for_each_child(struct class *c
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -940,7 +940,7 @@ struct class_device *class_find_child(st
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -952,7 +952,7 @@ struct class_device *class_find_child(st
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -971,7 +971,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)
@@ -981,7 +981,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;
}
@@ -995,7 +995,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)
@@ -1005,7 +1005,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-15 14:04:26.000000000 +0800
+++ linux.new/drivers/base/core.c 2008-01-15 14:04:26.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-15 14:04:26.000000000 +0800
+++ linux.new/include/linux/device.h 2008-01-15 14:04:26.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 children, devices, interfaces */
+ struct mutex mutex; /* locks children, devices, interfaces */
struct class_attribute * class_attrs;
struct class_device_attribute * class_dev_attrs;
struct device_attribute * dev_attrs;
On Tue, Jan 15, 2008 at 05:15:27PM +0800, Dave Young wrote:
> Convert the class semaphore to mutex.
>
> Signed-off-by: Dave Young <[email protected]>
>
> ---
> drivers/base/class.c | 38 +++++++++++++++++++-------------------
> drivers/base/core.c | 18 ++++++++----------
> include/linux/device.h | 3 ++-
> 3 files changed, 29 insertions(+), 30 deletions(-)
>
> diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
> --- linux/drivers/base/class.c 2008-01-15 14:04:26.000000000 +0800
> +++ linux.new/drivers/base/class.c 2008-01-15 14:04:26.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);
I hope I'm wrong with this (I don't know this code at all...), and
of course I should've noticed this earlier after all, but I wonder
about this _NESTING corretness here. So, if these variables names
are right, and say about real nesting dependency, then it seems
mutex_lock_nested() should be used consistently even if (currently?)
not forced by lockdep warnings; otherwise this could possibly cover
some other warnings. Alas, if accidentally I'm right, it seems a
bit of new testing would be necessary...
Regards,
Jarek P.
On Jan 15, 2008 9:56 PM, Jarek Poplawski <[email protected]> wrote:
>
> On Tue, Jan 15, 2008 at 05:15:27PM +0800, Dave Young wrote:
> > Convert the class semaphore to mutex.
> >
> > Signed-off-by: Dave Young <[email protected]>
> >
> > ---
> > drivers/base/class.c | 38 +++++++++++++++++++-------------------
> > drivers/base/core.c | 18 ++++++++----------
> > include/linux/device.h | 3 ++-
> > 3 files changed, 29 insertions(+), 30 deletions(-)
> >
> > diff -upr linux/drivers/base/class.c linux.new/drivers/base/class.c
> > --- linux/drivers/base/class.c 2008-01-15 14:04:26.000000000 +0800
> > +++ linux.new/drivers/base/class.c 2008-01-15 14:04:26.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);
>
> I hope I'm wrong with this (I don't know this code at all...), and
> of course I should've noticed this earlier after all, but I wonder
> about this _NESTING corretness here. So, if these variables names
> are right, and say about real nesting dependency, then it seems
> mutex_lock_nested() should be used consistently even if (currently?)
> not forced by lockdep warnings; otherwise this could possibly cover
> some other warnings. Alas, if accidentally I'm right, it seems a
> bit of new testing would be necessary...
The lockdep warining was posted in the below thread, actually, I have
built and run this patced kernel for several days, there's no more
warnings.
http://lkml.org/lkml/2008/1/3/2
>
> Regards,
> Jarek P.
>
On Wed, Jan 16, 2008 at 09:03:03AM +0800, Dave Young wrote:
...
> The lockdep warining was posted in the below thread, actually, I have
> built and run this patced kernel for several days, there's no more
> warnings.
> http://lkml.org/lkml/2008/1/3/2
Right... But, with something like this:
... have_some_fun(... cls)
{
mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
have_other_fun(cls);
mutex_unlock(&cls->mutex);
}
... have_more_fun(...)
{
...
mutex_init(&cls->mutex);
mutex_lock(&cls->mutex);
have_some_fun(cls);
mutex_unlock(&cls->mutex);
}
probably you wouldn't get any lockdep warning too...
Of course, if we know all the locking is right such proper lockdep
annotating shouldn't matter too much. (And of course this could be
improved later.)
Regards,
Jarek P.
On Wed, 16 Jan 2008, Dave Young wrote:
> The lockdep warining was posted in the below thread, actually, I have
> built and run this patced kernel for several days, there's no more
> warnings.
> http://lkml.org/lkml/2008/1/3/2
Your meaning isn't clear. Do you mean that your patch doesn't generate
any lockdep warnings at all? Or do you mean that it generates a single
lockdep warning at boot time and then no more warnings afterward?
Alan Stern
On Wed, Jan 16, 2008 at 10:27:54AM -0500, Alan Stern wrote:
> On Wed, 16 Jan 2008, Dave Young wrote:
>
> > The lockdep warining was posted in the below thread, actually, I have
> > built and run this patced kernel for several days, there's no more
> > warnings.
> > http://lkml.org/lkml/2008/1/3/2
>
> Your meaning isn't clear. Do you mean that your patch doesn't generate
> any lockdep warnings at all? Or do you mean that it generates a single
> lockdep warning at boot time and then no more warnings afterward?
So it wasn't only my sleeping problem! This:
"One lockdep warning detected as following, thus use mutex_lock_nested
with SINGLE_DEPTH_NESTING in class_device_add"
seems to say that mutex_lock_nested is used in the patch just to remove
this one possible warning...
Regards,
Jarek P.
On Jan 16, 2008 11:27 PM, Alan Stern <[email protected]> wrote:
> On Wed, 16 Jan 2008, Dave Young wrote:
>
> > The lockdep warining was posted in the below thread, actually, I have
> > built and run this patced kernel for several days, there's no more
> > warnings.
> > http://lkml.org/lkml/2008/1/3/2
>
> Your meaning isn't clear. Do you mean that your patch doesn't generate
> any lockdep warnings at all? Or do you mean that it generates a single
> lockdep warning at boot time and then no more warnings afterward?
I means the latter one.
>
> Alan Stern
>
>
On Jan 16, 2008 4:34 PM, Jarek Poplawski <[email protected]> wrote:
> On Wed, Jan 16, 2008 at 09:03:03AM +0800, Dave Young wrote:
> ...
> > The lockdep warining was posted in the below thread, actually, I have
> > built and run this patced kernel for several days, there's no more
> > warnings.
> > http://lkml.org/lkml/2008/1/3/2
>
> Right... But, with something like this:
>
> ... have_some_fun(... cls)
> {
> mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
> have_other_fun(cls);
> mutex_unlock(&cls->mutex);
>
> }
>
> ... have_more_fun(...)
> {
> ...
>
> mutex_init(&cls->mutex);
>
> mutex_lock(&cls->mutex);
> have_some_fun(cls);
> mutex_unlock(&cls->mutex);
> }
>
> probably you wouldn't get any lockdep warning too...
Sorry for late reply.
Actually, I don't know much about lockdep. Could you tell how to use
it properly in this scenario?
>
> Of course, if we know all the locking is right such proper lockdep
> annotating shouldn't matter too much. (And of course this could be
> improved later.)
>
> Regards,
> Jarek P.
>
On 17-01-2008 02:17, Dave Young wrote:
> On Jan 16, 2008 4:34 PM, Jarek Poplawski <[email protected]> wrote:
>> On Wed, Jan 16, 2008 at 09:03:03AM +0800, Dave Young wrote:
>> ...
>>> The lockdep warining was posted in the below thread, actually, I have
>>> built and run this patced kernel for several days, there's no more
>>> warnings.
>>> http://lkml.org/lkml/2008/1/3/2
>> Right... But, with something like this:
>>
>> ... have_some_fun(... cls)
>> {
>> mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
>> have_other_fun(cls);
>> mutex_unlock(&cls->mutex);
>>
>> }
>>
>> ... have_more_fun(...)
>> {
>> ...
>>
>> mutex_init(&cls->mutex);
>>
>> mutex_lock(&cls->mutex);
>> have_some_fun(cls);
>> mutex_unlock(&cls->mutex);
>> }
>>
>> probably you wouldn't get any lockdep warning too...
>
> Sorry for late reply.
> Actually, I don't know much about lockdep. Could you tell how to use
> it properly in this scenario?
As you have noticed while working on this patch, if two different
instances of the same structure containig some lock are created in
the same place, lockdep will treat this one (the same) lock. It looks
strange, but actually it's a feature which enables to track
dependencies between different locks on 'class' level instead of
instance 'level'. The downside is that lockdep is very often too
sensitive by default, so you have to 'annotate' when instancess are
actually on different level (e.g. parents and children here) and
could be locked at the same time or in some order.
You can use e.g. mutex_lock_nested() or lockdep_set_class*() for this.
Then lockdep simply trusts you, and starts to think they are different
locks. If you do it wrong there will be simply no more warnings, but
undercover lockups still possible (and diagnosing a bit harder then).
So, since in your patch there are two levels of locking, and you
started to annotate lockdep about a child taking parent's class lock
with:
mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
you should do the same everywhere in a situation like this.
lockdep will treat this simply as lock B vs. A (mutex_lock(&cls))
dependencies.
Regards,
Jarek P.
PS: BTW, it seems after this patch 1/1 the locking was changed a bit,
so these previous tests could be not enough.
On Jan 17, 2008 4:38 PM, Jarek Poplawski <[email protected]> wrote:
> On 17-01-2008 02:17, Dave Young wrote:
> > On Jan 16, 2008 4:34 PM, Jarek Poplawski <[email protected]> wrote:
> >> On Wed, Jan 16, 2008 at 09:03:03AM +0800, Dave Young wrote:
> >> ...
> >>> The lockdep warining was posted in the below thread, actually, I have
> >>> built and run this patced kernel for several days, there's no more
> >>> warnings.
> >>> http://lkml.org/lkml/2008/1/3/2
> >> Right... But, with something like this:
> >>
> >> ... have_some_fun(... cls)
> >> {
> >> mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
> >> have_other_fun(cls);
> >> mutex_unlock(&cls->mutex);
> >>
> >> }
> >>
> >> ... have_more_fun(...)
> >> {
> >> ...
> >>
> >> mutex_init(&cls->mutex);
> >>
> >> mutex_lock(&cls->mutex);
> >> have_some_fun(cls);
> >> mutex_unlock(&cls->mutex);
> >> }
> >>
> >> probably you wouldn't get any lockdep warning too...
> >
> > Sorry for late reply.
> > Actually, I don't know much about lockdep. Could you tell how to use
> > it properly in this scenario?
>
> As you have noticed while working on this patch, if two different
> instances of the same structure containig some lock are created in
> the same place, lockdep will treat this one (the same) lock. It looks
> strange, but actually it's a feature which enables to track
> dependencies between different locks on 'class' level instead of
> instance 'level'. The downside is that lockdep is very often too
> sensitive by default, so you have to 'annotate' when instancess are
> actually on different level (e.g. parents and children here) and
> could be locked at the same time or in some order.
>
> You can use e.g. mutex_lock_nested() or lockdep_set_class*() for this.
> Then lockdep simply trusts you, and starts to think they are different
> locks. If you do it wrong there will be simply no more warnings, but
> undercover lockups still possible (and diagnosing a bit harder then).
> So, since in your patch there are two levels of locking, and you
> started to annotate lockdep about a child taking parent's class lock
> with:
> mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
>
> you should do the same everywhere in a situation like this.
> lockdep will treat this simply as lock B vs. A (mutex_lock(&cls))
> dependencies.
Thanks for the explain, let me check the lockings again.
>
> Regards,
> Jarek P.
>
> PS: BTW, it seems after this patch 1/1 the locking was changed a bit,
> so these previous tests could be not enough.
>
On Thu, 17 Jan 2008, Dave Young wrote:
> > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > any lockdep warnings at all? Or do you mean that it generates a single
> > lockdep warning at boot time and then no more warnings afterward?
>
> I means the latter one.
That's very bad.
For each type of violation, lockdep only gives one error message. So
the fact that you get one message at boot time and then no more doesn't
mean the code is almost right -- it probably means the code has lots of
errors and you're seeing only the first one.
Alan Stern
On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> On Thu, 17 Jan 2008, Dave Young wrote:
>
> > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > any lockdep warnings at all? Or do you mean that it generates a single
> > > lockdep warning at boot time and then no more warnings afterward?
> >
> > I means the latter one.
>
> That's very bad.
>
> For each type of violation, lockdep only gives one error message. So
> the fact that you get one message at boot time and then no more doesn't
> mean the code is almost right -- it probably means the code has lots of
> errors and you're seeing only the first one.
I hope it's better than this: lockdep really stops checking after first
warning, but I've understood from David's description that after fixing
this one place lockdep seems to be pleased.
On the other hand, according to Greg the code is OK, so if there are any
such warnings they simply have to be false! (...Unless you trust lockdep
more?!)
Regards,
Jarek P.
On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > On Thu, 17 Jan 2008, Dave Young wrote:
> >
> > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > lockdep warning at boot time and then no more warnings afterward?
> > >
> > > I means the latter one.
> >
> > That's very bad.
> >
> > For each type of violation, lockdep only gives one error message. So
> > the fact that you get one message at boot time and then no more doesn't
> > mean the code is almost right -- it probably means the code has lots of
> > errors and you're seeing only the first one.
>
> I hope it's better than this: lockdep really stops checking after first
> warning, but I've understood from David's description that after fixing
> this one place lockdep seems to be pleased.
That isn't what Dave said above; he said that lockdep produces a single
warning at bootup. If he did mention anything about one place being
fixed up or lockdep being pleased, it was a while back and I've lost
track of it.
If I recall correctly the nature of the warning was that a method
routine for one class (called with the class's mutex held) was creating
a second class and locking that class's mutex. In principle this is
perfectly legal and should be allowed for arbitrary depths of nesting,
even though it is the sort of thing lockdep is currently unable to
handle.
> On the other hand, according to Greg the code is OK, so if there are any
> such warnings they simply have to be false! (...Unless you trust lockdep
> more?!)
It's not a matter of trust or of false warnings. People shouldn't
tolerate any lockdep warnings at all; otherwise they will start to
ignore the valid ones.
Alan Stern
P.S.: Just because Greg says the code is okay doesn't mean it will
please lockdep -- it doesn't even mean the code really is okay! I've
known Greg to make an occasional mistake.
On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> On Thu, 17 Jan 2008, Jarek Poplawski wrote:
>
> > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > On Thu, 17 Jan 2008, Dave Young wrote:
> > >
> > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > lockdep warning at boot time and then no more warnings afterward?
> > > >
> > > > I means the latter one.
> > >
> > > That's very bad.
> > >
> > > For each type of violation, lockdep only gives one error message. So
> > > the fact that you get one message at boot time and then no more doesn't
> > > mean the code is almost right -- it probably means the code has lots of
> > > errors and you're seeing only the first one.
> >
> > I hope it's better than this: lockdep really stops checking after first
> > warning, but I've understood from David's description that after fixing
> > this one place lockdep seems to be pleased.
>
> That isn't what Dave said above; he said that lockdep produces a single
> warning at bootup. If he did mention anything about one place being
> fixed up or lockdep being pleased, it was a while back and I've lost
> track of it.
>
> If I recall correctly the nature of the warning was that a method
> routine for one class (called with the class's mutex held) was creating
> a second class and locking that class's mutex. In principle this is
> perfectly legal and should be allowed for arbitrary depths of nesting,
> even though it is the sort of thing lockdep is currently unable to
> handle.
You are definitely right! After first reading Dave's description I got
it the same way, but after re-reading I probably was misled with this
"thus"! Only now I've had a look at this warning and there is really
mutex_lock_nested(). Sorry Alan!
David, I don't think a patch which causes such a warning can be merged
even to -mm, because, as I wrote earlier it would automatically turn
off lockdep for everybody. So, every such warning needs to be fixed or,
if it's impossible because of some lockdep deficiency, it should be
considered if it's better to wait for lockdep changes, or do the change
with lockdep turned off locally for each lock (IMHO, it's better,
because with sems there is no such control as well, and some other
aspects could be tested in the meantime).
> > On the other hand, according to Greg the code is OK, so if there are any
> > such warnings they simply have to be false! (...Unless you trust lockdep
> > more?!)
>
> It's not a matter of trust or of false warnings. People shouldn't
> tolerate any lockdep warnings at all; otherwise they will start to
> ignore the valid ones.
>
> Alan Stern
>
> P.S.: Just because Greg says the code is okay doesn't mean it will
> please lockdep -- it doesn't even mean the code really is okay! I've
> known Greg to make an occasional mistake.
Alan, you are 200% right! I apologize for my bad jokes too!
Regards,
Jarek P.
On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> > On the other hand, according to Greg the code is OK, so if there are any
> > such warnings they simply have to be false! (...Unless you trust lockdep
> > more?!)
>
> It's not a matter of trust or of false warnings. People shouldn't
> tolerate any lockdep warnings at all; otherwise they will start to
> ignore the valid ones.
>
> Alan Stern
>
> P.S.: Just because Greg says the code is okay doesn't mean it will
> please lockdep -- it doesn't even mean the code really is okay! I've
> known Greg to make an occasional mistake.
I've known Greg to make lots of mistakes :)
I don't remember ever saying that the "code is correct with the lockdep
warnings", I think I said, "Make sure there are no lockdep warnings with
any conversion you do."
thanks,
greg k-h
On Thu, Jan 17, 2008 at 01:11:01PM -0800, Greg KH wrote:
...
> I've known Greg to make lots of mistakes :)
Right! Above is one example...
> I don't remember ever saying that the "code is correct with the lockdep
> warnings", I think I said, "Make sure there are no lockdep warnings with
> any conversion you do."
I've meant your discussion with David Miller, where you doubted the need
to change properly working code. But, as a matter of fact, I really can't
see any reason to doubt this, and care so much about lockdep now, if there
are no known problems with lockups, and this change has been done done
"one for one".
Sorry again for any possible misleading,
Jarek P.
On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
...
> > If I recall correctly the nature of the warning was that a method
> > routine for one class (called with the class's mutex held) was creating
> > a second class and locking that class's mutex. In principle this is
> > perfectly legal and should be allowed for arbitrary depths of nesting,
> > even though it is the sort of thing lockdep is currently unable to
> > handle.
BTW, Dave, if it's only about one such "second class" here, then it
shouldn't be so hard to try this one more level of nesting. I think,
the real problem for lockdep starts when there are more such "second
classes", but it's probably in another place.
You could also have a look at e.g. enum_inode_i_mutex_lock_class in
include/linux/fs.h and fh_lock_nested() in include/linux/nfsd/nfsfh.h,
and maybe define similar enum for this.
Jarek P.
On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> > On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> >
> > > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > > On Thu, 17 Jan 2008, Dave Young wrote:
> > > >
> > > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > > lockdep warning at boot time and then no more warnings afterward?
> > > > >
> > > > > I means the latter one.
> > > >
> > > > That's very bad.
> > > >
> > > > For each type of violation, lockdep only gives one error message. So
> > > > the fact that you get one message at boot time and then no more doesn't
> > > > mean the code is almost right -- it probably means the code has lots of
> > > > errors and you're seeing only the first one.
> > >
> > > I hope it's better than this: lockdep really stops checking after first
> > > warning, but I've understood from David's description that after fixing
> > > this one place lockdep seems to be pleased.
> >
> > That isn't what Dave said above; he said that lockdep produces a single
> > warning at bootup. If he did mention anything about one place being
> > fixed up or lockdep being pleased, it was a while back and I've lost
> > track of it.
> >
> > If I recall correctly the nature of the warning was that a method
> > routine for one class (called with the class's mutex held) was creating
> > a second class and locking that class's mutex. In principle this is
> > perfectly legal and should be allowed for arbitrary depths of nesting,
> > even though it is the sort of thing lockdep is currently unable to
> > handle.
>
> You are definitely right! After first reading Dave's description I got
> it the same way, but after re-reading I probably was misled with this
> "thus"! Only now I've had a look at this warning and there is really
> mutex_lock_nested(). Sorry Alan!
But, on the other hand, mutex_lock() is really mutex_lock_nested(), and
after second checking this lockdep warning from Jan. 3, it seems
impossible it was get after this patch...
Dave, could you please answer with full sentence if there is any lockdep
warning possible after applying these 1-7/7 patches, and if so, attach
current warning? Otherwise, I'll have apologized for this everybody from
the list soon!
Jarek P.
On Jan 18, 2008 7:26 AM, Jarek Poplawski <[email protected]> wrote:
>
> On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> > On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> > > On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> > >
> > > > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > > > On Thu, 17 Jan 2008, Dave Young wrote:
> > > > >
> > > > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > > > lockdep warning at boot time and then no more warnings afterward?
> > > > > >
> > > > > > I means the latter one.
> > > > >
> > > > > That's very bad.
> > > > >
> > > > > For each type of violation, lockdep only gives one error message. So
> > > > > the fact that you get one message at boot time and then no more doesn't
> > > > > mean the code is almost right -- it probably means the code has lots of
> > > > > errors and you're seeing only the first one.
> > > >
> > > > I hope it's better than this: lockdep really stops checking after first
> > > > warning, but I've understood from David's description that after fixing
> > > > this one place lockdep seems to be pleased.
> > >
> > > That isn't what Dave said above; he said that lockdep produces a single
> > > warning at bootup. If he did mention anything about one place being
> > > fixed up or lockdep being pleased, it was a while back and I've lost
> > > track of it.
> > >
> > > If I recall correctly the nature of the warning was that a method
> > > routine for one class (called with the class's mutex held) was creating
> > > a second class and locking that class's mutex. In principle this is
> > > perfectly legal and should be allowed for arbitrary depths of nesting,
> > > even though it is the sort of thing lockdep is currently unable to
> > > handle.
> >
> > You are definitely right! After first reading Dave's description I got
> > it the same way, but after re-reading I probably was misled with this
> > "thus"! Only now I've had a look at this warning and there is really
> > mutex_lock_nested(). Sorry Alan!
>
> But, on the other hand, mutex_lock() is really mutex_lock_nested(), and
> after second checking this lockdep warning from Jan. 3, it seems
> impossible it was get after this patch...
>
> Dave, could you please answer with full sentence if there is any lockdep
> warning possible after applying these 1-7/7 patches, and if so, attach
> current warning? Otherwise, I'll have apologized for this everybody from
> the list soon!
After digging the class usage code again, I found that the only
possible double lock place is the class_interface_register/unregister
in which the class_device api could be called.
The scsi and pcmcia use the class_interface api, I just found the
warning above caused by scsi part then.
So I think I will need to use mutex_lock_nesting for the
class_device_* functions.
Thank you a lot.
>
> Jarek P.
>
On Jan 17, 2008 11:16 PM, Alan Stern <[email protected]> wrote:
> On Thu, 17 Jan 2008, Dave Young wrote:
>
> > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > any lockdep warnings at all? Or do you mean that it generates a single
> > > lockdep warning at boot time and then no more warnings afterward?
> >
> > I means the latter one.
>
> That's very bad.
>
> For each type of violation, lockdep only gives one error message. So
> the fact that you get one message at boot time and then no more doesn't
> mean the code is almost right -- it probably means the code has lots of
> errors and you're seeing only the first one.
You are right, I should have check all possible one.
>
> Alan Stern
>
>
On Jan 18, 2008 2:42 AM, Dave Young <[email protected]> wrote:
>
> On Jan 18, 2008 7:26 AM, Jarek Poplawski <[email protected]> wrote:
> >
> > On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> > > On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> > > > On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> > > >
> > > > > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > > > > On Thu, 17 Jan 2008, Dave Young wrote:
> > > > > >
> > > > > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > > > > lockdep warning at boot time and then no more warnings afterward?
> > > > > > >
> > > > > > > I means the latter one.
> > > > > >
> > > > > > That's very bad.
> > > > > >
> > > > > > For each type of violation, lockdep only gives one error message. So
> > > > > > the fact that you get one message at boot time and then no more doesn't
> > > > > > mean the code is almost right -- it probably means the code has lots of
> > > > > > errors and you're seeing only the first one.
> > > > >
> > > > > I hope it's better than this: lockdep really stops checking after first
> > > > > warning, but I've understood from David's description that after fixing
> > > > > this one place lockdep seems to be pleased.
> > > >
> > > > That isn't what Dave said above; he said that lockdep produces a single
> > > > warning at bootup. If he did mention anything about one place being
> > > > fixed up or lockdep being pleased, it was a while back and I've lost
> > > > track of it.
> > > >
> > > > If I recall correctly the nature of the warning was that a method
> > > > routine for one class (called with the class's mutex held) was creating
> > > > a second class and locking that class's mutex. In principle this is
> > > > perfectly legal and should be allowed for arbitrary depths of nesting,
> > > > even though it is the sort of thing lockdep is currently unable to
> > > > handle.
> > >
> > > You are definitely right! After first reading Dave's description I got
> > > it the same way, but after re-reading I probably was misled with this
> > > "thus"! Only now I've had a look at this warning and there is really
> > > mutex_lock_nested(). Sorry Alan!
> >
> > But, on the other hand, mutex_lock() is really mutex_lock_nested(), and
> > after second checking this lockdep warning from Jan. 3, it seems
> > impossible it was get after this patch...
> >
> > Dave, could you please answer with full sentence if there is any lockdep
> > warning possible after applying these 1-7/7 patches, and if so, attach
> > current warning? Otherwise, I'll have apologized for this everybody from
> > the list soon!
>
> After digging the class usage code again, I found that the only
> possible double lock place is the class_interface_register/unregister
> in which the class_device api could be called.
>
> The scsi and pcmcia use the class_interface api, I just found the
> warning above caused by scsi part then.
>
> So I think I will need to use mutex_lock_nesting for the
> class_device_* functions.
All "struct class_device" stuff will go away very soon, and only
"struct device" will stay.
The conversion for remaining users is already in -mm. Only SCSI and IB
are missing,
but experimental patches for these exist already.
Kay
On Jan 18, 2008 9:55 AM, Kay Sievers <[email protected]> wrote:
>
> On Jan 18, 2008 2:42 AM, Dave Young <[email protected]> wrote:
> >
> > On Jan 18, 2008 7:26 AM, Jarek Poplawski <[email protected]> wrote:
> > >
> > > On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> > > > On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> > > > > On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> > > > >
> > > > > > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > > > > > On Thu, 17 Jan 2008, Dave Young wrote:
> > > > > > >
> > > > > > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > > > > > lockdep warning at boot time and then no more warnings afterward?
> > > > > > > >
> > > > > > > > I means the latter one.
> > > > > > >
> > > > > > > That's very bad.
> > > > > > >
> > > > > > > For each type of violation, lockdep only gives one error message. So
> > > > > > > the fact that you get one message at boot time and then no more doesn't
> > > > > > > mean the code is almost right -- it probably means the code has lots of
> > > > > > > errors and you're seeing only the first one.
> > > > > >
> > > > > > I hope it's better than this: lockdep really stops checking after first
> > > > > > warning, but I've understood from David's description that after fixing
> > > > > > this one place lockdep seems to be pleased.
> > > > >
> > > > > That isn't what Dave said above; he said that lockdep produces a single
> > > > > warning at bootup. If he did mention anything about one place being
> > > > > fixed up or lockdep being pleased, it was a while back and I've lost
> > > > > track of it.
> > > > >
> > > > > If I recall correctly the nature of the warning was that a method
> > > > > routine for one class (called with the class's mutex held) was creating
> > > > > a second class and locking that class's mutex. In principle this is
> > > > > perfectly legal and should be allowed for arbitrary depths of nesting,
> > > > > even though it is the sort of thing lockdep is currently unable to
> > > > > handle.
> > > >
> > > > You are definitely right! After first reading Dave's description I got
> > > > it the same way, but after re-reading I probably was misled with this
> > > > "thus"! Only now I've had a look at this warning and there is really
> > > > mutex_lock_nested(). Sorry Alan!
> > >
> > > But, on the other hand, mutex_lock() is really mutex_lock_nested(), and
> > > after second checking this lockdep warning from Jan. 3, it seems
> > > impossible it was get after this patch...
> > >
> > > Dave, could you please answer with full sentence if there is any lockdep
> > > warning possible after applying these 1-7/7 patches, and if so, attach
> > > current warning? Otherwise, I'll have apologized for this everybody from
> > > the list soon!
> >
> > After digging the class usage code again, I found that the only
> > possible double lock place is the class_interface_register/unregister
> > in which the class_device api could be called.
> >
> > The scsi and pcmcia use the class_interface api, I just found the
> > warning above caused by scsi part then.
> >
> > So I think I will need to use mutex_lock_nesting for the
> > class_device_* functions.
>
> All "struct class_device" stuff will go away very soon, and only
> "struct device" will stay.
> The conversion for remaining users is already in -mm. Only SCSI and IB
> are missing,
> but experimental patches for these exist already.
>
Hi, kay
Then what's your opinon about the lockdep warning fix? I wonder
whether the "soon" means we should do mutex convert after the
class_device going away?
Regards
dave
On Fri, 2008-01-18 at 10:28 +0800, Dave Young wrote:
> On Jan 18, 2008 9:55 AM, Kay Sievers <[email protected]> wrote:
> >
> > On Jan 18, 2008 2:42 AM, Dave Young <[email protected]> wrote:
> > >
> > > On Jan 18, 2008 7:26 AM, Jarek Poplawski <[email protected]> wrote:
> > > >
> > > > On Thu, Jan 17, 2008 at 09:31:55PM +0100, Jarek Poplawski wrote:
> > > > > On Thu, Jan 17, 2008 at 02:57:36PM -0500, Alan Stern wrote:
> > > > > > On Thu, 17 Jan 2008, Jarek Poplawski wrote:
> > > > > >
> > > > > > > On Thu, Jan 17, 2008 at 10:16:30AM -0500, Alan Stern wrote:
> > > > > > > > On Thu, 17 Jan 2008, Dave Young wrote:
> > > > > > > >
> > > > > > > > > > Your meaning isn't clear. Do you mean that your patch doesn't generate
> > > > > > > > > > any lockdep warnings at all? Or do you mean that it generates a single
> > > > > > > > > > lockdep warning at boot time and then no more warnings afterward?
> > > > > > > > >
> > > > > > > > > I means the latter one.
> > > > > > > >
> > > > > > > > That's very bad.
> > > > > > > >
> > > > > > > > For each type of violation, lockdep only gives one error message. So
> > > > > > > > the fact that you get one message at boot time and then no more doesn't
> > > > > > > > mean the code is almost right -- it probably means the code has lots of
> > > > > > > > errors and you're seeing only the first one.
> > > > > > >
> > > > > > > I hope it's better than this: lockdep really stops checking after first
> > > > > > > warning, but I've understood from David's description that after fixing
> > > > > > > this one place lockdep seems to be pleased.
> > > > > >
> > > > > > That isn't what Dave said above; he said that lockdep produces a single
> > > > > > warning at bootup. If he did mention anything about one place being
> > > > > > fixed up or lockdep being pleased, it was a while back and I've lost
> > > > > > track of it.
> > > > > >
> > > > > > If I recall correctly the nature of the warning was that a method
> > > > > > routine for one class (called with the class's mutex held) was creating
> > > > > > a second class and locking that class's mutex. In principle this is
> > > > > > perfectly legal and should be allowed for arbitrary depths of nesting,
> > > > > > even though it is the sort of thing lockdep is currently unable to
> > > > > > handle.
> > > > >
> > > > > You are definitely right! After first reading Dave's description I got
> > > > > it the same way, but after re-reading I probably was misled with this
> > > > > "thus"! Only now I've had a look at this warning and there is really
> > > > > mutex_lock_nested(). Sorry Alan!
> > > >
> > > > But, on the other hand, mutex_lock() is really mutex_lock_nested(), and
> > > > after second checking this lockdep warning from Jan. 3, it seems
> > > > impossible it was get after this patch...
> > > >
> > > > Dave, could you please answer with full sentence if there is any lockdep
> > > > warning possible after applying these 1-7/7 patches, and if so, attach
> > > > current warning? Otherwise, I'll have apologized for this everybody from
> > > > the list soon!
> > >
> > > After digging the class usage code again, I found that the only
> > > possible double lock place is the class_interface_register/unregister
> > > in which the class_device api could be called.
> > >
> > > The scsi and pcmcia use the class_interface api, I just found the
> > > warning above caused by scsi part then.
> > >
> > > So I think I will need to use mutex_lock_nesting for the
> > > class_device_* functions.
> >
> > All "struct class_device" stuff will go away very soon, and only
> > "struct device" will stay.
> > The conversion for remaining users is already in -mm. Only SCSI and IB
> > are missing,
> > but experimental patches for these exist already.
> Then what's your opinon about the lockdep warning fix? I wonder
> whether the "soon" means we should do mutex convert after the
> class_device going away?
Yeah, might be better to wait until class_device is gone, otherwise you
may need to fix stuff that is just going to be removed. Your change to
have iterators for the class devices look like a nice preparation for
future changes though.
Our rough plan is:
2.6.25:
- get the ~100 patches in Greg's tree (in -mm) merged :)
2.6.26:
 - remove the 20 char limit in "struct device"
- get rid of "struct class_device"
2.6.27
- merge most of "struct class" and "struct bus_type" and have
only one type of list and iterator for all devices of all subsystems
2.6.27+
- allow multiple "drivers" to bind a single device (now that the
difference between class and bus devices is gone)
Thanks,
Kay
On Fri, Jan 18, 2008 at 04:18:43AM +0100, Kay Sievers wrote:
> On Fri, 2008-01-18 at 10:28 +0800, Dave Young wrote:
> > Then what's your opinon about the lockdep warning fix? I wonder
> > whether the "soon" means we should do mutex convert after the
> > class_device going away?
>
> Yeah, might be better to wait until class_device is gone, otherwise you
> may need to fix stuff that is just going to be removed. Your change to
> have iterators for the class devices look like a nice preparation for
> future changes though.
>
> Our rough plan is:
> 2.6.25:
> - get the ~100 patches in Greg's tree (in -mm) merged :)
> 2.6.26:
> ??? - remove the 20 char limit in "struct device"
> - get rid of "struct class_device"
> 2.6.27
> - merge most of "struct class" and "struct bus_type" and have
> only one type of list and iterator for all devices of all subsystems
> 2.6.27+
> - allow multiple "drivers" to bind a single device (now that the
> difference between class and bus devices is gone)
I'm hoping that we might be able to get this last one by 2.6.26 or .27,
depending on how much time I get to work on it...
Other than that, I agree with this timeline.
thanks,
greg k-h
On Fri, Jan 18, 2008 at 01:31:17PM +0800, Dave Young wrote:
> On Jan 18, 2008 11:18 AM, Kay Sievers <[email protected]> wrote:
...
> > Yeah, might be better to wait until class_device is gone, otherwise you
> > may need to fix stuff that is just going to be removed. Your change to
> > have iterators for the class devices look like a nice preparation for
> > future changes though.
> >
> > Our rough plan is:
> > 2.6.25:
> > - get the ~100 patches in Greg's tree (in -mm) merged :)
> > 2.6.26:
> > ??? - remove the 20 char limit in "struct device"
> > - get rid of "struct class_device"
>
> Fine, thanks.
>
> Let's wait for other people's comment.
Dave, I doubt you'll ever manage to do this if you're going to wait:
probably there will be always some new changes like this around...
IMHO, it would be nice to get the real state of current lockdep
problems here to figure out if there is any chance to do this right &
without any warnings with current lockdep. If I got it right from
earlier threads it might be impossible with USB, at least.
So, since I think these nesting levels seem to be wrong in 7/7 patch,
maybe it's better to exclude it from this patchset, and to try this as
testing for some time.
My proposal is to do the annotations with mutex_lock_nested(),
everywhere in this patch, according to 'real' relations between these
classes from thread POV, so e.g.:
mutex_lock_nested(&some_class->mutex, CLASS_PARENT);
mutex_lock_nested(&some_class->mutex, CLASS_CLASS);
...or more if needed:
mutex_lock_nested(&some_class->mutex, CLASS_CHILD);
mutex_lock_nested(&some_class->mutex, CLASS_ROOT);
...etc.
using adequate names for these enums, and only after this check what
lockdep thinks about it. Then, if there are no obvious mistakes, but
lockdep doesn't like it, send the patch and report without trying to
'silence' lockdep, so we could see what's going on, and if there are
any chances to make it right.
Thanks,
Jarek P.
On Jan 18, 2008 3:38 PM, Jarek Poplawski <[email protected]> wrote:
> On Fri, Jan 18, 2008 at 01:31:17PM +0800, Dave Young wrote:
> > On Jan 18, 2008 11:18 AM, Kay Sievers <[email protected]> wrote:
> ...
> > > Yeah, might be better to wait until class_device is gone, otherwise you
> > > may need to fix stuff that is just going to be removed. Your change to
> > > have iterators for the class devices look like a nice preparation for
> > > future changes though.
> > >
> > > Our rough plan is:
> > > 2.6.25:
> > > - get the ~100 patches in Greg's tree (in -mm) merged :)
> > > 2.6.26:
> > > ??? - remove the 20 char limit in "struct device"
> > > - get rid of "struct class_device"
> >
> > Fine, thanks.
> >
> > Let's wait for other people's comment.
>
> Dave, I doubt you'll ever manage to do this if you're going to wait:
> probably there will be always some new changes like this around...
Maybe you are right.
>
> IMHO, it would be nice to get the real state of current lockdep
> problems here to figure out if there is any chance to do this right &
> without any warnings with current lockdep. If I got it right from
> earlier threads it might be impossible with USB, at least.
I don't think so, usb doesn't be affected by struct class mutex, they
only use the lock of struct device. As I replied before, the lockdep
issue exist only between class_interface and class_device.
>
> So, since I think these nesting levels seem to be wrong in 7/7 patch,
> maybe it's better to exclude it from this patchset, and to try this as
> testing for some time.
I may file the updated patch with more nesting changes and test it of
course. Actually I should have done it, thanks.
>
> My proposal is to do the annotations with mutex_lock_nested(),
> everywhere in this patch, according to 'real' relations between these
> classes from thread POV, so e.g.:
>
> mutex_lock_nested(&some_class->mutex, CLASS_PARENT);
> mutex_lock_nested(&some_class->mutex, CLASS_CLASS);
> ...or more if needed:
> mutex_lock_nested(&some_class->mutex, CLASS_CHILD);
> mutex_lock_nested(&some_class->mutex, CLASS_ROOT);
> ...etc.
>
> using adequate names for these enums, and only after this check what
> lockdep thinks about it. Then, if there are no obvious mistakes, but
> lockdep doesn't like it, send the patch and report without trying to
> 'silence' lockdep, so we could see what's going on, and if there are
> any chances to make it right.
1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
or
2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
class_device functions because it is the only possible nest-lock place
as I know.
Do you agree?
>
> Thanks,
> Jarek P.
>
On Fri, Jan 18, 2008 at 09:42:25AM +0800, Dave Young wrote:
...
> After digging the class usage code again, I found that the only
> possible double lock place is the class_interface_register/unregister
> in which the class_device api could be called.
OK, but currently after using mostly:
mutex_lock(&parent_class)
and once:
mutex_lock_nested(&parent_class, SINGLE_DEPTH_NESTING)
lockdep mostly thinks these parent classes are 2 different objects,
with only 2 possible levels of nesting, so this parent_class has
to have wrong name (2 parents can't be locked from the same thread,
so maybe it's class_grandparent sometimes?).
Jarek P.
On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
> On Jan 18, 2008 3:38 PM, Jarek Poplawski <[email protected]> wrote:
...
> > IMHO, it would be nice to get the real state of current lockdep
> > problems here to figure out if there is any chance to do this right &
> > without any warnings with current lockdep. If I got it right from
> > earlier threads it might be impossible with USB, at least.
>
> I don't think so, usb doesn't be affected by struct class mutex, they
> only use the lock of struct device. As I replied before, the lockdep
> issue exist only between class_interface and class_device.
OK, but I've meant possibility of changing their own semaphores later.
> > So, since I think these nesting levels seem to be wrong in 7/7 patch,
> > maybe it's better to exclude it from this patchset, and to try this as
> > testing for some time.
>
> I may file the updated patch with more nesting changes and test it of
> course. Actually I should have done it, thanks.
...
> 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
> or
> 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
> class_device functions because it is the only possible nest-lock place
> as I know.
If SINGLE_LEVEL_NESTING is enough? (means 2 levels total)
I think you should more care about real (logical) relations here, than
what's enough to get rid of lockdep warnings.
Since there are not so much of these changes, you can try both
variants. I'll be glad to look at this - maybe I'll mangage to figure
out BTW, what it's all about...
Jarek P.
On Fri, Jan 18, 2008 at 09:00:34AM +0100, Jarek Poplawski wrote:
> On Fri, Jan 18, 2008 at 09:42:25AM +0800, Dave Young wrote:
> ...
> > After digging the class usage code again, I found that the only
> > possible double lock place is the class_interface_register/unregister
> > in which the class_device api could be called.
>
> OK, but currently after using mostly:
> mutex_lock(&parent_class)
>
> and once:
> mutex_lock_nested(&parent_class, SINGLE_DEPTH_NESTING)
>
> lockdep mostly thinks these parent classes are 2 different objects,
> with only 2 possible levels of nesting, so this parent_class has
> to have wrong name (2 parents can't be locked from the same thread,
> so maybe it's class_grandparent sometimes?).
...Hmm... I was probably wrong: this could be right if there are only
two levels of nesting used and class locks it's parent only!
Jarek P.
On Jan 18, 2008 4:23 PM, Jarek Poplawski <[email protected]> wrote:
> On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
> > On Jan 18, 2008 3:38 PM, Jarek Poplawski <[email protected]> wrote:
> ...
> > > IMHO, it would be nice to get the real state of current lockdep
> > > problems here to figure out if there is any chance to do this right &
> > > without any warnings with current lockdep. If I got it right from
> > > earlier threads it might be impossible with USB, at least.
> >
> > I don't think so, usb doesn't be affected by struct class mutex, they
> > only use the lock of struct device. As I replied before, the lockdep
> > issue exist only between class_interface and class_device.
>
> OK, but I've meant possibility of changing their own semaphores later.
>
> > > So, since I think these nesting levels seem to be wrong in 7/7 patch,
> > > maybe it's better to exclude it from this patchset, and to try this as
> > > testing for some time.
> >
> > I may file the updated patch with more nesting changes and test it of
> > course. Actually I should have done it, thanks.
> ...
> > 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
> > or
> > 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
> > class_device functions because it is the only possible nest-lock place
> > as I know.
>
> If SINGLE_LEVEL_NESTING is enough? (means 2 levels total)
I think so.
>
> I think you should more care about real (logical) relations here, than
> what's enough to get rid of lockdep warnings.
You are quite right, thanks.
>
> Since there are not so much of these changes, you can try both
> variants.
Will do.
>I'll be glad to look at this - maybe I'll mangage to figure
> out BTW, what it's all about...
>
> Jarek P.
>
On Fri, 2008-01-18 at 08:38 +0100, Jarek Poplawski wrote:
> On Fri, Jan 18, 2008 at 01:31:17PM +0800, Dave Young wrote:
> > On Jan 18, 2008 11:18 AM, Kay Sievers <[email protected]> wrote:
> ...
> > > Yeah, might be better to wait until class_device is gone, otherwise you
> > > may need to fix stuff that is just going to be removed. Your change to
> > > have iterators for the class devices look like a nice preparation for
> > > future changes though.
> > >
> > > Our rough plan is:
> > > 2.6.25:
> > > - get the ~100 patches in Greg's tree (in -mm) merged :)
> > > 2.6.26:
> > > ??? - remove the 20 char limit in "struct device"
> > > - get rid of "struct class_device"
> >
> > Fine, thanks.
> >
> > Let's wait for other people's comment.
>
> Dave, I doubt you'll ever manage to do this if you're going to wait:
> probably there will be always some new changes like this around...
Well there are not "changes" in that sense, the class_device stuff will
be entirely ripped out, and I doubt we will want to change anything
there, just shortly before it's deleted.
Also your assumptions about device nesting are not really true, there is
no limit, even when there are no current users nesting deeper, and
"struct device" can be any nesting depth, and that's where it gets
interesting.
Kay
On Fri, Jan 18, 2008 at 11:45:12AM +0100, Kay Sievers wrote:
> On Fri, 2008-01-18 at 08:38 +0100, Jarek Poplawski wrote:
> > On Fri, Jan 18, 2008 at 01:31:17PM +0800, Dave Young wrote:
> > > On Jan 18, 2008 11:18 AM, Kay Sievers <[email protected]> wrote:
> > ...
> > > > Yeah, might be better to wait until class_device is gone, otherwise you
> > > > may need to fix stuff that is just going to be removed. Your change to
> > > > have iterators for the class devices look like a nice preparation for
> > > > future changes though.
> > > >
> > > > Our rough plan is:
> > > > 2.6.25:
> > > > - get the ~100 patches in Greg's tree (in -mm) merged :)
> > > > 2.6.26:
> > > > ??? - remove the 20 char limit in "struct device"
> > > > - get rid of "struct class_device"
> > >
> > > Fine, thanks.
> > >
> > > Let's wait for other people's comment.
> >
> > Dave, I doubt you'll ever manage to do this if you're going to wait:
> > probably there will be always some new changes like this around...
>
> Well there are not "changes" in that sense, the class_device stuff will
> be entirely ripped out, and I doubt we will want to change anything
> there, just shortly before it's deleted.
So, 2.6.26 means shortly... And this all needs some time for testing,
debugging or maybe some change of concept, so this would take a while...
Well, it's not my problem, but since this stuff will go away, shouldn't
we care more about the staff that will stay?
> Also your assumptions about device nesting are not really true, there is
> no limit, even when there are no current users nesting deeper, and
> "struct device" can be any nesting depth, and that's where it gets
> interesting.
I'm just trying to figure this out. It seems this is a real problem
while freezing, but not necessarily here (but I can miss something).
Regards,
Jarek P.
Dave Young wrote, On 01/18/2008 10:07 AM:
> On Jan 18, 2008 4:23 PM, Jarek Poplawski <[email protected]> wrote:
>> On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
...
>>> 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
>>> or
>>> 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
>>> class_device functions because it is the only possible nest-lock place
>>> as I know.
Dave, after looking a bit at this it seems you could be "mostly" right
with this 2). Maybe I've missed something (I didn't verify this yet), but
it looks like +1 level (SINGLE_LEVEL_NESTING) could be needed in:
class_device_add() (as you did), but probably also class_device_del() and
class_device_destroy().
...But, there seems to be "little" problem, if there is used this recursion
with: class_intf->add()/remove() in class_device_add()/del()?! Then Kay
is right about possibility of deeper nesting. If this path is really used,
and any of these class_device_* functions with locking are called, then
this patch couldn't work like this. So, there is a question: how deep
nesting is currently used here?
Regards,
Jarek P.
On Sat, Jan 19, 2008 at 10:39:33AM +0100, Jarek Poplawski wrote:
> Dave Young wrote, On 01/18/2008 10:07 AM:
>
> > On Jan 18, 2008 4:23 PM, Jarek Poplawski <[email protected]> wrote:
>
> >> On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
>
> ...
>
> >>> 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
> >>> or
> >>> 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
> >>> class_device functions because it is the only possible nest-lock place
> >>> as I know.
>
>
> Dave, after looking a bit at this it seems you could be "mostly" right
> with this 2). Maybe I've missed something (I didn't verify this yet), but
> it looks like +1 level (SINGLE_LEVEL_NESTING) could be needed in:
> class_device_add() (as you did), but probably also class_device_del() and
> class_device_destroy().
Yes, I think so too.
>
> ...But, there seems to be "little" problem, if there is used this recursion
> with: class_intf->add()/remove() in class_device_add()/del()?! Then Kay
> is right about possibility of deeper nesting. If this path is really used,
> and any of these class_device_* functions with locking are called, then
> this patch couldn't work like this. So, there is a question: how deep
> nesting is currently used here?
Currently I couldn't find such use in kernel source. IMO, drivers would not use it like this in the future because class_device will going away soon.
>
> Regards,
> Jarek P.
On Sat, Jan 19, 2008 at 10:39:33AM +0100, Jarek Poplawski wrote:
> Dave Young wrote, On 01/18/2008 10:07 AM:
>
> > On Jan 18, 2008 4:23 PM, Jarek Poplawski <[email protected]> wrote:
>
> >> On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
>
> ...
>
> >>> 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
> >>> or
> >>> 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
> >>> class_device functions because it is the only possible nest-lock place
> >>> as I know.
>
>
Althoug the class_device will be away soon, the patch is updated as following because the problem is clear and only several lines are changed. At least, it could be used for test.
Convert the class semaphore to mutex.
class_interface_register/unregister use class_device_* functions, so SINGLE_DEPTH_NESTING added for lockdep please in these functions.
Signed-off-by: Dave Young <[email protected]>
---
drivers/base/class.c | 38 +++++++++++++++++++-------------------
drivers/base/core.c | 18 ++++++++----------
include/linux/device.h | 3 ++-
3 files changed, 29 insertions(+), 30 deletions(-)
Index: linux-2.6.23/drivers/base/class.c
===================================================================
--- linux-2.6.23.orig/drivers/base/class.c 2008-01-17 15:19:05.000000000 +0800
+++ linux-2.6.23/drivers/base/class.c 2008-01-18 15:56:00.000000000 +0800
@@ -144,7 +144,7 @@
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 @@
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 @@
struct class_interface *class_intf;
if (parent_class) {
- down(&parent_class->sem);
+ mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
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 @@
struct class_device *class_dev = NULL;
struct class_device *class_dev_tmp;
- down(&cls->sem);
+ mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
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);
@@ -818,7 +818,7 @@
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -829,7 +829,7 @@
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -860,7 +860,7 @@
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -872,7 +872,7 @@
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -898,7 +898,7 @@
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -909,7 +909,7 @@
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -940,7 +940,7 @@
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -952,7 +952,7 @@
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -971,7 +971,7 @@
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)
@@ -981,7 +981,7 @@
list_for_each_entry(dev, &parent->devices, node)
class_intf->add_dev(dev, class_intf);
}
- up(&parent->sem);
+ mutex_unlock(&parent->mutex);
return 0;
}
@@ -995,7 +995,7 @@
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)
@@ -1005,7 +1005,7 @@
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);
}
Index: linux-2.6.23/drivers/base/core.c
===================================================================
--- linux-2.6.23.orig/drivers/base/core.c 2008-01-17 15:19:05.000000000 +0800
+++ linux-2.6.23/drivers/base/core.c 2008-01-18 11:30:51.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"
@@ -590,6 +588,7 @@
spin_lock(&dev->class->class_dirs.list_lock);
list_for_each_entry(k, &dev->class->class_dirs.list, entry)
if (k->parent == parent_kobj) {
+ printk("hidave : get reference of k\n");
kobj = kobject_get(k);
break;
}
@@ -783,7 +782,7 @@
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 +790,7 @@
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 +927,14 @@
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 +945,7 @@
* 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 +958,7 @@
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 +1167,14 @@
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);
@@ -1320,10 +1319,8 @@
pr_debug("DEVICE: moving '%s' to '%s'\n", dev->bus_id,
new_parent ? new_parent->bus_id : "<NULL>");
error = kobject_move(&dev->kobj, new_parent_kobj);
- if (error) {
- put_device(new_parent);
- goto out;
- }
+ if (error)
+ goto out_put_new_parent;
old_parent = dev->parent;
dev->parent = new_parent;
if (old_parent)
@@ -1331,7 +1328,7 @@
if (new_parent)
klist_add_tail(&dev->knode_parent, &new_parent->klist_children);
if (!dev->class)
- goto out_put;
+ goto out_put_old;
error = device_move_class_links(dev, old_parent, new_parent);
if (error) {
/* We ignore errors on cleanup since we're hosed anyway... */
@@ -1343,11 +1340,15 @@
klist_add_tail(&dev->knode_parent,
&old_parent->klist_children);
}
- put_device(new_parent);
- goto out;
+ goto out_put_new;
}
-out_put:
+out_put_old:
put_device(old_parent);
+out_put_new:
+ if (new_parent)
+ put_device(new_parent);
+ else
+ kobject_put(new_parent_kobj);
out:
put_device(dev);
return error;
Index: linux-2.6.23/include/linux/device.h
===================================================================
--- linux-2.6.23.orig/include/linux/device.h 2008-01-17 15:19:05.000000000 +0800
+++ linux-2.6.23/include/linux/device.h 2008-01-17 17:45:34.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 list_head devices;
struct list_head interfaces;
struct kset class_dirs;
- struct semaphore sem; /* locks children, devices, interfaces */
+ struct mutex mutex; /* locks children, devices, interfaces */
struct class_attribute * class_attrs;
struct class_device_attribute * class_dev_attrs;
struct device_attribute * dev_attrs;
On Mon, Jan 21, 2008 at 09:30:21AM +0800, Dave Young wrote:
> On Sat, Jan 19, 2008 at 10:39:33AM +0100, Jarek Poplawski wrote:
> > Dave Young wrote, On 01/18/2008 10:07 AM:
> >
> > > On Jan 18, 2008 4:23 PM, Jarek Poplawski <[email protected]> wrote:
> >
> > >> On Fri, Jan 18, 2008 at 03:48:02PM +0800, Dave Young wrote:
> >
> > ...
> >
> > >>> 1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
> > >>> or
> > >>> 2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
> > >>> class_device functions because it is the only possible nest-lock place
> > >>> as I know.
> >
> >
> Althoug the class_device will be away soon, the patch is updated as following because the problem is clear and only several lines are changed. At least, it could be used for test.
>
>
> Convert the class semaphore to mutex.
> class_interface_register/unregister use class_device_* functions, so SINGLE_DEPTH_NESTING added for lockdep please in these functions.
>
> Signed-off-by: Dave Young <[email protected]>
>
Sorry for the dirty patch, please don't test the previous one.
I should have drink a cup of coffe before posting.
post the right one again.
---
Convert the class semaphore to mutex.
class_interface_register/unregister use class_device_* functions, so SINGLE_DEPTH_NESTING added for lockdep please in these functions.
Signed-off-by: Dave Young <[email protected]>
---
drivers/base/class.c | 38 +++++++++++++++++++-------------------
drivers/base/core.c | 18 ++++++++----------
include/linux/device.h | 3 ++-
3 files changed, 29 insertions(+), 30 deletions(-)
Index: linux-2.6.23/drivers/base/class.c
===================================================================
--- linux-2.6.23.orig/drivers/base/class.c 2008-01-21 09:36:43.000000000 +0800
+++ linux-2.6.23/drivers/base/class.c 2008-01-21 09:38:14.000000000 +0800
@@ -144,7 +144,7 @@
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 @@
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 @@
struct class_interface *class_intf;
if (parent_class) {
- down(&parent_class->sem);
+ mutex_lock_nested(&parent_class->mutex, SINGLE_DEPTH_NESTING);
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 @@
struct class_device *class_dev = NULL;
struct class_device *class_dev_tmp;
- down(&cls->sem);
+ mutex_lock_nested(&cls->mutex, SINGLE_DEPTH_NESTING);
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);
@@ -818,7 +818,7 @@
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -829,7 +829,7 @@
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -860,7 +860,7 @@
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->devices, node) {
dev = get_device(dev);
if (dev) {
@@ -872,7 +872,7 @@
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -898,7 +898,7 @@
if (!class)
return -EINVAL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -909,7 +909,7 @@
if (error)
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return error;
}
@@ -940,7 +940,7 @@
if (!class)
return NULL;
- down(&class->sem);
+ mutex_lock(&class->mutex);
list_for_each_entry(dev, &class->children, node) {
dev = class_device_get(dev);
if (dev) {
@@ -952,7 +952,7 @@
} else
break;
}
- up(&class->sem);
+ mutex_unlock(&class->mutex);
return found ? dev : NULL;
}
@@ -971,7 +971,7 @@
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)
@@ -981,7 +981,7 @@
list_for_each_entry(dev, &parent->devices, node)
class_intf->add_dev(dev, class_intf);
}
- up(&parent->sem);
+ mutex_unlock(&parent->mutex);
return 0;
}
@@ -995,7 +995,7 @@
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)
@@ -1005,7 +1005,7 @@
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);
}
Index: linux-2.6.23/drivers/base/core.c
===================================================================
--- linux-2.6.23.orig/drivers/base/core.c 2008-01-21 09:35:37.000000000 +0800
+++ linux-2.6.23/drivers/base/core.c 2008-01-21 09:36:47.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 @@
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 @@
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 @@
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 @@
* 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 @@
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 @@
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);
Index: linux-2.6.23/include/linux/device.h
===================================================================
--- linux-2.6.23.orig/include/linux/device.h 2008-01-21 09:36:43.000000000 +0800
+++ linux-2.6.23/include/linux/device.h 2008-01-21 09:36:47.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 list_head devices;
struct list_head interfaces;
struct kset class_dirs;
- struct semaphore sem; /* locks children, devices, interfaces */
+ struct mutex mutex; /* locks children, devices, interfaces */
struct class_attribute * class_attrs;
struct class_device_attribute * class_dev_attrs;
struct device_attribute * dev_attrs;
On Mon, Jan 21, 2008 at 09:43:35AM +0800, Dave Young wrote:
...
> Convert the class semaphore to mutex.
> class_interface_register/unregister use class_device_* functions, so SINGLE_DEPTH_NESTING added for lockdep please in these functions.
Looks fine to me now, but... I think you forgot again to make it
clear that there were no lockdep warnings observed after this patch.
And it seems there should be used something more complex than usb
mouse or modem to verify this. Then IMHO it would be very nice if
Andrew dares to merge this to -mm...
Regards,
Jarek P.
PS: I miss a lot function names in this patch...
On Jan 21, 2008 4:36 PM, Jarek Poplawski <[email protected]> wrote:
> On Mon, Jan 21, 2008 at 09:43:35AM +0800, Dave Young wrote:
> ...
> > Convert the class semaphore to mutex.
> > class_interface_register/unregister use class_device_* functions, so SINGLE_DEPTH_NESTING added for lockdep please in these functions.
>
> Looks fine to me now, but... I think you forgot again to make it
> clear that there were no lockdep warnings observed after this patch.
> And it seems there should be used something more complex than usb
> mouse or modem to verify this. Then IMHO it would be very nice if
> Andrew dares to merge this to -mm...
I applied it in my kernel, built and run without warnings, but it need
more testing.
I will be very glad to see the test result about this if you could, thanks.
>
> Regards,
> Jarek P.
>
> PS: I miss a lot function names in this patch...
>
On Mon, Jan 21, 2008 at 04:44:36PM +0800, Dave Young wrote:
...
> I applied it in my kernel, built and run without warnings, but it need
> more testing.
> I will be very glad to see the test result about this if you could, thanks.
I'll try this of course, but alas I don't have anything such more
sophisticated to verify this possibility of deeper nesting.
Thanks,
Jarek P.
Dave Young wrote, On 01/21/2008 09:44 AM:
...
> I applied it in my kernel, built and run without warnings, but it need
> more testing.
> I will be very glad to see the test result about this if you could, thanks.
Bad news. (Alas I won't be able to check this today.)
=============================================
[ INFO: possible recursive locking detected ]
2.6.24-rc8 #5
---------------------------------------------
modprobe/536 is trying to acquire lock:
(&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
but task is already holding lock:
(&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
other info that might help us debug this:
2 locks held by modprobe/536:
#0: (&shost->scan_mutex){--..}, at: [<ce82795b>] __scsi_add_device+0x60/0xc9 [scsi_mod]
#1: (&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
stack backtrace:
Pid: 536, comm: modprobe Not tainted 2.6.24-rc8 #5
[<c0138edf>] __lock_acquire+0x962/0x10d4
[<c028be37>] _spin_unlock_irqrestore+0x34/0x39
[<c015e693>] kfree+0x87/0xad
[<c01380cb>] trace_hardirqs_on+0xba/0x15b
[<c01396c2>] lock_acquire+0x71/0x8b
[<c0217e43>] class_device_add+0x2c6/0x322
[<c028a87f>] mutex_lock_nested+0x92/0x2bc
[<c0217e43>] class_device_add+0x2c6/0x322
[<c0217e43>] class_device_add+0x2c6/0x322
[<c0217e43>] class_device_add+0x2c6/0x322
[<c01afd42>] kobject_get+0xf/0x13
[<c01afe68>] kobject_init+0x29/0x38
[<c0217f2c>] class_device_create+0x7d/0x9e
[<cebfe5b4>] sg_add+0x144/0x39f [sg]
[<c0217e71>] class_device_add+0x2f4/0x322
[<ce8289f7>] scsi_sysfs_add_sdev+0x64/0x1ac [scsi_mod]
[<ce840ece>] ata_scsi_dev_config+0x14/0x91 [libata]
[<ce826b5c>] scsi_probe_and_add_lun+0x9c0/0x9e0 [scsi_mod]
[<ce82795b>] __scsi_add_device+0x60/0xc9 [scsi_mod]
[<ce8279c2>] __scsi_add_device+0xc7/0xc9 [scsi_mod]
[<ce841401>] ata_scsi_scan_host+0xd3/0x26a [libata]
[<ce83e7e6>] ata_host_register+0x205/0x280 [libata]
[<ce83f430>] ata_interrupt+0x0/0x200 [libata]
[<ce83e8ec>] ata_host_activate+0x8b/0xf2 [libata]
[<ceb95504>] svia_init_one+0x2e2/0x511 [sata_via]
[<c01ca42c>] pci_match_device+0xa5/0xb6
[<ceb95222>] svia_init_one+0x0/0x511 [sata_via]
[<c01ca4e9>] pci_device_probe+0x40/0x5f
[<c02173ae>] driver_probe_device+0x7c/0x175
[<c02175ed>] __driver_attach+0xa2/0xa4
[<c02168dd>] bus_for_each_dev+0x3c/0x5a
[<c0217263>] driver_attach+0x16/0x1a
[<c021754b>] __driver_attach+0x0/0xa4
[<c0216bea>] bus_add_driver+0x72/0x1c0
[<c01ca642>] __pci_register_driver+0x56/0x89
[<c013f11d>] sys_init_module+0xf8/0x1891
[<ce83901f>] ata_port_start+0x0/0x65 [libata]
[<c01380cb>] trace_hardirqs_on+0xba/0x15b
[<c0102936>] syscall_call+0x7/0xb
=======================
scsi 2:0:0:0: Attached scsi generic sg1 type 0
Regards,
Jarek P.
On Jan 22, 2008 5:16 AM, Jarek Poplawski <[email protected]> wrote:
> Dave Young wrote, On 01/21/2008 09:44 AM:
> ...
> > I applied it in my kernel, built and run without warnings, but it need
> > more testing.
> > I will be very glad to see the test result about this if you could, thanks.
>
> Bad news. (Alas I won't be able to check this today.)
Hi, thanks your effort. Now I think we should stop this thread and
waiting the class_device going away :)
Hope the iteration patches 1-6/7 could be applied.
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 2.6.24-rc8 #5
> ---------------------------------------------
> modprobe/536 is trying to acquire lock:
> (&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
>
> but task is already holding lock:
> (&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
>
> other info that might help us debug this:
> 2 locks held by modprobe/536:
> #0: (&shost->scan_mutex){--..}, at: [<ce82795b>] __scsi_add_device+0x60/0xc9 [scsi_mod]
> #1: (&cls->mutex/1){--..}, at: [<c0217e43>] class_device_add+0x2c6/0x322
>
> stack backtrace:
> Pid: 536, comm: modprobe Not tainted 2.6.24-rc8 #5
> [<c0138edf>] __lock_acquire+0x962/0x10d4
> [<c028be37>] _spin_unlock_irqrestore+0x34/0x39
> [<c015e693>] kfree+0x87/0xad
> [<c01380cb>] trace_hardirqs_on+0xba/0x15b
> [<c01396c2>] lock_acquire+0x71/0x8b
> [<c0217e43>] class_device_add+0x2c6/0x322
> [<c028a87f>] mutex_lock_nested+0x92/0x2bc
> [<c0217e43>] class_device_add+0x2c6/0x322
> [<c0217e43>] class_device_add+0x2c6/0x322
> [<c0217e43>] class_device_add+0x2c6/0x322
> [<c01afd42>] kobject_get+0xf/0x13
> [<c01afe68>] kobject_init+0x29/0x38
> [<c0217f2c>] class_device_create+0x7d/0x9e
> [<cebfe5b4>] sg_add+0x144/0x39f [sg]
> [<c0217e71>] class_device_add+0x2f4/0x322
> [<ce8289f7>] scsi_sysfs_add_sdev+0x64/0x1ac [scsi_mod]
> [<ce840ece>] ata_scsi_dev_config+0x14/0x91 [libata]
> [<ce826b5c>] scsi_probe_and_add_lun+0x9c0/0x9e0 [scsi_mod]
> [<ce82795b>] __scsi_add_device+0x60/0xc9 [scsi_mod]
> [<ce8279c2>] __scsi_add_device+0xc7/0xc9 [scsi_mod]
> [<ce841401>] ata_scsi_scan_host+0xd3/0x26a [libata]
> [<ce83e7e6>] ata_host_register+0x205/0x280 [libata]
> [<ce83f430>] ata_interrupt+0x0/0x200 [libata]
> [<ce83e8ec>] ata_host_activate+0x8b/0xf2 [libata]
> [<ceb95504>] svia_init_one+0x2e2/0x511 [sata_via]
> [<c01ca42c>] pci_match_device+0xa5/0xb6
> [<ceb95222>] svia_init_one+0x0/0x511 [sata_via]
> [<c01ca4e9>] pci_device_probe+0x40/0x5f
> [<c02173ae>] driver_probe_device+0x7c/0x175
> [<c02175ed>] __driver_attach+0xa2/0xa4
> [<c02168dd>] bus_for_each_dev+0x3c/0x5a
> [<c0217263>] driver_attach+0x16/0x1a
> [<c021754b>] __driver_attach+0x0/0xa4
> [<c0216bea>] bus_add_driver+0x72/0x1c0
> [<c01ca642>] __pci_register_driver+0x56/0x89
> [<c013f11d>] sys_init_module+0xf8/0x1891
> [<ce83901f>] ata_port_start+0x0/0x65 [libata]
> [<c01380cb>] trace_hardirqs_on+0xba/0x15b
> [<c0102936>] syscall_call+0x7/0xb
> =======================
> scsi 2:0:0:0: Attached scsi generic sg1 type 0
>
> Regards,
> Jarek P.
>
On Tue, Jan 22, 2008 at 08:55:05AM +0800, Dave Young wrote:
> On Jan 22, 2008 5:16 AM, Jarek Poplawski <[email protected]> wrote:
> > Dave Young wrote, On 01/21/2008 09:44 AM:
> > ...
> > > I applied it in my kernel, built and run without warnings, but it need
> > > more testing.
> > > I will be very glad to see the test result about this if you could, thanks.
> >
> > Bad news. (Alas I won't be able to check this today.)
>
> Hi, thanks your effort. Now I think we should stop this thread and
> waiting the class_device going away :)
>
> Hope the iteration patches 1-6/7 could be applied.
Can you resend them again, and CC: me on all of them, with the latest
updates, so I know what I should be reviewing this time around?
thanks,
greg k-h
> >
> > Hope the iteration patches 1-6/7 could be applied.
>
> Can you resend them again, and CC: me on all of them, with the latest
> updates, so I know what I should be reviewing this time around?
Hi, sent.
>
> thanks,
>
> greg k-h
>
On 22-01-2008 01:55, Dave Young wrote:
...
> Hi, thanks your effort. Now I think we should stop this thread and
> waiting the class_device going away :)
Sure! But, if you change your mind I'm interested in this subject.
Thanks,
Jarek P.