This patch (as1554) fixes a lockdep false-positive report. The
problem arises because lockdep is unable to deal with the
tree-structured locks created by the device core and sysfs.
This particular problem involves a sysfs attribute method that
unregisters itself, not from the device it was called for, but from a
descendant device. Lockdep doesn't understand the distinction and
reports a possible deadlock, even though the operation is safe.
This is the sort of thing that would normally be handled by using a
nested lock annotation; unfortunately it's not feasible to do that
here. There's no sensible way to tell sysfs when attribute removal
occurs in the context of a parent attribute method.
As a workaround, the patch adds a new flag to struct attribute
telling sysfs not to inform lockdep when it acquires a readlock on a
sysfs_dirent instance for the attribute. The readlock is still
acquired, but lockdep doesn't know about it and hence does not
complain about impossible deadlock scenarios.
Also added are macros for static initialization of attribute
structures with the ignore_lockdep flag set. The three offending
attributes in the USB subsystem are converted to use the new macros.
Signed-off-by: Alan Stern <[email protected]>
Acked-by: Tejun Heo <[email protected]>
CC: Eric W. Biederman <[email protected]>
CC: Peter Zijlstra <[email protected]>
---
drivers/usb/core/sysfs.c | 6 +++---
fs/sysfs/dir.c | 31 ++++++++++++++++++++++++++-----
include/linux/device.h | 3 +++
include/linux/sysfs.h | 12 ++++++++++++
4 files changed, 44 insertions(+), 8 deletions(-)
Index: usb-3.4/include/linux/sysfs.h
===================================================================
--- usb-3.4.orig/include/linux/sysfs.h
+++ usb-3.4/include/linux/sysfs.h
@@ -27,6 +27,7 @@ struct attribute {
const char *name;
umode_t mode;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ bool ignore_lockdep:1;
struct lock_class_key *key;
struct lock_class_key skey;
#endif
@@ -80,6 +81,17 @@ struct attribute_group {
#define __ATTR_NULL { .attr = { .name = NULL } }
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) { \
+ .attr = {.name = __stringify(_name), .mode = _mode, \
+ .ignore_lockdep = true }, \
+ .show = _show, \
+ .store = _store, \
+}
+#else
+#define __ATTR_IGNORE_LOCKDEP __ATTR
+#endif
+
#define attr_name(_attr) (_attr).attr.name
struct file;
Index: usb-3.4/fs/sysfs/dir.c
===================================================================
--- usb-3.4.orig/fs/sysfs/dir.c
+++ usb-3.4/fs/sysfs/dir.c
@@ -132,6 +132,24 @@ static void sysfs_unlink_sibling(struct
rb_erase(&sd->s_rb, &sd->s_parent->s_dir.children);
}
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
+/* Test for attributes that want to ignore lockdep for read-locking */
+static bool ignore_lockdep(struct sysfs_dirent *sd)
+{
+ return sysfs_type(sd) == SYSFS_KOBJ_ATTR &&
+ sd->s_attr.attr->ignore_lockdep;
+}
+
+#else
+
+static inline bool ignore_lockdep(struct sysfs_dirent *sd)
+{
+ return true;
+}
+
+#endif
+
/**
* sysfs_get_active - get an active reference to sysfs_dirent
* @sd: sysfs_dirent to get an active reference to
@@ -155,15 +173,17 @@ struct sysfs_dirent *sysfs_get_active(st
return NULL;
t = atomic_cmpxchg(&sd->s_active, v, v + 1);
- if (likely(t == v)) {
- rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
- return sd;
- }
+ if (likely(t == v))
+ break;
if (t < 0)
return NULL;
cpu_relax();
}
+
+ if (likely(!ignore_lockdep(sd)))
+ rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
+ return sd;
}
/**
@@ -180,7 +200,8 @@ void sysfs_put_active(struct sysfs_diren
if (unlikely(!sd))
return;
- rwsem_release(&sd->dep_map, 1, _RET_IP_);
+ if (likely(!ignore_lockdep(sd)))
+ rwsem_release(&sd->dep_map, 1, _RET_IP_);
v = atomic_dec_return(&sd->s_active);
if (likely(v != SD_DEACTIVATED_BIAS))
return;
Index: usb-3.4/include/linux/device.h
===================================================================
--- usb-3.4.orig/include/linux/device.h
+++ usb-3.4/include/linux/device.h
@@ -503,6 +503,9 @@ ssize_t device_store_int(struct device *
#define DEVICE_INT_ATTR(_name, _mode, _var) \
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_ulong, device_store_ulong), &(_var) }
+#define DEVICE_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
+ struct device_attribute dev_attr_##_name = \
+ __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
extern int device_create_file(struct device *device,
const struct device_attribute *entry);
Index: usb-3.4/drivers/usb/core/sysfs.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/sysfs.c
+++ usb-3.4/drivers/usb/core/sysfs.c
@@ -73,7 +73,7 @@ set_bConfigurationValue(struct device *d
return (value < 0) ? value : count;
}
-static DEVICE_ATTR(bConfigurationValue, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR_IGNORE_LOCKDEP(bConfigurationValue, S_IRUGO | S_IWUSR,
show_bConfigurationValue, set_bConfigurationValue);
/* String fields */
@@ -595,7 +595,7 @@ static ssize_t usb_dev_authorized_store(
return result < 0? result : size;
}
-static DEVICE_ATTR(authorized, 0644,
+static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, 0644,
usb_dev_authorized_show, usb_dev_authorized_store);
/* "Safely remove a device" */
@@ -618,7 +618,7 @@ static ssize_t usb_remove_store(struct d
usb_unlock_device(udev);
return rc;
}
-static DEVICE_ATTR(remove, 0200, NULL, usb_remove_store);
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0200, NULL, usb_remove_store);
static struct attribute *dev_attrs[] = {
On Mon, May 14, 2012 at 01:30:03PM -0400, Alan Stern wrote:
> This patch (as1554) fixes a lockdep false-positive report. The
> problem arises because lockdep is unable to deal with the
> tree-structured locks created by the device core and sysfs.
>
> This particular problem involves a sysfs attribute method that
> unregisters itself, not from the device it was called for, but from a
> descendant device. Lockdep doesn't understand the distinction and
> reports a possible deadlock, even though the operation is safe.
>
> This is the sort of thing that would normally be handled by using a
> nested lock annotation; unfortunately it's not feasible to do that
> here. There's no sensible way to tell sysfs when attribute removal
> occurs in the context of a parent attribute method.
>
> As a workaround, the patch adds a new flag to struct attribute
> telling sysfs not to inform lockdep when it acquires a readlock on a
> sysfs_dirent instance for the attribute. The readlock is still
> acquired, but lockdep doesn't know about it and hence does not
> complain about impossible deadlock scenarios.
>
> Also added are macros for static initialization of attribute
> structures with the ignore_lockdep flag set. The three offending
> attributes in the USB subsystem are converted to use the new macros.
>
> Signed-off-by: Alan Stern <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
> CC: Eric W. Biederman <[email protected]>
> CC: Peter Zijlstra <[email protected]>
>
> ---
>
> drivers/usb/core/sysfs.c | 6 +++---
> fs/sysfs/dir.c | 31 ++++++++++++++++++++++++++-----
> include/linux/device.h | 3 +++
> include/linux/sysfs.h | 12 ++++++++++++
> 4 files changed, 44 insertions(+), 8 deletions(-)
>
> Index: usb-3.4/include/linux/sysfs.h
> ===================================================================
> --- usb-3.4.orig/include/linux/sysfs.h
> +++ usb-3.4/include/linux/sysfs.h
Just a note about this patch, from a meta-point of view (I have no
objection to the patch at all, I'll go apply it in a bit.)
You do use git to generate these patches, right? Or are you using
something else? The "Index:" lines seem odd, like cvs things.
Also, I just learned about the '--3way' option to 'git am', which, when
I have merge problems with a patch (like, for example this one, which
had rejects in the device.h portion), should be able to help me out, if
you used git to generate the patch.
But, if you don't use git, no problems, I was just curious as to what
was creating the "Index:" lines.
thanks,
greg k-h
On Mon, 14 May 2012, Greg KH wrote:
> > ---
> >
> > drivers/usb/core/sysfs.c | 6 +++---
> > fs/sysfs/dir.c | 31 ++++++++++++++++++++++++++-----
> > include/linux/device.h | 3 +++
> > include/linux/sysfs.h | 12 ++++++++++++
> > 4 files changed, 44 insertions(+), 8 deletions(-)
> >
> > Index: usb-3.4/include/linux/sysfs.h
> > ===================================================================
> > --- usb-3.4.orig/include/linux/sysfs.h
> > +++ usb-3.4/include/linux/sysfs.h
>
> Just a note about this patch, from a meta-point of view (I have no
> objection to the patch at all, I'll go apply it in a bit.)
>
> You do use git to generate these patches, right? Or are you using
> something else? The "Index:" lines seem odd, like cvs things.
No, I use quilt, with QUILT_REFRESH_ARGS set to "--diffstat
--no-timestamps -p1" in my .quiltrc file. There's an option to
suppress the Index: lines but I never bothered to set it.
> Also, I just learned about the '--3way' option to 'git am', which, when
> I have merge problems with a patch (like, for example this one, which
> had rejects in the device.h portion), should be able to help me out, if
> you used git to generate the patch.
>
> But, if you don't use git, no problems, I was just curious as to what
> was creating the "Index:" lines.
They are added explicitly by /usr/share/quilt/scripts/patchfns -- grep
for "Index:". I don't know why quilt adds those lines, though.
Trying to resemble cvs output, maybe?
Alan Stern
On Mon, May 14, 2012 at 04:03:59PM -0400, Alan Stern wrote:
> On Mon, 14 May 2012, Greg KH wrote:
>
> > > ---
> > >
> > > drivers/usb/core/sysfs.c | 6 +++---
> > > fs/sysfs/dir.c | 31 ++++++++++++++++++++++++++-----
> > > include/linux/device.h | 3 +++
> > > include/linux/sysfs.h | 12 ++++++++++++
> > > 4 files changed, 44 insertions(+), 8 deletions(-)
> > >
> > > Index: usb-3.4/include/linux/sysfs.h
> > > ===================================================================
> > > --- usb-3.4.orig/include/linux/sysfs.h
> > > +++ usb-3.4/include/linux/sysfs.h
> >
> > Just a note about this patch, from a meta-point of view (I have no
> > objection to the patch at all, I'll go apply it in a bit.)
> >
> > You do use git to generate these patches, right? Or are you using
> > something else? The "Index:" lines seem odd, like cvs things.
>
> No, I use quilt, with QUILT_REFRESH_ARGS set to "--diffstat
> --no-timestamps -p1" in my .quiltrc file. There's an option to
> suppress the Index: lines but I never bothered to set it.
>
> > Also, I just learned about the '--3way' option to 'git am', which, when
> > I have merge problems with a patch (like, for example this one, which
> > had rejects in the device.h portion), should be able to help me out, if
> > you used git to generate the patch.
> >
> > But, if you don't use git, no problems, I was just curious as to what
> > was creating the "Index:" lines.
>
> They are added explicitly by /usr/share/quilt/scripts/patchfns -- grep
> for "Index:". I don't know why quilt adds those lines, though.
> Trying to resemble cvs output, maybe?
Ah, yeah, I turned that off a long time ago in my .quiltrc, here's what
I use:
QUILT_REFRESH_ARGS="--diffstat --strip-trailing-whitespace --no-timestamps --no-index --sort -p1 -p ab"
QUILT_DIFF_ARGS="--no-timestamps --no-index --sort --color=auto -p ab"
QUILT_DIFF_OPTS="-p"
Anyway, thanks for letting me know, just curious.
greg k-h