[PATCH] add transport class symlink to device object

[PATCH] add transport class symlink to device object

[James.Smart]

In prior conversations with folks doing utilities such as lsscsi, it became
rather apparent that it is difficult to establish the relationships between
the class device and its base device. The class->device symlink is good for
one direction, but the reverse direction didn't exist. In order to go from
the device to its class device, you needed to either search all class
devices to search for a device backlink that pointed to you, or had to build
in name relationships what the subset class devices that you need to look at.

This patch adds a symlink from the base device to the class device. The name
of the symlink is "class:<classname>".  I had originally considered just
<classname> for the symlink name, but it seemed to get lost in the attributes.

-- James S

Example:

# cd /sys/class/scsi_host/host0
# ls -ld device
lrwxrwxrwx  1 root root 0 Aug 13 11:11 device -> ../../../devices/platform/host0
# cd ../../../devices/platform/host0
# pwd
/sys/devices/platform/host0
# ls
class:scsi_host  power  target0:0:0  target0:0:1  target0:0:2  target0:0:3
# ls -ld class*
lrwxrwxrwx  1 root root 0 Aug 13 07:08 class:scsi_host -> ../../../class/scsi_host/host0




diff -upNr a/drivers/base/class.c b/drivers/base/class.c
--- a/drivers/base/class.c	2005-08-13 10:46:33.000000000 -0400
+++ b/drivers/base/class.c	2005-08-13 11:06:51.000000000 -0400
@@ -495,9 +495,13 @@ int class_device_add(struct class_device
 	}
 
 	class_device_add_attrs(class_dev);
-	if (class_dev->dev)
+	if (class_dev->dev) {
+		char buf[40];
 		sysfs_create_link(&class_dev->kobj,
 				  &class_dev->dev->kobj, "device");
+		snprintf(buf, 40, "class:%s", class_dev->class->name);
+		sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj, buf);
+	}
 
 	/* notify any interfaces this device is now here */
 	if (parent) {
@@ -589,8 +593,12 @@ void class_device_del(struct class_devic
 		up(&parent->sem);
 	}
 
-	if (class_dev->dev)
+	if (class_dev->dev) {
+		char buf[40];
+		snprintf(buf, 40, "class:%s", class_dev->class->name);
+		sysfs_remove_link(&class_dev->dev->kobj, buf);
 		sysfs_remove_link(&class_dev->kobj, "device");
+	}
 	if (class_dev->devt_attr) {
 		class_device_remove_file(class_dev, class_dev->devt_attr);
 		kfree(class_dev->devt_attr);

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Sat, Aug 13, 2005 at 11:34:17AM -0400, James.Smart@Emulex.Com wrote:
> In prior conversations with folks doing utilities such as lsscsi, it became
> rather apparent that it is difficult to establish the relationships between
> the class device and its base device. The class->device symlink is good for
> one direction, but the reverse direction didn't exist. In order to go from
> the device to its class device, you needed to either search all class
> devices to search for a device backlink that pointed to you, or had to build
> in name relationships what the subset class devices that you need to look at.
> 
> This patch adds a symlink from the base device to the class device. The name
> of the symlink is "class:<classname>".  I had originally considered just
> <classname> for the symlink name, but it seemed to get lost in the attributes.

Heh, I already have a patch like this pending for 2.6.14 at:
	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/driver-link-device-and-class.patch

Andrew keeps disabling it in the -mm releases due to it misbehaiving on
some of his scsi devices.  Perhaps he can post that oops message again
so that someone here can fix it up before I add it to the main tree?

thanks,

greg k-h

Re: [PATCH] add transport class symlink to device object

[James Bottomley]

On Sat, 2005-08-13 at 14:39 -0700, Greg KH wrote:
> Andrew keeps disabling it in the -mm releases due to it misbehaiving on
> some of his scsi devices.  Perhaps he can post that oops message again
> so that someone here can fix it up before I add it to the main tree?

Actually, no need, it oopses for me too:

scsi0 : sym-2.2.1
Unable to handle kernel NULL pointer dereference (address
00000000000000c0)
modprobe[790]: Oops 8821862825984 [1]
Modules linked in: sym53c8xx scsi_transport_spi scsi_mod

Pid: 790, CPU 0, comm:             modprobe
psr : 00001010081a6018 ifs : 800000000000058d ip  : [<a0000001001b3740>]
Not tainted
ip is at sysfs_hash_and_remove+0x40/0x300
unat: 0000000000000000 pfs : 0000000000000287 rsc : 0000000000000003
rnat: 00000000000011a7 bsps: 0000000000000000 pr  : 0000000000069a65
ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c8a70433f
csd : 0000000000000000 ssd : 0000000000000000
b0  : a0000001001b7d50 b6  : a0000001001ef340 b7  : a00000010033eba0
f6  : 1003e6b6b6b6b6b6b6b6b f7  : 0ffe2a800000000000000
f8  : 1003e0000000000000a80 f9  : 1003e000000000000001c
f10 : 10003dfffffffff200000 f11 : 1003e000000000000001c
r1  : a000000100985710 r2  : e000000001f28638 r3  : 0000000000000001
r8  : 0000000000000000 r9  : e000000001da51b8 r10 : a0000002008db0b8
r11 : e00000407ea1a9c0 r12 : e0000000048ffc30 r13 : e0000000048f8000
r14 : e000000001f28510 r15 : a0000002008db0c8 r16 : 0000000000004000
r17 : 0000000000000013 r18 : e0000000014bc328 r19 : 0000000000000001
r20 : 0000000000000000 r21 : e00000407ea1aa30 r22 : 0000000000010c00
r23 : 0000000000000001 r24 : 00000000170fc2a5 r25 : 00000000170fc2a5
r26 : e00000407e870c40 r27 : e00000407e870c50 r28 : e00000407e870c48
r29 : 0000000000010c00 r30 : 0000000000000001 r31 : 0000000000000000

Call Trace:
 [<a000000100010380>] show_stack+0x80/0xa0
                                sp=e0000000048ff7d0 bsp=e0000000048f94c8
 [<a000000100010be0>] show_regs+0x7e0/0x800
                                sp=e0000000048ff9a0 bsp=e0000000048f9468
 [<a0000001000357b0>] die+0x150/0x1c0
                                sp=e0000000048ff9b0 bsp=e0000000048f9420
 [<a0000001000595f0>] ia64_do_page_fault+0x3f0/0xa60
                                sp=e0000000048ff9b0 bsp=e0000000048f93b0
 [<a00000010000b740>] ia64_leave_kernel+0x0/0x280
                                sp=e0000000048ffa60 bsp=e0000000048f93b0
 [<a0000001001b3740>] sysfs_hash_and_remove+0x40/0x300
                                sp=e0000000048ffc30 bsp=e0000000048f9348
 [<a0000001001b7d50>] sysfs_remove_link+0x30/0x60
                                sp=e0000000048ffc30 bsp=e0000000048f9320
 [<a000000100339ca0>] class_device_del+0x280/0x2e0
                                sp=e0000000048ffc30 bsp=e0000000048f92d8
 [<a00000010033e710>] attribute_container_class_device_del+0x30/0x60
                                sp=e0000000048ffc30 bsp=e0000000048f92b8
 [<a00000010033ec50>] transport_remove_classdev+0xb0/0x100
                                sp=e0000000048ffc30 bsp=e0000000048f9288
 [<a00000010033e220>] attribute_container_device_trigger+0x220/0x240
                                sp=e0000000048ffc30 bsp=e0000000048f9238
 [<a00000010033ecd0>] transport_remove_device+0x30/0x60
                                sp=e0000000048ffc30 bsp=e0000000048f9218
 [<a00000020091d590>] scsi_target_reap+0x190/0x1e0 [scsi_mod]
                                sp=e0000000048ffc30 bsp=e0000000048f91e0
 [<a00000020091fa60>] scsi_scan_target+0x160/0x300 [scsi_mod]
                                sp=e0000000048ffc30 bsp=e0000000048f9190
 [<a00000020091fd10>] scsi_scan_channel+0x110/0x140 [scsi_mod]
                                sp=e0000000048ffc40 bsp=e0000000048f9140
 [<a00000020091ff80>] scsi_scan_host_selected+0x240/0x2e0 [scsi_mod]
                                sp=e0000000048ffc40 bsp=e0000000048f90f0
 [<a000000200920060>] scsi_scan_host+0x40/0x60 [scsi_mod]
                                sp=e0000000048ffc40 bsp=e0000000048f90d0
 [<a0000002009875e0>] sym2_probe+0x1e0/0x2a0 [sym53c8xx]
                                sp=e0000000048ffc40 bsp=e0000000048f90a0
 [<a000000100202a80>] __pci_device_probe+0xe0/0x100
                                sp=e0000000048ffe10 bsp=e0000000048f9068
 [<a000000100202af0>] pci_device_probe+0x50/0xa0
                                sp=e0000000048ffe10 bsp=e0000000048f9040
 [<a000000100337210>] driver_probe_device+0xb0/0x1a0
                                sp=e0000000048ffe10 bsp=e0000000048f9008
 [<a0000001003374d0>] __driver_attach+0x70/0xc0
                                sp=e0000000048ffe10 bsp=e0000000048f8fd0
 [<a000000100335df0>] bus_for_each_dev+0xb0/0x100
                                sp=e0000000048ffe10 bsp=e0000000048f8f90
 [<a000000100337550>] driver_attach+0x30/0x60
                                sp=e0000000048ffe30 bsp=e0000000048f8f70
 [<a000000100336860>] bus_add_driver+0x100/0x1a0
                                sp=e0000000048ffe30 bsp=e0000000048f8f38
 [<a000000100337d60>] driver_register+0x60/0x80
                                sp=e0000000048ffe30 bsp=e0000000048f8f18
 [<a0000001002031f0>] pci_register_driver+0xf0/0x120
                                sp=e0000000048ffe30 bsp=e0000000048f8ef8
 [<a0000002008b8100>] sym2_init+0x80/0xf0 [sym53c8xx]
                                sp=e0000000048ffe30 bsp=e0000000048f8ed0
 [<a0000001000d11c0>] sys_init_module+0x460/0x620
                                sp=e0000000048ffe30 bsp=e0000000048f8e50
 [<a00000010000b5a0>] ia64_ret_from_syscall+0x0/0x20
                                sp=e0000000048ffe30 bsp=e0000000048f8e50

And the reason why yours crashes and James' doesn't is the kobject_name
().  Kobject names aren't valid until the object is visible, which is
long after the class is created in the SCSI case.  The actual panic is
in removal on a NULL kobject name.

So, perhaps James' might be the patch you'd prefer to your current one?

James

Re: [PATCH] add transport class symlink to device object

[James Bottomley]

On Sat, 2005-08-13 at 18:36 -0500, James Bottomley wrote:
> And the reason why yours crashes and James' doesn't is the kobject_name
> ().  Kobject names aren't valid until the object is visible, which is
> long after the class is created in the SCSI case.  The actual panic is
> in removal on a NULL kobject name.
> 
> So, perhaps James' might be the patch you'd prefer to your current one?

OK, should have engaged test apparatus before putting mouth in gear.
Both of these patches fail in the same way.  I'm sure the key is object
visibility.  In the failure case, the object is created, has classes
added and is destroyed without being made visible.

I'll look into it.

James

Re: [PATCH] add transport class symlink to device object

[James Bottomley]

On Sat, 2005-08-13 at 19:42 -0500, James Bottomley wrote:
> I'll look into it.

OK, I think this fix will work for either patch.  The problem is that if
the class hasn't been made visible, then the dentry has no inode, so in
that case we can just exit out of sysfs_hash_and_remove().

Someone needs to check this though ... I've ventured further into sysfs
than a sane person ought to go.  In fact if I don't leave now, Mr.
Flibble's going to be very cross ...

James

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -228,6 +228,10 @@ void sysfs_hash_and_remove(struct dentry
 	struct sysfs_dirent * sd;
 	struct sysfs_dirent * parent_sd = dir->d_fsdata;
 
+	if (dir->d_inode == NULL)
+		/* no inode means this hasn't been made visible yet */
+		return;
+
 	down(&dir->d_inode->i_sem);
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
 		if (!sd->s_element)

RE: [PATCH] add transport class symlink to device object

[James.Smart]

I'll trust that James B's fix resolves things. Thought my testing was
straight-forward and ok. Guess not. Humbled again as a mere mortal in
the world of sysfs and transport/container logic... :)

Anyway, the last item that needs discussion is what name should the
symlink have ?

Here's a few points on why I chose a "class:<classname>" over 
"<classobjname>".

- Visually, I get more meaning out of seeing the class's name, than
  what is usually a redundant object name (many device and class
  objects have the same name).
- If a device could ever be associated with more than 1 class, it's
  supported (and no issues of name collision).
- The prefix of "class:" highlights what it is; makes it harder to lose
  in a long list of attributes; gives a simple handle for regex
  parsing (ex: "ls class*"); and helps avoid name collision with other
  attributes.
- The class object's name is always derivable from what the symlink
  points to.
- The class name is a good hint for utilities
(ok, last 2 are a little weak - you can parse the symlink for either of
 of them).

Preferences ?

-- james

RE: [PATCH] add transport class symlink to device object

[James Bottomley]

On Sun, 2005-08-14 at 08:42 -0400, James.Smart@Emulex.Com wrote:
> I'll trust that James B's fix resolves things. Thought my testing was
> straight-forward and ok. Guess not. Humbled again as a mere mortal in
> the world of sysfs and transport/container logic... :)

Well ... I confidently predict that you only tested in an all fibre
environment.  The bug only shows up with scanning (which we no longer do
for fibre).

> - Visually, I get more meaning out of seeing the class's name, than
>   what is usually a redundant object name (many device and class
>   objects have the same name).

This is also an argument for a directory rather than a prefix.

> - If a device could ever be associated with more than 1 class, it's
>   supported (and no issues of name collision).

This is a killer reason for not using the other class.  We allow this,
and already have several examples.  The host device, for instance is a
member of both the spi_host class and the scsi_host class.  Indeed, you
can see the problem with Greg's original patch:

titanic:~# ls -l /sys/class/scsi_host/host0/device/
total 0
lrwxrwxrwx  1 root root 0 Aug 14 09:05 host0 -> ../../../../class/spi_host/host0
lrwxrwxrwx  1 root root 0 Aug 14 09:05 host0 -> ../../../../class/spi_host/host0
drwxr-xr-x  2 root root 0 Aug 14 09:02 power

In fact, not naming the link after the class is particularly daft since
the object is to tell you what classes a device belongs to.

By contrast, this is what you get under James' patch:

titanic:~# ls -l /sys/class/scsi_host/host0/device/
total 0
lrwxrwxrwx  1 root root 0 Aug 14 09:16 class:scsi_host -> ../../../../class/scsi_host/host0
lrwxrwxrwx  1 root root 0 Aug 14 09:16 class:spi_host -> ../../../../class/spi_host/host0
drwxr-xr-x  2 root root 0 Aug 14 09:13 power

> - The prefix of "class:" highlights what it is; makes it harder to lose
>   in a long list of attributes; gives a simple handle for regex
>   parsing (ex: "ls class*"); and helps avoid name collision with other
>   attributes.

> - The class object's name is always derivable from what the symlink
>   points to.

Again, I think this one is a requirement.

James

Re: [PATCH] add transport class symlink to device object

[Matthew Wilcox]

On Sat, Aug 13, 2005 at 02:39:56PM -0700, Greg KH wrote:
> Heh, I already have a patch like this pending for 2.6.14 at:
> 	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/driver-link-device-and-class.patch

Last time I tried to do something like this, it fell over with
multi-function serial ports.  Look at this example:

# ls -l /sys/class/tty/ttyS*/device | cut -c40-
/sys/class/tty/ttyS0/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
/sys/class/tty/ttyS1/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
/sys/class/tty/ttyS2/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
/sys/class/tty/ttyS3/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
/sys/class/tty/ttyS4/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0

Adding the reverse links gets you three links in the 0000:00:04.0
directory all called 'tty' (or 'class:tty', whatever), each pointing to
a different place.  This doesn't happen for scsi devices as the class is
attached to the scsi_dev, not the pci_dev.  I think the tty subsystem
needs to be modified to add tty_devs as subdevices of the pci_dev.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] add transport class symlink to device object

[Russell King]

On Sun, Aug 14, 2005 at 04:02:31PM +0100, Matthew Wilcox wrote:
> On Sat, Aug 13, 2005 at 02:39:56PM -0700, Greg KH wrote:
> > Heh, I already have a patch like this pending for 2.6.14 at:
> > 	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/driver-link-device-and-class.patch
> 
> Last time I tried to do something like this, it fell over with
> multi-function serial ports.  Look at this example:
> 
> # ls -l /sys/class/tty/ttyS*/device | cut -c40-
> /sys/class/tty/ttyS0/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS1/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS2/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS3/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> /sys/class/tty/ttyS4/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> 
> Adding the reverse links gets you three links in the 0000:00:04.0
> directory all called 'tty' (or 'class:tty', whatever), each pointing to
> a different place.  This doesn't happen for scsi devices as the class is
> attached to the scsi_dev, not the pci_dev.  I think the tty subsystem
> needs to be modified to add tty_devs as subdevices of the pci_dev.

Eww.  Do you really want one struct device per tty with all the
memory each one eats?

If that's really what you want you need to talk to Alan and not me.
Alan looks after tty level stuff, I look after serial level stuff.
The above is a tty level issue not a serial level issue.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] add transport class symlink to device object

[Matthew Wilcox]

On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> On Sun, Aug 14, 2005 at 04:02:31PM +0100, Matthew Wilcox wrote:
> > Last time I tried to do something like this, it fell over with
> > multi-function serial ports.  Look at this example:
> > 
> > # ls -l /sys/class/tty/ttyS*/device | cut -c40-
> > /sys/class/tty/ttyS0/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS1/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS2/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > 
> > Adding the reverse links gets you three links in the 0000:00:04.0
> > directory all called 'tty' (or 'class:tty', whatever), each pointing to
> > a different place.  This doesn't happen for scsi devices as the class is
> > attached to the scsi_dev, not the pci_dev.  I think the tty subsystem
> > needs to be modified to add tty_devs as subdevices of the pci_dev.
> 
> Eww.  Do you really want one struct device per tty with all the
> memory each one eats?
> 
> If that's really what you want you need to talk to Alan and not me.
> Alan looks after tty level stuff, I look after serial level stuff.
> The above is a tty level issue not a serial level issue.

mmm.  I don't know whether it's really a tty level issue or a serial
issue.  The only tty classes with corresponding devices are the serial
ones, at least on my system.  If this is the case, then the right fix
would seem to be something like creating a new struct device for each
serial port, then making that the uart_port->dev instead of the pci_dev
or whatever.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] add transport class symlink to device object

[Russell King]

On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > Eww.  Do you really want one struct device per tty with all the
> > memory each one eats?
> > 
> > If that's really what you want you need to talk to Alan and not me.
> > Alan looks after tty level stuff, I look after serial level stuff.
> > The above is a tty level issue not a serial level issue.
> 
> mmm.  I don't know whether it's really a tty level issue or a serial
> issue.  The only tty classes with corresponding devices are the serial
> ones, at least on my system.  If this is the case, then the right fix
> would seem to be something like creating a new struct device for each
> serial port, then making that the uart_port->dev instead of the pci_dev
> or whatever.

What's the reason for enforcing one struct device per struct class_dev ?
I thought one of the points of class_dev was that you could have multiple
of them per struct device.

Please note that I have other commitments for the next fortnight, so my
time this week is _very_ limited.  Can this wait until September?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] add transport class symlink to device object

[James Bottomley]

On Sun, 2005-08-14 at 16:02 +0100, Matthew Wilcox wrote:
> /sys/class/tty/ttyS0/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS1/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS2/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> /sys/class/tty/ttyS3/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> /sys/class/tty/ttyS4/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0

Actually, isn't the fix to all of this to combine Greg and James'
patches?

The Greg one fails in SCSI because we don't have unique class device
names (by convention we use the same name as the device bus_id) and
James' one fails for ttys because the class name isn't unique.  However,
if the link were derived from something like

<class name>:<class device name>

Then is would be unique in both cases.

Unless anyone can think of any more failing cases?

James

RE: [PATCH] add transport class symlink to device object

[James.Smart]

Actually, I view this as being a little odd...

What is "0000:00:04:0" in this case ? The "device" is not a serial
port, which is what the ttyXX back link would lead you to believe.
Thus, it's a serial port multiplexer that supports up to N ports,
right ? and wouldn't the more correct representation have been to
enumerate a device for each serial port ? (e.g. 0000:00:04.0/line0,
0000:00:04.0/line1, or similar)

Think if SCSI used this same style of representation. For example,
if there was no scsi target device entity, but class entities did
exist and they just pointed back to the scsi host device entry.

My vote is to make the multiplexor instantiate each serial line
as a separate device.

-- james s

> On Sun, 2005-08-14 at 16:02 +0100, Matthew Wilcox wrote:
> > /sys/class/tty/ttyS0/device -> 
> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS1/device -> 
> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS2/device -> 
> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS3/device -> 
> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> > /sys/class/tty/ttyS4/device -> 
> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> 
> Actually, isn't the fix to all of this to combine Greg and James'
> patches?
> 
> The Greg one fails in SCSI because we don't have unique class device
> names (by convention we use the same name as the device bus_id) and
> James' one fails for ttys because the class name isn't 
> unique.  However,
> if the link were derived from something like
> 
> <class name>:<class device name>
> 
> Then is would be unique in both cases.
> 
> Unless anyone can think of any more failing cases?
> 
> James
> 
> 
> 

RE: [PATCH] add transport class symlink to device object

[James Bottomley]

On Mon, 2005-08-15 at 20:52 -0400, James.Smart@Emulex.Com wrote:
> What is "0000:00:04:0" in this case ? The "device" is not a serial
> port, which is what the ttyXX back link would lead you to believe.
> Thus, it's a serial port multiplexer that supports up to N ports,
> right ? and wouldn't the more correct representation have been to
> enumerate a device for each serial port ? (e.g. 0000:00:04.0/line0,
> 0000:00:04.0/line1, or similar)

It's PCI segment 0, bus 0, slot 4, function 0, which is apparently a 3
port serial card (probably the GSP function of a pa8800?)

> Think if SCSI used this same style of representation. For example,
> if there was no scsi target device entity, but class entities did
> exist and they just pointed back to the scsi host device entry.

Yes, it's theoretically possible to have had SCSI do this.  We didn't do
it at the time because class_devices didn't exist when the SCSI tree was
first put together.  It would, however, have rather put the mockers on
doing transport classes since class devices can't point at other class
devices.

> My vote is to make the multiplexor instantiate each serial line
> as a separate device.

That's a choice that's up to the maintainer of the serial driver ...

James

Re: [PATCH] add transport class symlink to device object

[Luben Tuikov]

On 08/15/05 20:52, James.Smart@Emulex.Com wrote:
> Actually, I view this as being a little odd...
> 
> What is "0000:00:04:0" in this case ? The "device" is not a serial
> port, which is what the ttyXX back link would lead you to believe.
> Thus, it's a serial port multiplexer that supports up to N ports,
> right ? and wouldn't the more correct representation have been to
> enumerate a device for each serial port ? (e.g. 0000:00:04.0/line0,
> 0000:00:04.0/line1, or similar)
> 
> Think if SCSI used this same style of representation. For example,
> if there was no scsi target device entity, but class entities did
> exist and they just pointed back to the scsi host device entry.
> 
> My vote is to make the multiplexor instantiate each serial line
> as a separate device.

Hi James,

Yes, you're absolutely and completely correct.  I think the same
way as you do.

	Luben

Re: [PATCH] add transport class symlink to device object

[Luben Tuikov]

On 08/15/05 21:08, James Bottomley wrote:
>>Think if SCSI used this same style of representation. For example,
>>if there was no scsi target device entity, but class entities did
>>exist and they just pointed back to the scsi host device entry.
> 
> 
> Yes, it's theoretically possible to have had SCSI do this.  We didn't do
> it at the time because class_devices didn't exist when the SCSI tree was
> first put together.  It would, however, have rather put the mockers on
> doing transport classes since class devices can't point at other class
> devices.

Well, so be it.

All in all, I'd like to point out that James S has a very good
and valid point, as anyone trained in SCSI protocols can see.

>>My vote is to make the multiplexor instantiate each serial line
>>as a separate device.
> 
> That's a choice that's up to the maintainer of the serial driver ...

I think James S, was making a point of concept.  Maybe SCSI Core can
learn from this?

	Luben

RE: [PATCH] add transport class symlink to device object

[James.Smart]

> On Mon, 2005-08-15 at 20:52 -0400, James.Smart@Emulex.Com wrote:
> > What is "0000:00:04:0" in this case ? The "device" is not a serial
> > port, which is what the ttyXX back link would lead you to believe.
> > Thus, it's a serial port multiplexer that supports up to N ports,
> > right ? and wouldn't the more correct representation have been to
> > enumerate a device for each serial port ? (e.g. 0000:00:04.0/line0,
> > 0000:00:04.0/line1, or similar)
> 
> It's PCI segment 0, bus 0, slot 4, function 0, which is apparently a 3
> port serial card (probably the GSP function of a pa8800?)

I guess you missed my point. The device (the PCI serial card) isn't the
member of the class, but rather an element of the device (one of the ports
on the pci card) is the class member. Thus, I believe the device backlink
for the class entity is misleading. The backlink implies the pci serial
card itself is the port. Ignoring this, the other headache is the device
backlink gives no clue as to what port or part of the pci adapter it
corresponds to.

In my conceptual thinking, there's a "wholeness" to the relationship of a
device and it's classes. E.g. trying to illustrate this with pci rather than
scsi - if there's a 3 slot pci bridge, with an adapter in each slot, I would
expect 4 different device entities: the bridge, and one for each adapter.
Each device would then be bound to whatever class makes sense for that
adapter. What I would not expect is a single device for the bridge and 3
class devices (one for each slot) that points back to the bridge device.

-- james s

Re: [PATCH] add transport class symlink to device object

[Russell King]

On Tue, Aug 16, 2005 at 09:37:18AM -0400, Luben Tuikov wrote:
> On 08/15/05 20:52, James.Smart@Emulex.Com wrote:
> > My vote is to make the multiplexor instantiate each serial line
> > as a separate device.
> 
> Yes, you're absolutely and completely correct.  I think the same
> way as you do.

Just don't expect it to happen any time in the next fortnight, even
if 2.6.13 were to appear tomorrow.  Firstly, I have the parport_serial
patches queued up since god knows when (a month or so back now I
think.)  Secondly I'm not around next week so I probably won't have
time to do anything with serial in the non-rc part of the 2.6.14
cycle.

Therefore, I think this will have to wait for 2.6.14 or so.  Sorry.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Mon, Aug 15, 2005 at 09:32:44AM +0100, Russell King wrote:
> On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> > On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > > Eww.  Do you really want one struct device per tty with all the
> > > memory each one eats?
> > > 
> > > If that's really what you want you need to talk to Alan and not me.
> > > Alan looks after tty level stuff, I look after serial level stuff.
> > > The above is a tty level issue not a serial level issue.
> > 
> > mmm.  I don't know whether it's really a tty level issue or a serial
> > issue.  The only tty classes with corresponding devices are the serial
> > ones, at least on my system.  If this is the case, then the right fix
> > would seem to be something like creating a new struct device for each
> > serial port, then making that the uart_port->dev instead of the pci_dev
> > or whatever.
> 
> What's the reason for enforcing one struct device per struct class_dev ?
> I thought one of the points of class_dev was that you could have multiple
> of them per struct device.

No such enforcement is needed at all, and not encouraged.

thanks,

greg k-h

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Mon, Aug 15, 2005 at 05:41:17PM -0500, James Bottomley wrote:
> On Sun, 2005-08-14 at 16:02 +0100, Matthew Wilcox wrote:
> > /sys/class/tty/ttyS0/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS1/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS2/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:04.0
> > /sys/class/tty/ttyS3/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> > /sys/class/tty/ttyS4/device -> ../../../devices/parisc/0/0:0/pci0000:00/0000:00:05.0
> 
> Actually, isn't the fix to all of this to combine Greg and James'
> patches?

Yes it is.

> The Greg one fails in SCSI because we don't have unique class device
> names (by convention we use the same name as the device bus_id) and
> James' one fails for ttys because the class name isn't unique.  However,
> if the link were derived from something like
> 
> <class name>:<class device name>
> 
> Then is would be unique in both cases.

I agree.

> Unless anyone can think of any more failing cases?

I'll try this out and see if anything breaks :)

thanks,

greg k-h

Re: [PATCH] add transport class symlink to device object

[Russell King]

On Wed, Aug 17, 2005 at 10:21:56PM -0700, Greg KH wrote:
> On Mon, Aug 15, 2005 at 09:32:44AM +0100, Russell King wrote:
> > On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> > > On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > > > Eww.  Do you really want one struct device per tty with all the
> > > > memory each one eats?
> > > > 
> > > > If that's really what you want you need to talk to Alan and not me.
> > > > Alan looks after tty level stuff, I look after serial level stuff.
> > > > The above is a tty level issue not a serial level issue.
> > > 
> > > mmm.  I don't know whether it's really a tty level issue or a serial
> > > issue.  The only tty classes with corresponding devices are the serial
> > > ones, at least on my system.  If this is the case, then the right fix
> > > would seem to be something like creating a new struct device for each
> > > serial port, then making that the uart_port->dev instead of the pci_dev
> > > or whatever.
> > 
> > What's the reason for enforcing one struct device per struct class_dev ?
> > I thought one of the points of class_dev was that you could have multiple
> > of them per struct device.
> 
> No such enforcement is needed at all, and not encouraged.

The complaint is that serial is registering several different class_devs
for the same class and device.

So that's precisely what is being done by adding the symlink as per this
sub-thread.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Wed, Aug 17, 2005 at 10:23:11PM -0700, Greg KH wrote:
> On Mon, Aug 15, 2005 at 05:41:17PM -0500, James Bottomley wrote:
> > Actually, isn't the fix to all of this to combine Greg and James'
> > patches?
> 
> Yes it is.
> 
> > The Greg one fails in SCSI because we don't have unique class device
> > names (by convention we use the same name as the device bus_id) and
> > James' one fails for ttys because the class name isn't unique.  However,
> > if the link were derived from something like
> > 
> > <class name>:<class device name>
> > 
> > Then is would be unique in both cases.
> 
> I agree.
> 
> > Unless anyone can think of any more failing cases?
> 
> I'll try this out and see if anything breaks :)

Ok, here's the patch.  I like it, and I think it will prevent any
duplicate symlinks from happening.  Anyone have any objections?

thanks,

greg k-h

--------------

Driver core: link device and all class devices derived from it.

To ease the task of locating class devices derived from a certain
device create symlinks from parent device to its class devices.
Change USB host class device name from usbX to usb_hostX to avoid
conflict when creating aforementioned links.

Tweaked by Greg to have the symlink be "class_name:class_device_name"
in order to prevent duplicate links.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/class.c   |   33 +++++++++++++++++++++++++++++++--
 drivers/usb/core/hcd.c |    2 +-
 2 files changed, 32 insertions(+), 3 deletions(-)

--- gregkh-2.6.orig/drivers/base/class.c	2005-08-17 23:27:02.000000000 -0700
+++ gregkh-2.6/drivers/base/class.c	2005-08-17 23:27:25.000000000 -0700
@@ -452,10 +452,29 @@ void class_device_initialize(struct clas
 	INIT_LIST_HEAD(&class_dev->node);
 }
 
+static char *make_class_name(struct class_device *class_dev)
+{
+	char *name;
+	int size;
+
+	size = strlen(class_dev->class->name) +
+		strlen(kobject_name(&class_dev->kobj)) + 2;
+
+	name = kmalloc(size, GFP_KERNEL);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	strcpy(name, class_dev->class->name);
+	strcat(name, ":");
+	strcat(name, kobject_name(&class_dev->kobj));
+	return name;
+}
+
 int class_device_add(struct class_device *class_dev)
 {
 	struct class * parent = NULL;
 	struct class_interface * class_intf;
+	char *class_name = NULL;
 	int error;
 
 	class_dev = class_device_get(class_dev);
@@ -500,9 +519,13 @@ int class_device_add(struct class_device
 	}
 
 	class_device_add_attrs(class_dev);
-	if (class_dev->dev)
+	if (class_dev->dev) {
+		class_name = make_class_name(class_dev);
 		sysfs_create_link(&class_dev->kobj,
 				  &class_dev->dev->kobj, "device");
+		sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
+				  class_name);
+	}
 
 	/* notify any interfaces this device is now here */
 	if (parent) {
@@ -519,6 +542,7 @@ int class_device_add(struct class_device
 	if (error && parent)
 		class_put(parent);
 	class_device_put(class_dev);
+	kfree(class_name);
 	return error;
 }
 
@@ -584,6 +608,7 @@ void class_device_del(struct class_devic
 {
 	struct class * parent = class_dev->class;
 	struct class_interface * class_intf;
+	char *class_name = NULL;
 
 	if (parent) {
 		down(&parent->sem);
@@ -594,8 +619,11 @@ void class_device_del(struct class_devic
 		up(&parent->sem);
 	}
 
-	if (class_dev->dev)
+	if (class_dev->dev) {
+		class_name = make_class_name(class_dev);
 		sysfs_remove_link(&class_dev->kobj, "device");
+		sysfs_remove_link(&class_dev->dev->kobj, class_name);
+	}
 	if (class_dev->devt_attr)
 		class_device_remove_file(class_dev, class_dev->devt_attr);
 	class_device_remove_attrs(class_dev);
@@ -605,6 +633,7 @@ void class_device_del(struct class_devic
 
 	if (parent)
 		class_put(parent);
+	kfree(class_name);
 }
 
 void class_device_unregister(struct class_device *class_dev)
--- gregkh-2.6.orig/drivers/usb/core/hcd.c	2005-08-17 23:26:55.000000000 -0700
+++ gregkh-2.6/drivers/usb/core/hcd.c	2005-08-17 23:27:04.000000000 -0700
@@ -782,7 +782,7 @@ static int usb_register_bus(struct usb_b
 		return -E2BIG;
 	}
 
-	bus->class_dev = class_device_create(usb_host_class, MKDEV(0,0), bus->controller, "usb%d", busnum);
+	bus->class_dev = class_device_create(usb_host_class, MKDEV(0,0), bus->controller, "usb_host%d", busnum);
 	if (IS_ERR(bus->class_dev)) {
 		clear_bit(busnum, busmap.busmap);
 		up(&usb_bus_list_lock);

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Thu, Aug 18, 2005 at 07:30:50AM +0100, Russell King wrote:
> On Wed, Aug 17, 2005 at 10:21:56PM -0700, Greg KH wrote:
> > On Mon, Aug 15, 2005 at 09:32:44AM +0100, Russell King wrote:
> > > On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> > > > On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > > > > Eww.  Do you really want one struct device per tty with all the
> > > > > memory each one eats?
> > > > > 
> > > > > If that's really what you want you need to talk to Alan and not me.
> > > > > Alan looks after tty level stuff, I look after serial level stuff.
> > > > > The above is a tty level issue not a serial level issue.
> > > > 
> > > > mmm.  I don't know whether it's really a tty level issue or a serial
> > > > issue.  The only tty classes with corresponding devices are the serial
> > > > ones, at least on my system.  If this is the case, then the right fix
> > > > would seem to be something like creating a new struct device for each
> > > > serial port, then making that the uart_port->dev instead of the pci_dev
> > > > or whatever.
> > > 
> > > What's the reason for enforcing one struct device per struct class_dev ?
> > > I thought one of the points of class_dev was that you could have multiple
> > > of them per struct device.
> > 
> > No such enforcement is needed at all, and not encouraged.
> 
> The complaint is that serial is registering several different class_devs
> for the same class and device.

That's because they are unique class devices, right?  I don't see a
problem here at all.

> So that's precisely what is being done by adding the symlink as per this
> sub-thread.

Ok, I think I'm just confused and I'll drop this now :)

thanks,

greg k-h

Re: [PATCH] add transport class symlink to device object

[Russell King]

On Wed, Aug 17, 2005 at 11:41:29PM -0700, Greg KH wrote:
> On Thu, Aug 18, 2005 at 07:30:50AM +0100, Russell King wrote:
> > On Wed, Aug 17, 2005 at 10:21:56PM -0700, Greg KH wrote:
> > > On Mon, Aug 15, 2005 at 09:32:44AM +0100, Russell King wrote:
> > > > On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> > > > > On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > > > > > Eww.  Do you really want one struct device per tty with all the
> > > > > > memory each one eats?
> > > > > > 
> > > > > > If that's really what you want you need to talk to Alan and not me.
> > > > > > Alan looks after tty level stuff, I look after serial level stuff.
> > > > > > The above is a tty level issue not a serial level issue.
> > > > > 
> > > > > mmm.  I don't know whether it's really a tty level issue or a serial
> > > > > issue.  The only tty classes with corresponding devices are the serial
> > > > > ones, at least on my system.  If this is the case, then the right fix
> > > > > would seem to be something like creating a new struct device for each
> > > > > serial port, then making that the uart_port->dev instead of the pci_dev
> > > > > or whatever.
> > > > 
> > > > What's the reason for enforcing one struct device per struct class_dev ?
> > > > I thought one of the points of class_dev was that you could have multiple
> > > > of them per struct device.
> > > 
> > > No such enforcement is needed at all, and not encouraged.
> > 
> > The complaint is that serial is registering several different class_devs
> > for the same class and device.
> 
> That's because they are unique class devices, right?  I don't see a
> problem here at all.

They are class devices called ttyS0, ttyS1, ttyS2 so you can say
they're uniquely named.

The problem is that Matthew wants to add a symlink from the device
device to the class device to complement the class device to device
symlink, since we end up with multiple symlinks in the devices subdir
all called the same.

This causes serial a problem because we have multiple class devices
per device.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Thu, Aug 18, 2005 at 07:50:27AM +0100, Russell King wrote:
> On Wed, Aug 17, 2005 at 11:41:29PM -0700, Greg KH wrote:
> > On Thu, Aug 18, 2005 at 07:30:50AM +0100, Russell King wrote:
> > > On Wed, Aug 17, 2005 at 10:21:56PM -0700, Greg KH wrote:
> > > > On Mon, Aug 15, 2005 at 09:32:44AM +0100, Russell King wrote:
> > > > > On Mon, Aug 15, 2005 at 01:43:03AM +0100, Matthew Wilcox wrote:
> > > > > > On Sun, Aug 14, 2005 at 11:25:25PM +0100, Russell King wrote:
> > > > > > > Eww.  Do you really want one struct device per tty with all the
> > > > > > > memory each one eats?
> > > > > > > 
> > > > > > > If that's really what you want you need to talk to Alan and not me.
> > > > > > > Alan looks after tty level stuff, I look after serial level stuff.
> > > > > > > The above is a tty level issue not a serial level issue.
> > > > > > 
> > > > > > mmm.  I don't know whether it's really a tty level issue or a serial
> > > > > > issue.  The only tty classes with corresponding devices are the serial
> > > > > > ones, at least on my system.  If this is the case, then the right fix
> > > > > > would seem to be something like creating a new struct device for each
> > > > > > serial port, then making that the uart_port->dev instead of the pci_dev
> > > > > > or whatever.
> > > > > 
> > > > > What's the reason for enforcing one struct device per struct class_dev ?
> > > > > I thought one of the points of class_dev was that you could have multiple
> > > > > of them per struct device.
> > > > 
> > > > No such enforcement is needed at all, and not encouraged.
> > > 
> > > The complaint is that serial is registering several different class_devs
> > > for the same class and device.
> > 
> > That's because they are unique class devices, right?  I don't see a
> > problem here at all.
> 
> They are class devices called ttyS0, ttyS1, ttyS2 so you can say
> they're uniquely named.
> 
> The problem is that Matthew wants to add a symlink from the device
> device to the class device to complement the class device to device
> symlink, since we end up with multiple symlinks in the devices subdir
> all called the same.
> 
> This causes serial a problem because we have multiple class devices
> per device.

Ah, yeah, but the patch I just posted fixes it:

$ tree /sys/devices/platform/serial8250/
/sys/devices/platform/serial8250/
|-- bus -> ../../../bus/platform
|-- driver -> ../../../bus/platform/drivers/serial8250
|-- power
|   `-- state
|-- tty:ttyS0 -> ../../../class/tty/ttyS0
|-- tty:ttyS1 -> ../../../class/tty/ttyS1
|-- tty:ttyS2 -> ../../../class/tty/ttyS2
`-- tty:ttyS3 -> ../../../class/tty/ttyS3


Matthew, this work for you?

thanks,

greg k-h

RE: [PATCH] add transport class symlink to device object

[James.Smart]

> They are class devices called ttyS0, ttyS1, ttyS2 so you can say
> they're uniquely named.
> 
> The problem is that Matthew wants to add a symlink from the device
> device to the class device to complement the class device to device
> symlink, since we end up with multiple symlinks in the devices subdir
> all called the same.
> 
> This causes serial a problem because we have multiple class devices
> per device.

Missing some key words IMHO:

"This causes serial a problem because we have multiple class devices
(and each are of the *same* class) per device"

-- james s

RE: [PATCH] add transport class symlink to device object

[James.Smart]

I can live with this.  I would have liked the "class:" prefix, but the name
does start to get long, and this is ok.

-- james s

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Thursday, August 18, 2005 2:37 AM
> To: James Bottomley
> Cc: Matthew Wilcox; Smart, James; Andrew Morton; SCSI Mailing List;
> Linux Kernel; Alan Cox; Russell King; Dmitry Torokhov
> Subject: Re: [PATCH] add transport class symlink to device object
> 
> 
> On Wed, Aug 17, 2005 at 10:23:11PM -0700, Greg KH wrote:
> > On Mon, Aug 15, 2005 at 05:41:17PM -0500, James Bottomley wrote:
> > > Actually, isn't the fix to all of this to combine Greg and James'
> > > patches?
> > 
> > Yes it is.
> > 
> > > The Greg one fails in SCSI because we don't have unique 
> class device
> > > names (by convention we use the same name as the device 
> bus_id) and
> > > James' one fails for ttys because the class name isn't 
> unique.  However,
> > > if the link were derived from something like
> > > 
> > > <class name>:<class device name>
> > > 
> > > Then is would be unique in both cases.
> > 
> > I agree.
> > 
> > > Unless anyone can think of any more failing cases?
> > 
> > I'll try this out and see if anything breaks :)
> 
> Ok, here's the patch.  I like it, and I think it will prevent any
> duplicate symlinks from happening.  Anyone have any objections?
> 
> thanks,
> 
> greg k-h
> 
> --------------
> 
> Driver core: link device and all class devices derived from it.
> 
> To ease the task of locating class devices derived from a certain
> device create symlinks from parent device to its class devices.
> Change USB host class device name from usbX to usb_hostX to avoid
> conflict when creating aforementioned links.
> 
> Tweaked by Greg to have the symlink be "class_name:class_device_name"
> in order to prevent duplicate links.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> ---
>  drivers/base/class.c   |   33 +++++++++++++++++++++++++++++++--
>  drivers/usb/core/hcd.c |    2 +-
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> --- gregkh-2.6.orig/drivers/base/class.c	2005-08-17 
> 23:27:02.000000000 -0700
> +++ gregkh-2.6/drivers/base/class.c	2005-08-17 
> 23:27:25.000000000 -0700
> @@ -452,10 +452,29 @@ void class_device_initialize(struct clas
>  	INIT_LIST_HEAD(&class_dev->node);
>  }
>  
> +static char *make_class_name(struct class_device *class_dev)
> +{
> +	char *name;
> +	int size;
> +
> +	size = strlen(class_dev->class->name) +
> +		strlen(kobject_name(&class_dev->kobj)) + 2;
> +
> +	name = kmalloc(size, GFP_KERNEL);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	strcpy(name, class_dev->class->name);
> +	strcat(name, ":");
> +	strcat(name, kobject_name(&class_dev->kobj));
> +	return name;
> +}
> +
>  int class_device_add(struct class_device *class_dev)
>  {
>  	struct class * parent = NULL;
>  	struct class_interface * class_intf;
> +	char *class_name = NULL;
>  	int error;
>  
>  	class_dev = class_device_get(class_dev);
> @@ -500,9 +519,13 @@ int class_device_add(struct class_device
>  	}
>  
>  	class_device_add_attrs(class_dev);
> -	if (class_dev->dev)
> +	if (class_dev->dev) {
> +		class_name = make_class_name(class_dev);
>  		sysfs_create_link(&class_dev->kobj,
>  				  &class_dev->dev->kobj, "device");
> +		sysfs_create_link(&class_dev->dev->kobj, 
> &class_dev->kobj,
> +				  class_name);
> +	}
>  
>  	/* notify any interfaces this device is now here */
>  	if (parent) {
> @@ -519,6 +542,7 @@ int class_device_add(struct class_device
>  	if (error && parent)
>  		class_put(parent);
>  	class_device_put(class_dev);
> +	kfree(class_name);
>  	return error;
>  }
>  
> @@ -584,6 +608,7 @@ void class_device_del(struct class_devic
>  {
>  	struct class * parent = class_dev->class;
>  	struct class_interface * class_intf;
> +	char *class_name = NULL;
>  
>  	if (parent) {
>  		down(&parent->sem);
> @@ -594,8 +619,11 @@ void class_device_del(struct class_devic
>  		up(&parent->sem);
>  	}
>  
> -	if (class_dev->dev)
> +	if (class_dev->dev) {
> +		class_name = make_class_name(class_dev);
>  		sysfs_remove_link(&class_dev->kobj, "device");
> +		sysfs_remove_link(&class_dev->dev->kobj, class_name);
> +	}
>  	if (class_dev->devt_attr)
>  		class_device_remove_file(class_dev, 
> class_dev->devt_attr);
>  	class_device_remove_attrs(class_dev);
> @@ -605,6 +633,7 @@ void class_device_del(struct class_devic
>  
>  	if (parent)
>  		class_put(parent);
> +	kfree(class_name);
>  }
>  
>  void class_device_unregister(struct class_device *class_dev)
> --- gregkh-2.6.orig/drivers/usb/core/hcd.c	2005-08-17 
> 23:26:55.000000000 -0700
> +++ gregkh-2.6/drivers/usb/core/hcd.c	2005-08-17 
> 23:27:04.000000000 -0700
> @@ -782,7 +782,7 @@ static int usb_register_bus(struct usb_b
>  		return -E2BIG;
>  	}
>  
> -	bus->class_dev = class_device_create(usb_host_class, 
> MKDEV(0,0), bus->controller, "usb%d", busnum);
> +	bus->class_dev = class_device_create(usb_host_class, 
> MKDEV(0,0), bus->controller, "usb_host%d", busnum);
>  	if (IS_ERR(bus->class_dev)) {
>  		clear_bit(busnum, busmap.busmap);
>  		up(&usb_bus_list_lock);
> 

Re: [PATCH] add transport class symlink to device object

[Matthew Wilcox]

On Thu, Aug 18, 2005 at 12:04:42AM -0700, Greg KH wrote:
> Matthew, this work for you?

Yep, that's fine by me.  Thanks.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] add transport class symlink to device object

[Dmitry Torokhov]

On 8/18/05, Greg KH <greg@kroah.com> wrote:
> @@ -500,9 +519,13 @@ int class_device_add(struct class_device
>        }
> 
>        class_device_add_attrs(class_dev);
> -       if (class_dev->dev)
> +       if (class_dev->dev) {
> +               class_name = make_class_name(class_dev);
>                sysfs_create_link(&class_dev->kobj,
>                                  &class_dev->dev->kobj, "device");
> +               sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
> +                                 class_name);
> +       }
> 

I wonder if we need to grab a reference to class_dev->dev here:

        dev = device_get(class_dev->dev);
        if (dev) {
             ....
        }

Otherwise, if device gets unregistered/deleted before class device is
deleted we'll get into trouble when removing the link since
class_dev->dev will be garbage.

.. But grabbing that reference will cause pains in SCSI system which,
when I looked, removed class devices from device's release function.

-- 
Dmitry

Re: [PATCH] add transport class symlink to device object

[Greg KH]

On Thu, Aug 18, 2005 at 02:50:19PM -0500, Dmitry Torokhov wrote:
> On 8/18/05, Greg KH <greg@kroah.com> wrote:
> > @@ -500,9 +519,13 @@ int class_device_add(struct class_device
> >        }
> > 
> >        class_device_add_attrs(class_dev);
> > -       if (class_dev->dev)
> > +       if (class_dev->dev) {
> > +               class_name = make_class_name(class_dev);
> >                sysfs_create_link(&class_dev->kobj,
> >                                  &class_dev->dev->kobj, "device");
> > +               sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
> > +                                 class_name);
> > +       }
> > 
> 
> I wonder if we need to grab a reference to class_dev->dev here:
> 
>         dev = device_get(class_dev->dev);
>         if (dev) {
>              ....
>         }
> 
> Otherwise, if device gets unregistered/deleted before class device is
> deleted we'll get into trouble when removing the link since
> class_dev->dev will be garbage.
> 
> .. But grabbing that reference will cause pains in SCSI system which,
> when I looked, removed class devices from device's release function.

No the sysfs_create_link() call increments the kobject reference on the
target of the symlink.  See sysfs_add_link() for details.  So this
should be just fine, right?

thanks,

greg k-h

Re: [PATCH] add transport class symlink to device object

[Dmitry Torokhov]

On Wednesday 31 August 2005 16:43, Greg KH wrote:
> On Thu, Aug 18, 2005 at 02:50:19PM -0500, Dmitry Torokhov wrote:
> > On 8/18/05, Greg KH <greg@kroah.com> wrote:
> > > @@ -500,9 +519,13 @@ int class_device_add(struct class_device
> > >        }
> > > 
> > >        class_device_add_attrs(class_dev);
> > > -       if (class_dev->dev)
> > > +       if (class_dev->dev) {
> > > +               class_name = make_class_name(class_dev);
> > >                sysfs_create_link(&class_dev->kobj,
> > >                                  &class_dev->dev->kobj, "device");
> > > +               sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
> > > +                                 class_name);
> > > +       }
> > > 
> > 
> > I wonder if we need to grab a reference to class_dev->dev here:
> > 
> >         dev = device_get(class_dev->dev);
> >         if (dev) {
> >              ....
> >         }
> > 
> > Otherwise, if device gets unregistered/deleted before class device is
> > deleted we'll get into trouble when removing the link since
> > class_dev->dev will be garbage.
> > 
> > .. But grabbing that reference will cause pains in SCSI system which,
> > when I looked, removed class devices from device's release function.
> 
> No the sysfs_create_link() call increments the kobject reference on the
> target of the symlink.  See sysfs_add_link() for details.  So this
> should be just fine, right?
>

Yes, you are right. Sorry for the moise.

-- 
Dmitry