[PATCH] usbcore: Fix handling of sysfs strings and other attributes

This patch (as592) makes a few small improvements to the way device
strings are handled, and it fixes some bugs in a couple of other sysfs
attribute routines.  (Look at show_configuration_string() to see what I
mean.)

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

drivers/usb/core/config.c

@@ -112,8 +112,12 @@ void usb_release_interface_cache(struct kref *ref)
 	struct usb_interface_cache *intfc = ref_to_usb_interface_cache(ref);
 	int j;
 
-	for (j = 0; j < intfc->num_altsetting; j++)
-		kfree(intfc->altsetting[j].endpoint);
+	for (j = 0; j < intfc->num_altsetting; j++) {
+		struct usb_host_interface *alt = &intfc->altsetting[j];
+
+		kfree(alt->endpoint);
+		kfree(alt->string);
+	}
 	kfree(intfc);
 }
 
@@ -420,8 +424,6 @@ void usb_destroy_configuration(struct usb_device *dev)
 		struct usb_host_config *cf = &dev->config[c];
 
 		kfree(cf->string);
-		cf->string = NULL;
-
 		for (i = 0; i < cf->desc.bNumInterfaces; i++) {
 			if (cf->intf_cache[i])
 				kref_put(&cf->intf_cache[i]->ref, 

drivers/usb/core/hub.c

@@ -1204,21 +1204,6 @@ static inline void show_string(struct usb_device *udev, char *id, char *string)
 {}
 #endif
 
-static void get_string(struct usb_device *udev, char **string, int index)
-{
-	char *buf;
-
-	if (!index)
-		return;
-	buf = kmalloc(256, GFP_KERNEL);
-	if (!buf)
-		return;
-	if (usb_string(udev, index, buf, 256) > 0)
-		*string = buf;
-	else
-		kfree(buf);
-}
-
 
 #ifdef	CONFIG_USB_OTG
 #include "otg_whitelist.h"
@@ -1257,9 +1242,10 @@ int usb_new_device(struct usb_device *udev)
 	}
 
 	/* read the standard strings and cache them if present */
-	get_string(udev, &udev->product, udev->descriptor.iProduct);
-	get_string(udev, &udev->manufacturer, udev->descriptor.iManufacturer);
-	get_string(udev, &udev->serial, udev->descriptor.iSerialNumber);
+	udev->product = usb_cache_string(udev, udev->descriptor.iProduct);
+	udev->manufacturer = usb_cache_string(udev,
+			udev->descriptor.iManufacturer);
+	udev->serial = usb_cache_string(udev, udev->descriptor.iSerialNumber);
 
 	/* Tell the world! */
 	dev_dbg(&udev->dev, "new device strings: Mfr=%d, Product=%d, "

drivers/usb/core/message.c

@@ -787,6 +787,31 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 	return err;
 }
 
+/**
+ * usb_cache_string - read a string descriptor and cache it for later use
+ * @udev: the device whose string descriptor is being read
+ * @index: the descriptor index
+ *
+ * Returns a pointer to a kmalloc'ed buffer containing the descriptor string,
+ * or NULL if the index is 0 or the string could not be read.
+ */
+char *usb_cache_string(struct usb_device *udev, int index)
+{
+	char *buf;
+	char *smallbuf = NULL;
+	int len;
+
+	if (index > 0 && (buf = kmalloc(256, GFP_KERNEL)) != NULL) {
+		if ((len = usb_string(udev, index, buf, 256)) > 0) {
+			if ((smallbuf = kmalloc(++len, GFP_KERNEL)) == NULL)
+				return buf;
+			memcpy(smallbuf, buf, len);
+		}
+		kfree(buf);
+	}
+	return smallbuf;
+}
+
 /*
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
  * @dev: the device whose device descriptor is being updated
@@ -1008,8 +1033,6 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
 			dev_dbg (&dev->dev, "unregistering interface %s\n",
 				interface->dev.bus_id);
 			usb_remove_sysfs_intf_files(interface);
-			kfree(interface->cur_altsetting->string);
-			interface->cur_altsetting->string = NULL;
 			device_del (&interface->dev);
 		}
 
@@ -1422,12 +1445,9 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		}
 		kfree(new_interfaces);
 
-		if ((cp->desc.iConfiguration) &&
-		    (cp->string == NULL)) {
-			cp->string = kmalloc(256, GFP_KERNEL);
-			if (cp->string)
-				usb_string(dev, cp->desc.iConfiguration, cp->string, 256);
-		}
+		if (cp->string == NULL)
+			cp->string = usb_cache_string(dev,
+					cp->desc.iConfiguration);
 
 		/* Now that all the interfaces are set up, register them
 		 * to trigger binding of drivers to interfaces.  probe()
@@ -1437,13 +1457,12 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		 */
 		for (i = 0; i < nintf; ++i) {
 			struct usb_interface *intf = cp->interface[i];
-			struct usb_interface_descriptor *desc;
+			struct usb_host_interface *alt = intf->cur_altsetting;
 
-			desc = &intf->altsetting [0].desc;
 			dev_dbg (&dev->dev,
 				"adding %s (config #%d, interface %d)\n",
 				intf->dev.bus_id, configuration,
-				desc->bInterfaceNumber);
+				alt->desc.bInterfaceNumber);
 			ret = device_add (&intf->dev);
 			if (ret != 0) {
 				dev_err(&dev->dev,
@@ -1452,13 +1471,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 					ret);
 				continue;
 			}
-			if ((intf->cur_altsetting->desc.iInterface) &&
-			    (intf->cur_altsetting->string == NULL)) {
-				intf->cur_altsetting->string = kmalloc(256, GFP_KERNEL);
-				if (intf->cur_altsetting->string)
-					usb_string(dev, intf->cur_altsetting->desc.iInterface,
-						   intf->cur_altsetting->string, 256);
-			}
 			usb_create_sysfs_intf_files (intf);
 		}
 	}

drivers/usb/core/sysfs.c

@@ -249,18 +249,12 @@ static ssize_t show_configuration_string(struct device *dev,
 {
 	struct usb_device *udev;
 	struct usb_host_config *actconfig;
-	int len;
 
 	udev = to_usb_device (dev);
 	actconfig = udev->actconfig;
 	if ((!actconfig) || (!actconfig->string))
 		return 0;
-	len = sprintf(buf, actconfig->string, PAGE_SIZE);
-	if (len < 0)
-		return 0;
-	buf[len] = '\n';
-	buf[len+1] = 0;
-	return len+1;
+	return sprintf(buf, "%s\n", actconfig->string);
 }
 static DEVICE_ATTR(configuration, S_IRUGO, show_configuration_string, NULL);
 
@@ -291,15 +285,9 @@ static ssize_t  show_##name(struct device *dev,				\
 		struct device_attribute *attr, char *buf)		\
 {									\
 	struct usb_device *udev;					\
-	int len;							\
 									\
 	udev = to_usb_device (dev);					\
-	len = snprintf(buf, 256, "%s", udev->name);			\
-	if (len < 0)							\
-		return 0;						\
-	buf[len] = '\n';						\
-	buf[len+1] = 0;							\
-	return len+1;							\
+	return sprintf(buf, "%s\n", udev->name);			\
 }									\
 static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL);
 
@@ -449,11 +437,11 @@ void usb_remove_sysfs_dev_files (struct usb_device *udev)
 	usb_remove_ep_files(&udev->ep0);
 	sysfs_remove_group(&dev->kobj, &dev_attr_grp);
 
-	if (udev->descriptor.iManufacturer)
+	if (udev->manufacturer)
 		device_remove_file(dev, &dev_attr_manufacturer);
-	if (udev->descriptor.iProduct)
+	if (udev->product)
 		device_remove_file(dev, &dev_attr_product);
-	if (udev->descriptor.iSerialNumber)
+	if (udev->serial)
 		device_remove_file(dev, &dev_attr_serial);
 	device_remove_file (dev, &dev_attr_configuration);
 }
@@ -535,15 +523,16 @@ static struct attribute_group intf_attr_grp = {
 	.attrs = intf_attrs,
 };
 
-static inline void usb_create_intf_ep_files(struct usb_interface *intf)
+static inline void usb_create_intf_ep_files(struct usb_interface *intf,
+		struct usb_device *udev)
 {
 	struct usb_host_interface *iface_desc;
 	int i;
 
 	iface_desc = intf->cur_altsetting;
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i)
 		usb_create_ep_files(&intf->dev.kobj, &iface_desc->endpoint[i],
-				interface_to_usbdev(intf));
+				udev);
 }
 
 static inline void usb_remove_intf_ep_files(struct usb_interface *intf)
@@ -558,11 +547,16 @@ static inline void usb_remove_intf_ep_files(struct usb_interface *intf)
 
 void usb_create_sysfs_intf_files (struct usb_interface *intf)
 {
+	struct usb_device *udev = interface_to_usbdev(intf);
+	struct usb_host_interface *alt = intf->cur_altsetting;
+
 	sysfs_create_group(&intf->dev.kobj, &intf_attr_grp);
 
-	if (intf->cur_altsetting->string)
+	if (alt->string == NULL)
+		alt->string = usb_cache_string(udev, alt->desc.iInterface);
+	if (alt->string)
 		device_create_file(&intf->dev, &dev_attr_interface);
-	usb_create_intf_ep_files(intf);
+	usb_create_intf_ep_files(intf, udev);
 }
 
 void usb_remove_sysfs_intf_files (struct usb_interface *intf)

drivers/usb/core/usb.h

@@ -13,6 +13,7 @@ extern void usb_disable_device (struct usb_device *dev, int skip_ep0);
 
 extern int usb_get_device_descriptor(struct usb_device *dev,
 		unsigned int size);
+extern char *usb_cache_string(struct usb_device *udev, int index);
 extern int usb_set_configuration(struct usb_device *dev, int configuration);
 
 extern void usb_lock_all_devices(void);

include/linux/usb.h

@@ -231,7 +231,7 @@ struct usb_interface_cache {
 struct usb_host_config {
 	struct usb_config_descriptor	desc;
 
-	char *string;
+	char *string;		/* iConfiguration string, if present */
 	/* the interfaces associated with this configuration,
 	 * stored in no particular order */
 	struct usb_interface *interface[USB_MAXINTERFACES];
@@ -351,9 +351,11 @@ struct usb_device {
 	int have_langid;		/* whether string_langid is valid */
 	int string_langid;		/* language ID for strings */
 
-	char *product;
-	char *manufacturer;
-	char *serial;			/* static strings from the device */
+	/* static strings from the device */
+	char *product;			/* iProduct string, if present */
+	char *manufacturer;		/* iManufacturer string, if present */
+	char *serial;			/* iSerialNumber string, if present */
+
 	struct list_head filelist;
 	struct class_device *class_dev;
 	struct dentry *usbfs_dentry;	/* usbfs dentry entry for the device */