[PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[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 <kay.sievers@vrfy.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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(-)


diff --git a/drivers/base/core.c b/drivers/base/core.c
index ba21118..bce5e4b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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);
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ae94176..2075622 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -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)
 {

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Greg KH]

On Thu, Apr 17, 2008 at 07:11:03PM -0700, Dan Williams wrote:
> 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 <kay.sievers@vrfy.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> 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

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Andrew Morton]

On Thu, 17 Apr 2008 19:11:03 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> 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.

> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ba21118..bce5e4b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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?

> +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?

> +#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;
> +#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) "-*")
 
+extern struct class *usb_classdev_class;
+
 #endif /* _DEVICE_H_ */
_

deworsens things slightly.

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Dan Williams]

On Thu, Apr 17, 2008 at 8:08 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 17 Apr 2008 19:11:03 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
>
>  > 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 :(.

>
>  >
>  > diff --git a/drivers/base/core.c b/drivers/base/core.c
>  > index ba21118..bce5e4b 100644
>  > --- a/drivers/base/core.c
>  > +++ b/drivers/base/core.c
>  > @@ -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?
>
>
>  > +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?
>

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.

>
>  > +#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;
>  > +#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) "-*")
>
>
>  +extern struct class *usb_classdev_class;
>  +
>   #endif /* _DEVICE_H_ */
>  _
>
>  deworsens things slightly.
>

Thanks,
Dan

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Greg KH]

On Thu, Apr 17, 2008 at 07:11:03PM -0700, Dan Williams wrote:
> 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

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Dan Williams]

On Thu, Apr 17, 2008 at 8:21 PM, Greg KH <greg@kroah.com> wrote:
> On Thu, Apr 17, 2008 at 07:11:03PM -0700, Dan Williams wrote:
>
> > 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...
>

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?

>  thanks,
>
>  greg k-h
>

Thanks,
Dan

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Kay Sievers]

On Thu, 2008-04-17 at 21:59 -0700, Dan Williams wrote:
> On Thu, Apr 17, 2008 at 8:21 PM, Greg KH <greg@kroah.com> wrote:
> > On Thu, Apr 17, 2008 at 07:11:03PM -0700, Dan Williams wrote:
> >
> > > 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...
> >
> 
> 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/.

> 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.

> 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 <kay.sievers@vrfy.org>
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 <dan.j.williams@intel.com>
Cc: Greg KH <greg@kroah.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index de925f8..afedadb 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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) {

Re: [PATCH mm] sysfs: add /sys/dev/usb to handle CONFIG_USB_DEVICE_CLASS=y

[Dan Williams]

On Fri, Apr 18, 2008 at 12:54 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>  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.

>  Best,
>  Kay
>

Regards,
Dan