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

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