[RFC][PATCH 2.6.13-rc1] driver core: subclasses

[RFC][PATCH 2.6.13-rc1] driver core: subclasses

[Matt Domsch]

Greg,

The below patch is a first pass at implementing subclasses, for review
and comment.

As a test, I modified drivers/input/input.c to allocate a new class,
and register it as a subclass.

Before:

/sys/class/input/
/sys/class/input_device/

After:
/sys/class/input/input_device/

In this pass I'm not exporting the subclass_register/unregister
functions, but am expecting callers to use the new
class_subclass_create() call, and older callers to continue using
class_create() which is now static inline.

Callers to class_create() are expected to call class_destroy() on the
subclasses before the parent class.

Feedback appreciated.

Signed-off-by: Matt Domsch <Matt_Domsch@dell.com>

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

--- linux-2.6/drivers/base/class.c	2005-07-06 14:19:37.000000000 -0400
+++ linux-2.6/drivers/base/class.c	2005-07-08 18:41:19.000000000 -0400
@@ -133,6 +133,26 @@ static void remove_class_attrs(struct cl
 	}
 }
 
+static int class_subclass_register(struct class *subclass)
+{
+	struct class *parent;
+
+	if (!subclass)
+		return -ENODEV;
+
+	parent = class_get(subclass->parent);
+	if (!parent)
+		return -EINVAL;
+
+	subclass->subsys.kset.kobj.parent = &parent->subsys.kset.kobj;
+
+	down(&parent->sem);
+	list_add_tail(&parent->subclasses, &subclass->subclass_node);
+	up(&parent->sem);
+
+	return 0;
+}
+
 int class_register(struct class * cls)
 {
 	int error;
@@ -141,6 +161,8 @@ int class_register(struct class * cls)
 
 	INIT_LIST_HEAD(&cls->children);
 	INIT_LIST_HEAD(&cls->interfaces);
+	INIT_LIST_HEAD(&cls->subclasses);
+	INIT_LIST_HEAD(&cls->subclass_node);
 	init_MUTEX(&cls->sem);
 	error = kobject_set_name(&cls->subsys.kset.kobj, "%s", cls->name);
 	if (error)
@@ -148,6 +170,7 @@ int class_register(struct class * cls)
 
 	subsys_set_kset(cls, class_subsys);
 
+	class_subclass_register(cls);
 	error = subsystem_register(&cls->subsys);
 	if (!error) {
 		error = add_class_attrs(class_get(cls));
@@ -156,10 +179,25 @@ int class_register(struct class * cls)
 	return error;
 }
 
+static void class_subclass_unregister(struct class *subclass)
+{
+	struct class *parent = subclass->parent;
+
+	if (!parent)
+		return;
+
+	down(&parent->sem);
+	list_del_init(&subclass->subclass_node);
+	subclass->parent = NULL;
+	up(&parent->sem);
+	class_put(parent);
+}
+
 void class_unregister(struct class * cls)
 {
 	pr_debug("device class '%s': unregistering\n", cls->name);
 	remove_class_attrs(cls);
+	class_subclass_unregister(cls);
 	subsystem_unregister(&cls->subsys);
 }
 
@@ -184,7 +222,7 @@ static void class_device_create_release(
  * Note, the pointer created here is to be destroyed when finished by
  * making a call to class_destroy().
  */
-struct class *class_create(struct module *owner, char *name)
+struct class *class_subclass_create(struct module *owner, char *name, struct class *parent)
 {
 	struct class *cls;
 	int retval;
@@ -200,6 +238,7 @@ struct class *class_create(struct module
 	cls->owner = owner;
 	cls->class_release = class_create_release;
 	cls->release = class_device_create_release;
+	cls->parent = parent;
 
 	retval = class_register(cls);
 	if (retval)
@@ -738,8 +777,8 @@ EXPORT_SYMBOL_GPL(class_register);
 EXPORT_SYMBOL_GPL(class_unregister);
 EXPORT_SYMBOL_GPL(class_get);
 EXPORT_SYMBOL_GPL(class_put);
-EXPORT_SYMBOL_GPL(class_create);
+EXPORT_SYMBOL_GPL(class_subclass_create);
 EXPORT_SYMBOL_GPL(class_destroy);
 
 EXPORT_SYMBOL_GPL(class_device_register);
--- linux-2.6/include/linux/device.h	2005-07-06 14:21:20.000000000 -0400
+++ linux-2.6-input/include/linux/device.h	2005-07-08 18:39:16.000000000 -0400
@@ -155,11 +155,14 @@ struct device * driver_find_device(struc
 struct class {
 	const char		* name;
 	struct module		* owner;
+	struct class            * parent; /* for subclasses */
 
 	struct subsystem	subsys;
-	struct list_head	children;
+	struct list_head	children; /* for class_devices */
 	struct list_head	interfaces;
-	struct semaphore	sem;	/* locks both the children and interfaces lists */
+	struct list_head	subclasses;
+	struct list_head	subclass_node; /* parent->sem held when accessing */
+	struct semaphore	sem;	/* locks children, subclasses, interfaces lists */
 
 	struct class_attribute		* class_attrs;
 	struct class_device_attribute	* class_dev_attrs;
@@ -258,7 +261,11 @@ struct class_interface {
 extern int class_interface_register(struct class_interface *);
 extern void class_interface_unregister(struct class_interface *);
 
-extern struct class *class_create(struct module *owner, char *name);
+extern struct class *class_subclass_create(struct module *owner, char *name, struct class *parent);
+static inline struct class *class_create(struct module *owner, char *name)
+{
+	return class_subclass_create(owner, name, NULL);
+}
 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, ...)

Re: [RFC][PATCH 2.6.13-rc1] driver core: subclasses

[Greg KH]

On Fri, Jul 08, 2005 at 05:54:48PM -0500, Matt Domsch wrote:
> Greg,
> 
> The below patch is a first pass at implementing subclasses, for review
> and comment.
> 
> As a test, I modified drivers/input/input.c to allocate a new class,
> and register it as a subclass.
> 
> Before:
> 
> /sys/class/input/
> /sys/class/input_device/
> 
> After:
> /sys/class/input/input_device/

Oops, when you mentioned this on irc, I thought you were referring to
class_devices not classes.  I don't want classes to be able to be
nested, only class devices.

I don't see a need for nested classes, as I thought the input thread had
resolved itself so that it didn't need it (but I stopped paying
attention, sorry, so I might be wrong here...)

Why can't you just use class_device's that can have children?  That way
the /sys/block stuff could be converted to also use this.

thanks,

greg k-h

Re: [RFC][PATCH 2.6.13-rc1] driver core: subclasses

[Matt Domsch]

On Fri, Jul 08, 2005 at 07:40:00PM -0700, Greg KH wrote:
> On Fri, Jul 08, 2005 at 05:54:48PM -0500, Matt Domsch wrote:
> > The below patch is a first pass at implementing subclasses, for review
> > and comment.
> 
> Oops, when you mentioned this on irc, I thought you were referring to
> class_devices not classes.  I don't want classes to be able to be
> nested, only class devices.
> 
> I don't see a need for nested classes, as I thought the input thread had
> resolved itself so that it didn't need it (but I stopped paying
> attention, sorry, so I might be wrong here...)

The tailing part of the thread is here:
http://marc.theaimsgroup.com/?l=linux-kernel&m=111873628324649&w=2
http://marc.theaimsgroup.com/?l=linux-hotplug-devel&m=111877313202313&w=2
where Hannes and Dmitry seemed to indicate (to my reading) that they
were looking for subclasses.  Perhaps it's not necessary.

> Why can't you just use class_device's that can have children?  That way
> the /sys/block stuff could be converted to also use this.

I will investigate.

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [RFC][PATCH 2.6.13-rc1] driver core: subclasses

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 4167 bytes --]

On Saturday 09 July 2005 08:04, Matt Domsch wrote:
> On Fri, Jul 08, 2005 at 07:40:00PM -0700, Greg KH wrote:
> > On Fri, Jul 08, 2005 at 05:54:48PM -0500, Matt Domsch wrote:
> > > The below patch is a first pass at implementing subclasses, for review
> > > and comment.
> > 
> > Oops, when you mentioned this on irc, I thought you were referring to
> > class_devices not classes.  I don't want classes to be able to be
> > nested, only class devices.
> > 
> > I don't see a need for nested classes,

[dtor@core ~]$ ls -1d /sys/class/ieee1394*
/sys/class/ieee1394
/sys/class/ieee1394_host
/sys/class/ieee1394_node
/sys/class/ieee1394_protocol

This one is worst. But USB, SCSI, I2C all could use subclasses too.

> > as I thought the input thread had 
> > resolved itself so that it didn't need it (but I stopped paying
> > attention, sorry, so I might be wrong here...)
>

As far as input subsystem goes we do need nested classes if we were to
keep compatibility with hotplug/udev installations. Please take a look
at the patch below. It complements my version of nested classes patch
(I am attaching it too since I think Matt's version is too complex).
Basically top class will define subsystem while nested classes will
define individual components of subsystem. This will allow keeping old
input hotplug handlers for both input devices and interface devices.

I want the tree look somethng like this:

/sys/class/input/input_dev/input_dev0
                |          input_dev1
                |          input_dev2
		|
                /input_intf/mouse0
                            mice
                            js1
                            js2 

Or we can go even deeper makeing input_intf a subtree with a subclass
for every interface type (evdev, mousedev, etc). But I think that would
be overkill.
 
> The tailing part of the thread is here:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=111873628324649&w=2
> http://marc.theaimsgroup.com/?l=linux-hotplug-devel&m=111877313202313&w=2
> where Hannes and Dmitry seemed to indicate (to my reading) that they
> were looking for subclasses.  Perhaps it's not necessary.
> 
> > Why can't you just use class_device's that can have children?  That way
> > the /sys/block stuff could be converted to also use this.
>

We need both I think.
 
-- 
Dmitry

===================================================================
Driver core: make parent class define subsystem for its children

When there is a hierarchy of classes make parent class define
subsystem as reported in hotplug notifications. The full class
path is reported in a new CLASS environment variable.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/class.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

Index: work/drivers/base/class.c
===================================================================
--- work.orig/drivers/base/class.c
+++ work/drivers/base/class.c
@@ -332,25 +332,35 @@ static int class_hotplug_filter(struct k
 static const char *class_hotplug_name(struct kset *kset, struct kobject *kobj)
 {
 	struct class_device *class_dev = to_class_dev(kobj);
+	struct class *class = class_dev->class;
 
-	return class_dev->class->name;
+	while (class->parent)
+		class = class->parent;
+
+	return class->name;
 }
 
 static int class_hotplug(struct kset *kset, struct kobject *kobj, char **envp,
 			 int num_envp, char *buffer, int buffer_size)
 {
 	struct class_device *class_dev = to_class_dev(kobj);
+	const char *path;
 	int i = 0;
 	int length = 0;
 	int retval = 0;
 
 	pr_debug("%s - name = %s\n", __FUNCTION__, class_dev->class_id);
 
+	path = kobject_get_path(&class_dev->class->subsys.kset.kobj, GFP_KERNEL);
+	add_hotplug_env_var(envp, num_envp, &i, buffer, buffer_size,
+			    &length, "CLASS=%s", path);
+	kfree(path);
+
 	if (class_dev->dev) {
 		/* add physical device, backing this device  */
 		struct device *dev = class_dev->dev;
-		char *path = kobject_get_path(&dev->kobj, GFP_KERNEL);
 
+		path = kobject_get_path(&dev->kobj, GFP_KERNEL);
 		add_hotplug_env_var(envp, num_envp, &i, buffer, buffer_size,
 				    &length, "PHYSDEVPATH=%s", path);
 		kfree(path);

[-- Attachment #2: nested-classes.patch --]
[-- Type: text/x-diff, Size: 1138 bytes --]

Subject: Allow nesting classes

Driver core: allow classes have children.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/class.c   |    5 ++++-
 include/linux/device.h |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

Index: work/drivers/base/class.c
===================================================================
--- work.orig/drivers/base/class.c
+++ work/drivers/base/class.c
@@ -146,7 +146,10 @@ int class_register(struct class * cls)
 	if (error)
 		return error;
 
-	subsys_set_kset(cls, class_subsys);
+	if (cls->parent)
+		subsys_set_kset(cls, cls->parent->subsys);
+	else
+		subsys_set_kset(cls, class_subsys);
 
 	error = subsystem_register(&cls->subsys);
 	if (!error) {
Index: work/include/linux/device.h
===================================================================
--- work.orig/include/linux/device.h
+++ work/include/linux/device.h
@@ -157,6 +157,7 @@ struct class {
 	struct module		* owner;
 
 	struct subsystem	subsys;
+	struct class		* parent;
 	struct list_head	children;
 	struct list_head	interfaces;
 	struct semaphore	sem;	/* locks both the children and interfaces lists */