Hi all,
There are 4 patches being posted here in response to this message that
start us on the way toward cleaning up the driver model code so that
it's actually usable by mere kernel developers :)
The main problem with the class code, is that _everyone_ gets it wrong
when trying to use it (and that includes me.) So, because of that, the
class_simple wrapper was written. So almost everyone used that. That
pretty much proved that the class_simple interface was the proper type
of interface for the main class code itself.
Because of that, Kay wrote a first cut at adding the class_simple type
of interface to the class core (he posted it to lkml a month or so ago.)
I've finally taken that code, tweaked it a bit (fixing a module
ownership issue that sprang up due to the class core changes, and
changed the locking model) and added it to my bk-driver tree. I've also
taken his tty and input patches that convert those subsystems over to
the new functions (it's pretty much a simple search and replace for
existing class_simple users.)
Then I moved the USB host controller code to use this new interface.
That was a bit more complex as it used the struct class and struct
class_device code directly. As you can see by the patch, the result is
pretty much identical, and actually a bit smaller in the end.
So I'll be slowly converting the kernel over to using this new
interface, and when finished, I can get rid of the old class apis (or
actually, just make them static) so that no one can implement them
improperly again...
Comments?
Oh, I need to go add kernel-doc to the new functions, will do that
next...
thanks,
greg k-h
patch 3 of 4
Subject: INPUT: move to use the new class code, instead of class_simple
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/drivers/input/evdev.c b/drivers/input/evdev.c
--- a/drivers/input/evdev.c 2005-03-15 08:54:28 -08:00
+++ b/drivers/input/evdev.c 2005-03-15 08:54:28 -08:00
@@ -431,9 +431,9 @@
devfs_mk_cdev(MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor),
S_IFCHR|S_IRUGO|S_IWUSR, "input/event%d", minor);
- class_simple_device_add(input_class,
- MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor),
- dev->dev, "event%d", minor);
+ class_device_create(input_class,
+ MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor),
+ dev->dev, "event%d", minor);
return &evdev->handle;
}
@@ -443,7 +443,8 @@
struct evdev *evdev = handle->private;
struct evdev_list *list;
- class_simple_device_remove(MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + evdev->minor));
+ class_device_destroy(input_class,
+ MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + evdev->minor));
devfs_remove("input/event%d", evdev->minor);
evdev->exist = 0;
diff -Nru a/drivers/input/input.c b/drivers/input/input.c
--- a/drivers/input/input.c 2005-03-15 08:54:28 -08:00
+++ b/drivers/input/input.c 2005-03-15 08:54:28 -08:00
@@ -702,13 +702,13 @@
static inline int input_proc_init(void) { return 0; }
#endif
-struct class_simple *input_class;
+struct class *input_class;
static int __init input_init(void)
{
int retval = -ENOMEM;
- input_class = class_simple_create(THIS_MODULE, "input");
+ input_class = class_create(THIS_MODULE, "input");
if (IS_ERR(input_class))
return PTR_ERR(input_class);
input_proc_init();
@@ -718,7 +718,7 @@
remove_proc_entry("devices", proc_bus_input_dir);
remove_proc_entry("handlers", proc_bus_input_dir);
remove_proc_entry("input", proc_bus);
- class_simple_destroy(input_class);
+ class_destroy(input_class);
return retval;
}
@@ -728,7 +728,7 @@
remove_proc_entry("handlers", proc_bus_input_dir);
remove_proc_entry("input", proc_bus);
unregister_chrdev(INPUT_MAJOR, "input");
- class_simple_destroy(input_class);
+ class_destroy(input_class);
}
return retval;
}
@@ -741,7 +741,7 @@
devfs_remove("input");
unregister_chrdev(INPUT_MAJOR, "input");
- class_simple_destroy(input_class);
+ class_destroy(input_class);
}
subsys_initcall(input_init);
diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
--- a/drivers/input/joydev.c 2005-03-15 08:54:28 -08:00
+++ b/drivers/input/joydev.c 2005-03-15 08:54:28 -08:00
@@ -452,9 +452,9 @@
devfs_mk_cdev(MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + minor),
S_IFCHR|S_IRUGO|S_IWUSR, "input/js%d", minor);
- class_simple_device_add(input_class,
- MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + minor),
- dev->dev, "js%d", minor);
+ class_device_create(input_class,
+ MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + minor),
+ dev->dev, "js%d", minor);
return &joydev->handle;
}
@@ -464,7 +464,7 @@
struct joydev *joydev = handle->private;
struct joydev_list *list;
- class_simple_device_remove(MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + joydev->minor));
+ class_device_destroy(input_class, MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + joydev->minor));
devfs_remove("input/js%d", joydev->minor);
joydev->exist = 0;
diff -Nru a/drivers/input/mousedev.c b/drivers/input/mousedev.c
--- a/drivers/input/mousedev.c 2005-03-15 08:54:28 -08:00
+++ b/drivers/input/mousedev.c 2005-03-15 08:54:28 -08:00
@@ -642,9 +642,9 @@
devfs_mk_cdev(MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + minor),
S_IFCHR|S_IRUGO|S_IWUSR, "input/mouse%d", minor);
- class_simple_device_add(input_class,
- MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + minor),
- dev->dev, "mouse%d", minor);
+ class_device_create(input_class,
+ MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + minor),
+ dev->dev, "mouse%d", minor);
return &mousedev->handle;
}
@@ -654,7 +654,8 @@
struct mousedev *mousedev = handle->private;
struct mousedev_list *list;
- class_simple_device_remove(MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + mousedev->minor));
+ class_device_destroy(input_class,
+ MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + mousedev->minor));
devfs_remove("input/mouse%d", mousedev->minor);
mousedev->exist = 0;
@@ -730,8 +731,8 @@
devfs_mk_cdev(MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + MOUSEDEV_MIX),
S_IFCHR|S_IRUGO|S_IWUSR, "input/mice");
- class_simple_device_add(input_class, MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + MOUSEDEV_MIX),
- NULL, "mice");
+ class_device_create(input_class,
+ MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + MOUSEDEV_MIX), NULL, "mice");
#ifdef CONFIG_INPUT_MOUSEDEV_PSAUX
if (!(psaux_registered = !misc_register(&psaux_mouse)))
@@ -750,7 +751,8 @@
misc_deregister(&psaux_mouse);
#endif
devfs_remove("input/mice");
- class_simple_device_remove(MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + MOUSEDEV_MIX));
+ class_device_destroy(input_class,
+ MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + MOUSEDEV_MIX));
input_unregister_handler(&mousedev_handler);
}
diff -Nru a/drivers/input/tsdev.c b/drivers/input/tsdev.c
--- a/drivers/input/tsdev.c 2005-03-15 08:54:28 -08:00
+++ b/drivers/input/tsdev.c 2005-03-15 08:54:28 -08:00
@@ -414,9 +414,9 @@
S_IFCHR|S_IRUGO|S_IWUSR, "input/ts%d", minor);
devfs_mk_cdev(MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + minor + TSDEV_MINORS/2),
S_IFCHR|S_IRUGO|S_IWUSR, "input/tsraw%d", minor);
- class_simple_device_add(input_class,
- MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + minor),
- dev->dev, "ts%d", minor);
+ class_device_create(input_class,
+ MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + minor),
+ dev->dev, "ts%d", minor);
return &tsdev->handle;
}
@@ -426,7 +426,8 @@
struct tsdev *tsdev = handle->private;
struct tsdev_list *list;
- class_simple_device_remove(MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + tsdev->minor));
+ class_device_destroy(input_class,
+ MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + tsdev->minor));
devfs_remove("input/ts%d", tsdev->minor);
devfs_remove("input/tsraw%d", tsdev->minor);
tsdev->exist = 0;
diff -Nru a/include/linux/input.h b/include/linux/input.h
--- a/include/linux/input.h 2005-03-15 08:54:28 -08:00
+++ b/include/linux/input.h 2005-03-15 08:54:28 -08:00
@@ -1010,7 +1010,7 @@
dev->absbit[LONG(axis)] |= BIT(axis);
}
-extern struct class_simple *input_class;
+extern struct class *input_class;
#endif
#endif
patch 1 of 4
Subject: CLASS: move a "simple" class logic into the class core.
One step on improving the class api so that it can not be used incorrectly.
This also fixes the module owner issue with the dev files that happened when
the devt logic moved to the class core.
Based on a patch originally written by Kay Sievers <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/drivers/base/class.c b/drivers/base/class.c
--- a/drivers/base/class.c 2005-03-15 08:52:00 -08:00
+++ b/drivers/base/class.c 2005-03-15 08:52:00 -08:00
@@ -16,6 +16,7 @@
#include <linux/init.h>
#include <linux/string.h>
#include <linux/kdev_t.h>
+#include <linux/err.h>
#include "base.h"
#define to_class_attr(_attr) container_of(_attr, struct class_attribute, attr)
@@ -162,6 +163,51 @@
subsystem_unregister(&cls->subsys);
}
+static void class_create_release(struct class *cls)
+{
+ kfree(cls);
+}
+
+static void class_device_create_release(struct class_device *class_dev)
+{
+ kfree(class_dev);
+}
+
+struct class *class_create(struct module *owner, char *name)
+{
+ struct class *cls;
+ int retval;
+
+ cls = kmalloc(sizeof(struct class), GFP_KERNEL);
+ if (!cls) {
+ retval = -ENOMEM;
+ goto error;
+ }
+ memset(cls, 0x00, sizeof(struct class));
+
+ cls->name = name;
+ cls->owner = owner;
+ cls->class_release = class_create_release;
+ cls->release = class_device_create_release;
+
+ retval = class_register(cls);
+ if (retval)
+ goto error;
+
+ return cls;
+
+error:
+ kfree(cls);
+ return ERR_PTR(retval);
+}
+
+void class_destroy(struct class *cls)
+{
+ if ((cls == NULL) || (IS_ERR(cls)))
+ return;
+
+ class_unregister(cls);
+}
/* Class Device Stuff */
@@ -375,7 +421,6 @@
{
return print_dev_t(buf, class_dev->devt);
}
-static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
void class_device_initialize(struct class_device *class_dev)
{
@@ -412,7 +457,31 @@
if ((error = kobject_add(&class_dev->kobj)))
goto register_done;
- /* now take care of our own registration */
+ /* add the needed attributes to this device */
+ if (MAJOR(class_dev->devt)) {
+ struct class_device_attribute *attr;
+ attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+ if (!attr) {
+ error = -ENOMEM;
+ kobject_del(&class_dev->kobj);
+ goto register_done;
+ }
+ memset(attr, sizeof(*attr), 0x00);
+ attr->attr.name = "dev";
+ attr->attr.mode = S_IRUGO;
+ attr->attr.owner = parent->owner;
+ attr->show = show_dev;
+ attr->store = NULL;
+ class_device_create_file(class_dev, attr);
+ class_dev->devt_attr = attr;
+ }
+
+ class_device_add_attrs(class_dev);
+ if (class_dev->dev)
+ sysfs_create_link(&class_dev->kobj,
+ &class_dev->dev->kobj, "device");
+
+ /* notify any interfaces this device is now here */
if (parent) {
down(&parent->sem);
list_add_tail(&class_dev->node, &parent->children);
@@ -422,14 +491,6 @@
up(&parent->sem);
}
- if (MAJOR(class_dev->devt))
- class_device_create_file(class_dev, &class_device_attr_dev);
-
- class_device_add_attrs(class_dev);
- if (class_dev->dev)
- sysfs_create_link(&class_dev->kobj,
- &class_dev->dev->kobj, "device");
-
register_done:
if (error && parent)
class_put(parent);
@@ -443,6 +504,41 @@
return class_device_add(class_dev);
}
+struct class_device *class_device_create(struct class *cls, dev_t devt,
+ struct device *device, char *fmt, ...)
+{
+ va_list args;
+ struct class_device *class_dev = NULL;
+ int retval = -ENODEV;
+
+ if (cls == NULL || IS_ERR(cls))
+ goto error;
+
+ class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+ if (!class_dev) {
+ retval = -ENOMEM;
+ goto error;
+ }
+ memset(class_dev, 0x00, sizeof(struct class_device));
+
+ class_dev->devt = devt;
+ class_dev->dev = device;
+ class_dev->class = cls;
+
+ va_start(args, fmt);
+ vsnprintf(class_dev->class_id, BUS_ID_SIZE, fmt, args);
+ va_end(args);
+ retval = class_device_register(class_dev);
+ if (retval)
+ goto error;
+
+ return class_dev;
+
+error:
+ kfree(class_dev);
+ return ERR_PTR(retval);
+}
+
void class_device_del(struct class_device *class_dev)
{
struct class * parent = class_dev->class;
@@ -459,6 +555,11 @@
if (class_dev->dev)
sysfs_remove_link(&class_dev->kobj, "device");
+ if (class_dev->devt_attr) {
+ class_device_remove_file(class_dev, class_dev->devt_attr);
+ kfree(class_dev->devt_attr);
+ class_dev->devt_attr = NULL;
+ }
class_device_remove_attrs(class_dev);
kobject_del(&class_dev->kobj);
@@ -475,6 +576,24 @@
class_device_put(class_dev);
}
+void class_device_destroy(struct class *cls, dev_t devt)
+{
+ struct class_device *class_dev = NULL;
+ struct class_device *class_dev_tmp;
+
+ down(&cls->sem);
+ list_for_each_entry(class_dev_tmp, &cls->children, node) {
+ if (class_dev_tmp->devt == devt) {
+ class_dev = class_dev_tmp;
+ break;
+ }
+ }
+ up(&cls->sem);
+
+ if (class_dev)
+ class_device_unregister(class_dev);
+}
+
int class_device_rename(struct class_device *class_dev, char *new_name)
{
int error = 0;
@@ -574,6 +693,8 @@
EXPORT_SYMBOL_GPL(class_unregister);
EXPORT_SYMBOL_GPL(class_get);
EXPORT_SYMBOL_GPL(class_put);
+EXPORT_SYMBOL_GPL(class_create);
+EXPORT_SYMBOL_GPL(class_destroy);
EXPORT_SYMBOL_GPL(class_device_register);
EXPORT_SYMBOL_GPL(class_device_unregister);
@@ -582,6 +703,8 @@
EXPORT_SYMBOL_GPL(class_device_del);
EXPORT_SYMBOL_GPL(class_device_get);
EXPORT_SYMBOL_GPL(class_device_put);
+EXPORT_SYMBOL_GPL(class_device_create);
+EXPORT_SYMBOL_GPL(class_device_destroy);
EXPORT_SYMBOL_GPL(class_device_create_file);
EXPORT_SYMBOL_GPL(class_device_remove_file);
EXPORT_SYMBOL_GPL(class_device_create_bin_file);
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h 2005-03-15 08:52:00 -08:00
+++ b/include/linux/device.h 2005-03-15 08:52:00 -08:00
@@ -143,6 +143,7 @@
*/
struct class {
char * name;
+ struct module * owner;
struct subsystem subsys;
struct list_head children;
@@ -185,6 +186,7 @@
struct kobject kobj;
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 */
void * class_data; /* class-specific data */
@@ -244,6 +246,12 @@
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 void class_destroy(struct class *cls);
+extern struct class_device *class_device_create(struct class *cls, dev_t devt,
+ struct device *device, char *fmt, ...);
+extern void class_device_destroy(struct class *cls, dev_t devt);
/* interface for class simple stuff */
extern struct class_simple *class_simple_create(struct module *owner, char *name);
patch 4 of 4
Subject: USB: move the usb hcd code to use the new class code.
This moves a kref into the main hcd structure, which detaches it from
the class device structure.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c 2005-03-15 08:54:43 -08:00
+++ b/drivers/usb/core/hcd.c 2005-03-15 08:54:43 -08:00
@@ -650,41 +650,43 @@
struct usb_bus *usb_bus_get(struct usb_bus *bus)
{
if (bus)
- class_device_get(&bus->class_dev);
+ kref_get(&bus->kref);
return bus;
}
EXPORT_SYMBOL_GPL(usb_bus_get);
+static void usb_host_release(struct kref *kref)
+{
+ struct usb_bus *bus = container_of(kref, struct usb_bus, kref);
+
+ if (bus->release)
+ bus->release(bus);
+}
+
void usb_bus_put(struct usb_bus *bus)
{
if (bus)
- class_device_put(&bus->class_dev);
+ kref_put(&bus->kref, usb_host_release);
}
EXPORT_SYMBOL_GPL(usb_bus_put);
/*-------------------------------------------------------------------------*/
-static void usb_host_release(struct class_device *class_dev)
-{
- struct usb_bus *bus = to_usb_bus(class_dev);
-
- if (bus->release)
- bus->release(bus);
-}
-
-static struct class usb_host_class = {
- .name = "usb_host",
- .release = &usb_host_release,
-};
+static struct class *usb_host_class;
int usb_host_init(void)
{
- return class_register(&usb_host_class);
+ int retval = 0;
+
+ usb_host_class = class_create(THIS_MODULE, "usb_host");
+ if (IS_ERR(usb_host_class))
+ retval = PTR_ERR(usb_host_class);
+ return retval;
}
void usb_host_cleanup(void)
{
- class_unregister(&usb_host_class);
+ class_destroy(usb_host_class);
}
/**
@@ -709,8 +711,7 @@
INIT_LIST_HEAD (&bus->bus_list);
- class_device_initialize(&bus->class_dev);
- bus->class_dev.class = &usb_host_class;
+ kref_init(&bus->kref);
}
EXPORT_SYMBOL (usb_bus_init);
@@ -753,7 +754,6 @@
int usb_register_bus(struct usb_bus *bus)
{
int busnum;
- int retval;
down (&usb_bus_list_lock);
busnum = find_next_zero_bit (busmap.busmap, USB_MAXBUS, 1);
@@ -766,15 +766,15 @@
return -E2BIG;
}
- snprintf(bus->class_dev.class_id, BUS_ID_SIZE, "usb%d", busnum);
- bus->class_dev.dev = bus->controller;
- retval = class_device_add(&bus->class_dev);
- if (retval) {
+ bus->class_dev = class_device_create(usb_host_class, MKDEV(0,0), bus->controller, "usb%d", busnum);
+ if (IS_ERR(bus->class_dev)) {
clear_bit(busnum, busmap.busmap);
up(&usb_bus_list_lock);
- return retval;
+ return PTR_ERR(bus->class_dev);
}
+ class_set_devdata(bus->class_dev, bus);
+
/* Add it to the local list of buses */
list_add (&bus->bus_list, &usb_bus_list);
up (&usb_bus_list_lock);
@@ -813,7 +813,7 @@
clear_bit (bus->busnum, busmap.busmap);
- class_device_del(&bus->class_dev);
+ class_device_unregister(bus->class_dev);
}
EXPORT_SYMBOL (usb_deregister_bus);
diff -Nru a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
--- a/drivers/usb/host/ehci-dbg.c 2005-03-15 08:54:43 -08:00
+++ b/drivers/usb/host/ehci-dbg.c 2005-03-15 08:54:43 -08:00
@@ -450,7 +450,7 @@
*buf = 0;
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ehci = hcd_to_ehci (hcd);
next = buf;
@@ -496,7 +496,7 @@
return 0;
seen_count = 0;
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ehci = hcd_to_ehci (hcd);
next = buf;
@@ -633,7 +633,7 @@
static char fmt [] = "%*s\n";
static char label [] = "";
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ehci = hcd_to_ehci (hcd);
next = buf;
@@ -735,7 +735,7 @@
static inline void create_debug_files (struct ehci_hcd *ehci)
{
- struct class_device *cldev = &ehci_to_hcd(ehci)->self.class_dev;
+ struct class_device *cldev = ehci_to_hcd(ehci)->self.class_dev;
class_device_create_file(cldev, &class_device_attr_async);
class_device_create_file(cldev, &class_device_attr_periodic);
@@ -744,7 +744,7 @@
static inline void remove_debug_files (struct ehci_hcd *ehci)
{
- struct class_device *cldev = &ehci_to_hcd(ehci)->self.class_dev;
+ struct class_device *cldev = ehci_to_hcd(ehci)->self.class_dev;
class_device_remove_file(cldev, &class_device_attr_async);
class_device_remove_file(cldev, &class_device_attr_periodic);
diff -Nru a/drivers/usb/host/ohci-dbg.c b/drivers/usb/host/ohci-dbg.c
--- a/drivers/usb/host/ohci-dbg.c 2005-03-15 08:54:43 -08:00
+++ b/drivers/usb/host/ohci-dbg.c 2005-03-15 08:54:43 -08:00
@@ -477,7 +477,7 @@
size_t temp;
unsigned long flags;
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ohci = hcd_to_ohci(hcd);
@@ -510,7 +510,7 @@
return 0;
seen_count = 0;
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ohci = hcd_to_ohci(hcd);
next = buf;
@@ -607,7 +607,7 @@
char *next;
u32 rdata;
- bus = to_usb_bus(class_dev);
+ bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
ohci = hcd_to_ohci(hcd);
regs = ohci->regs;
@@ -678,7 +678,7 @@
static inline void create_debug_files (struct ohci_hcd *ohci)
{
- struct class_device *cldev = &ohci_to_hcd(ohci)->self.class_dev;
+ struct class_device *cldev = ohci_to_hcd(ohci)->self.class_dev;
class_device_create_file(cldev, &class_device_attr_async);
class_device_create_file(cldev, &class_device_attr_periodic);
@@ -688,7 +688,7 @@
static inline void remove_debug_files (struct ohci_hcd *ohci)
{
- struct class_device *cldev = &ohci_to_hcd(ohci)->self.class_dev;
+ struct class_device *cldev = ohci_to_hcd(ohci)->self.class_dev;
class_device_remove_file(cldev, &class_device_attr_async);
class_device_remove_file(cldev, &class_device_attr_periodic);
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h 2005-03-15 08:54:43 -08:00
+++ b/include/linux/usb.h 2005-03-15 08:54:43 -08:00
@@ -287,15 +287,14 @@
struct dentry *usbfs_dentry; /* usbfs dentry entry for the bus */
- struct class_device class_dev; /* class device for this bus */
+ struct class_device *class_dev; /* class device for this bus */
+ struct kref kref; /* handles reference counting this bus */
void (*release)(struct usb_bus *bus); /* function to destroy this bus's memory */
#if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
struct mon_bus *mon_bus; /* non-null when associated */
int monitored; /* non-zero when monitored */
#endif
};
-#define to_usb_bus(d) container_of(d, struct usb_bus, class_dev)
-
/* -------------------------------------------------------------------------- */
patch 2 of 4
Subject: tty: move to use the new class code, instead of class_simple
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/drivers/char/tty_io.c b/drivers/char/tty_io.c
--- a/drivers/char/tty_io.c 2005-03-15 08:54:22 -08:00
+++ b/drivers/char/tty_io.c 2005-03-15 08:54:22 -08:00
@@ -2653,7 +2653,7 @@
tty->driver->write(tty, &ch, 1);
}
-static struct class_simple *tty_class;
+static struct class *tty_class;
/**
* tty_register_device - register a tty device
@@ -2686,7 +2686,7 @@
pty_line_name(driver, index, name);
else
tty_line_name(driver, index, name);
- class_simple_device_add(tty_class, dev, device, name);
+ class_device_create(tty_class, dev, device, name);
}
/**
@@ -2700,7 +2700,7 @@
void tty_unregister_device(struct tty_driver *driver, unsigned index)
{
devfs_remove("%s%d", driver->devfs_name, index + driver->name_base);
- class_simple_device_remove(MKDEV(driver->major, driver->minor_start) + index);
+ class_device_destroy(tty_class, MKDEV(driver->major, driver->minor_start) + index);
}
EXPORT_SYMBOL(tty_register_device);
@@ -2917,7 +2917,7 @@
static int __init tty_class_init(void)
{
- tty_class = class_simple_create(THIS_MODULE, "tty");
+ tty_class = class_create(THIS_MODULE, "tty");
if (IS_ERR(tty_class))
return PTR_ERR(tty_class);
return 0;
@@ -2946,14 +2946,14 @@
register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
panic("Couldn't register /dev/tty driver\n");
devfs_mk_cdev(MKDEV(TTYAUX_MAJOR, 0), S_IFCHR|S_IRUGO|S_IWUGO, "tty");
- class_simple_device_add(tty_class, MKDEV(TTYAUX_MAJOR, 0), NULL, "tty");
+ class_device_create(tty_class, MKDEV(TTYAUX_MAJOR, 0), NULL, "tty");
cdev_init(&console_cdev, &console_fops);
if (cdev_add(&console_cdev, MKDEV(TTYAUX_MAJOR, 1), 1) ||
register_chrdev_region(MKDEV(TTYAUX_MAJOR, 1), 1, "/dev/console") < 0)
panic("Couldn't register /dev/console driver\n");
devfs_mk_cdev(MKDEV(TTYAUX_MAJOR, 1), S_IFCHR|S_IRUSR|S_IWUSR, "console");
- class_simple_device_add(tty_class, MKDEV(TTYAUX_MAJOR, 1), NULL, "console");
+ class_device_create(tty_class, MKDEV(TTYAUX_MAJOR, 1), NULL, "console");
#ifdef CONFIG_UNIX98_PTYS
cdev_init(&ptmx_cdev, &ptmx_fops);
@@ -2961,7 +2961,7 @@
register_chrdev_region(MKDEV(TTYAUX_MAJOR, 2), 1, "/dev/ptmx") < 0)
panic("Couldn't register /dev/ptmx driver\n");
devfs_mk_cdev(MKDEV(TTYAUX_MAJOR, 2), S_IFCHR|S_IRUGO|S_IWUGO, "ptmx");
- class_simple_device_add(tty_class, MKDEV(TTYAUX_MAJOR, 2), NULL, "ptmx");
+ class_device_create(tty_class, MKDEV(TTYAUX_MAJOR, 2), NULL, "ptmx");
#endif
#ifdef CONFIG_VT
@@ -2970,7 +2970,7 @@
register_chrdev_region(MKDEV(TTY_MAJOR, 0), 1, "/dev/vc/0") < 0)
panic("Couldn't register /dev/tty0 driver\n");
devfs_mk_cdev(MKDEV(TTY_MAJOR, 0), S_IFCHR|S_IRUSR|S_IWUSR, "vc/0");
- class_simple_device_add(tty_class, MKDEV(TTY_MAJOR, 0), NULL, "tty0");
+ class_device_create(tty_class, MKDEV(TTY_MAJOR, 0), NULL, "tty0");
vty_init();
#endif
Hi Greg,
On Tue, 15 Mar 2005 09:08:34 -0800, Greg KH <[email protected]> wrote:
> So I'll be slowly converting the kernel over to using this new
> interface, and when finished, I can get rid of the old class apis (or
> actually, just make them static) so that no one can implement them
> improperly again...
I disagree with this last step. What I liked about the driver model is
that once you convert (properly) subsystem to using it you
automatically get your proper refcounting and memory gets released at
proper time. The change as it proposed disconnects class device
instance from the meat so separate refcounting implementation
isneeded. This increases maintenance costs.
I always viewed class_simple as a stop-gap measure to get hotplug
events in place until proper implementation is done. Please leave the
original interface in place so it can still be used if one wshes to do
so.
And what about device_driver and device structure? Are they going to
be changed over to be separately allocated linked objects? If not then
its enouther reason to keep original class interface - uniformity of
driver model interface.
--
Dmitry
On Tue, Mar 15, 2005 at 09:08:34AM -0800, Greg KH wrote:
> Then I moved the USB host controller code to use this new interface.
> That was a bit more complex as it used the struct class and struct
> class_device code directly. As you can see by the patch, the result is
> pretty much identical, and actually a bit smaller in the end.
>
> So I'll be slowly converting the kernel over to using this new
> interface, and when finished, I can get rid of the old class apis (or
> actually, just make them static) so that no one can implement them
> improperly again...
>
> Comments?
The "old" class api _forced_ you to think of reference counting of
dynamically allocated objects, while it gets easier to get reference
counting wrong using this "simple"/"new" interface: while struct class will
always have fine reference counting, the "parent" struct [with struct class
no longer being embedded] needs to be thought of individually; and the
reference count cannot be shared any longer.
Also, it seems to me that you view the class subsystem to be too closely
related to /dev entries -- and for these /dev entries class_simple was
introduced, IIRC. However, /dev is not the reason the class subsystem was
introduced for -- instead, it describes _types_ of devices which want to
share (userspace and in-kernel) interfaces. For example pcmcia sockets which
can reside on different buses, but can be handled (mostly) the same way by
kernel- and userspace. For example, temperature sensors could be exported
using /sys/class/temp_sensors/... -- then userspace wouldn't need to know
whether the temperature was determined using an ACPI BIOS call or by
accessing an i2c device. Such "abstractions", and other kernel code whcih
uses these "abstractions" (a.k.a. class interfaces) are a great feature to
have, and one too less used by now.
Dominik
On Tue, 15 Mar 2005 20:08:47 +0100, Dominik Brodowski
<[email protected]> wrote:
> On Tue, Mar 15, 2005 at 09:08:34AM -0800, Greg KH wrote:
> > Then I moved the USB host controller code to use this new interface.
> > That was a bit more complex as it used the struct class and struct
> > class_device code directly. As you can see by the patch, the result is
> > pretty much identical, and actually a bit smaller in the end.
> >
> > So I'll be slowly converting the kernel over to using this new
> > interface, and when finished, I can get rid of the old class apis (or
> > actually, just make them static) so that no one can implement them
> > improperly again...
> >
> > Comments?
>
> The "old" class api _forced_ you to think of reference counting of
> dynamically allocated objects, while it gets easier to get reference
> counting wrong using this "simple"/"new" interface: while struct class will
> always have fine reference counting, the "parent" struct [with struct class
> no longer being embedded] needs to be thought of individually; and the
> reference count cannot be shared any longer.
>
> Also, it seems to me that you view the class subsystem to be too closely
> related to /dev entries -- and for these /dev entries class_simple was
> introduced, IIRC. However, /dev is not the reason the class subsystem was
> introduced for -- instead, it describes _types_ of devices which want to
> share (userspace and in-kernel) interfaces.
Exactly! I am working on input_dev class that lies between actual
devices and input class devices exported by evdev, tsdev, etc. It does
not have /dev entry, it is just a building block for the rest of the
subsystem. evdev and the rest will be interfaces (as per driver model)
for the input_dev class and will in turn continue exporting input
class devices that do have /dev entries.
--
Dmitry
On 03/15/05 13:08:47, Dominik Brodowski wrote:
> On Tue, Mar 15, 2005 at 09:08:34AM -0800, Greg KH wrote:
> > Then I moved the USB host controller code to use this new interface.
> > That was a bit more complex as it used the struct class and struct
> > class_device code directly. As you can see by the patch, the result is
> > pretty much identical, and actually a bit smaller in the end.
> >
> > So I'll be slowly converting the kernel over to using this new
> > interface, and when finished, I can get rid of the old class apis (or
> > actually, just make them static) so that no one can implement them
> > improperly again...
> >
> > Comments?
>
> Also, it seems to me that you view the class subsystem to be too closely
> related to /dev entries -- and for these /dev entries class_simple was
> introduced, IIRC. However, /dev is not the reason the class subsystem was
> introduced for -- instead, it describes _types_ of devices which want to
> share (userspace and in-kernel) interfaces.
Exactly. I hope you take a close look at
drivers/video/backlight/backlight.c and drivers/video/backlight/lcd.c and
how it uses the class device. Neither does not create anything in /dev.
The model and design that is used in the backlight.c and lcd.c code is also
being used in some (currently out of tree) new code I am working on.
Either continue to support the current class API or provide a design and
API that works with backlight.c
John
On Tue, March 15, 2005 2:08 pm, Dominik Brodowski said:
> For example, temperature sensors could be exported
> using /sys/class/temp_sensors/... -- then userspace wouldn't need to know
> whether the temperature was determined using an ACPI BIOS call or by
> accessing an i2c device. Such "abstractions", and other kernel code whcih
> uses these "abstractions" (a.k.a. class interfaces) are a great feature
> to have, and one too less used by now.
That really sounds like a job for user space, why complicate the kernel?
A simple user space library could handle returning temperatures without
the caller knowing from where the value was obtained. Isn't this the
exact type of thing that just bloats a kernel needlessly?
Sean
On Tue, Mar 15, 2005 at 12:47:38PM -0500, Dmitry Torokhov wrote:
> Hi Greg,
>
> On Tue, 15 Mar 2005 09:08:34 -0800, Greg KH <[email protected]> wrote:
>
> > So I'll be slowly converting the kernel over to using this new
> > interface, and when finished, I can get rid of the old class apis (or
> > actually, just make them static) so that no one can implement them
> > improperly again...
>
> I disagree with this last step. What I liked about the driver model is
> that once you convert (properly) subsystem to using it you
> automatically get your proper refcounting and memory gets released at
> proper time. The change as it proposed disconnects class device
> instance from the meat so separate refcounting implementation
> isneeded. This increases maintenance costs.
I agree. The big point is that "properly" statement. _Everyone_ gets
this wrong the first time they do it. And the second. Hopefully, if
they are persistant enough, the third time they get closer, and so on...
We need to make the driver model interface easier to use. The class
code is used by more individual drivers than the struct device, so
that's the first place to make these kinds of changes, as it is the most
necessary.
> I always viewed class_simple as a stop-gap measure to get hotplug
> events in place until proper implementation is done. Please leave the
> original interface in place so it can still be used if one wshes to do
> so.
No, no one has so far done the "proper implementation" and I don't blame
them. It's not simple, and it gives them a very low payback for their
time. The old interface is good and powerful and hard to use.
> And what about device_driver and device structure? Are they going to
> be changed over to be separately allocated linked objects?
The driver stuff probably will be, and the device stuff possibly.
However, they are used by a very small ammount of core code (the bus
drivers), so changing that interface is not that important at this time.
> If not then its enouther reason to keep original class interface -
> uniformity of driver model interface.
Ease-of-use trumps uniformity in this case, sorry. I want to make it
easy to write code. The current interface sucks at this goal.
thanks,
greg k-h
On Tue, Mar 15, 2005 at 08:08:47PM +0100, Dominik Brodowski wrote:
> On Tue, Mar 15, 2005 at 09:08:34AM -0800, Greg KH wrote:
> > Then I moved the USB host controller code to use this new interface.
> > That was a bit more complex as it used the struct class and struct
> > class_device code directly. As you can see by the patch, the result is
> > pretty much identical, and actually a bit smaller in the end.
> >
> > So I'll be slowly converting the kernel over to using this new
> > interface, and when finished, I can get rid of the old class apis (or
> > actually, just make them static) so that no one can implement them
> > improperly again...
> >
> > Comments?
>
> The "old" class api _forced_ you to think of reference counting of
> dynamically allocated objects, while it gets easier to get reference
> counting wrong using this "simple"/"new" interface: while struct class will
> always have fine reference counting, the "parent" struct [with struct class
> no longer being embedded] needs to be thought of individually; and the
> reference count cannot be shared any longer.
The reference counting will now be correct. That is implicit in the
interface, and was done because, not to sound like a broken record,
_everyone_ got it wrong when they tried to do it...
> Also, it seems to me that you view the class subsystem to be too closely
> related to /dev entries -- and for these /dev entries class_simple was
> introduced, IIRC. However, /dev is not the reason the class subsystem was
> introduced for -- instead, it describes _types_ of devices which want to
> share (userspace and in-kernel) interfaces.
I agree, I know it isn't directly related to /dev entries, but that _is_
the most common usage of it, so I can't ignore it :)
Anyway, it's very simple to convert over to using the new functions, and
still have all of your sysfs and reference counting functionality. See
the USB patch that I posted in this series as an example of how to do
this. Just use a kref and a pointer to the class_device. You have all
of the previous functionality that you needed before right there.
> For example pcmcia sockets which
> can reside on different buses, but can be handled (mostly) the same way by
> kernel- and userspace. For example, temperature sensors could be exported
> using /sys/class/temp_sensors/... -- then userspace wouldn't need to know
> whether the temperature was determined using an ACPI BIOS call or by
> accessing an i2c device. Such "abstractions", and other kernel code whcih
> uses these "abstractions" (a.k.a. class interfaces) are a great feature to
> have, and one too less used by now.
class interfaces are not going away, there's a good need for them like
you have pointed out. I'm not expecting to just delete those api
functions tomorrow, but slowly phase out the use of them over time, and
hopefully, eventually get rid of them. I think that with my USB host
controller patch, I've proved that they are not as needed as you might
think they were.
It's easy to make a complex, powerful, all-singing-all-dancing api.
That's what we have today. It's hard to make such an api easy to use,
that's what we need to realize and start to fix. This is the first of
such steps to try to achieve this.
thanks,
greg k-h
On Tue, 15 Mar 2005 11:34:15 -0800, Greg KH <[email protected]> wrote:
> On Tue, Mar 15, 2005 at 12:47:38PM -0500, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Tue, 15 Mar 2005 09:08:34 -0800, Greg KH <[email protected]> wrote:
> >
> > > So I'll be slowly converting the kernel over to using this new
> > > interface, and when finished, I can get rid of the old class apis (or
> > > actually, just make them static) so that no one can implement them
> > > improperly again...
> >
> > I disagree with this last step. What I liked about the driver model is
> > that once you convert (properly) subsystem to using it you
> > automatically get your proper refcounting and memory gets released at
> > proper time. The change as it proposed disconnects class device
> > instance from the meat so separate refcounting implementation
> > isneeded. This increases maintenance costs.
>
> I agree. The big point is that "properly" statement. _Everyone_ gets
> this wrong the first time they do it. And the second. Hopefully, if
> they are persistant enough, the third time they get closer, and so on...
>
> We need to make the driver model interface easier to use. The class
> code is used by more individual drivers than the struct device, so
> that's the first place to make these kinds of changes, as it is the most
> necessary.
>
> > I always viewed class_simple as a stop-gap measure to get hotplug
> > events in place until proper implementation is done. Please leave the
> > original interface in place so it can still be used if one wshes to do
> > so.
>
> No, no one has so far done the "proper implementation" and I don't blame
> them. It's not simple, and it gives them a very low payback for their
> time. The old interface is good and powerful and hard to use.
>
It takes only one proper implementation and the rest will follow. If
buses and devices can be implemented properly so can be calss_devices.
There is nothing magic about them.
Besides, if you divorce class device and the "meat" like that most of
the time you just ignoring the same problems that exist with the
"meat". Class is not useful by itself, it is additional attributes and
such that gets used. And with additional attributes you again have
your lifetime rules, refounting and such. In effect the change does
not buy you anything unless class interface is extremely limited.
--
Dmitry
On Tue, Mar 15, 2005 at 11:34:15AM -0800, Greg KH wrote:
> > And what about device_driver and device structure? Are they going to
> > be changed over to be separately allocated linked objects?
>
> The driver stuff probably will be, and the device stuff possibly.
> However, they are used by a very small ammount of core code (the bus
> drivers), so changing that interface is not that important at this time.
So this means every device will have yet another reference count, and you
need to be aware of _each_ lifetime to write correct code. And the
_reference counting_ is the hard thing to get right, so we should make
_that_ easier. The existing class API was a step towards this direction, and
with the changes you're suggesting here we'd do two jumps backwards.
> > If not then its enouther reason to keep original class interface -
> > uniformity of driver model interface.
>
> Ease-of-use trumps uniformity
Ease-of-use, maybe. However, it also means
ease-of-getting-reference-counting-wrong. And reference counting trumps it
all :)
Thanks,
Dominik
On Tue, 15 Mar 2005 11:51:21 -0800, Greg KH <[email protected]> wrote:
>
> class interfaces are not going away, there's a good need for them like
> you have pointed out. I'm not expecting to just delete those api
> functions tomorrow, but slowly phase out the use of them over time, and
> hopefully, eventually get rid of them. I think that with my USB host
> controller patch, I've proved that they are not as needed as you might
> think they were.
>
It looks to me (and I might be wrong) that USB was never really
integrated into the driver model. It was glued with it but the driver
model came after most of the domain was defined, and it did not get to
be "bones" of the subsystem. This is why it is so easy to deatch it.
--
Dmitry
On Tuesday 15 March 2005 12:14 pm, Dmitry Torokhov wrote:
>
> It looks to me (and I might be wrong) that USB was never really
> integrated into the driver model. It was glued with it but the driver
> model came after most of the domain was defined, and it did not get to
> be "bones" of the subsystem. This is why it is so easy to deatch it.
That doesn't seem accurate to me. Are you thinking maybe about
just how it uses the class device stuff? Like the rest of the
class device support (for all busses!) that did indeed come later.
You may recall that the first versions of the driver model had
more or less a big "fixme" where class devices sat... Or are
you maybe thinking about peripheral side (not host side) USB?
But the "struct device" core of the driver model sure looks like
the bones of USB to me. Host controllers, hubs, devices, and
interfaces all use it well, behave well with hot-unplug (which
is more than many subsystems can say even in 2.6.11!), and even
handling funky cases like drivers needing to bind to multiple
interfaces on one device. That last took quite a while to land,
it involved ripping out the last pre-driver-model binding code.
I would really _not_ want to try to detach any of that stuff!
- Dave
On Tue, Mar 15, 2005 at 11:51:21AM -0800, Greg KH wrote:
> > Also, it seems to me that you view the class subsystem to be too closely
> > related to /dev entries -- and for these /dev entries class_simple was
> > introduced, IIRC. However, /dev is not the reason the class subsystem was
> > introduced for -- instead, it describes _types_ of devices which want to
> > share (userspace and in-kernel) interfaces.
>
> I agree, I know it isn't directly related to /dev entries, but that _is_
> the most common usage of it, so I can't ignore it :)
I did not say "ignore". Having the new interface available (patches 1-3)
seems to be a sensible thing to do. Removing the so-called "old" api
(patches 4-x) is a different topic, though.
> Anyway, it's very simple to convert over to using the new functions, and
> still have all of your sysfs and reference counting functionality. See
> the USB patch that I posted in this series as an example of how to do
> this. Just use a kref and a pointer to the class_device. You have all
> of the previous functionality that you needed before right there.
However, you need to do it in _addition_ -- you don't get it "for free" if
using struct class. Which means prgrammers need to think of two things
instead of one. And that's bad(TM).
Also, the lack of proper reference counting in several parts of the kernel
is proof enough that there's need to "encourage" reference counting. And
that's something the class subsystem did and does by providing easy
"embedded" reference counting, by warning on missing !release functions etc.
> class interfaces are not going away, there's a good need for them like
> you have pointed out. I'm not expecting to just delete those api
> functions tomorrow, but slowly phase out the use of them over time, and
> hopefully, eventually get rid of them. I think that with my USB host
> controller patch, I've proved that they are not as needed as you might
> think they were.
Indeed, such _workarounds_ are possible. A patch which converts pcmcia to
this new infrastructure would be quite trivial to write. However: I think we
should NOT do it. As it is a workaround to the more elegant solution the
"old" class API alllowed for.
> It's easy to make a complex, powerful, all-singing-all-dancing api.
> That's what we have today. It's hard to make such an api easy to use,
> that's what we need to realize and start to fix. This is the first of
> such steps to try to achieve this.
Math is hard, so let's go shopping. The "I want a /dev-entry"-api may get
easier with your patches 1-3. With your patch 4 there is no improvement in
this area, however the "I want sane device-related reference counting"-api
will be much more difficult to get right.
Thanks,
Dominik
On Tue, 15 Mar 2005 12:35:02 -0800, David Brownell <[email protected]> wrote:
> On Tuesday 15 March 2005 12:14 pm, Dmitry Torokhov wrote:
> >
> > It looks to me (and I might be wrong) that USB was never really
> > integrated into the driver model. It was glued with it but the driver
> > model came after most of the domain was defined, and it did not get to
> > be "bones" of the subsystem. This is why it is so easy to deatch it.
>
> That doesn't seem accurate to me. Are you thinking maybe about
> just how it uses the class device stuff? Like the rest of the
> class device support (for all busses!) that did indeed come later.
> You may recall that the first versions of the driver model had
> more or less a big "fixme" where class devices sat... Or are
> you maybe thinking about peripheral side (not host side) USB?
>
> But the "struct device" core of the driver model sure looks like
> the bones of USB to me. Host controllers, hubs, devices, and
> interfaces all use it well, behave well with hot-unplug (which
> is more than many subsystems can say even in 2.6.11!), and even
> handling funky cases like drivers needing to bind to multiple
> interfaces on one device. That last took quite a while to land,
> it involved ripping out the last pre-driver-model binding code.
>
David,
I was not criticizing the code, not at all, I was commenting on
evolution of the code (at least the way I perceive it). The fact that
there is (or was until recently) pre-driver-model binding code shows
that merging is still ongoing and this fact makes reversing the
process easier.
--
Dmitry
On Tuesday 15 March 2005 12:48 pm, Dmitry Torokhov wrote:
> On Tue, 15 Mar 2005 12:35:02 -0800, David Brownell <[email protected]> wrote:
> > On Tuesday 15 March 2005 12:14 pm, Dmitry Torokhov wrote:
> > >
> > > It looks to me (and I might be wrong) that USB was never really
> > > integrated into the driver model. It was glued with it but the driver
> > > model came after most of the domain was defined, and it did not get to
> > > be "bones" of the subsystem. This is why it is so easy to deatch it.
> >
> > That doesn't seem accurate to me. Are you thinking maybe about
> > just how it uses the class device stuff? ...
> >
>
> David,
>
> I was not criticizing the code, not at all, I was commenting on
> evolution of the code (at least the way I perceive it). The fact that
> there is (or was until recently) pre-driver-model binding code shows
> that merging is still ongoing and this fact makes reversing the
> process easier.
You still haven't answered my question. My observation was that
only the class code can in any sense be called "new" ... so your
blanket statement seemed to overlook several essential points!
Which parts of the driver model were you thinking of?
That pre-driver model stuff went away in maybe 2.6.5 or so, I
forget just when. If you think those changes can easily be
reversed, I suggest you think again ... they enabled a LOT of
likewise-overdue cleanups. And they only affected the case of
drivers that bound to multiple interfaces, gettng rid of a
funky "half bound" state and making it look like the primary
case (drivers binding to one interface at a time), which has
been working since 2.5.early.
It's been a long slog to get to a usb core that's a good
match to the relatively complex requirements of USB. With a
few notable exceptions (like PM non-support for wakeup events
and for selective suspend, and strange locking side effects),
converting to the driver model has been a win at every step
of the way. It's gone both ways; the driver core has changed
to work better with USB too.
- Dave
On Tue, Mar 15, 2005 at 01:14:40PM -0800, David Brownell wrote:
> That pre-driver model stuff went away in maybe 2.6.5 or so, I
> forget just when. If you think those changes can easily be
> reversed, I suggest you think again ... they enabled a LOT of
> likewise-overdue cleanups.
...
> converting to the driver model has been a win at every step
> of the way. It's gone both ways; the driver core has changed
> to work better with USB too.
Exactly my point: the driver code forces/encourages you to write better
code. With proper reference counting. And reverting this by making
"class_simple" default, and possibly doing a similar transition for struct
device and struct device_driver means that we lose this encouragement. and
we lose this win-win situation.
Thanks,
Dominik
On Tue, 15 Mar 2005 13:14:40 -0800, David Brownell <[email protected]> wrote:
> You still haven't answered my question. My observation was that
> only the class code can in any sense be called "new" ... so your
> blanket statement seemed to overlook several essential points!
>
> Which parts of the driver model were you thinking of?
>
> That pre-driver model stuff went away in maybe 2.6.5 or so, I
> forget just when.
I think I was shopping around for the examples of proper driver model
integration in 2.6.2 - 2.6.3 timeframe for the serio bus. I was
looking at how USB was working around the fact that one can not
add/remove children from the probe/remove methods. I can not tell you
what exactly gave me the impression that conversion is still in
progress, probably the comments like this:
/* FIXME should device_bind_driver() */
iface->driver = driver;
usb_set_intfdata(iface, priv);
return 0;
Now I see it was changed shortly after I looked there. And I see that
my impression was wrong, it _is_ tightly integrated with the driver
model now.
> If you think those changes can easily be
> reversed, I suggest you think again ... they enabled a LOT of
> likewise-overdue cleanups.
Note that I am arguing for keeping existing interface, not removing it.
--
Dmitry
On Tue, Mar 15, 2005 at 09:15:03PM +0100, Dominik Brodowski wrote:
> On Tue, Mar 15, 2005 at 11:34:15AM -0800, Greg KH wrote:
> > > And what about device_driver and device structure? Are they going to
> > > be changed over to be separately allocated linked objects?
> >
> > The driver stuff probably will be, and the device stuff possibly.
> > However, they are used by a very small ammount of core code (the bus
> > drivers), so changing that interface is not that important at this time.
>
> So this means every device will have yet another reference count, and you
> need to be aware of _each_ lifetime to write correct code. And the
> _reference counting_ is the hard thing to get right, so we should make
> _that_ easier. The existing class API was a step towards this direction, and
> with the changes you're suggesting here we'd do two jumps backwards.
You are correct, it was a step forward in this direction.
But we now have a kref to handle the reference counting for the device,
which make things a whole lot easier than ever before.
But the both of you are correct, there is a real need for the class code
to support trees of devices that are presented to userspace (which is
what the class code is for). I'm not taking that away, just trying to
make the interface to that code simpler.
I'm also not saying that I'm going to go off and delete those functions
from the kernel today, or tomorrow. Just that we need to slowly, over
time, make this easier to use, as it's too hard to do so today. I will
not be removing any functionality, don't worry :)
> > > If not then its enouther reason to keep original class interface -
> > > uniformity of driver model interface.
> >
> > Ease-of-use trumps uniformity
>
> Ease-of-use, maybe. However, it also means
> ease-of-getting-reference-counting-wrong. And reference counting trumps it
> all :)
It will not make the reference counting logic easier to get wrong, or
easier to get right. It totally takes it away from the user, and makes
them implement it themselves if they so wish (like the USB HCD patch
does.)
Anyway, don't worry, the code isn't going away anytime soon, we just
need to make it easier to use. Any suggestions that any of you have to
make this that way (as you are the ones who had to use it to start with)
would be greatly appreciated.
thanks,
greg k-h
On Tuesday 15 March 2005 2:05 pm, Dmitry Torokhov wrote:
> I think I was shopping around for the examples of proper driver model
> integration in 2.6.2 - 2.6.3 timeframe for the serio bus. I was
> looking at how USB was working around the fact that one can not
> add/remove children from the probe/remove methods.
Yes, the constraints on when children can be added/removed aren't
always useful. In fact they can seem nastily arbitrary, and some
of them need careful workarounds elsewhere in USB. The current
usbcore code should behave well in all common cases outside of
suspend/resume. (Yes, it's routine to unplug USB devices when
the system is suspended. That's not wholly worked around yet.
By the time World Domination is achieved, it'll be fixed!)
> I can not tell you
> what exactly gave me the impression that conversion is still in
> progress, probably the comments like this:
>
> /* FIXME should device_bind_driver() */
> iface->driver = driver;
> usb_set_intfdata(iface, priv);
> return 0;
>
> Now I see it was changed shortly after I looked there. And I see that
> my impression was wrong, it _is_ tightly integrated with the driver
> model now.
That was a FIXME that I added, and indeed it's now fixed. The
patch had a dry run around 2.6.0 but had some issues, and they
took time to fix/test properly given more pressing issues. The
drivers that used those APIs needed changes/retesting too. :)
> > If you think those changes can easily be
> > reversed, I suggest you think again ... they enabled a LOT of
> > likewise-overdue cleanups.
>
> Note that I am arguing for keeping existing interface, not removing it.
Those particular interfaces still exist even after Greg's patches
for the class support... he didn't change the physical device tree.
I tend to think the class support really does need to be treated
differently from the driver model core. It's all sort of separate
anyway, since class devices aren't "real" ones. They don't show up
in the physical device tree, and I understand the main reason to use
them is to mask that physical view behind a more "logical" view of
the hardware. Keeping those views separate makes sense to me,
although I've not paid close attention to class device support.
- Dave
> --
> Dmitry
>
On Tue, Mar 15, 2005 at 02:14:31PM -0800, Greg KH wrote:
> > So this means every device will have yet another reference count, and you
> > need to be aware of _each_ lifetime to write correct code. And the
> > _reference counting_ is the hard thing to get right, so we should make
> > _that_ easier. The existing class API was a step towards this direction, and
> > with the changes you're suggesting here we'd do two jumps backwards.
>
> You are correct, it was a step forward in this direction.
>
> But we now have a kref to handle the reference counting for the device,
> which make things a whole lot easier than ever before.
Is it really easier if you have to be aware of _both_ the class reference
possibly having reached zero yet and the kref device reference
possibly having reached zero yet? Using your approach, you need to take
_two_ lifetimes into account instead of one. Think of class device
attributes being opened / still being accessed when kref device reference
reaching zero... you need to check for that in code now, AFAICS, while you
could rely on "we still have a reference to the _device_" in "historic"
class device attribute access paths.
> But the both of you are correct, there is a real need for the class code
> to support trees of devices that are presented to userspace (which is
> what the class code is for). I'm not taking that away, just trying to
> make the interface to that code simpler.
The interface may get simpler, but we lose the advantages. And I prefer a
interface which reduces the chances of doing things wrongly; and at least
the existing warnings on empty release functions force you to _think_ about
what you do.
> I'm also not saying that I'm going to go off and delete those functions
> from the kernel today, or tomorrow.
...
> Anyway, don't worry, the code isn't going away anytime soon,
That's totally besides the point. If the decision was made to indeed do this
transition, I'd be all for doing this fast. If the "old" code was gone
within two weeks, I wouldn't care because of the short period, but because
of the functionality being lost:
> I will not be removing any functionality, don't worry :)
the "functionality" of the device core to teach, encourage, and forcing to
think of reference counting is being lost by this approach. Independent of
the question whether the transition will take two weeks or two years.
> It will not make the reference counting logic easier to get wrong, or
> easier to get right. It totally takes it away from the user, and makes
> them implement it themselves if they so wish (like the USB HCD patch
> does.).
Keeping the chance to do the "new"/class_simple way is a good thing -- so
that anybody who _knows_ _exactly_ what he does can shoot himself in his
foot^W^W^W^W^W do what is best for the affected code.
> we just
> need to make it easier to use. Any suggestions that any of you have to
> make this that way (as you are the ones who had to use it to start with)
> would be greatly appreciated.
drivers/base/class_simple.c:
....
printk("are you really sure you don't want not to have reference counting for free by using struct class instead of struct class_simple *?\n");
....
:)
Thanks,
Dominik
On Tuesday 15 March 2005 17:14, Greg KH wrote:
> > Ease-of-use, maybe. However, it also means
> > ease-of-getting-reference-counting-wrong. And reference counting trumps it
> > all :)
>
> It will not make the reference counting logic easier to get wrong, or
> easier to get right. ?It totally takes it away from the user, and makes
> them implement it themselves if they so wish (like the USB HCD patch
> does.)
Exactly, _IF_ they wish. And as practice shows proper reference counting
is not the very first thing people are concerned about.
I see the the new class interface useful in the following scenario:
- We have a proper subsystem that already does proper refcounting and
one might not want to consolidate core reference counting with
subsystems as it is too invasive.
If you consider the following scenario I do not think we want to
encourage it:
- We have "bad" system and user says "ah, I'll just use the new model
so I don't have to think about lifetime rules at the moment, I don't
have time/have something more interesting to do/I'll do that later
when I have time".
There is a third scenario:
- We have a completely new system or a system undergoing overhaul: the
coder tries to do it right and does consider all lifetime rules and
makes sure that all objects are properly accounted for. In this case
old interface is much more clear and easier to use than the new one.
I also not quite sure why a bus with its devices and drivers can be
implemented correctly (I believe we have a bunch of them now - PCI, USB,
serio, gameport) but class interface cannot be tamed?
--
Dmitry
On Tue, 15 Mar 2005 09:08:34 -0800, Greg KH <[email protected]> wrote:
> Hi all,
>
> There are 4 patches being posted here in response to this message that
> start us on the way toward cleaning up the driver model code so that
> it's actually usable by mere kernel developers :)
Is this going to let me make subdirectories in /sys/class/xxx
directories that generate hotplug events?
One example:
/sys/class/graphics/fb0/monitor(edid)
If the monitor is hotplugged the monitor directory will be
created/destroyed causing a hotplug event.
--
Jon Smirl
[email protected]
On Wed, Mar 16, 2005 at 06:16:19PM -0500, Jon Smirl wrote:
> On Tue, 15 Mar 2005 09:08:34 -0800, Greg KH <[email protected]> wrote:
> > Hi all,
> >
> > There are 4 patches being posted here in response to this message that
> > start us on the way toward cleaning up the driver model code so that
> > it's actually usable by mere kernel developers :)
>
> Is this going to let me make subdirectories in /sys/class/xxx
> directories that generate hotplug events?
Not yet, no, sorry.
> One example:
> /sys/class/graphics/fb0/monitor(edid)
>
> If the monitor is hotplugged the monitor directory will be
> created/destroyed causing a hotplug event.
That would be a good thing, on the todo list...
greg k-h
On Tue, Mar 15, 2005 at 02:14:31PM -0800, Greg KH wrote:
> It will not make the reference counting logic easier to get wrong, or
> easier to get right. It totally takes it away from the user, and makes
> them implement it themselves if they so wish (like the USB HCD patch
> does.)
Hi,
While looking more closely at your patches, I noticed the following race:
A) attribute is opened -> class_device's reference count is increased
B) usb/host/ohci-dbg.c::remove_debug_files() -- succeeds, as it doesn't check
class_device's reference count()
B) usb/core/hcd.c::usb_deregister_count() -- class_device_unregister doesn't
wait until class_device's reference count reaches zero, so
struct class_device still has "struct usb_bus *bus" saved as class_data
and continues to exist.
B) possibly the kref count of struct usb_bus reaches zero, and struct usb_bus *
is kfreed.
A) attribute is read -> e.g. usb/host/ohci-dbg.c::show_periodic()
bus = class_get_devdata(class_dev);
hcd = bus->hcpriv;
--> accessing kfree'd structure. Ooops.
A) ... [if it hadn't oopsed] attribute is closed, reference count reaches zero,
class_device is removed.
If both reference counts were kept unified (as with previous struct
class{,_device} design) this couldn't happen. The proper reference counting
for dynamically allocated objects and their "attributes" is _the_ advantage
of sysfs/driver model in favour of procfs.
Or am I missing something?
Thanks and Happy Easter,
Dominik