sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

PATCH In review Dan Williams 4 participants 8 messages Apr 18, 2008

Subsystems (2)

USB Driver core

lore

Share & tools

lore

Dan Williams

author

2008-04-18 02:11 UTC #0 lore raw

PATCH 1/1 sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.

Cc Kay Sievers <[email protected]>

Signed-off-by Dan Williams <[email protected]>

---
This boots up without warning on my dev system. We end up with the following situation:

$ ls -l /sys/dev/usb/189:0
lrwxrwxrwx 1 root root 0 2008-04-17 12:00 /sys/dev/usb/189:0 -> ../../class/usb_device/usbdev1.1
$ ls -l /sys/dev/char/189:0
lrwxrwxrwx 1 root root 0 2008-04-17 12:01 /sys/dev/char/189:0 -> ../../devices/pci0000:00/0000:00:1d.7/usb1

 drivers/base/core.c      |   25 +++++++++++++++++++++++--
 drivers/usb/core/devio.c |    2 +-
 2 files changed, 24 insertions(+), 3 deletions(-)

drivers/base/core.c +23 -2

index ba21118..bce5e4b 100644
@@ -30,6 +30,10 @@ int (*platform_notify_remove)(struct device *dev) = NULL;
 static struct kobject *dev_kobj;
 static struct kobject *char_kobj;
 static struct kobject *block_kobj;
+#ifdef CONFIG_USB_DEVICE_CLASS
+extern struct class *usb_classdev_class;
+static struct kobject *usb_kobj;
+#endif
 
 #ifdef CONFIG_BLOCK
 static inline int device_is_not_partition(struct device *dev)

@@ -764,7 +768,13 @@ static void device_remove_class_symlinks(struct device *dev)
 
 static struct kobject *device_to_dev_kobj(struct device *dev)
 {
-	return dev->class == &block_class ? block_kobj : char_kobj;
+	if (dev->class == &block_class)
+		return block_kobj;
+#ifdef CONFIG_USB_DEVICE_CLASS	
+	if (usb_classdev_class && dev->class == usb_classdev_class)
+		return usb_kobj;
+#endif 
+	return char_kobj;
 }
 
 /**

@@ -1087,9 +1097,17 @@ int __init devices_init(void)
 	char_kobj = kobject_create_and_add("char", dev_kobj);
 	if (!char_kobj)
 		goto char_kobj_err;
-
+#ifdef CONFIG_USB_DEVICE_CLASS
+	usb_kobj = kobject_create_and_add("usb", dev_kobj);
+	if (!usb_kobj)
+		goto usb_kobj_err;
+#endif
 	return 0;
 
+#ifdef CONFIG_USB_DEVICE_CLASS
+ usb_kobj_err:
+	kobject_put(char_kobj);
+#endif
  char_kobj_err:
 	kobject_put(block_kobj);
  block_kobj_err:

@@ -1421,6 +1439,9 @@ void device_shutdown(void)
 			dev->driver->shutdown(dev);
 		}
 	}
+#ifdef CONFIG_USB_DEVICE_CLASS
+	kobject_put(usb_kobj);
+#endif
 	kobject_put(char_kobj);
 	kobject_put(block_kobj);
 	kobject_put(dev_kobj);

drivers/usb/core/devio.c +1 -1

index ae94176..2075622 100644
@@ -1660,7 +1660,7 @@ const struct file_operations usbdev_file_operations = {
 };
 
 #ifdef CONFIG_USB_DEVICE_CLASS
-static struct class *usb_classdev_class;
+struct class *usb_classdev_class;
 
 static int usb_classdev_add(struct usb_device *dev)
 {

Greg KH

2008-04-18 03:01 UTC #1 lore raw

Quoted 10 lines · Dan Williams

The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.
Cc Kay Sievers <[email protected]>
Signed-off-by Dan Williams <[email protected]>
---
This boots up without warning on my dev system. We end up with the following situation:

This patch is not going to work if usbcore is a module :(

thanks,

greg k-h

Andrew Morton

2008-04-18 03:09 UTC #2 lore raw

In reply to Dan Williams

Quoted 5 lines · On Thu, 17 Apr 2008 19:11:03 -0700 Dan Williams <[email protected]>…

The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.

hm. That's a somewhat nasty patch you have there.

drivers/base/core.c int (*platform_notify_remove)(struct device *dev) = NULL; · line 34

Quoted 11 lines

drivers/base/core.c +2 -0

index ba21118..bce5e4b 100644
@@ -30,6 +30,10 @@ int (*platform_notify_remove)(struct device *dev) = NULL;
 static struct kobject *dev_kobj;
 static struct kobject *char_kobj;
 static struct kobject *block_kobj;
+#ifdef CONFIG_USB_DEVICE_CLASS
+extern struct class *usb_classdev_class;

This should be in a header file, but where?

drivers/base/core.c static void device_remove_class_symlinks(struct device *dev) · line 772

Quoted 12 lines

drivers/base/core.c +4 -1

+static struct kobject *usb_kobj;
+#endif
 
 #ifdef CONFIG_BLOCK
 static inline int device_is_not_partition(struct device *dev)

@@ -764,7 +768,13 @@ static void device_remove_class_symlinks(struct device *dev)
 
 static struct kobject *device_to_dev_kobj(struct device *dev)
 {
-	return dev->class == &block_class ? block_kobj : char_kobj;
+	if (dev->class == &block_class)
+		return block_kobj;

Does the existing code compile-n-work with CONFIG_BLOCK=n?

Quoted 6 lines

drivers/base/core.c +6 -0

+#ifdef CONFIG_USB_DEVICE_CLASS	
+	if (usb_classdev_class && dev->class == usb_classdev_class)
+		return usb_kobj;
+#endif 
+#ifdef CONFIG_USB_DEVICE_CLASS
+	usb_kobj = kobject_create_and_add("usb", dev_kobj);

drivers/base/core.c static void device_remove_class_symlinks(struct device *dev)

Quoted 10 lines

drivers/base/core.c +10 -0

+	if (!usb_kobj)
+		goto usb_kobj_err;
+#endif
+#ifdef CONFIG_USB_DEVICE_CLASS
+ usb_kobj_err:
+	kobject_put(char_kobj);
+#endif
+#ifdef CONFIG_USB_DEVICE_CLASS
+	kobject_put(usb_kobj);
+#endif

mutter. I'd have thought that with suitable fiddling most of this code could be moved into the USB core and some of it (the kobject_create_and_add()) can be done via USB's initcalls.

this:

drivers/base/core.c renamed from drivers/base/core.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix +0 -1

@@ -31,7 +31,6 @@ static struct kobject *dev_kobj;
 static struct kobject *char_kobj;
 static struct kobject *block_kobj;
 #ifdef CONFIG_USB_DEVICE_CLASS
-extern struct class *usb_classdev_class;
 static struct kobject *usb_kobj;
 #endif

drivers/usb/core/devio.c renamed from drivers/usb/core/devio.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix +1 -0

diff -puN drivers/usb/core/devio.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix drivers/usb/core/devio.c
@@ -43,6 +43,7 @@
 #include <linux/signal.h>
 #include <linux/poll.h>
 #include <linux/module.h>
+#include <linux/device.h>
 #include <linux/usb.h>
 #include <linux/usbdevice_fs.h>
 #include <linux/cdev.h>

include/linux/device.h renamed from include/linux/device.h~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix +2 -0

diff -puN include/linux/device.h~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix include/linux/device.h
@@ -623,4 +623,6 @@ extern const char *dev_driver_string(str
 #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-*")
 
+extern struct class *usb_classdev_class;
+
 #endif /* _DEVICE_H_ */

deworsens things slightly.

Dan Williams

author

2008-04-18 03:44 UTC #3 lore raw

Quoted 10 lines · Andrew Morton

Quoted 5 lines · On Thu, 17 Apr 2008 19:11:03 -0700 Dan Williams <[email protected]>…
The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.
hm. That's a somewhat nasty patch you have there.

Yeah, it was a frantic unbreak -mm attempt more than anything else :(.

Quoted 23 lines

drivers/base/core.c int (*platform_notify_remove)(struct device *dev) = NULL; · line 34

Quoted 12 lines

drivers/base/core.c +2 -0

index ba21118..bce5e4b 100644
@@ -30,6 +30,10 @@ int (*platform_notify_remove)(struct device *dev) = NULL;
 static struct kobject *dev_kobj;
 static struct kobject *char_kobj;
 static struct kobject *block_kobj;
+#ifdef CONFIG_USB_DEVICE_CLASS
+extern struct class *usb_classdev_class;

This should be in a header file, but where?

Quoted 7 lines

drivers/base/core.c +2 -0

+static struct kobject *usb_kobj;
+#endif
 
 #ifdef CONFIG_BLOCK
 static inline int device_is_not_partition(struct device *dev)

@@ -764,7 +768,13 @@ static void device_remove_class_symlinks(struct device *dev)

Quoted 8 lines

drivers/base/core.c static void device_remove_class_symlinks(struct device *dev) · line 772
Quoted 5 lines
drivers/base/core.c +2 -1
 static struct kobject *device_to_dev_kobj(struct device *dev)
 {
-     return dev->class == &block_class ? block_kobj : char_kobj;
+     if (dev->class == &block_class)
+             return block_kobj;
Does the existing code compile-n-work with CONFIG_BLOCK=n?

I'll take a look. And Greg shares my suspicion that there might be other duplicate devices like these USB class devices, so need to look for that as well.

Quoted 57 lines

Quoted 10 lines
drivers/base/core.c +8 -0
+#ifdef CONFIG_USB_DEVICE_CLASS
+     if (usb_classdev_class && dev->class == usb_classdev_class)
+             return usb_kobj;
+#endif
 
+#ifdef CONFIG_USB_DEVICE_CLASS
+     usb_kobj = kobject_create_and_add("usb", dev_kobj);
+     if (!usb_kobj)
+             goto usb_kobj_err;
drivers/base/core.c static void device_remove_class_symlinks(struct device *dev)
Quoted 10 lines
drivers/base/core.c +8 -0
+#endif
 
+#ifdef CONFIG_USB_DEVICE_CLASS
+ usb_kobj_err:
+     kobject_put(char_kobj);
+#endif
 
+#ifdef CONFIG_USB_DEVICE_CLASS
+     kobject_put(usb_kobj);
+#endif
mutter. I'd have thought that with suitable fiddling most of this code could be moved into the USB core and some of it (the kobject_create_and_add()) can be done via USB's initcalls.
this:
--- a/drivers/base/core.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix +++ a/drivers/base/core.c
@@ -31,7 +31,6 @@ static struct kobject *dev_kobj;
static struct kobject *char_kobj;
static struct kobject *block_kobj;
#ifdef CONFIG_USB_DEVICE_CLASS
-extern struct class *usb_classdev_class;
static struct kobject *usb_kobj;
#endif
diff -puN drivers/usb/core/devio.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix drivers/usb/core/devio.c --- a/drivers/usb/core/devio.c~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix
+++ a/drivers/usb/core/devio.c
@@ -43,6 +43,7 @@
#include <linux/signal.h>
#include <linux/poll.h>
#include <linux/module.h>
+#include <linux/device.h>
#include <linux/usb.h>
#include <linux/usbdevice_fs.h>
#include <linux/cdev.h>
diff -puN include/linux/device.h~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix include/linux/device.h --- a/include/linux/device.h~sysfs-add-sys-dev-usb-to-handle-config_usb_device_class=y-fix
+++ a/include/linux/device.h
@@ -623,4 +623,6 @@ extern const char *dev_driver_string(str
  #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
        MODULE_ALIAS("char-major-" __stringify(major) "-*")

Quoted 7 lines

+extern struct class *usb_classdev_class;
+
#endif /* _DEVICE_H_ */
_
deworsens things slightly.

Thanks,
Dan

Greg KH

2008-04-18 03:16 UTC #4 lore raw

In reply to Dan Williams

Quoted 4 lines · Dan Williams

The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.

Hm, no they do not, they are not a new address space, we are just reusing them as the other user wasn't using them, and the code is deprecated and will be removed eventually. Neither of these char devices are really hooked up to anything within the kernel, so it doesn't matter yet.

I wonder how many other duplicates we have floating around...

thanks,

greg k-h

Dan Williams

author

2008-04-18 04:59 UTC #5 lore raw

Quoted 7 lines · Greg KH

Quoted 4 lines · Dan Williams
The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.

Quoted 8 lines

Hm, no they do not, they are not a new address space, we are just reusing them as the other user wasn't using them, and the code is deprecated and will be removed eventually. Neither of these char devices are really hooked up to anything within the kernel, so it doesn't matter yet.
I wonder how many other duplicates we have floating around...

Hm, so how about making this an opt-in capability of the class? At device_add() time the class is queried to see if a link should be created in /sys/dev/block or /sys/dev/char? Although, this leads to inconsistent coverage. But maybe that does not matter as many of these character devices do not have interesting attributes to be accessed? Sigh, I am beginning to wonder if the character device side of this capability is a solution looking for a problem...

Ideas?

Quoted 4 lines

thanks,
greg k-h

Thanks,
Dan

Kay Sievers

2008-04-18 07:55 UTC #6 lore raw

Quoted 14 lines · Dan Williams

Quoted 13 lines · Greg KH
Quoted 4 lines · Dan Williams
The deprecated config option CONFIG_USB_DEVICE_CLASS causes class devices with duplicate major:minor numbers to be registered. In effect they represent a usb specific address space for major:minor numbers so add 'usb' as a directory along side 'block' and 'char'.
Hm, no they do not, they are not a new address space, we are just reusing them as the other user wasn't using them, and the code is deprecated and will be removed eventually. Neither of these char devices are really hooked up to anything within the kernel, so it doesn't matter yet.

Quoted 6 lines

Quoted 2 lines
I wonder how many other duplicates we have floating around...
Hm, so how about making this an opt-in capability of the class? At device_add() time the class is queried to see if a link should be created in /sys/dev/block or /sys/dev/char?

This could work, yes. We could just set a flag in the class, that prevents the device entries in /sys/dev/.

Quoted 4 lines

Although, this leads to
inconsistent coverage. But maybe that does not matter as many of these character devices do not have interesting attributes to be accessed?

The usual case is that one of the duplicated /sys devices is deprecated, so it should be fine, to always point to the new one.

Quoted 2 lines

Sigh, I am beginning to wonder if the character device side of this capability is a solution looking for a problem...

We will find a solution. :)

The following should work for the common case.

Best,
Kay

From: Kay Sievers <[email protected]>
Subject: sysfs: fix duplicated device number registration in /sys/dev/

If the parent device has the same dev_t, we skip the registration for the device number at /sys/dev.

Cc: Dan Williams <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Andrew Morton <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---

 core.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/base/core.c +11 -6

index de925f8..afedadb 100644
@@ -821,10 +821,13 @@ int device_add(struct device *dev)
 		if (error)
 			goto ueventattrError;
 
-		format_dev_t(devt_str, dev->devt);
-		error = sysfs_create_link(kobj, &dev->kobj, devt_str);
-		if (error)
-			goto devtattrError;
+		/* do not create /sys/dev/ entry if parent already did */
+		if (!(dev->parent && dev->parent->devt == dev->devt)) {
+			format_dev_t(devt_str, dev->devt);
+			error = sysfs_create_link(kobj, &dev->kobj, devt_str);
+			if (error)
+				goto devtattrError;
+		}
 	}
 
 	error = device_add_class_symlinks(dev);

@@ -950,8 +953,10 @@ void device_del(struct device *dev)
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt)) {
-		format_dev_t(devt_str, dev->devt);
-		sysfs_remove_link(device_to_dev_kobj(dev), devt_str);
+		if (!(parent && parent->devt == dev->devt)) {
+			format_dev_t(devt_str, dev->devt);
+			sysfs_remove_link(device_to_dev_kobj(dev), devt_str);
+		}
 		device_remove_file(dev, &devt_attr);
 	}
 	if (dev->class) {

Dan Williams

author

2008-04-18 16:38 UTC #7 lore raw

Quoted 4 lines · Kay Sievers

We will find a solution. :)
The following should work for the common case.

Thanks! This resolves the warnings here. Now I'll take a shot at fixing the CONFIG_BLOCK=n case.

Quoted 3 lines

Best,
Kay

Regards,
Dan