Message-ID: <Pine.LNX.4.44L0.1205041459240.1166-100000@iolanthe.rowland.org>
Date:	Fri, 4 May 2012 15:08:53 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Tejun Heo <tj@...nel.org>
cc:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: Lockdep false positive in sysfs

On Fri, 4 May 2012, Tejun Heo wrote:

> > Is there any reasonable way to get from the kobject and the attribute
> > to the appropriate sysfs_dirent?  Search through all the groups
> > attached to the kobject?  Restrict the new interface so that it can be
> > used only by attributes at the kobject's top level (i.e., not in a
> > named group)?
> > 
> > Any suggestions?
> 
> Urgh... I can't think of anything pretty.  How about just marking the
> attr as "I'm special" and let sysfs code override lockdep annotation?

Never mind, I figured it out.  The caller knows what the group is (if 
there's a group), so it can simply pass that information along.

To make things clearer, I broke the interface into two parts: one to 
find the sysfs_dirent and the other to handle the lockdep annotations.  
Here's the patch -- it works.  What do you think?

Alan Stern

P.S.: I'm not sure about the namespace arguments to sysfs_get_dirent().  
Is NULL appropriate?  Or should I use sysfs_ns_type(kobj->sd)?




Index: usb-3.4/include/linux/sysfs.h
===================================================================
--- usb-3.4.orig/include/linux/sysfs.h
+++ usb-3.4/include/linux/sysfs.h
@@ -121,6 +121,10 @@ struct sysfs_dirent;
 
 int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
 			    void *data, struct module *owner);
+struct sysfs_dirent *sysfs_find_dirent_for_attr(struct kobject *kobj,
+			const struct attribute *attr, const char *group);
+void sysfs_unannotate_readlock(struct sysfs_dirent *sd);
+void sysfs_reannotate_readlock(struct sysfs_dirent *sd);
 
 int __must_check sysfs_create_dir(struct kobject *kobj);
 void sysfs_remove_dir(struct kobject *kobj);
@@ -188,6 +192,21 @@ static inline int sysfs_schedule_callbac
 	return -ENOSYS;
 }
 
+static inline struct sysfs_dirent *sysfs_find_dirent_for_attr(
+			struct kobject *kobj,
+			const struct attribute *attr, const char *group)
+{
+	return NULL;
+}
+
+static inline void sysfs_unannotate_readlock(struct sysfs_dirent *sd)
+{
+}
+
+static inline void sysfs_reannotate_readlock(struct sysfs_dirent *sd)
+{
+}
+
 static inline int sysfs_create_dir(struct kobject *kobj)
 {
 	return 0;
Index: usb-3.4/include/linux/device.h
===================================================================
--- usb-3.4.orig/include/linux/device.h
+++ usb-3.4/include/linux/device.h
@@ -514,6 +514,11 @@ extern void device_remove_bin_file(struc
 				   const struct bin_attribute *attr);
 extern int device_schedule_callback_owner(struct device *dev,
 		void (*func)(struct device *dev), struct module *owner);
+extern void *device_start_attribute_infanticide(
+		struct device *dev, const struct device_attribute *attr,
+		const char *group);
+extern void device_end_attribute_infanticide(void *cookie);
+
 
 /* This is a macro to avoid include problems with THIS_MODULE */
 #define device_schedule_callback(dev, func)			\
Index: usb-3.4/fs/sysfs/dir.c
===================================================================
--- usb-3.4.orig/fs/sysfs/dir.c
+++ usb-3.4/fs/sysfs/dir.c
@@ -133,6 +133,74 @@ static void sysfs_unlink_sibling(struct
 }
 
 /**
+ *	sysfs_find_dirent_for_attr - find sysfs_dirent for a given kobject and attribute
+ *	@kobj: kobject the attribute is attached to
+ *	@attr: attribute whose sysfs_dirent should be found
+ *	@group: name of the group containing @attr, or NULL
+ *
+ *	Looks up and returns a pointer to the sysfs_dirent structure
+ *	corresponding to the instance of @attr in @group attached to @kobj.
+ *	Returns NULL if the sysfs_dirent cannot be found.
+ *
+ *	Retains a reference to the returned sysfs_dirent.  The caller must
+ *	release this reference, as done by sysfs_reannotate_readlock()
+ *	below.
+ */
+struct sysfs_dirent *sysfs_find_dirent_for_attr(struct kobject *kobj,
+			const struct attribute *attr, const char *group)
+{
+	struct sysfs_dirent *dir_sd;
+	struct sysfs_dirent *sd;
+
+	if (group)
+		dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+	else
+		dir_sd = sysfs_get(kobj->sd);
+	if (!dir_sd)
+		return NULL;
+
+	sd = sysfs_get_dirent(dir_sd, NULL, attr->name);
+	sysfs_put(dir_sd);
+	return sd;
+}
+
+/**
+ *	sysfs_unannotate_readlock - drop the lockdep annotation for an attribute
+ *	@sd: sysfs_dirent for the attribute whose lock annotation is dropped
+ *
+ *	Lockdep is not able to cope with the tree-structured locks used
+ *	by the device core.  It considers all the s_active lock associated
+ *	a particular attribute to be the same.  This causes a false
+ *	positive report when a method for an attribute attached to a device
+ *	wants to remove the same attribute attached to a child device.
+ *
+ *	To avoid such false positives, the method can call this routine to
+ *	drop the lockdep annotation indicating that the attribute's dirent is
+ *	currently locked, remove the child's attribute, and then reacquire
+ *	the annotation.
+ */
+void sysfs_unannotate_readlock(struct sysfs_dirent *sd)
+{
+	rwsem_release(&sd->dep_map, 1, _RET_IP_);
+}
+
+/**
+ *	sysfs_reannotate_readlock - reacquire the lockdep annotation for an attribute
+ *	@sd: sysfs_dirent for the attribute whose lock annotation is reacquired
+ *
+ *	After an attribute method has called sysfs_unannotate_readlock()
+ *	and removed the attributes from various child devices, it should
+ *	call this routine to reacquire the lockdep annotation (and also to
+ *	drop the reference to @sd obtained by
+ *	sysfs_get_dirent_for_attribute()) before returning.
+ */
+void sysfs_reannotate_readlock(struct sysfs_dirent *sd)
+{
+	rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
+	sysfs_put(sd);
+}
+
+/**
  *	sysfs_get_active - get an active reference to sysfs_dirent
  *	@sd: sysfs_dirent to get an active reference to
  *
Index: usb-3.4/drivers/base/core.c
===================================================================
--- usb-3.4.orig/drivers/base/core.c
+++ usb-3.4/drivers/base/core.c
@@ -609,6 +609,48 @@ int device_schedule_callback_owner(struc
 }
 EXPORT_SYMBOL_GPL(device_schedule_callback_owner);
 
+/**
+ * device_start_attribute_infanticide - prepare to remove child attributes
+ * @dev: device passed to the calling method
+ * @attr: attribute passed to the calling method
+ * @group: name of the group containing @attr, or NULL
+ *
+ * To avoid false positive reports from lockdep, this routine should be
+ * called by an attribute method that wants to remove the same attribute
+ * from a descendant of the device for which it was called (an
+ * "infanticidal" attribute).
+ *
+ * The returned value should be treated as an opaque cookie and passed to
+ * device_end_attribute_infanticide() below after the descendant
+ * attributes have been removed.
+ */
+void *device_start_attribute_infanticide(
+		struct device *dev, const struct device_attribute *attr,
+		const char *group)
+{
+	struct sysfs_dirent *sd;
+
+	sd = sysfs_find_dirent_for_attr(&dev->kobj, &attr->attr, group);
+	if (sd)
+		sysfs_unannotate_readlock(sd);
+	return sd;
+}
+EXPORT_SYMBOL_GPL(device_start_attribute_infanticide);
+
+/**
+ * device_end_attribute_infanticide - finish removing child attributes
+ * @cookie: value returned by device_start_attribute_infanticide() above
+ *
+ * This routine should be called by attribute methods after they have
+ * finished removing the corresponding attributes from descendant devices.
+ */
+void device_end_attribute_infanticide(void *cookie)
+{
+	if (cookie)
+		sysfs_reannotate_readlock(cookie);
+}
+EXPORT_SYMBOL_GPL(device_end_attribute_infanticide);
+
 static void klist_children_get(struct klist_node *n)
 {
 	struct device_private *p = to_device_private_parent(n);
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
@@ -64,11 +64,14 @@ set_bConfigurationValue(struct device *d
 {
 	struct usb_device	*udev = to_usb_device(dev);
 	int			config, value;
+	void			*cookie;
 
 	if (sscanf(buf, "%d", &config) != 1 || config < -1 || config > 255)
 		return -EINVAL;
 	usb_lock_device(udev);
+	cookie = device_start_attribute_infanticide(dev, attr, NULL);
 	value = usb_set_configuration(udev, config);
+	device_end_attribute_infanticide(cookie);
 	usb_unlock_device(udev);
 	return (value < 0) ? value : count;
 }
@@ -588,10 +591,15 @@ static ssize_t usb_dev_authorized_store(
 	result = sscanf(buf, "%u\n", &val);
 	if (result != 1)
 		result = -EINVAL;
-	else if (val == 0)
+	else if (val == 0) {
+		void *cookie;
+
+		cookie = device_start_attribute_infanticide(dev, attr, NULL);
 		result = usb_deauthorize_device(usb_dev);
-	else
+		device_end_attribute_infanticide(cookie);
+	} else {
 		result = usb_authorize_device(usb_dev);
+	}
 	return result < 0? result : size;
 }
 
@@ -605,12 +613,15 @@ static ssize_t usb_remove_store(struct d
 {
 	struct usb_device *udev = to_usb_device(dev);
 	int rc = 0;
+	void *cookie;
 
 	usb_lock_device(udev);
 	if (udev->state != USB_STATE_NOTATTACHED) {
 
 		/* To avoid races, first unconfigure and then remove */
+		cookie = device_start_attribute_infanticide(dev, attr, NULL);
 		usb_set_configuration(udev, -1);
+		device_end_attribute_infanticide(cookie);
 		rc = usb_remove_device(udev);
 	}
 	if (rc == 0)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists