Alright, here's a patch that will add the ability to nest struct
class_device structures in sysfs. This should give everyone the ability
to model what they need to in the class directory (input, video, etc.).
Dmitry, care to update your input patchset to take advantage of this?
It should work out for what you want to do, and if not, please let me
know.
udev will have to be changed to properly support this, but I think that
Kay already has that taken care of, and is just waiting for some kernel
code to take advantage of this.
Comments?
And yes, I have a follow-on patch that fixes up all callers of the
class_device_create() function, as the api has changed now. It's in my
patch tree, along with this one, if you want to see it (should show up
in the next -mm).
thanks,
greg k-h
----
From: Greg Kroah-Hartman <[email protected]>
Subject: Driver Core: add the ability for class_device structures to be nested
This patch allows struct class_device to be nested, so that another
struct class_device can be the parent of a new one, instead of only
having the struct class be the parent. This will allow us to
(hopefully) fix up the input and video class subsystem mess.
But please people, don't go crazy and start making huge trees of class
devices, you should only need 2 levels deep to get everything to work
(remember to use a class_interface to get notification of a new class
device being added to the system.)
Oh, this also allows us to have the possibility of potentially, someday,
moving /sys/block into /sys/class. The main hindrance is that pesky
/dev numberspace issue...
Signed-off-by: Greg Kroah-Hartman <[email protected]>
drivers/base/class.c | 84 +++++++++++++++++++++++++++++--------------------
include/linux/device.h | 10 ++++-
2 files changed, 58 insertions(+), 36 deletions(-)
--- gregkh-2.6.orig/drivers/base/class.c 2005-09-28 13:56:21.000000000 -0700
+++ gregkh-2.6/drivers/base/class.c 2005-09-28 13:57:10.000000000 -0700
@@ -99,7 +99,8 @@
void class_put(struct class * cls)
{
- subsys_put(&cls->subsys);
+ if (cls)
+ subsys_put(&cls->subsys);
}
@@ -469,31 +470,36 @@
int class_device_add(struct class_device *class_dev)
{
- struct class * parent = NULL;
- struct class_interface * class_intf;
+ struct class *parent_class = NULL;
+ struct class_device *parent_class_dev = NULL;
+ struct class_interface *class_intf;
char *class_name = NULL;
- int error;
+ int error = -EINVAL;
class_dev = class_device_get(class_dev);
if (!class_dev)
return -EINVAL;
- if (!strlen(class_dev->class_id)) {
- error = -EINVAL;
+ if (!strlen(class_dev->class_id))
goto register_done;
- }
- parent = class_get(class_dev->class);
+ parent_class = class_get(class_dev->class);
+ if (!parent_class)
+ goto register_done;
+ parent_class_dev = class_device_get(class_dev->parent);
pr_debug("CLASS: registering class device: ID = '%s'\n",
class_dev->class_id);
/* first, register with generic layer. */
kobject_set_name(&class_dev->kobj, "%s", class_dev->class_id);
- if (parent)
- class_dev->kobj.parent = &parent->subsys.kset.kobj;
+ if (parent_class_dev)
+ class_dev->kobj.parent = &parent_class_dev->kobj;
+ else
+ class_dev->kobj.parent = &parent_class->subsys.kset.kobj;
- if ((error = kobject_add(&class_dev->kobj)))
+ error = kobject_add(&class_dev->kobj);
+ if (error)
goto register_done;
/* add the needed attributes to this device */
@@ -508,7 +514,7 @@
attr->attr.name = "dev";
attr->attr.mode = S_IRUGO;
- attr->attr.owner = parent->owner;
+ attr->attr.owner = parent_class->owner;
attr->show = show_dev;
attr->store = NULL;
class_device_create_file(class_dev, attr);
@@ -527,18 +533,20 @@
kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
/* notify any interfaces this device is now here */
- if (parent) {
- down(&parent->sem);
- list_add_tail(&class_dev->node, &parent->children);
- list_for_each_entry(class_intf, &parent->interfaces, node)
+ if (parent_class) {
+ down(&parent_class->sem);
+ list_add_tail(&class_dev->node, &parent_class->children);
+ list_for_each_entry(class_intf, &parent_class->interfaces, node)
if (class_intf->add)
class_intf->add(class_dev, class_intf);
- up(&parent->sem);
+ up(&parent_class->sem);
}
register_done:
- if (error && parent)
- class_put(parent);
+ if (error) {
+ class_put(parent_class);
+ class_device_put(parent_class_dev);
+ }
class_device_put(class_dev);
kfree(class_name);
return error;
@@ -553,21 +561,28 @@
/**
* class_device_create - creates a class device and registers it with sysfs
* @cs: pointer to the struct class that this device should be registered to.
+ * @parent: pointer to the parent struct class_device of this new device, if any.
* @dev: the dev_t for the char device to be added.
* @device: a pointer to a struct device that is assiociated with this class device.
* @fmt: string for the class device's name
*
* This function can be used by char device classes. A struct
* class_device will be created in sysfs, registered to the specified
- * class. A "dev" file will be created, showing the dev_t for the
- * device. The pointer to the struct class_device will be returned from
- * the call. Any further sysfs files that might be required can be
- * created using this pointer.
+ * class.
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct class_device is passed in, the newly
+ * created struct class_device will be a child of that device in sysfs.
+ * The pointer to the struct class_device will be returned from the
+ * call. Any further sysfs files that might be required can be created
+ * using this pointer.
*
* Note: the struct class passed to this function must have previously
* been created with a call to class_create().
*/
-struct class_device *class_device_create(struct class *cls, dev_t devt,
+struct class_device *class_device_create(struct class *cls,
+ struct class_device *parent,
+ dev_t devt,
struct device *device, char *fmt, ...)
{
va_list args;
@@ -586,6 +601,7 @@
class_dev->devt = devt;
class_dev->dev = device;
class_dev->class = cls;
+ class_dev->parent = parent;
va_start(args, fmt);
vsnprintf(class_dev->class_id, BUS_ID_SIZE, fmt, args);
@@ -603,17 +619,18 @@
void class_device_del(struct class_device *class_dev)
{
- struct class * parent = class_dev->class;
- struct class_interface * class_intf;
+ struct class *parent_class = class_dev->class;
+ struct class_device *parent_device = class_dev->parent;
+ struct class_interface *class_intf;
char *class_name = NULL;
- if (parent) {
- down(&parent->sem);
+ if (parent_class) {
+ down(&parent_class->sem);
list_del_init(&class_dev->node);
- list_for_each_entry(class_intf, &parent->interfaces, node)
+ list_for_each_entry(class_intf, &parent_class->interfaces, node)
if (class_intf->remove)
class_intf->remove(class_dev, class_intf);
- up(&parent->sem);
+ up(&parent_class->sem);
}
if (class_dev->dev) {
@@ -628,8 +645,8 @@
kobject_hotplug(&class_dev->kobj, KOBJ_REMOVE);
kobject_del(&class_dev->kobj);
- if (parent)
- class_put(parent);
+ class_device_put(parent_device);
+ class_put(parent_class);
kfree(class_name);
}
@@ -709,7 +726,8 @@
void class_device_put(struct class_device *class_dev)
{
- kobject_put(&class_dev->kobj);
+ if (class_dev)
+ kobject_put(&class_dev->kobj);
}
--- gregkh-2.6.orig/include/linux/device.h 2005-09-28 13:56:21.000000000 -0700
+++ gregkh-2.6/include/linux/device.h 2005-09-28 13:57:10.000000000 -0700
@@ -200,6 +200,7 @@
struct class_device_attribute *devt_attr;
struct device * dev; /* not necessary, but nice to have */
void * class_data; /* class-specific data */
+ struct class_device *parent; /* parent of this child device, if there is one */
char class_id[BUS_ID_SIZE]; /* unique to this class */
};
@@ -260,9 +261,12 @@
extern struct class *class_create(struct module *owner, char *name);
extern void class_destroy(struct class *cls);
-extern struct class_device *class_device_create(struct class *cls, dev_t devt,
- struct device *device, char *fmt, ...)
- __attribute__((format(printf,4,5)));
+extern struct class_device *class_device_create(struct class *cls,
+ struct class_device *parent,
+ dev_t devt,
+ struct device *device,
+ char *fmt, ...)
+ __attribute__((format(printf,5,6)));
extern void class_device_destroy(struct class *cls, dev_t devt);
On Wednesday 28 September 2005 18:31, Greg KH wrote:
> Alright, here's a patch that will add the ability to nest struct
> class_device structures in sysfs. This should give everyone the ability
> to model what they need to in the class directory (input, video, etc.).
>
> Dmitry, care to update your input patchset to take advantage of this?
> It should work out for what you want to do, and if not, please let me
> know.
>
> udev will have to be changed to properly support this, but I think that
> Kay already has that taken care of, and is just waiting for some kernel
> code to take advantage of this.
>
> Comments?
>
Hi Greg,
I still do not believe it is the solution we want:
1. It does not allow installing separate hotplug handlers for devices
and their sub-devices. This will cause hotplug handlers to resort to
having some flags or otherwise detect the king of class device hotplug
hanlder is being called for and change behavior accordingly - YUCK!
2. It does not allow user/program to scan for devices of given subclass.
I understand that you want to tech udev about all existing handlers
and having hotplug events allows to filter out unneeded names but for
other programs I do not think we want to do that. Again, consider task
of wanting to know all input interfaces, ot all available partiotions
in a system. Do not say that the program has to know that there are hdX
and sdX and ubX and adopt every time new type of device comes along
since you would need to determine whether the name you see is an
ordinary attribute or attribute group or the subdevice you are interested
in. You can't really rely on presence of 'dev' attribute since subdevice
does not have to have it. A better way would be to do
"ls /sys/class/block/partitions/" or "ls /sys/class/input/interfaces"
and get all this information.
3. It does not work well when you have an object that in your model is a
logical subdevice but does not have a parent (or has multiple parents).
Consider 'mice' multiplexor. Where would you put it? Together with
inputX? But it is not an input_dev, it should not be there.
4. subdevice should have only one parent, your implementation allows to
link subdevice to a class device and a real device at the same time,
which IMHO is wrong. Only top-level class devices should be linked with
physical /sys/devices/ device.
I firmly believe that creating sub-classes (which solves hotplug issues)
and linking sub-class devices to their parent via 'device' link, much like
we link top-level class device to their physical parent devices (which
solves 2, 3 and 4) is much cleanier way. It provides everything that your
implementation does plus allows different views useful for other tasks
besides udev.
--
Dmitry
On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
> On Wednesday 28 September 2005 18:31, Greg KH wrote:
> > Alright, here's a patch that will add the ability to nest struct
> > class_device structures in sysfs. This should give everyone the ability
> > to model what they need to in the class directory (input, video, etc.).
>
> I still do not believe it is the solution we want:
>
> 1. It does not allow installing separate hotplug handlers for devices
> and their sub-devices. This will cause hotplug handlers to resort to
> having some flags or otherwise detect the king of class device hotplug
> hanlder is being called for and change behavior accordingly - YUCK!
Huh? I don't understand your complaint here. Why would we ever want to
have separate hotplug handlers for the same class? If you do want that,
then create separate classes.
> 2. It does not allow user/program to scan for devices of given subclass.
There is nothing called "subclass" anymore, so sure, you can't scan for
it :)
> I understand that you want to tech udev about all existing handlers
> and having hotplug events allows to filter out unneeded names but for
> other programs I do not think we want to do that. Again, consider task
> of wanting to know all input interfaces, ot all available partiotions
> in a system. Do not say that the program has to know that there are hdX
> and sdX and ubX and adopt every time new type of device comes along
> since you would need to determine whether the name you see is an
> ordinary attribute or attribute group or the subdevice you are interested
> in. You can't really rely on presence of 'dev' attribute since subdevice
> does not have to have it. A better way would be to do
> "ls /sys/class/block/partitions/" or "ls /sys/class/input/interfaces"
> and get all this information.
Yes, class devices can have a "dev" file, and be nested below another
class device, that's fine. It's only a simple scan of sysfs to find
them, just like we do today for the block devices. Again, I really
don't see the problem here.
> 3. It does not work well when you have an object that in your model is a
> logical subdevice but does not have a parent (or has multiple parents).
> Consider 'mice' multiplexor. Where would you put it? Together with
> inputX? But it is not an input_dev, it should not be there.
Ok, the "mice" thing is a hack. A big hack, but one that is needed.
Don't try to bring that one in... Anyway, I can handle that one too,
see below.
> 4. subdevice should have only one parent, your implementation allows to
> link subdevice to a class device and a real device at the same time,
> which IMHO is wrong. Only top-level class devices should be linked with
> physical /sys/devices/ device.
Why? Why arbirtrary make that distinction? See my example below for an
example of that symlink being in both class devices, and everything is
just fine.
> I firmly believe that creating sub-classes (which solves hotplug issues)
> and linking sub-class devices to their parent via 'device' link, much like
> we link top-level class device to their physical parent devices (which
> solves 2, 3 and 4) is much cleanier way. It provides everything that your
> implementation does plus allows different views useful for other tasks
> besides udev.
I thought this way too. Until I started to implement the sub-class
device code, and realized that I was just creating the same thing as the
existing class_device code.
And yes, I realize that this is different from nesting classes, but I
_really_ do not think that is the proper solution for this at all.
So, here's a small patch below, on top of the other input patches from
you that I've applied, that moves the mouse devices into the input_dev
class tree. This works just fine on my box, and is what I think we want
to do. After this, I'll just convert over the other input driver types,
and then rename the input_dev class to input, and we should be set.
Then you can still do your "convert input drivers to be class
interfaces" as I agree that this is a good thing to do too.
Here's what sysfs looks like on this box with the patch below:
$ tree /sys/class/input_dev/ -d
/sys/class/input_dev/
|-- input0
| |-- capabilities
| `-- id
|-- input1
| |-- capabilities
| |-- device -> ../../../devices/platform/i8042/serio1
| `-- id
`-- input2
|-- capabilities
|-- device -> ../../../devices/pci0000:00/0000:00:1d.0/usb1/1-2/1-2:1.0
|-- id
`-- mouse0
`-- device -> ../../../../devices/pci0000:00/0000:00:1d.0/usb1/1-2/1-2:1.0
All of the files are in there too, I just did not show them as it
doesn't really matter (there's a "dev" file in
/sys/class/input_dev/input2/mouse0 like expected).
And yes, udev works just fine, as long as udevd is up and running before
you load the mouse drivers, udevstart is what is needed to be fixed up
to handle this properly (and Kay has some patches for that already.)
thanks,
greg k-h
Subject: Input: start nesting the mouse drivers in the proper place in sysfs
This puts the mouse drivers under their proper inputX class_device in
sysfs.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/input/input.c | 8 ++++----
drivers/input/mousedev.c | 4 ++--
include/linux/input.h | 1 +
3 files changed, 7 insertions(+), 6 deletions(-)
--- gregkh-2.6.orig/drivers/input/input.c
+++ gregkh-2.6/drivers/input/input.c
@@ -724,7 +724,7 @@ static void input_dev_release(struct cla
module_put(THIS_MODULE);
}
-static struct class input_dev_class = {
+struct class input_dev_class = {
.name = "input_dev",
.release = input_dev_release,
.class_dev_attrs = input_dev_attrs,
@@ -795,6 +795,9 @@ void input_register_device(struct input_
INIT_LIST_HEAD(&dev->h_list);
list_add_tail(&dev->node, &input_dev_list);
+ if (dev->dynalloc)
+ input_register_classdevice(dev);
+
list_for_each_entry(handler, &input_handler_list, node)
if (!handler->blacklist || !input_match_device(handler->blacklist, dev))
if ((id = input_match_device(handler->id_table, dev)))
@@ -802,9 +805,6 @@ void input_register_device(struct input_
input_link_handle(handle);
- if (dev->dynalloc)
- input_register_classdevice(dev);
-
#ifdef CONFIG_HOTPLUG
input_call_hotplug("add", dev);
#endif
--- gregkh-2.6.orig/drivers/input/mousedev.c
+++ gregkh-2.6/drivers/input/mousedev.c
@@ -648,9 +648,9 @@ static struct input_handle *mousedev_con
mousedev_table[minor] = mousedev;
- class_device_create(input_class, NULL,
+ class_device_create(&input_dev_class, &dev->cdev,
MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + minor),
- dev->dev, "mouse%d", minor);
+ dev->cdev.dev, "mouse%d", minor);
return &mousedev->handle;
}
--- gregkh-2.6.orig/include/linux/input.h
+++ gregkh-2.6/include/linux/input.h
@@ -1075,6 +1075,7 @@ static inline void input_set_abs_params(
}
extern struct class *input_class;
+extern struct class input_dev_class;
#endif
#endif
On Wed, Oct 05, 2005 at 05:09:51PM -0700, Greg KH wrote:
> - class_device_create(input_class, NULL,
> + class_device_create(&input_dev_class, &dev->cdev,
> MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + minor),
> - dev->dev, "mouse%d", minor);
> + dev->cdev.dev, "mouse%d", minor);
Yeah, I know mixing this call with the input_dev_class is a ripe for
badness, I'll fix it up properly soon, this was just a "proof of
concept".
thanks,
greg k-h
On Wed, Oct 05, 2005 at 05:09:51PM -0700, Greg KH wrote:
> Here's what sysfs looks like on this box with the patch below:
>
> $ tree /sys/class/input_dev/ -d
> /sys/class/input_dev/
> |-- input0
> | |-- capabilities
> | `-- id
> |-- input1
> | |-- capabilities
> | |-- device -> ../../../devices/platform/i8042/serio1
> | `-- id
> `-- input2
> |-- capabilities
> |-- device -> ../../../devices/pci0000:00/0000:00:1d.0/usb1/1-2/1-2:1.0
> |-- id
> `-- mouse0
> `-- device -> ../../../../devices/pci0000:00/0000:00:1d.0/usb1/1-2/1-2:1.0
And here it is with all input devices converted over. Just rename
input_dev to "input" and we are home free...
$ tree -d /sys/class/input_dev/
/sys/class/input_dev/
|-- input0
| |-- capabilities
| |-- event0
| `-- id
|-- input1
| |-- capabilities
| |-- device -> ../../../devices/platform/i8042/serio1
| |-- event1
| | `-- device -> ../../../../devices/platform/i8042/serio1
| `-- id
|-- input2
| |-- capabilities
| |-- device -> ../../../devices/pci0000:00/0000:00:1d.0/usb2/2-2/2-2:1.0
| |-- event2
| | `-- device -> ../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-2/2-2:1.0
| |-- id
| |-- mouse0
| | `-- device -> ../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-2/2-2:1.0
| `-- ts0
| `-- device -> ../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-2/2-2:1.0
`-- mice
thanks,
greg k-h
On Wednesday 05 October 2005 19:09, Greg KH wrote:
> On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
> > On Wednesday 28 September 2005 18:31, Greg KH wrote:
> > > Alright, here's a patch that will add the ability to nest struct
> > > class_device structures in sysfs. This should give everyone the ability
> > > to model what they need to in the class directory (input, video, etc.).
> >
> > I still do not believe it is the solution we want:
> >
> > 1. It does not allow installing separate hotplug handlers for devices
> > and their sub-devices. This will cause hotplug handlers to resort to
> > having some flags or otherwise detect the king of class device hotplug
> > hanlder is being called for and change behavior accordingly - YUCK!
>
> Huh? I don't understand your complaint here. Why would we ever want to
> have separate hotplug handlers for the same class? If you do want that,
> then create separate classes.
>
Yes. I do want separate [sub]classes. I just want them grouped together
under some <subsystem> name. And I want separate hotplug handlers because
actions that are needed for these objects are different. When a new
input_dev appears you want to match its capabilities with one or more
input handlers and load appropriate handler module if it has not been
loaded yet. When a new input interface device appears you want to create
a new device node for it. The handlers should be diffrent if you want
clean implementation, do you see?
> > 2. It does not allow user/program to scan for devices of given subclass.
>
> There is nothing called "subclass" anymore, so sure, you can't scan for
> it :)
>
I am pointing out what I think are deficiences in your design. I do not
think saying "We won't have it at all so problem does not exists" is
allowed. I do want to be able to easily get/enumerate devices of a logical
subclass whether they are represented this way in sysfs or not.
> > I understand that you want to tech udev about all existing handlers
> > and having hotplug events allows to filter out unneeded names but for
> > other programs I do not think we want to do that. Again, consider task
> > of wanting to know all input interfaces, ot all available partiotions
> > in a system. Do not say that the program has to know that there are hdX
> > and sdX and ubX and adopt every time new type of device comes along
> > since you would need to determine whether the name you see is an
> > ordinary attribute or attribute group or the subdevice you are interested
> > in. You can't really rely on presence of 'dev' attribute since subdevice
> > does not have to have it. A better way would be to do
> > "ls /sys/class/block/partitions/" or "ls /sys/class/input/interfaces"
> > and get all this information.
>
> Yes, class devices can have a "dev" file, and be nested below another
> class device, that's fine. It's only a simple scan of sysfs to find
> them, just like we do today for the block devices. Again, I really
> don't see the problem here.
>
Ok, so how do you scan for all instances of input interfaces under your
proposal of you don't know their names? Remember you need to support
special instances that do not have parent devices too.
> > 3. It does not work well when you have an object that in your model is a
> > logical subdevice but does not have a parent (or has multiple parents).
> > Consider 'mice' multiplexor. Where would you put it? Together with
> > inputX? But it is not an input_dev, it should not be there.
>
> Ok, the "mice" thing is a hack. A big hack, but one that is needed.
> Don't try to bring that one in... Anyway, I can handle that one too,
> see below.
>
Still a hck as far as I can see ;(
> > 4. subdevice should have only one parent, your implementation allows to
> > link subdevice to a class device and a real device at the same time,
> > which IMHO is wrong. Only top-level class devices should be linked with
> > physical /sys/devices/ device.
>
> Why? Why arbirtrary make that distinction? See my example below for an
> example of that symlink being in both class devices, and everything is
> just fine.
>
Because at the interface level all I care is what inputX device my mouse0
is connected to and what capabilities it has. When I talk about mouse0
I do not care that input_dev is actually connected to
/sys/bus/serio/devices/serio1. If I want this info I will just travel
dev->device->device->device chain and get this data. What you doing is just
cluttering /sys with unneeded data.
> > I firmly believe that creating sub-classes (which solves hotplug issues)
> > and linking sub-class devices to their parent via 'device' link, much like
> > we link top-level class device to their physical parent devices (which
> > solves 2, 3 and 4) is much cleanier way. It provides everything that your
> > implementation does plus allows different views useful for other tasks
> > besides udev.
>
> I thought this way too. Until I started to implement the sub-class
> device code, and realized that I was just creating the same thing as the
> existing class_device code.
>
See the patch below - it simply allows you to link class device to any
valid kobject. No code duplication and together with bested classes
it works nicely:
[dtor@core ~]$ tree -d /sys/class/input/devices/input2
/sys/class/input/devices/input2
|-- capabilities
|-- device -> ../../../../devices/platform/i8042/serio0/serio2
|-- id
|-- interfaces:event2 -> ../../../../class/input/interfaces/event2
|-- interfaces:mouse1 -> ../../../../class/input/interfaces/mouse1
`-- interfaces:ts1 -> ../../../../class/input/interfaces/ts1
> And yes, I realize that this is different from nesting classes, but I
> _really_ do not think that is the proper solution for this at all.
>
Arguments please. I think I explained why I do not like your proposal but
I have not heard why you think nesting classes is a bad idea.
--
Dmitry
Driver core: allow linking class devices to any kobject
This will allow linking class devices not only to physical
devices but also to other class devices.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/base/class.c | 31 +++++++++++++++++--------------
include/linux/device.h | 3 ++-
2 files changed, 19 insertions(+), 15 deletions(-)
Index: work/drivers/base/class.c
===================================================================
--- work.orig/drivers/base/class.c
+++ work/drivers/base/class.c
@@ -485,9 +485,9 @@ static char *make_class_name(struct clas
int class_device_add(struct class_device *class_dev)
{
- struct class * parent = NULL;
- struct class_interface * class_intf;
- char *class_name = NULL;
+ struct class *parent = NULL;
+ struct class_interface *class_intf;
+ char *class_name;
int error;
class_dev = class_device_get(class_dev);
@@ -532,12 +532,16 @@ int class_device_add(struct class_device
}
class_device_add_attrs(class_dev);
- if (class_dev->dev) {
+
+ if (class_dev->dev)
+ class_dev->parent_dev = &class_dev->dev->kobj;
+ if (class_dev->parent_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_dev->parent_dev, "device");
+ sysfs_create_link(class_dev->parent_dev, &class_dev->kobj,
class_name);
+ kfree(class_name);
}
kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
@@ -556,7 +560,6 @@ int class_device_add(struct class_device
if (error && parent)
class_put(parent);
class_device_put(class_dev);
- kfree(class_name);
return error;
}
@@ -621,7 +624,7 @@ void class_device_del(struct class_devic
{
struct class * parent = class_dev->class;
struct class_interface * class_intf;
- char *class_name = NULL;
+ char *class_name;
if (parent) {
down(&parent->sem);
@@ -635,7 +638,8 @@ void class_device_del(struct class_devic
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);
+ sysfs_remove_link(class_dev->parent_dev, class_name);
+ kfree(class_name);
}
if (class_dev->devt_attr)
class_device_remove_file(class_dev, class_dev->devt_attr);
@@ -646,7 +650,6 @@ void class_device_del(struct class_devic
if (parent)
class_put(parent);
- kfree(class_name);
}
void class_device_unregister(struct class_device *class_dev)
@@ -695,18 +698,18 @@ int class_device_rename(struct class_dev
pr_debug("CLASS: renaming '%s' to '%s'\n", class_dev->class_id,
new_name);
- if (class_dev->dev)
+ if (class_dev->parent_dev)
old_class_name = make_class_name(class_dev);
strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);
error = kobject_rename(&class_dev->kobj, new_name);
- if (class_dev->dev) {
+ if (class_dev->parent_dev) {
new_class_name = make_class_name(class_dev);
- sysfs_create_link(&class_dev->dev->kobj, &class_dev->kobj,
+ sysfs_create_link(class_dev->parent_dev, &class_dev->kobj,
new_class_name);
- sysfs_remove_link(&class_dev->dev->kobj, old_class_name);
+ sysfs_remove_link(class_dev->parent_dev, old_class_name);
}
class_device_put(class_dev);
Index: work/include/linux/device.h
===================================================================
--- work.orig/include/linux/device.h
+++ work/include/linux/device.h
@@ -199,7 +199,8 @@ struct class_device {
struct class * class; /* required */
dev_t devt; /* dev_t, creates the sysfs "dev" */
struct class_device_attribute *devt_attr;
- struct device * dev; /* not necessary, but nice to have */
+ struct kobject * parent_dev; /* parent [class_]device */
+ struct device * dev; /* going away */
void * class_data; /* class-specific data */
char class_id[BUS_ID_SIZE]; /* unique to this class */
Dmitry Torokhov wrote:
> On Wednesday 05 October 2005 19:09, Greg KH wrote:
>
>>On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
>>
>>>On Wednesday 28 September 2005 18:31, Greg KH wrote:
>>>
>>>>Alright, here's a patch that will add the ability to nest struct
>>>>class_device structures in sysfs. This should give everyone the ability
>>>>to model what they need to in the class directory (input, video, etc.).
>>>
>>>I still do not believe it is the solution we want:
>>>
>>>1. It does not allow installing separate hotplug handlers for devices
>>> and their sub-devices. This will cause hotplug handlers to resort to
>>> having some flags or otherwise detect the king of class device hotplug
>>> hanlder is being called for and change behavior accordingly - YUCK!
>>
>>Huh? I don't understand your complaint here. Why would we ever want to
>>have separate hotplug handlers for the same class? If you do want that,
>>then create separate classes.
>>
>
>
> Yes. I do want separate [sub]classes. I just want them grouped together
> under some <subsystem> name. And I want separate hotplug handlers because
> actions that are needed for these objects are different. When a new
> input_dev appears you want to match its capabilities with one or more
> input handlers and load appropriate handler module if it has not been
> loaded yet. When a new input interface device appears you want to create
> a new device node for it. The handlers should be diffrent if you want
> clean implementation, do you see?
How about using current classes, but name them to have common prefix,
eg input_dev, input_interface etc class names - this way, if a program
wants to enumerare all input <whatever>, it enumerates /sys/class,
finds all directories matching input*, and looks inside...
Maybe not that elegant, but may work.
/mjt
On 10/6/05, Michael Tokarev <[email protected]> wrote:
> Dmitry Torokhov wrote:
> > On Wednesday 05 October 2005 19:09, Greg KH wrote:
> >
> >>On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
> >>
> >>>On Wednesday 28 September 2005 18:31, Greg KH wrote:
> >>>
> >>>>Alright, here's a patch that will add the ability to nest struct
> >>>>class_device structures in sysfs. This should give everyone the ability
> >>>>to model what they need to in the class directory (input, video, etc.).
> >>>
> >>>I still do not believe it is the solution we want:
> >>>
> >>>1. It does not allow installing separate hotplug handlers for devices
> >>> and their sub-devices. This will cause hotplug handlers to resort to
> >>> having some flags or otherwise detect the king of class device hotplug
> >>> hanlder is being called for and change behavior accordingly - YUCK!
> >>
> >>Huh? I don't understand your complaint here. Why would we ever want to
> >>have separate hotplug handlers for the same class? If you do want that,
> >>then create separate classes.
> >>
> >
> >
> > Yes. I do want separate [sub]classes. I just want them grouped together
> > under some <subsystem> name. And I want separate hotplug handlers because
> > actions that are needed for these objects are different. When a new
> > input_dev appears you want to match its capabilities with one or more
> > input handlers and load appropriate handler module if it has not been
> > loaded yet. When a new input interface device appears you want to create
> > a new device node for it. The handlers should be diffrent if you want
> > clean implementation, do you see?
>
> How about using current classes, but name them to have common prefix,
> eg input_dev, input_interface etc class names - this way, if a program
> wants to enumerare all input <whatever>, it enumerates /sys/class,
> finds all directories matching input*, and looks inside...
>
> Maybe not that elegant, but may work.
>
We have it right now (see /sys/class/{ieee1394_*|scsi-*|usb_*}) and it
pretty much sucks IMHO. I would like to move from a flat class model
to a more tree-like structure.
--
Dmitry
On Thu, Oct 06, 2005 at 01:29:27AM -0500, Dmitry Torokhov wrote:
> On Wednesday 05 October 2005 19:09, Greg KH wrote:
> > On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
> > > On Wednesday 28 September 2005 18:31, Greg KH wrote:
> > > > Alright, here's a patch that will add the ability to nest struct
> > > > class_device structures in sysfs. This should give everyone the ability
> > > > to model what they need to in the class directory (input, video, etc.).
> > >
> > > I still do not believe it is the solution we want:
> > >
> > > 1. It does not allow installing separate hotplug handlers for devices
> > > and their sub-devices. This will cause hotplug handlers to resort to
> > > having some flags or otherwise detect the king of class device hotplug
> > > hanlder is being called for and change behavior accordingly - YUCK!
> >
> > Huh? I don't understand your complaint here. Why would we ever want to
> > have separate hotplug handlers for the same class? If you do want that,
> > then create separate classes.
> >
>
> Yes. I do want separate [sub]classes. I just want them grouped together
> under some <subsystem> name. And I want separate hotplug handlers because
> actions that are needed for these objects are different. When a new
> input_dev appears you want to match its capabilities with one or more
> input handlers and load appropriate handler module if it has not been
> loaded yet. When a new input interface device appears you want to create
> a new device node for it. The handlers should be diffrent if you want
> clean implementation, do you see?
Yes, now I understand. You also need different release() functions,
which my current patch can not handle without some nasty hacks (for both
the hotplug and release stuff.)
Ugh, back to the drawing board...
> > > 2. It does not allow user/program to scan for devices of given subclass.
> >
> > There is nothing called "subclass" anymore, so sure, you can't scan for
> > it :)
> >
>
> I am pointing out what I think are deficiences in your design. I do not
> think saying "We won't have it at all so problem does not exists" is
> allowed. I do want to be able to easily get/enumerate devices of a logical
> subclass whether they are represented this way in sysfs or not.
Yes, sorry. I now agree with you.
> > > I understand that you want to tech udev about all existing handlers
> > > and having hotplug events allows to filter out unneeded names but for
> > > other programs I do not think we want to do that. Again, consider task
> > > of wanting to know all input interfaces, ot all available partiotions
> > > in a system. Do not say that the program has to know that there are hdX
> > > and sdX and ubX and adopt every time new type of device comes along
> > > since you would need to determine whether the name you see is an
> > > ordinary attribute or attribute group or the subdevice you are interested
> > > in. You can't really rely on presence of 'dev' attribute since subdevice
> > > does not have to have it. A better way would be to do
> > > "ls /sys/class/block/partitions/" or "ls /sys/class/input/interfaces"
> > > and get all this information.
> >
> > Yes, class devices can have a "dev" file, and be nested below another
> > class device, that's fine. It's only a simple scan of sysfs to find
> > them, just like we do today for the block devices. Again, I really
> > don't see the problem here.
> >
>
> Ok, so how do you scan for all instances of input interfaces under your
> proposal of you don't know their names? Remember you need to support
> special instances that do not have parent devices too.
I don't understand the problem here. With my directory structure (which
is what I want to see everything looking like in the end anyway, and is
what Vojtech and Kay also agreed with when I talked with them last
week), how is it hard to scan for this?
> > > 3. It does not work well when you have an object that in your model is a
> > > logical subdevice but does not have a parent (or has multiple parents).
> > > Consider 'mice' multiplexor. Where would you put it? Together with
> > > inputX? But it is not an input_dev, it should not be there.
> >
> > Ok, the "mice" thing is a hack. A big hack, but one that is needed.
> > Don't try to bring that one in... Anyway, I can handle that one too,
> > see below.
> >
>
> Still a hck as far as I can see ;(
Yeah, but we have to keep this hack, due to backward compatibility and
nasty X servers which can not handle mice disappearing and showing up
dynamically.
> > > 4. subdevice should have only one parent, your implementation allows to
> > > link subdevice to a class device and a real device at the same time,
> > > which IMHO is wrong. Only top-level class devices should be linked with
> > > physical /sys/devices/ device.
> >
> > Why? Why arbirtrary make that distinction? See my example below for an
> > example of that symlink being in both class devices, and everything is
> > just fine.
> >
>
> Because at the interface level all I care is what inputX device my mouse0
> is connected to and what capabilities it has. When I talk about mouse0
> I do not care that input_dev is actually connected to
> /sys/bus/serio/devices/serio1. If I want this info I will just travel
> dev->device->device->device chain and get this data. What you doing is just
> cluttering /sys with unneeded data.
Heh, it's only one extra symlink, which makes it much easier to find the
real device for the specific level in the class heirachy. It also makes
it easier for libsysfs and udev to work this way, otherwise we have to
start special casing that device symlink, which will get very messy.
> > > I firmly believe that creating sub-classes (which solves hotplug issues)
> > > and linking sub-class devices to their parent via 'device' link, much like
> > > we link top-level class device to their physical parent devices (which
> > > solves 2, 3 and 4) is much cleanier way. It provides everything that your
> > > implementation does plus allows different views useful for other tasks
> > > besides udev.
> >
> > I thought this way too. Until I started to implement the sub-class
> > device code, and realized that I was just creating the same thing as the
> > existing class_device code.
> >
>
> See the patch below - it simply allows you to link class device to any
> valid kobject. No code duplication and together with bested classes
> it works nicely:
>
> [dtor@core ~]$ tree -d /sys/class/input/devices/input2
> /sys/class/input/devices/input2
> |-- capabilities
> |-- device -> ../../../../devices/platform/i8042/serio0/serio2
> |-- id
> |-- interfaces:event2 -> ../../../../class/input/interfaces/event2
> |-- interfaces:mouse1 -> ../../../../class/input/interfaces/mouse1
> `-- interfaces:ts1 -> ../../../../class/input/interfaces/ts1
But this puts 2 extra levels for classes, "devices" and "interfaces".
I don't think that is necessary.
> > And yes, I realize that this is different from nesting classes, but I
> > _really_ do not think that is the proper solution for this at all.
> >
>
> Arguments please. I think I explained why I do not like your proposal but
> I have not heard why you think nesting classes is a bad idea.
Ok, sorry, I thought I said that before. I don't like nexting "struct
class" because they should remain separate. If you need different
classes for things, create different classes. Nesting them is only
solving the "/sys/class/scsi-*" namespace "issue" (and I don't really
feel that is a real issue).
Also, nesting classes does not let us do things like what is needed to
move /sys/block/ under /sys/class. I don't want to create something
that will only work right now for input, and then have to go off and
create something else for block devices (and possibly bluetooth and
video.)
> Driver core: allow linking class devices to any kobject
>
> This will allow linking class devices not only to physical
> devices but also to other class devices.
You don't need this patch, you can create symlinks like this on your own
if you wish today to any kobject. No, we need to keep that struct
device pointer in there, as "device" _must_ point to that, and not
something else, otherwise we _really_ break existing userspace programs.
Ok, let me think about the release() and hotplug() issues some more and
get back to you...
thanks,
greg k-h
On 10/6/05, Greg KH <[email protected]> wrote:
> On Thu, Oct 06, 2005 at 01:29:27AM -0500, Dmitry Torokhov wrote:
> > On Wednesday 05 October 2005 19:09, Greg KH wrote:
> > > On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
> > > > I understand that you want to tech udev about all existing handlers
> > > > and having hotplug events allows to filter out unneeded names but for
> > > > other programs I do not think we want to do that. Again, consider task
> > > > of wanting to know all input interfaces, ot all available partiotions
> > > > in a system. Do not say that the program has to know that there are hdX
> > > > and sdX and ubX and adopt every time new type of device comes along
> > > > since you would need to determine whether the name you see is an
> > > > ordinary attribute or attribute group or the subdevice you are interested
> > > > in. You can't really rely on presence of 'dev' attribute since subdevice
> > > > does not have to have it. A better way would be to do
> > > > "ls /sys/class/block/partitions/" or "ls /sys/class/input/interfaces"
> > > > and get all this information.
> > >
> > > Yes, class devices can have a "dev" file, and be nested below another
> > > class device, that's fine. It's only a simple scan of sysfs to find
> > > them, just like we do today for the block devices. Again, I really
> > > don't see the problem here.
> > >
> >
> > Ok, so how do you scan for all instances of input interfaces under your
> > proposal of you don't know their names? Remember you need to support
> > special instances that do not have parent devices too.
>
> I don't understand the problem here. With my directory structure (which
> is what I want to see everything looking like in the end anyway, and is
> what Vojtech and Kay also agreed with when I talked with them last
> week), how is it hard to scan for this?
>
Because (please correct me if I am wrong) with your proposal you have
to know all possible names of your interfaces/block devices or use
some other tricks to identify them. The knowledge/tricks may work for
udev but I do not think it is maintainable and will require
maintaining close sync between udev and kernel.
To illustrate the above: imagine you have
/sys/class/input/input0/caps0 and /sys/class/input/input0/event0. How
do you know that 'caps0' is just some attribute group but 'event0' is
really an interface device? With my proposal you only need to know
name of the [sub]class to be able to enumerate all input interfaces.
If tomorrow I come out wuth 'blahdev' input handler you most likely
will not need to change anything in udev or its scripts/rules, you
will jsut get hotplug event for 'input' sybsystem,
'/sys/class/input/interfaces' class and 'blahX' device. And that's all
you really care.
> > > > 4. subdevice should have only one parent, your implementation allows to
> > > > link subdevice to a class device and a real device at the same time,
> > > > which IMHO is wrong. Only top-level class devices should be linked with
> > > > physical /sys/devices/ device.
> > >
> > > Why? Why arbirtrary make that distinction? See my example below for an
> > > example of that symlink being in both class devices, and everything is
> > > just fine.
> > >
> >
> > Because at the interface level all I care is what inputX device my mouse0
> > is connected to and what capabilities it has. When I talk about mouse0
> > I do not care that input_dev is actually connected to
> > /sys/bus/serio/devices/serio1. If I want this info I will just travel
> > dev->device->device->device chain and get this data. What you doing is just
> > cluttering /sys with unneeded data.
>
> Heh, it's only one extra symlink, which makes it much easier to find the
> real device for the specific level in the class heirachy. It also makes
> it easier for libsysfs and udev to work this way, otherwise we have to
> start special casing that device symlink, which will get very messy.
This is a layering violation as well. mouse0 does not know anything
about serioX. It does not work with serio abstraction, it works with
input_dev abstraction and therefore it's device should point to
corresponding inputX.
Why does udev care what physical device mouseX is connected to,
especially given that serioX numbers are not stable? If you are using
it for identification purposes you are much better off using 'phys'
attribute of corresponding inputX object.
>
> > > > I firmly believe that creating sub-classes (which solves hotplug issues)
> > > > and linking sub-class devices to their parent via 'device' link, much like
> > > > we link top-level class device to their physical parent devices (which
> > > > solves 2, 3 and 4) is much cleanier way. It provides everything that your
> > > > implementation does plus allows different views useful for other tasks
> > > > besides udev.
> > >
> > > I thought this way too. Until I started to implement the sub-class
> > > device code, and realized that I was just creating the same thing as the
> > > existing class_device code.
> > >
> >
> > See the patch below - it simply allows you to link class device to any
> > valid kobject. No code duplication and together with bested classes
> > it works nicely:
> >
> > [dtor@core ~]$ tree -d /sys/class/input/devices/input2
> > /sys/class/input/devices/input2
> > |-- capabilities
> > |-- device -> ../../../../devices/platform/i8042/serio0/serio2
> > |-- id
> > |-- interfaces:event2 -> ../../../../class/input/interfaces/event2
> > |-- interfaces:mouse1 -> ../../../../class/input/interfaces/mouse1
> > `-- interfaces:ts1 -> ../../../../class/input/interfaces/ts1
>
> But this puts 2 extra levels for classes, "devices" and "interfaces".
> I don't think that is necessary.
>
This is what allows you to distinguish between input devices and input
interfaces. I agree it is not necessary if you want to keep flat
/sys/class structure and have /sys/class/input and
/sys/class/input_dev. But I am afraid this will eventually turn
/sys/class into a bug dumpster, similar to /proc. I would much rather
have it look like /sys/devices/... hierarchies.
> > > And yes, I realize that this is different from nesting classes, but I
> > > _really_ do not think that is the proper solution for this at all.
> > >
> >
> > Arguments please. I think I explained why I do not like your proposal but
> > I have not heard why you think nesting classes is a bad idea.
>
> Ok, sorry, I thought I said that before. I don't like nexting "struct
> class" because they should remain separate.
I am still missing ".. they should remain separate because 1)... 2).. 3)..."
> If you need different
> classes for things, create different classes. Nesting them is only
> solving the "/sys/class/scsi-*" namespace "issue" (and I don't really
> feel that is a real issue).
>
> Also, nesting classes does not let us do things like what is needed to
> move /sys/block/ under /sys/class. I don't want to create something
> that will only work right now for input, and then have to go off and
> create something else for block devices (and possibly bluetooth and
> video.)
Could you please elaborate on this one? I think my solution could be
useful with /sys/class/block/devices and
/sys/class/block/partitions...
>
> > Driver core: allow linking class devices to any kobject
> >
> > This will allow linking class devices not only to physical
> > devices but also to other class devices.
>
> You don't need this patch, you can create symlinks like this on your own
> if you wish today to any kobject. No, we need to keep that struct
> device pointer in there, as "device" _must_ point to that, and not
> something else, otherwise we _really_ break existing userspace programs.
>
It is not "needed" needed, it is just for convenience, so if you want
to link 2 class devices driver core can do that for you too.
--
Dmitry
On Fri, Sep 30, 2005 at 12:32:49AM -0500, Dmitry Torokhov wrote:
>
> I firmly believe that creating sub-classes (which solves hotplug issues)
> and linking sub-class devices to their parent via 'device' link, much like
> we link top-level class device to their physical parent devices (which
> solves 2, 3 and 4) is much cleanier way. It provides everything that your
> implementation does plus allows different views useful for other tasks
> besides udev.
Hi Dmitry,
Could you please outline the class device requirements for the input
subsystem? Specifically I'm interested in the class -> subclass layering.
I'm trying to understand what is needed and how the input subsystem should
be structured.
I can think of other subystems that might benefit from class layering. One
example might be wireless cards: net and net/wireless. In this case devices
that belong to class "wireless" would inherit all of the capabilities of the
"net" class and also implement the "wireless"-specific feature set.
Thanks,
Adam