Skip to contentSkip to conversation

sysfs'ify video4linux

RFC/PATCH Gerd Knorr 6 participants 21 messages Jul 15, 2003 – Jul 21, 2003 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

Gerd Knorr

author
#0 lore raw

Hi,

This patch moves the video4linux subsystem from procfs to sysfs. Changes:

* procfs support is completely gone, i.e. /proc/video doesn't exist any more.

* there is a new device class instead: /sys/class/video4linux. All video4linux devices which used to be listed in /proc/video/dev are moved to that place.

Changes required/recommended in video4linux drivers:

* some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko and ov511.ko) use the video_proc_entry() to add additional procfs files. These drivers must be converted to sysfs too because video_proc_entry() doesn't exist any more.

* struct video_device has a new "dev" field pointing to a struct device. Drivers should fill that one if possible. It isn't required through. It will give you fancy device + driver symlinks in /sys/class/video4linux/<name>.
The patch below includes the changes for bttv.

Comments?

Gerd

drivers/media/video/videodev.c renamed from drivers/media/video/videodev.c.sysfs +32 -192
@@ -15,7 +15,6 @@
  *		- Added procfs support
  */
 
-#include <linux/config.h>
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/types.h>

@@ -35,33 +34,31 @@
 
 #include <linux/videodev.h>
 
-#define VIDEO_NUM_DEVICES	256 
+#define VIDEO_NUM_DEVICES	256
+#define VIDEO_NAME              "video4linux"
 
 /*
- *	Active devices 
+ *	sysfs stuff
  */
- 
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DECLARE_MUTEX(videodev_lock);
 
+static size_t show_name(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = cd->class_data;
+	return sprintf(buf,"%.*s\n",sizeof(vfd->name),vfd->name);
+}
 
-#ifdef CONFIG_VIDEO_PROC_FS
-
-#include <linux/proc_fs.h>
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 
-struct videodev_proc_data {
-	struct list_head proc_list;
-	char name[16];
-	struct video_device *vdev;
-	struct proc_dir_entry *proc_entry;
+static struct class video_class = {
+        .name = VIDEO_NAME,
 };
 
-static struct proc_dir_entry *video_dev_proc_entry = NULL;
-struct proc_dir_entry *video_proc_entry = NULL;
-EXPORT_SYMBOL(video_proc_entry);
-LIST_HEAD(videodev_proc_list);
-
-#endif /* CONFIG_VIDEO_PROC_FS */
+/*
+ *	Active devices 
+ */
+ 
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DECLARE_MUTEX(videodev_lock);
 
 struct video_device* video_devdata(struct file *file)
 {

@@ -219,156 +216,6 @@
 	return 0;
 }
 
-/*
- *	/proc support
- */
-
-#ifdef CONFIG_VIDEO_PROC_FS
-
-/* Hmm... i'd like to see video_capability information here, but
- * how can I access it (without changing the other drivers? -claudio
- */
-static int videodev_proc_read(char *page, char **start, off_t off,
-			       int count, int *eof, void *data)
-{
-	char *out = page;
-	struct video_device *vfd = data;
-	struct videodev_proc_data *d;
-	struct list_head *tmp;
-	int len;
-	char c = ' ';
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev)
-			break;
-	}
-
-	/* Sanity check */
-	if (tmp == &videodev_proc_list)
-		goto skip;
-		
-#define PRINT_VID_TYPE(x) do { if (vfd->type & x) \
-	out += sprintf (out, "%c%s", c, #x); c='|';} while (0)
-
-	out += sprintf (out, "name            : %s\n", vfd->name);
-	out += sprintf (out, "type            :");
-		PRINT_VID_TYPE(VID_TYPE_CAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_TUNER);
-		PRINT_VID_TYPE(VID_TYPE_TELETEXT);
-		PRINT_VID_TYPE(VID_TYPE_OVERLAY);
-		PRINT_VID_TYPE(VID_TYPE_CHROMAKEY);
-		PRINT_VID_TYPE(VID_TYPE_CLIPPING);
-		PRINT_VID_TYPE(VID_TYPE_FRAMERAM);
-		PRINT_VID_TYPE(VID_TYPE_SCALES);
-		PRINT_VID_TYPE(VID_TYPE_MONOCHROME);
-		PRINT_VID_TYPE(VID_TYPE_SUBCAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_ENCODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_ENCODER);
-	out += sprintf (out, "\n");
-	out += sprintf (out, "hardware        : 0x%x\n", vfd->hardware);
-#if 0
-	out += sprintf (out, "channels        : %d\n", d->vcap.channels);
-	out += sprintf (out, "audios          : %d\n", d->vcap.audios);
-	out += sprintf (out, "maxwidth        : %d\n", d->vcap.maxwidth);
-	out += sprintf (out, "maxheight       : %d\n", d->vcap.maxheight);
-	out += sprintf (out, "minwidth        : %d\n", d->vcap.minwidth);
-	out += sprintf (out, "minheight       : %d\n", d->vcap.minheight);
-#endif
-
-skip:
-	len = out - page;
-	len -= off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0)
-			return 0;
-	} else
-		len = count;
-
-	*start = page + off;
-
-	return len;
-}
-
-static void videodev_proc_create(void)
-{
-	video_proc_entry = create_proc_entry("video", S_IFDIR, &proc_root);
-
-	if (video_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video\n");
-		return;
-	}
-
-	video_proc_entry->owner = THIS_MODULE;
-	video_dev_proc_entry = create_proc_entry("dev", S_IFDIR, video_proc_entry);
-
-	if (video_dev_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video/dev\n");
-		return;
-	}
-
-	video_dev_proc_entry->owner = THIS_MODULE;
-}
-
-static void __exit videodev_proc_destroy(void)
-{
-	if (video_dev_proc_entry != NULL)
-		remove_proc_entry("dev", video_proc_entry);
-
-	if (video_proc_entry != NULL)
-		remove_proc_entry("video", &proc_root);
-}
-
-static void videodev_proc_create_dev (struct video_device *vfd, char *name)
-{
-	struct videodev_proc_data *d;
-	struct proc_dir_entry *p;
-
-	if (video_dev_proc_entry == NULL)
-		return;
-
-	d = kmalloc (sizeof (struct videodev_proc_data), GFP_KERNEL);
-	if (!d)
-		return;
-
-	p = create_proc_entry(name, S_IFREG|S_IRUGO|S_IWUSR, video_dev_proc_entry);
-	if (!p) {
-		kfree(d);
-		return;
-	}
-	p->data = vfd;
-	p->read_proc = videodev_proc_read;
-
-	d->proc_entry = p;
-	d->vdev = vfd;
-	strcpy (d->name, name);
-
-	/* How can I get capability information ? */
-
-	list_add (&d->proc_list, &videodev_proc_list);
-}
-
-static void videodev_proc_destroy_dev (struct video_device *vfd)
-{
-	struct list_head *tmp;
-	struct videodev_proc_data *d;
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev) {
-			remove_proc_entry(d->name, video_dev_proc_entry);
-			list_del (&d->proc_list);
-			kfree(d);
-			break;
-		}
-	}
-}
-
-#endif /* CONFIG_VIDEO_PROC_FS */
-
 extern struct file_operations video_fops;
 
 /**

@@ -456,15 +303,17 @@
 	devfs_mk_cdev(MKDEV(VIDEO_MAJOR, vfd->minor),
 			S_IFCHR | S_IRUSR | S_IWUSR, vfd->devfs_name);
 	init_MUTEX(&vfd->lock);
-	
-#ifdef CONFIG_VIDEO_PROC_FS
-{
-	char name[16];
-	sprintf(name, "%s%d", name_base, i - base);
-	videodev_proc_create_dev(vfd, name);
-}
-#endif
 
+	/* sysfs class */
+        memset(&vfd->class_dev, 0x00, sizeof(vfd->class_dev));
+	if (vfd->dev)
+		vfd->class_dev.dev = vfd->dev;
+	vfd->class_dev.class       = &video_class;
+	vfd->class_dev.class_data  = vfd;
+	strlcpy(vfd->class_dev.class_id, vfd->devfs_name + 4, BUS_ID_SIZE);
+	class_device_register(&vfd->class_dev);
+	class_device_create_file(&vfd->class_dev,
+				 &class_device_attr_name);
 	return 0;
 }
 

@@ -482,10 +331,7 @@
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");
 
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy_dev (vfd);
-#endif
-
+	class_device_unregister(&vfd->class_dev);
 	devfs_remove(vfd->devfs_name);
 	video_device[vfd->minor]=NULL;
 	up(&videodev_lock);

@@ -506,24 +352,18 @@
 static int __init videodev_init(void)
 {
 	printk(KERN_INFO "Linux video capture interface: v1.00\n");
-	if (register_chrdev(VIDEO_MAJOR,"video_capture", &video_fops)) {
+	if (register_chrdev(VIDEO_MAJOR,VIDEO_NAME, &video_fops)) {
 		printk("video_dev: unable to get major %d\n", VIDEO_MAJOR);
 		return -EIO;
 	}
-
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_create ();
-#endif
-	
+	class_register(&video_class);
 	return 0;
 }
 
 static void __exit videodev_exit(void)
 {
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy ();
-#endif
-	unregister_chrdev(VIDEO_MAJOR, "video_capture");
+	class_unregister(&video_class);
+	unregister_chrdev(VIDEO_MAJOR, VIDEO_NAME);
 }
 
 module_init(videodev_init)
drivers/media/video/bttv-driver.c renamed from drivers/media/video/bttv-driver.c.sysfs +6 -3
@@ -3413,11 +3413,14 @@
 	memcpy(&btv->radio_dev, &radio_template,      sizeof(radio_template));
 	memcpy(&btv->vbi_dev,   &bttv_vbi_template,   sizeof(bttv_vbi_template));
         btv->video_dev.minor = -1;
-	btv->video_dev.priv = btv;
+	btv->video_dev.priv  = btv;
+	btv->video_dev.dev   = &dev->dev;
         btv->radio_dev.minor = -1;
-	btv->radio_dev.priv = btv;
+	btv->radio_dev.priv  = btv;
+	btv->radio_dev.dev   = &dev->dev;
         btv->vbi_dev.minor = -1;
-	btv->vbi_dev.priv = btv;
+	btv->vbi_dev.priv  = btv;
+	btv->vbi_dev.dev   = &dev->dev;
 	btv->has_radio=radio[btv->nr];
 	
 	/* pci stuff (init, get irq/mmip, ... */
include/linux/videodev.h renamed from include/linux/videodev.h.sysfs +3 -0
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/version.h>
+#include <linux/device.h>
 
 #if 1
 /*

@@ -24,6 +25,7 @@
 struct video_device
 {
 	struct module *owner;
+	struct device *dev;
      	char name[32];
  	int type;       /* v4l1 */
  	int type2;      /* v4l2 */

@@ -39,6 +41,7 @@
 	int users;
 	struct semaphore lock;
 	char devfs_name[64];	/* devfs */
+	struct class_device class_dev;
 };
 
 #define VIDEO_MAJOR	81
#1 lore raw

Hey Gerd,

Quoted 1 line · Gerd Knorr

This patch moves the video4linux subsystem from procfs to sysfs.

[..cool stuff, especially the driver integration fanciness with the device struct stuff..]

Quoted 1 line

Comments?

Is /proc going away alltogether?

(I'm not actively following 2.5.x development; I currently can't get any computer here boot up any recent (>=2.5.72) 2.5 kernel alltogether, see LKML for details...)

Ronald

Signature · 2 lines
Ronald Bultje <[email protected]>
Linux Video/Multimedia developer
#2 lore raw
Quoted 9 lines · Ronald Bultje

Hey Gerd,

Quoted 1 line · Gerd Knorr

This patch moves the video4linux subsystem from procfs to sysfs.

[..cool stuff, especially the driver integration fanciness with the device struct stuff..]

Quoted 1 line

Comments?

Is /proc going away alltogether?

Well, it does need to provide per-process info like it was originally intended. It's all the device crap that doesn't belong there that is moving.

Regards,

Signature · 2 lines
Matt Porter
[email protected]
#3 lore raw
In reply to Gerd Knorr
Quoted 7 lines · Gerd Knorr

Hi,

This patch moves the video4linux subsystem from procfs to sysfs. Changes:

* procfs support is completely gone, i.e. /proc/video doesn't exist any more.

Yeah!

Quoted 3 lines

* there is a new device class instead: /sys/class/video4linux. All video4linux devices which used to be listed in /proc/video/dev are moved to that place.

Nice.

Quoted 6 lines

Changes required/recommended in video4linux drivers:

* some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko and ov511.ko) use the video_proc_entry() to add additional procfs files. These drivers must be converted to sysfs too because video_proc_entry() doesn't exist any more.

I'd be glad to do this work once your change makes it into the core. Is there any need for these drivers to export anything through sysfs now instead of /proc? From what I remember, it only looked like debugging and other general info stuff.

Quoted 5 lines

* struct video_device has a new "dev" field pointing to a struct device. Drivers should fill that one if possible. It isn't required through. It will give you fancy device + driver symlinks in /sys/class/video4linux/<name>.
The patch below includes the changes for bttv.

So dev should point to the dev of the video class device?

Quoted 1 line

Comments?

You _have_ to set up a release function for your class device. You can't just kfree it like I think you are doing, otherwise any users of the sysfs files will oops the kernel after the video class device is gone. I can point you to some examples of how to do this if you like.

Other than that, how about exporting the dev_t value for the video device? Then you automatically get udev support, and I don't have to go add it to this code later :)

thanks,

greg k-h

author
#4 lore raw
Quoted 9 lines
Quoted 4 lines

* some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko and ov511.ko) use the video_proc_entry() to add additional procfs files. These drivers must be converted to sysfs too because video_proc_entry() doesn't exist any more.

I'd be glad to do this work once your change makes it into the core. Is there any need for these drivers to export anything through sysfs now instead of /proc? From what I remember, it only looked like debugging and other general info stuff.

IIRC some tuning / debugging / info stuff, the drivers should work just fine without. Temporarely disableling that wouldn't be a big issue IMHO.

Quoted 1 line

So dev should point to the dev of the video class device?

Exactly. videodev.o will put that into class_device->dev which in turn will produce these symlinks:

eskarina kraxel /sys/class/video4linux/video0# ll device
lrwxrwxrwx 1 root root 53 Jul 15 17:20 device -> ../../../devices/pci0000:00/0000:00:06.0/0000:02:07.0/

Quoted 6 lines
Quoted 1 line

Comments?

You _have_ to set up a release function for your class device. You can't just kfree it like I think you are doing, otherwise any users of the sysfs files will oops the kernel after the video class device is gone.

class_device_unregister() is called from video_unregister_device() which looks fine to me. Or do you talk about something else?

Quoted 3 lines

Other than that, how about exporting the dev_t value for the video device? Then you automatically get udev support, and I don't have to go add it to this code later :)

Do you have a pointer to sample code for that?

Gerd

Signature · 1 line
sigfault
#5 lore raw
Quoted 9 lines · Gerd Knorr
Quoted 6 lines
Quoted 1 line

Comments?

You _have_ to set up a release function for your class device. You can't just kfree it like I think you are doing, otherwise any users of the sysfs files will oops the kernel after the video class device is gone.

class_device_unregister() is called from video_unregister_device() which looks fine to me. Or do you talk about something else?

Heh, yes, that is correct. But then what do you do with the object that had the class_device in it after class_device_unregister() returns? I don't see you freeing the device directly, which is good, but who does?

You _have_ to let the structure that the kobject is in free itself in a release() function, you can't guess at what the kobject reference count is at any time and just assume you can throw it away at that moment.

LWN did an article about how to do this properly at:

	http://lwn.net/Articles/36850/

But for some code that does this in a simple manner, look at drivers/char/tty_io.c, the release_tty_dev() function. It's set up in the tty_class to handle destroying the structure when the last reference to the tty_dev structure goes away. Look at the tty_remove_class_device() to see that the structure isn't kfreed on it's own.

It looks like the video drivers include a "struct video_device" structure within their own structures, right? That will have to be changed to a pointer to that structure in order for the lifetime rules to work properly. I know Hanna is fighting this same battle with the input layer right now...

Does this help out?

As an example of why you _have_ to do this, consider a user who opens a sysfs file of a video device, and then removes the video device from the system. Then they do a read on the sysfs file. Oops...

Quoted 5 lines
Quoted 3 lines

Other than that, how about exporting the dev_t value for the video device? Then you automatically get udev support, and I don't have to go add it to this code later :)

Do you have a pointer to sample code for that?

Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in /sys/class/usb/*, and so on...

I need to make a general function to support this to make it easier for everyone to export a dev_t to userspace, but until then, if you want to look at the show_dev() function in drivers/usb/core/file.c it shows what you need to do.

thanks,

greg k-h

author
#6 lore raw
Quoted 2 lines

It looks like the video drivers include a "struct video_device" structure within their own structures, right?

Yes, it is allocated/freed by the driver, most seem to simply include one ore more "struct video_device" somewhere in the per-device struct.

Quoted 2 lines

That will have to be changed to a pointer to that structure in order for the lifetime rules to work properly.

Hmm. I doubt it will be that simple. struct video_device has a priv field which can be used by the drivers to hook in some driver-private data. That may point into nowhere if struct video_device has a longer live time due to some kobject still being referenced. Wouldn't be a issue for videodev.o itself, but might become a problem for drivers which want add private properties and rely on video_device->priv for finding the per-device data. Problem isn't solved but justed moved to the next corner ...

Maybe let video_unregister sleep on a semaphore which gets woken up by the release function? That should make sure the sysfs objects are not referenced any more if video_unregister() returns. I use a similar method in some places when shutting down kernel threads, to make sure it is really stopped before rmmod frees the memory.

Quoted 2 lines

Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in /sys/class/usb/*, and so on...

I've found the code in drivers/block/genhd.c in the meantime :)

Gerd

Signature · 1 line
sigfault
#7 lore raw
Quoted 5 lines · Gerd Knorr
Quoted 2 lines

It looks like the video drivers include a "struct video_device" structure within their own structures, right?

Yes, it is allocated/freed by the driver, most seem to simply include one ore more "struct video_device" somewhere in the per-device struct.

So you CAN NOT just blindly put a kobject (meaning a class_device) structure inside of there.

Quoted 11 lines
Quoted 2 lines

That will have to be changed to a pointer to that structure in order for the lifetime rules to work properly.

Hmm. I doubt it will be that simple. struct video_device has a priv field which can be used by the drivers to hook in some driver-private data. That may point into nowhere if struct video_device has a longer live time due to some kobject still being referenced. Wouldn't be a issue for videodev.o itself, but might become a problem for drivers which want add private properties and rely on video_device->priv for finding the per-device data. Problem isn't solved but justed moved to the next corner ...

No, just have the video drivers have a release callback to do the freeing. It's pretty simple, look at usb_host_release() in drivers/usb/core/hcd.c. That's exactly what that is for. Hm, I shouldn't have to check for bus->release() there, as release is now required...

Quoted 5 lines

Maybe let video_unregister sleep on a semaphore which gets woken up by the release function? That should make sure the sysfs objects are not referenced any more if video_unregister() returns. I use a similar method in some places when shutting down kernel threads, to make sure it is really stopped before rmmod frees the memory.

Ick, no. Try doing what I did for usb hosts, it's much simpler.

Quoted 4 lines
Quoted 2 lines

Look at the dev file in /sys/class/tty/*, or in /sys/block/hd* or in /sys/class/usb/*, and so on...

I've found the code in drivers/block/genhd.c in the meantime :)

genhd.c uses "raw" kobjects. It might be easier to look at a class_device example like the above mentioned usb_host one.

If you have any other questions/problems, feel free to ask.

thanks,

greg k-h

author
#8 lore raw
Quoted 6 lines · Greg KH
Quoted 2 lines · Gerd Knorr

Yes, it is allocated/freed by the driver, most seem to simply include one ore more "struct video_device" somewhere in the per-device struct.

So you CAN NOT just blindly put a kobject (meaning a class_device) structure inside of there.

Why not ...

Quoted 6 lines
Quoted 3 lines

which want add private properties and rely on video_device->priv for finding the per-device data. Problem isn't solved but justed moved to the next corner ...

No, just have the video drivers have a release callback to do the freeing.

... if a ->release() callback is required anyway to fix it? I see two ways to handle it:

(1) mandatory ->release() callback, drivers must make sure the stuff is not freed before the callback was called. In that case the class_device can be left embedded inside the drivers provate structs.
(2) optional ->release() callback (for those drivers which want add private attributes), "struct video_device" must be moved out of the drivers private structs then and released in a new function (which also calls the drivers ->release callback if present). Should probably also be allocated by videodev.c for symmetry.

Both approaches require touching all v4l drivers in non-trivial ways through, not sure whenever it is a good idea to do that now. Any chance to get that in before 2.6.0? Or should I better make that change in 2.7.x and live with /proc for the time being?

Gerd

Signature · 1 line
sigfault
#9 lore raw
Quoted 17 lines · Gerd Knorr
Quoted 6 lines · Greg KH
Quoted 2 lines · Gerd Knorr

Yes, it is allocated/freed by the driver, most seem to simply include one ore more "struct video_device" somewhere in the per-device struct.

So you CAN NOT just blindly put a kobject (meaning a class_device) structure inside of there.

Why not ...

Quoted 6 lines
Quoted 3 lines

which want add private properties and rely on video_device->priv for finding the per-device data. Problem isn't solved but justed moved to the next corner ...

No, just have the video drivers have a release callback to do the freeing.

Quoted 7 lines

... if a ->release() callback is required anyway to fix it? I see two ways to handle it:

(1) mandatory ->release() callback, drivers must make sure the stuff is not freed before the callback was called. In that case the class_device can be left embedded inside the drivers provate structs.

That sounds fine, and is the same as what I did for usb host devices.

Quoted 5 lines

(2) optional ->release() callback (for those drivers which want add private attributes), "struct video_device" must be moved out of the drivers private structs then and released in a new function (which also calls the drivers ->release callback if present). Should probably also be allocated by videodev.c for symmetry.

That's "prettier", but probably requires a lot more work in every v4l driver, right?

Quoted 4 lines

Both approaches require touching all v4l drivers in non-trivial ways through, not sure whenever it is a good idea to do that now. Any chance to get that in before 2.6.0? Or should I better make that change in 2.7.x and live with /proc for the time being?

I'd say you should try for it now. It's required if you want support for things like udev in 2.6, which is what a lot of people seem to want...

I know there are still a few other subsystems that need this kind of change in 2.6, so you are not alone (input, misc, parallel port, sound, etc.)

I'd be glad to help you out with this if you want.

thanks,

greg k-h

author
#10 lore raw
Quoted 9 lines
Quoted 7 lines

... if a ->release() callback is required anyway to fix it? I see two ways to handle it:

(1) mandatory ->release() callback, drivers must make sure the stuff is not freed before the callback was called. In that case the class_device can be left embedded inside the drivers provate structs.

That sounds fine, and is the same as what I did for usb host devices.

Ok.

Quoted 8 lines
Quoted 5 lines

(2) optional ->release() callback (for those drivers which want add private attributes), "struct video_device" must be moved out of the drivers private structs then and released in a new function (which also calls the drivers ->release callback if present). Should probably also be allocated by videodev.c for symmetry.

That's "prettier", but probably requires a lot more work in every v4l driver, right?

Not sure which of them is actually more work.

Version (1) can be done without breaking the build, with a hack along the lines "if (no release callback) printk(KERN_WARN please fix your driver)", so the drivers can be fixed step-by-step afterwards.

Fixing the drivers might be non-trivial through. Some drivers are hot-pluggable (usb cams), some drivers register more than one v4l device (bttv wants up to three). Those drivers can't just call kfree() in the ->release callback because there might be other references, some open file handle for a unplugged usb cam, another not-yet unregistered v4l device, whatever. Probably they must implement some reference counting scheme to get that right. The very simple ones (in drivers/media/radio for example) likely just need the kfree() moved from the ->remove function into the new ->release() callback.

Version (2) needs every v4l driver touched to make it compile again. Not sure how hard it is to fixup them. I'd guess it is pretty straigt forward to do the conversion, it's just alot of work. I also could do a number of other cleanups along the way. Maybe I trap into some hidden pitfalls through, havn't looked at all the drivers in detail yet.

Gerd

Signature · 1 line
sigfault
#11 lore raw
Quoted 4 lines · Gerd Knorr

Version (1) can be done without breaking the build, with a hack along the lines "if (no release callback) printk(KERN_WARN please fix your driver)", so the drivers can be fixed step-by-step afterwards.

That sounds like a nice way to start.

Quoted 9 lines

Fixing the drivers might be non-trivial through. Some drivers are hot-pluggable (usb cams), some drivers register more than one v4l device (bttv wants up to three). Those drivers can't just call kfree() in the ->release callback because there might be other references, some open file handle for a unplugged usb cam, another not-yet unregistered v4l device, whatever. Probably they must implement some reference counting scheme to get that right. The very simple ones (in drivers/media/radio for example) likely just need the kfree() moved from the ->remove function into the new ->release() callback.

I don't think it will be that bad for them. Just have them change the v4l device from being a structure included in their structure, into a pointer, and then create it before registering, and free it in the release() callback.

That being said, I haven't actually looked at the code to verify that this is all that is needed :)

Quoted 5 lines

Version (2) needs every v4l driver touched to make it compile again. Not sure how hard it is to fixup them. I'd guess it is pretty straigt forward to do the conversion, it's just alot of work. I also could do a number of other cleanups along the way. Maybe I trap into some hidden pitfalls through, havn't looked at all the drivers in detail yet.

Breaking the build is a very good thing to do at times, to ensure that stuff gets fixed properly. Users might go for a while without realizing that there really is a problem in their driver.

But in the end, it's up to you...

thanks,

greg k-h

author
#12 lore raw
Quoted 41 lines · Greg KH
Quoted 4 lines · Gerd Knorr

Version (1) can be done without breaking the build, with a hack along the lines "if (no release callback) printk(KERN_WARN please fix your driver)", so the drivers can be fixed step-by-step afterwards.

That sounds like a nice way to start.

Quoted 9 lines

Fixing the drivers might be non-trivial through. Some drivers are hot-pluggable (usb cams), some drivers register more than one v4l device (bttv wants up to three). Those drivers can't just call kfree() in the ->release callback because there might be other references, some open file handle for a unplugged usb cam, another not-yet unregistered v4l device, whatever. Probably they must implement some reference counting scheme to get that right. The very simple ones (in drivers/media/radio for example) likely just need the kfree() moved from the ->remove function into the new ->release() callback.

I don't think it will be that bad for them. Just have them change the v4l device from being a structure included in their structure, into a pointer, and then create it before registering, and free it in the release() callback.

That being said, I haven't actually looked at the code to verify that this is all that is needed :)

Quoted 5 lines

Version (2) needs every v4l driver touched to make it compile again. Not sure how hard it is to fixup them. I'd guess it is pretty straigt forward to do the conversion, it's just alot of work. I also could do a number of other cleanups along the way. Maybe I trap into some hidden pitfalls through, havn't looked at all the drivers in detail yet.

Breaking the build is a very good thing to do at times, to ensure that stuff gets fixed properly. Users might go for a while without realizing that there really is a problem in their driver.

But in the end, it's up to you...

thanks,

greg k-h

Signature · 1 line
sigfault

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#14 lore raw
Quoted 2 lines

Hm, it just looks like you quoted my message and didn't write anything new. Did something not go through correctly?

:-/ ... I should learn to use my mailer correctly ... Should have been that one:

==============================[ cut here ]==============================
To: Greg KH <[email protected]>
Subject: Re: [RFC/PATCH] sysfs'ify video4linux
In-Reply-To: <[email protected]>

Quoted 10 lines
Quoted 3 lines

Version (1) can be done without breaking the build, with a hack along the lines "if (no release callback) printk(KERN_WARN please fix your driver)", so the drivers can be fixed step-by-step afterwards.

That sounds like a nice way to start.

I don't think it will be that bad for them. Just have them change the v4l device from being a structure included in their structure, into a pointer, and then create it before registering, and free it in the release() callback.

Good point. So the mandatory ->release() callback version is also the more flexible one. I like that :)

Quoted 5 lines

Breaking the build is a very good thing to do at times, to ensure that stuff gets fixed properly. Users might go for a while without realizing that there really is a problem in their driver.

But in the end, it's up to you...

Breaking the build _right now_ with 2.6 becoming stable is IMHO not a good idea, I think I better try to avoid that until 2.7.

New patch will come later today or early next week.

Gerd

Signature · 1 line
sigfault
#15 lore raw

Hi Gerd,

Quoted 2 lines · Gerd Knorr

Breaking the build _right now_ with 2.6 becoming stable is IMHO not a good idea, I think I better try to avoid that until 2.7.

The build is broken for quite some v4l drivers already (zr36120, zr36067, the mpeg card, afaik), and really, the ones not in the kernel will not officially "support" 2.6.x anyway before it's stable. It shouldn't be that much work. I'd suggest to try to get it in before 2.6.0. This whole "we'll now turn number 5->6 and everything will be stable" is there to help developers in releasing a new stable version, not to prevent them from innovating and improving. I mean, this won't actually be a totally new subsystem like the VM that needs to be debugged in the next few months just after we've release 2.6.0, right? It's just a small move. Fixing the drivers shouldn't be hard, and if we/you ("we" as in the v4l driver developers) synchronize that nicely with each other, there shouldn't be an actual problem.

(Related, I [finally] got 2.6.0-test1 to work so a new patch for my driver is coming up in one of the next few days; please, someone kick it forward to Linus then).

Ronald

Signature · 1 line
Ronald Bultje <[email protected]>
author
#16 lore raw

[RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core

In reply to Gerd Knorr
Quoted 1 line

New patch will come later today or early next week.

Here we go. Changes:

* dropped /proc support from videodev.c
* added sysfs support to videodev.c
* added a number of helper functions for v4l
drivers.

v4l drivers will continue to work, but must be adapted to be race-free[tm]. videodev.o will print a warning on not-yet fixed drivers.

Gerd

==============================[ cut here ]==============================

drivers/media/video/videodev.c +76 -191
diff -u linux-2.6.0-test1/drivers/media/video/videodev.c linux/drivers/media/video/videodev.c
@@ -15,7 +15,6 @@
  *		- Added procfs support
  */
 
-#include <linux/config.h>
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/types.h>

@@ -28,6 +27,7 @@
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 #include <linux/devfs_fs_kernel.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -35,33 +35,67 @@
 
 #include <linux/videodev.h>
 
-#define VIDEO_NUM_DEVICES	256 
+#define VIDEO_NUM_DEVICES	256
+#define VIDEO_NAME              "video4linux"
 
 /*
- *	Active devices 
+ *	sysfs stuff
  */
- 
-static struct video_device *video_device[VIDEO_NUM_DEVICES];
-static DECLARE_MUTEX(videodev_lock);
 
+static ssize_t show_name(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = to_video_device(cd);
+	return sprintf(buf,"%.*s\n",(int)sizeof(vfd->name),vfd->name);
+}
+
+static ssize_t show_dev(struct class_device *cd, char *buf)
+{
+	struct video_device *vfd = to_video_device(cd);
+	dev_t dev = MKDEV(VIDEO_MAJOR, vfd->minor);
+	return sprintf(buf,"%04x\n",(int)dev);
+}
 
-#ifdef CONFIG_VIDEO_PROC_FS
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static CLASS_DEVICE_ATTR(dev,  S_IRUGO, show_dev, NULL);
 
-#include <linux/proc_fs.h>
+struct video_device *video_device_alloc(void)
+{
+	struct video_device *vfd;
 
-struct videodev_proc_data {
-	struct list_head proc_list;
-	char name[16];
-	struct video_device *vdev;
-	struct proc_dir_entry *proc_entry;
-};
+	vfd = kmalloc(sizeof(*vfd),GFP_KERNEL);
+	if (NULL == vfd)
+		return NULL;
+	memset(vfd,0,sizeof(*vfd));
+	return vfd;
+}
+
+void video_device_release(struct video_device *vfd)
+{
+	kfree(vfd);
+}
+
+static void video_release(struct class_device *cd)
+{
+	struct video_device *vfd = container_of(cd, struct video_device, class_dev);
 
-static struct proc_dir_entry *video_dev_proc_entry = NULL;
-struct proc_dir_entry *video_proc_entry = NULL;
-EXPORT_SYMBOL(video_proc_entry);
-LIST_HEAD(videodev_proc_list);
+#if 1 /* needed until all drivers are fixed */
+	if (!vfd->release)
+		return;
+#endif
+	vfd->release(vfd);
+}
 
-#endif /* CONFIG_VIDEO_PROC_FS */
+static struct class video_class = {
+        .name    = VIDEO_NAME,
+	.release = video_release,
+};
+
+/*
+ *	Active devices 
+ */
+ 
+static struct video_device *video_device[VIDEO_NUM_DEVICES];
+static DECLARE_MUTEX(videodev_lock);
 
 struct video_device* video_devdata(struct file *file)
 {

@@ -219,156 +253,6 @@
 	return 0;
 }
 
-/*
- *	/proc support
- */
-
-#ifdef CONFIG_VIDEO_PROC_FS
-
-/* Hmm... i'd like to see video_capability information here, but
- * how can I access it (without changing the other drivers? -claudio
- */
-static int videodev_proc_read(char *page, char **start, off_t off,
-			       int count, int *eof, void *data)
-{
-	char *out = page;
-	struct video_device *vfd = data;
-	struct videodev_proc_data *d;
-	struct list_head *tmp;
-	int len;
-	char c = ' ';
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev)
-			break;
-	}
-
-	/* Sanity check */
-	if (tmp == &videodev_proc_list)
-		goto skip;
-		
-#define PRINT_VID_TYPE(x) do { if (vfd->type & x) \
-	out += sprintf (out, "%c%s", c, #x); c='|';} while (0)
-
-	out += sprintf (out, "name            : %s\n", vfd->name);
-	out += sprintf (out, "type            :");
-		PRINT_VID_TYPE(VID_TYPE_CAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_TUNER);
-		PRINT_VID_TYPE(VID_TYPE_TELETEXT);
-		PRINT_VID_TYPE(VID_TYPE_OVERLAY);
-		PRINT_VID_TYPE(VID_TYPE_CHROMAKEY);
-		PRINT_VID_TYPE(VID_TYPE_CLIPPING);
-		PRINT_VID_TYPE(VID_TYPE_FRAMERAM);
-		PRINT_VID_TYPE(VID_TYPE_SCALES);
-		PRINT_VID_TYPE(VID_TYPE_MONOCHROME);
-		PRINT_VID_TYPE(VID_TYPE_SUBCAPTURE);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MPEG_ENCODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_DECODER);
-		PRINT_VID_TYPE(VID_TYPE_MJPEG_ENCODER);
-	out += sprintf (out, "\n");
-	out += sprintf (out, "hardware        : 0x%x\n", vfd->hardware);
-#if 0
-	out += sprintf (out, "channels        : %d\n", d->vcap.channels);
-	out += sprintf (out, "audios          : %d\n", d->vcap.audios);
-	out += sprintf (out, "maxwidth        : %d\n", d->vcap.maxwidth);
-	out += sprintf (out, "maxheight       : %d\n", d->vcap.maxheight);
-	out += sprintf (out, "minwidth        : %d\n", d->vcap.minwidth);
-	out += sprintf (out, "minheight       : %d\n", d->vcap.minheight);
-#endif
-
-skip:
-	len = out - page;
-	len -= off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0)
-			return 0;
-	} else
-		len = count;
-
-	*start = page + off;
-
-	return len;
-}
-
-static void videodev_proc_create(void)
-{
-	video_proc_entry = create_proc_entry("video", S_IFDIR, &proc_root);
-
-	if (video_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video\n");
-		return;
-	}
-
-	video_proc_entry->owner = THIS_MODULE;
-	video_dev_proc_entry = create_proc_entry("dev", S_IFDIR, video_proc_entry);
-
-	if (video_dev_proc_entry == NULL) {
-		printk("video_dev: unable to initialise /proc/video/dev\n");
-		return;
-	}
-
-	video_dev_proc_entry->owner = THIS_MODULE;
-}
-
-static void __exit videodev_proc_destroy(void)
-{
-	if (video_dev_proc_entry != NULL)
-		remove_proc_entry("dev", video_proc_entry);
-
-	if (video_proc_entry != NULL)
-		remove_proc_entry("video", &proc_root);
-}
-
-static void videodev_proc_create_dev (struct video_device *vfd, char *name)
-{
-	struct videodev_proc_data *d;
-	struct proc_dir_entry *p;
-
-	if (video_dev_proc_entry == NULL)
-		return;
-
-	d = kmalloc (sizeof (struct videodev_proc_data), GFP_KERNEL);
-	if (!d)
-		return;
-
-	p = create_proc_entry(name, S_IFREG|S_IRUGO|S_IWUSR, video_dev_proc_entry);
-	if (!p) {
-		kfree(d);
-		return;
-	}
-	p->data = vfd;
-	p->read_proc = videodev_proc_read;
-
-	d->proc_entry = p;
-	d->vdev = vfd;
-	strcpy (d->name, name);
-
-	/* How can I get capability information ? */
-
-	list_add (&d->proc_list, &videodev_proc_list);
-}
-
-static void videodev_proc_destroy_dev (struct video_device *vfd)
-{
-	struct list_head *tmp;
-	struct videodev_proc_data *d;
-
-	list_for_each (tmp, &videodev_proc_list) {
-		d = list_entry(tmp, struct videodev_proc_data, proc_list);
-		if (vfd == d->vdev) {
-			remove_proc_entry(d->name, video_dev_proc_entry);
-			list_del (&d->proc_list);
-			kfree(d);
-			break;
-		}
-	}
-}
-
-#endif /* CONFIG_VIDEO_PROC_FS */
-
 extern struct file_operations video_fops;
 
 /**

@@ -456,15 +340,23 @@
 	devfs_mk_cdev(MKDEV(VIDEO_MAJOR, vfd->minor),
 			S_IFCHR | S_IRUSR | S_IWUSR, vfd->devfs_name);
 	init_MUTEX(&vfd->lock);
-	
-#ifdef CONFIG_VIDEO_PROC_FS
-{
-	char name[16];
-	sprintf(name, "%s%d", name_base, i - base);
-	videodev_proc_create_dev(vfd, name);
-}
-#endif
 
+	/* sysfs class */
+        memset(&vfd->class_dev, 0x00, sizeof(vfd->class_dev));
+	if (vfd->dev)
+		vfd->class_dev.dev = vfd->dev;
+	vfd->class_dev.class       = &video_class;
+	strlcpy(vfd->class_dev.class_id, vfd->devfs_name + 4, BUS_ID_SIZE);
+	class_device_register(&vfd->class_dev);
+	video_device_create_file(vfd, &class_device_attr_name);
+	video_device_create_file(vfd, &class_device_attr_dev);
+
+#if 1 /* needed until all drivers are fixed */
+	if (!vfd->release)
+		printk(KERN_WARNING "videodev: \"%s\" has no release callback. "
+		       "Please fix your driver for proper sysfs support, see "
+		       "http://lwn.net/Articles/36850/\n", vfd->name);
+#endif
 	return 0;
 }
 

@@ -482,10 +374,7 @@
 	if(video_device[vfd->minor]!=vfd)
 		panic("videodev: bad unregister");
 
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy_dev (vfd);
-#endif
-
+	class_device_unregister(&vfd->class_dev);
 	devfs_remove(vfd->devfs_name);
 	video_device[vfd->minor]=NULL;
 	up(&videodev_lock);

@@ -506,24 +395,18 @@
 static int __init videodev_init(void)
 {
 	printk(KERN_INFO "Linux video capture interface: v1.00\n");
-	if (register_chrdev(VIDEO_MAJOR,"video_capture", &video_fops)) {
+	if (register_chrdev(VIDEO_MAJOR,VIDEO_NAME, &video_fops)) {
 		printk("video_dev: unable to get major %d\n", VIDEO_MAJOR);
 		return -EIO;
 	}
-
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_create ();
-#endif
-	
+	class_register(&video_class);
 	return 0;
 }
 
 static void __exit videodev_exit(void)
 {
-#ifdef CONFIG_VIDEO_PROC_FS
-	videodev_proc_destroy ();
-#endif
-	unregister_chrdev(VIDEO_MAJOR, "video_capture");
+	class_unregister(&video_class);
+	unregister_chrdev(VIDEO_MAJOR, VIDEO_NAME);
 }
 
 module_init(videodev_init)

@@ -535,6 +418,8 @@
 EXPORT_SYMBOL(video_usercopy);
 EXPORT_SYMBOL(video_exclusive_open);
 EXPORT_SYMBOL(video_exclusive_release);
+EXPORT_SYMBOL(video_device_alloc);
+EXPORT_SYMBOL(video_device_release);
 
 MODULE_AUTHOR("Alan Cox");
 MODULE_DESCRIPTION("Device registrar for Video4Linux drivers");
include/linux/videodev.h +47 -20

100 lines not on this page · raw message

author
#17 lore raw

[RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver

Hi,

This patch adapts the bttv driver to the recent videodev changes. struct video_device is now dynamically allocated and freed on ->release() to fix sysfs race. Also adds a private sysfs driver attribute file.

Gerd

==============================[ cut here ]==============================

drivers/media/video/bttv-cards.c +1 -5
diff -u linux-2.6.0-test1/drivers/media/video/bttv-cards.c linux/drivers/media/video/bttv-cards.c
@@ -1851,12 +1851,8 @@
 		btv->type=card[btv->nr];
 	
 	/* print which card config we are using */
-	sprintf(btv->video_dev.name,"BT%d%s(%.23s)",
-		btv->id,
-		(btv->id==848 && btv->revision==0x12) ? "A" : "",
-		bttv_tvcards[btv->type].name);
 	printk(KERN_INFO "bttv%d: using: %s [card=%d,%s]\n",btv->nr,
-	       btv->video_dev.name,btv->type,
+	       bttv_tvcards[btv->type].name, btv->type,
 	       card[btv->nr] < bttv_num_tvcards
 	       ? "insmod option" : "autodetected");
drivers/media/video/bttv-driver.c +99 -38

217 lines not on this page · raw message

drivers/media/video/bttvp.h +3 -3
diff -u linux-2.6.0-test1/drivers/media/video/bttvp.h linux/drivers/media/video/bttvp.h
@@ -276,9 +276,9 @@
 	int                        i2c_state, i2c_rc;
 
 	/* video4linux (1) */
-	struct video_device video_dev;
-	struct video_device radio_dev;
-	struct video_device vbi_dev;
+	struct video_device *video_dev;
+	struct video_device *radio_dev;
+	struct video_device *vbi_dev;
 
 	/* locking */
 	spinlock_t s_lock;
#18 lore raw

Re: [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core

In reply to Gerd Knorr
Quoted 4 lines · Gerd Knorr
Quoted 1 line

New patch will come later today or early next week.

Here we go. Changes:

Quoted 8 lines

* dropped /proc support from videodev.c
* added sysfs support to videodev.c
* added a number of helper functions for v4l
drivers.

v4l drivers will continue to work, but must be adapted to be race-free[tm]. videodev.o will print a warning on not-yet fixed drivers.

Very nice, looks good to me. Thanks for putting up with the lack of documentation for some of this :)

thanks,

greg k-h

#19 lore raw
In reply to Greg KH
Quoted 5 lines · Greg KH
Quoted 2 lines · Gerd Knorr

Changes required/recommended in video4linux drivers:

Quoted 9 lines
Quoted 6 lines

* some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko and ov511.ko) use the video_proc_entry() to add additional procfs files. These drivers must be converted to sysfs too because video_proc_entry() doesn't exist any more.

I'd be glad to do this work once your change makes it into the core.

I can do the ov511 work if you want. I can have it done in two days (or less).

Quoted 3 lines

Is there any need for these drivers to export anything through sysfs now instead of /proc?

Yes, at least with ov511. Some of the info that it puts in /proc is no longer necessary. However, there are various bits of hardware info that still need to get to userspace, for scripts that need to tell otherwise identical (same VID/PID/revision) cameras apart when creating /dev nodes.

Is there a convention for naming driver-specific files in /sys? E.g.: ov511_foo, _foo, etc...? I don't want to pollute the namespace.

Quoted 2 lines

From what I remember, it only looked like debugging and other general info stuff.

There *is* some debugging stuff I would like to keep, at least for now. Approximately half of the bug reports I get are resolved after I see the user's /proc info. Is it acceptable to put it all in one file, since it isn't meant to be machine-parseable anyway? Some of the HCI drivers do that, right?

Signature · 2 lines
Mark McClelland
[email protected]
#20 lore raw
Quoted 12 lines
Quoted 9 lines · Greg KH
Quoted 6 lines · Gerd Knorr

Changes required/recommended in video4linux drivers:

* some usb webcam drivers (usbvideo.ko, stv680.ko, se401.ko and ov511.ko) use the video_proc_entry() to add additional procfs files. These drivers must be converted to sysfs too because video_proc_entry() doesn't exist any more.

<snip>

Quoted 8 lines
Quoted 3 lines

Is there any need for these drivers to export anything through sysfs now instead of /proc?

Yes, at least with ov511. Some of the info that it puts in /proc is no longer necessary. However, there are various bits of hardware info that still need to get to userspace, for scripts that need to tell otherwise identical (same VID/PID/revision) cameras apart when creating /dev nodes.

Also, there is some other information that scripts need. Exposure gives information about light levels, which can be used to compute the absolute brightness of a scene captured.

#21 lore raw
In reply to Mark McClelland
Quoted 3 lines · Mark McClelland

Is there a convention for naming driver-specific files in /sys? E.g.:
ov511_foo, _foo, etc...? I don't want to pollute the namespace.

You get your own subdirectory to play in, so there really isn't much you can do to hurt the namespace :)

That being said, stay away from files named "device", "driver", and "dev". They are used by the driver core (well, "dev" isn't, but each class provides it...).

Just remember, one value per file and everyone will be happy.

thanks,

greg k-h