* [RFC/PATCH] sysfs'ify video4linux @ 2003-07-15 14:31 Gerd Knorr 2003-07-15 15:21 ` Ronald Bultje 2003-07-15 21:27 ` Greg KH 0 siblings, 2 replies; 21+ messages in thread From: Gerd Knorr @ 2003-07-15 14:31 UTC (permalink / raw) To: Kernel List, video4linux list 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 --- linux-2.6.0-test1/drivers/media/video/videodev.c.sysfs 2003-07-15 13:30:26.000000000 +0200 +++ linux-2.6.0-test1/drivers/media/video/videodev.c 2003-07-15 14:52:52.000000000 +0200 @@ -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) --- linux-2.6.0-test1/drivers/media/video/bttv-driver.c.sysfs 2003-07-14 12:37:01.000000000 +0200 +++ linux-2.6.0-test1/drivers/media/video/bttv-driver.c 2003-07-15 14:29:02.000000000 +0200 @@ -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, ... */ --- linux-2.6.0-test1/include/linux/videodev.h.sysfs 2003-07-14 12:05:25.000000000 +0200 +++ linux-2.6.0-test1/include/linux/videodev.h 2003-07-15 13:40:50.000000000 +0200 @@ -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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-15 14:31 [RFC/PATCH] sysfs'ify video4linux Gerd Knorr @ 2003-07-15 15:21 ` Ronald Bultje 2003-07-15 16:19 ` Matt Porter 2003-07-15 21:27 ` Greg KH 1 sibling, 1 reply; 21+ messages in thread From: Ronald Bultje @ 2003-07-15 15:21 UTC (permalink / raw) To: video4linux-list; +Cc: Kernel List Hey Gerd, On Tue, 2003-07-15 at 16:31, Gerd Knorr wrote: > This patch moves the video4linux subsystem from procfs to sysfs. [..cool stuff, especially the driver integration fanciness with the device struct stuff..] > 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 -- Ronald Bultje <rbultje@ronald.bitfreak.net> Linux Video/Multimedia developer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-15 15:21 ` Ronald Bultje @ 2003-07-15 16:19 ` Matt Porter 0 siblings, 0 replies; 21+ messages in thread From: Matt Porter @ 2003-07-15 16:19 UTC (permalink / raw) To: Ronald Bultje; +Cc: video4linux-list, Kernel List On Tue, Jul 15, 2003 at 05:21:13PM +0200, Ronald Bultje wrote: > Hey Gerd, > > On Tue, 2003-07-15 at 16:31, Gerd Knorr wrote: > > This patch moves the video4linux subsystem from procfs to sysfs. > [..cool stuff, especially the driver integration fanciness with the > device struct stuff..] > > 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, -- Matt Porter mporter@kernel.crashing.org ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-15 14:31 [RFC/PATCH] sysfs'ify video4linux Gerd Knorr 2003-07-15 15:21 ` Ronald Bultje @ 2003-07-15 21:27 ` Greg KH 2003-07-16 8:44 ` Gerd Knorr 2003-07-16 13:33 ` [RFC/PATCH] sysfs'ify video4linux Mark McClelland 1 sibling, 2 replies; 21+ messages in thread From: Greg KH @ 2003-07-15 21:27 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote: > 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! > * 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. > 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. > * 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? > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-15 21:27 ` Greg KH @ 2003-07-16 8:44 ` Gerd Knorr 2003-07-16 16:19 ` Greg KH 2003-07-16 13:33 ` [RFC/PATCH] sysfs'ify video4linux Mark McClelland 1 sibling, 1 reply; 21+ messages in thread From: Gerd Knorr @ 2003-07-16 8:44 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list > > * 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. > 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/ > > 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? > 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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 8:44 ` Gerd Knorr @ 2003-07-16 16:19 ` Greg KH 2003-07-16 20:20 ` Gerd Knorr 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-07-16 16:19 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Wed, Jul 16, 2003 at 10:44:48AM +0200, Gerd Knorr wrote: > > > 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... > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 16:19 ` Greg KH @ 2003-07-16 20:20 ` Gerd Knorr 2003-07-16 21:08 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Gerd Knorr @ 2003-07-16 20:20 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list > 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. > 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. > 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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 20:20 ` Gerd Knorr @ 2003-07-16 21:08 ` Greg KH 2003-07-17 12:01 ` Gerd Knorr 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-07-16 21:08 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote: > > 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. > > 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... > 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. > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 21:08 ` Greg KH @ 2003-07-17 12:01 ` Gerd Knorr 2003-07-17 14:57 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Gerd Knorr @ 2003-07-17 12:01 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list On Wed, Jul 16, 2003 at 02:08:00PM -0700, Greg KH wrote: > On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote: > > 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 ... > > 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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-17 12:01 ` Gerd Knorr @ 2003-07-17 14:57 ` Greg KH 2003-07-17 16:37 ` Gerd Knorr 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-07-17 14:57 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Thu, Jul 17, 2003 at 02:01:21PM +0200, Gerd Knorr wrote: > On Wed, Jul 16, 2003 at 02:08:00PM -0700, Greg KH wrote: > > On Wed, Jul 16, 2003 at 10:20:18PM +0200, Gerd Knorr wrote: > > > 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 ... > > > > 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. That sounds fine, and is the same as what I did for usb host devices. > (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? > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-17 14:57 ` Greg KH @ 2003-07-17 16:37 ` Gerd Knorr 2003-07-17 21:49 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Gerd Knorr @ 2003-07-17 16:37 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list > > ... 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. > > (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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-17 16:37 ` Gerd Knorr @ 2003-07-17 21:49 ` Greg KH 2003-07-18 9:59 ` Gerd Knorr 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-07-17 21:49 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Thu, Jul 17, 2003 at 06:37:15PM +0200, Gerd Knorr wrote: > > 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. > 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 :) > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-17 21:49 ` Greg KH @ 2003-07-18 9:59 ` Gerd Knorr [not found] ` <20030718234359.GK1583@kroah.com> 0 siblings, 1 reply; 21+ messages in thread From: Gerd Knorr @ 2003-07-18 9:59 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list On Thu, Jul 17, 2003 at 02:49:07PM -0700, Greg KH wrote: > On Thu, Jul 17, 2003 at 06:37:15PM +0200, Gerd Knorr wrote: > > > > 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. > > > 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 :) > > > 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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20030718234359.GK1583@kroah.com>]
* Re: [RFC/PATCH] sysfs'ify video4linux [not found] ` <20030718234359.GK1583@kroah.com> @ 2003-07-21 7:28 ` Gerd Knorr 2003-07-21 7:55 ` Ronald Bultje 2003-07-21 15:43 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Gerd Knorr 0 siblings, 2 replies; 21+ messages in thread From: Gerd Knorr @ 2003-07-21 7:28 UTC (permalink / raw) To: Greg KH; +Cc: Kernel List, video4linux list > 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 <greg@kroah.com> Subject: Re: [RFC/PATCH] sysfs'ify video4linux In-Reply-To: <20030717214907.GA3255@kroah.com> > > 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 :) > 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 -- sigfault ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-21 7:28 ` Gerd Knorr @ 2003-07-21 7:55 ` Ronald Bultje 2003-07-21 15:43 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Gerd Knorr 1 sibling, 0 replies; 21+ messages in thread From: Ronald Bultje @ 2003-07-21 7:55 UTC (permalink / raw) To: video4linux-list; +Cc: Greg KH, Kernel List Hi Gerd, On Mon, 2003-07-21 at 09:28, Gerd Knorr wrote: > 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 -- Ronald Bultje <rbultje@ronald.bitfreak.net> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core 2003-07-21 7:28 ` Gerd Knorr 2003-07-21 7:55 ` Ronald Bultje @ 2003-07-21 15:43 ` Gerd Knorr 2003-07-21 15:47 ` [RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver Gerd Knorr 2003-07-21 16:27 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Greg KH 1 sibling, 2 replies; 21+ messages in thread From: Gerd Knorr @ 2003-07-21 15:43 UTC (permalink / raw) To: Greg KH, Kernel List, video4linux list > 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 ]============================== diff -u linux-2.6.0-test1/drivers/media/video/videodev.c linux/drivers/media/video/videodev.c --- linux-2.6.0-test1/drivers/media/video/videodev.c 2003-07-21 11:49:26.000000000 +0200 +++ linux/drivers/media/video/videodev.c 2003-07-21 15:03:36.000000000 +0200 @@ -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"); diff -u linux-2.6.0-test1/include/linux/videodev.h linux/include/linux/videodev.h --- linux-2.6.0-test1/include/linux/videodev.h 2003-07-21 11:47:48.000000000 +0200 +++ linux/include/linux/videodev.h 2003-07-21 14:53:28.000000000 +0200 @@ -3,18 +3,10 @@ #include <linux/types.h> #include <linux/version.h> +#include <linux/device.h> -#if 1 -/* - * v4l2 is still work-in-progress, integration planed for 2.5.x - * documentation: http://bytesex.org/v4l/ - * patches available from: http://bytesex.org/patches/ - */ -# define HAVE_V4L2 1 -# include <linux/videodev2.h> -#else -# undef HAVE_V4L2 -#endif +#define HAVE_V4L2 1 +#include <linux/videodev2.h> #ifdef __KERNEL__ @@ -23,35 +15,70 @@ struct video_device { - struct module *owner; + /* device info */ + struct device *dev; char name[32]; int type; /* v4l1 */ int type2; /* v4l2 */ int hardware; int minor; - /* new interface -- we will use file_operations directly - * like soundcore does. */ + /* device ops + callbacks */ struct file_operations *fops; - void *priv; /* Used to be 'private' but that upsets C++ */ + void (*release)(struct video_device *vfd); + +#if 1 /* to be removed in 2.7.x */ + /* obsolete -- fops->owner is used instead */ + struct module *owner; + /* dev->driver_data will be used instead some day. + * Use the video_{get|set}_drvdata() helper functions, + * so the switch over will be transparent for you. + * Or use {pci|usb|dev}_{get|set}_drvdata() directly. */ + void *priv; +#endif - /* for videodev.c intenal usage -- don't touch */ - int users; - struct semaphore lock; - char devfs_name[64]; /* devfs */ + /* for videodev.c intenal usage -- please don't touch */ + int users; /* video_exclusive_{open|close} ... */ + struct semaphore lock; /* ... helper function uses these */ + char devfs_name[64]; /* devfs */ + struct class_device class_dev; /* sysfs */ }; #define VIDEO_MAJOR 81 -extern int video_register_device(struct video_device *, int type, int nr); #define VFL_TYPE_GRABBER 0 #define VFL_TYPE_VBI 1 #define VFL_TYPE_RADIO 2 #define VFL_TYPE_VTX 3 +extern int video_register_device(struct video_device *, int type, int nr); extern void video_unregister_device(struct video_device *); extern struct video_device* video_devdata(struct file*); +#define to_video_device(cd) container_of(cd, struct video_device, class_dev) +static inline void +video_device_create_file(struct video_device *vfd, + struct class_device_attribute *attr) +{ + class_device_create_file(&vfd->class_dev, attr); +} + +/* helper functions to alloc / release struct video_device, the + later can be used for video_device->release() */ +struct video_device *video_device_alloc(void); +void video_device_release(struct video_device *vfd); + +/* helper functions to access driver private data. */ +static inline void *video_get_drvdata(struct video_device *dev) +{ + return dev->priv; +} + +static inline void video_set_drvdata(struct video_device *dev, void *data) +{ + dev->priv = data; +} + extern int video_exclusive_open(struct inode *inode, struct file *file); extern int video_exclusive_release(struct inode *inode, struct file *file); extern int video_usercopy(struct inode *inode, struct file *file, ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver 2003-07-21 15:43 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Gerd Knorr @ 2003-07-21 15:47 ` Gerd Knorr 2003-07-21 16:27 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Greg KH 1 sibling, 0 replies; 21+ messages in thread From: Gerd Knorr @ 2003-07-21 15:47 UTC (permalink / raw) To: Greg KH, Kernel List, video4linux list 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 ]============================== diff -u linux-2.6.0-test1/drivers/media/video/bttv-cards.c linux/drivers/media/video/bttv-cards.c --- linux-2.6.0-test1/drivers/media/video/bttv-cards.c 2003-07-21 11:49:42.000000000 +0200 +++ linux/drivers/media/video/bttv-cards.c 2003-07-21 11:49:43.000000000 +0200 @@ -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"); diff -u linux-2.6.0-test1/drivers/media/video/bttv-driver.c linux/drivers/media/video/bttv-driver.c --- linux-2.6.0-test1/drivers/media/video/bttv-driver.c 2003-07-21 11:49:42.000000000 +0200 +++ linux/drivers/media/video/bttv-driver.c 2003-07-21 17:08:32.183882024 +0200 @@ -32,6 +32,7 @@ #include <linux/sched.h> #include <linux/interrupt.h> #include <linux/kdev_t.h> +#include <linux/device.h> #include <asm/io.h> #include <asm/byteorder.h> @@ -123,6 +124,17 @@ #endif /* ----------------------------------------------------------------------- */ +/* sysfs */ + +static ssize_t show_card(struct class_device *cd, char *buf) +{ + struct video_device *vfd = to_video_device(cd); + struct bttv *btv = dev_get_drvdata(vfd->dev); + return sprintf(buf, "%d\n", btv ? btv->type : UNSET); +} +static CLASS_DEVICE_ATTR(card, S_IRUGO, show_card, NULL); + +/* ----------------------------------------------------------------------- */ /* static data */ /* special timing tables from conexant... */ @@ -2039,7 +2051,7 @@ struct video_capability *cap = arg; memset(cap,0,sizeof(*cap)); - strcpy(cap->name,btv->video_dev.name); + strcpy(cap->name,btv->video_dev->name); if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type) { /* vbi */ cap->type = VID_TYPE_TUNER|VID_TYPE_TELETEXT; @@ -2390,7 +2402,7 @@ if (0 == v4l2) return -EINVAL; strcpy(cap->driver,"bttv"); - strlcpy(cap->card,btv->video_dev.name,sizeof(cap->card)); + strlcpy(cap->card,btv->video_dev->name,sizeof(cap->card)); sprintf(cap->bus_info,"PCI:%s",btv->dev->slot_name); cap->version = BTTV_VERSION_CODE; cap->capabilities = @@ -2767,12 +2779,12 @@ dprintk(KERN_DEBUG "bttv: open minor=%d\n",minor); for (i = 0; i < bttv_num; i++) { - if (bttvs[i].video_dev.minor == minor) { + if (bttvs[i].video_dev->minor == minor) { btv = &bttvs[i]; type = V4L2_BUF_TYPE_VIDEO_CAPTURE; break; } - if (bttvs[i].vbi_dev.minor == minor) { + if (bttvs[i].vbi_dev->minor == minor) { btv = &bttvs[i]; type = V4L2_BUF_TYPE_VBI_CAPTURE; break; @@ -2873,8 +2885,8 @@ static struct video_device bttv_video_template = { .name = "UNSET", - type: VID_TYPE_CAPTURE|VID_TYPE_TUNER|VID_TYPE_OVERLAY| - VID_TYPE_CLIPPING|VID_TYPE_SCALES, + .type = VID_TYPE_CAPTURE|VID_TYPE_TUNER|VID_TYPE_OVERLAY| + VID_TYPE_CLIPPING|VID_TYPE_SCALES, .hardware = VID_HARDWARE_BT848, .fops = &bttv_fops, .minor = -1, @@ -2902,7 +2914,7 @@ dprintk("bttv: open minor=%d\n",minor); for (i = 0; i < bttv_num; i++) { - if (bttvs[i].radio_dev.minor == minor) { + if (bttvs[i].radio_dev->minor == minor) { btv = &bttvs[i]; break; } @@ -2947,7 +2959,7 @@ struct video_capability *cap = arg; memset(cap,0,sizeof(*cap)); - strcpy(cap->name,btv->radio_dev.name); + strcpy(cap->name,btv->radio_dev->name); cap->type = VID_TYPE_TUNER; cap->channels = 1; cap->audios = 1; @@ -3337,30 +3349,83 @@ /* ----------------------------------------------------------------------- */ /* initialitation */ +static struct video_device *vdev_init(struct bttv *btv, + struct video_device *template, + char *type) +{ + struct video_device *vfd; + + vfd = video_device_alloc(); + if (NULL == vfd) + return NULL; + *vfd = *template; + vfd->minor = -1; + vfd->release = video_device_release; + vfd->dev = &btv->dev->dev; + snprintf(vfd->name, sizeof(vfd->name), "BT%d%s %s (%s)", + btv->id, (btv->id==848 && btv->revision==0x12) ? "A" : "", + type, bttv_tvcards[btv->type].name); + return vfd; +} + /* register video4linux devices */ static int __devinit bttv_register_video(struct bttv *btv) { - if(video_register_device(&btv->video_dev,VFL_TYPE_GRABBER,video_nr)<0) - return -1; + /* video */ + btv->video_dev = vdev_init(btv, &bttv_video_template, "video"); + if (NULL == btv->video_dev) + goto err; + if (video_register_device(btv->video_dev,VFL_TYPE_GRABBER,video_nr)<0) + goto err; printk(KERN_INFO "bttv%d: registered device video%d\n", - btv->nr,btv->video_dev.minor & 0x1f); + btv->nr,btv->video_dev->minor & 0x1f); + video_device_create_file(btv->video_dev, &class_device_attr_card); - if(video_register_device(&btv->vbi_dev,VFL_TYPE_VBI,vbi_nr)<0) { - video_unregister_device(&btv->video_dev); - return -1; - } + /* vbi */ + btv->vbi_dev = vdev_init(btv, &bttv_vbi_template, "vbi"); + if (NULL == btv->vbi_dev) + goto err; + if (video_register_device(btv->vbi_dev,VFL_TYPE_VBI,vbi_nr)<0) + goto err; printk(KERN_INFO "bttv%d: registered device vbi%d\n", - btv->nr,btv->vbi_dev.minor & 0x1f); + btv->nr,btv->vbi_dev->minor & 0x1f); if (!btv->has_radio) return 0; - if (video_register_device(&btv->radio_dev, VFL_TYPE_RADIO,radio_nr)<0) { - video_unregister_device(&btv->vbi_dev); - video_unregister_device(&btv->video_dev); - return -1; - } + /* radio */ + btv->radio_dev = vdev_init(btv, &radio_template, "radio"); + if (NULL == btv->radio_dev) + goto err; + if (video_register_device(btv->radio_dev, VFL_TYPE_RADIO,radio_nr)<0) + goto err; printk(KERN_INFO "bttv%d: registered device radio%d\n", - btv->nr,btv->radio_dev.minor & 0x1f); + btv->nr,btv->radio_dev->minor & 0x1f); + + /* all done */ + return 0; + + err: + if (btv->video_dev) { + if (-1 != btv->video_dev->minor) + video_unregister_device(btv->video_dev); + else + video_device_release(btv->video_dev); + btv->video_dev = NULL; + } + if (btv->vbi_dev) { + if (-1 != btv->vbi_dev->minor) + video_unregister_device(btv->vbi_dev); + else + video_device_release(btv->vbi_dev); + btv->vbi_dev = NULL; + } + if (btv->radio_dev) { + if (-1 != btv->radio_dev->minor) + video_unregister_device(btv->radio_dev); + else + video_device_release(btv->radio_dev); + btv->radio_dev = NULL; + } return 0; } @@ -3408,16 +3473,6 @@ btv->i2c_rc = -1; btv->tuner_type = UNSET; btv->pinnacle_id = UNSET; - - memcpy(&btv->video_dev, &bttv_video_template, sizeof(bttv_video_template)); - 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->radio_dev.minor = -1; - btv->radio_dev.priv = btv; - btv->vbi_dev.minor = -1; - btv->vbi_dev.priv = btv; btv->has_radio=radio[btv->nr]; /* pci stuff (init, get irq/mmip, ... */ @@ -3576,12 +3631,18 @@ i2c_bit_del_bus(&btv->i2c_adap); /* unregister video4linux */ - if (btv->video_dev.minor!=-1) - video_unregister_device(&btv->video_dev); - if (btv->radio_dev.minor!=-1) - video_unregister_device(&btv->radio_dev); - if (btv->vbi_dev.minor!=-1) - video_unregister_device(&btv->vbi_dev); + if (btv->video_dev) { + video_unregister_device(btv->video_dev); + btv->video_dev = NULL; + } + if (btv->radio_dev) { + video_unregister_device(btv->radio_dev); + btv->radio_dev = NULL; + } + if (btv->vbi_dev) { + video_unregister_device(btv->vbi_dev); + btv->vbi_dev = NULL; + } /* free allocated memory */ btcx_riscmem_free(btv->dev,&btv->main); diff -u linux-2.6.0-test1/drivers/media/video/bttvp.h linux/drivers/media/video/bttvp.h --- linux-2.6.0-test1/drivers/media/video/bttvp.h 2003-07-21 11:49:42.000000000 +0200 +++ linux/drivers/media/video/bttvp.h 2003-07-21 11:49:43.000000000 +0200 @@ -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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core 2003-07-21 15:43 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Gerd Knorr 2003-07-21 15:47 ` [RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver Gerd Knorr @ 2003-07-21 16:27 ` Greg KH 1 sibling, 0 replies; 21+ messages in thread From: Greg KH @ 2003-07-21 16:27 UTC (permalink / raw) To: Gerd Knorr; +Cc: Kernel List, video4linux list On Mon, Jul 21, 2003 at 05:43:40PM +0200, Gerd Knorr wrote: > > 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. Very nice, looks good to me. Thanks for putting up with the lack of documentation for some of this :) thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-15 21:27 ` Greg KH 2003-07-16 8:44 ` Gerd Knorr @ 2003-07-16 13:33 ` Mark McClelland 2003-07-16 14:10 ` root 2003-07-16 16:23 ` Greg KH 1 sibling, 2 replies; 21+ messages in thread From: Mark McClelland @ 2003-07-16 13:33 UTC (permalink / raw) To: Greg KH; +Cc: Gerd Knorr, Kernel List, video4linux list Greg KH wrote: >On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote: > > >>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. > I can do the ov511 work if you want. I can have it done in two days (or less). >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. >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? -- Mark McClelland mark@alpha.dyndns.org ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 13:33 ` [RFC/PATCH] sysfs'ify video4linux Mark McClelland @ 2003-07-16 14:10 ` root 2003-07-16 16:23 ` Greg KH 1 sibling, 0 replies; 21+ messages in thread From: root @ 2003-07-16 14:10 UTC (permalink / raw) To: Mark McClelland; +Cc: Greg KH, Gerd Knorr, Kernel List, video4linux list > > Greg KH wrote: > > >On Tue, Jul 15, 2003 at 04:31:19PM +0200, Gerd Knorr wrote: > > > > > >>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> > >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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/PATCH] sysfs'ify video4linux 2003-07-16 13:33 ` [RFC/PATCH] sysfs'ify video4linux Mark McClelland 2003-07-16 14:10 ` root @ 2003-07-16 16:23 ` Greg KH 1 sibling, 0 replies; 21+ messages in thread From: Greg KH @ 2003-07-16 16:23 UTC (permalink / raw) To: Mark McClelland; +Cc: Gerd Knorr, Kernel List, video4linux list On Wed, Jul 16, 2003 at 06:33:02AM -0700, Mark McClelland wrote: > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2003-07-21 16:20 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-07-15 14:31 [RFC/PATCH] sysfs'ify video4linux Gerd Knorr 2003-07-15 15:21 ` Ronald Bultje 2003-07-15 16:19 ` Matt Porter 2003-07-15 21:27 ` Greg KH 2003-07-16 8:44 ` Gerd Knorr 2003-07-16 16:19 ` Greg KH 2003-07-16 20:20 ` Gerd Knorr 2003-07-16 21:08 ` Greg KH 2003-07-17 12:01 ` Gerd Knorr 2003-07-17 14:57 ` Greg KH 2003-07-17 16:37 ` Gerd Knorr 2003-07-17 21:49 ` Greg KH 2003-07-18 9:59 ` Gerd Knorr [not found] ` <20030718234359.GK1583@kroah.com> 2003-07-21 7:28 ` Gerd Knorr 2003-07-21 7:55 ` Ronald Bultje 2003-07-21 15:43 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Gerd Knorr 2003-07-21 15:47 ` [RFC/PATCH] 2/2 v4l: sysfs'ify bttv driver Gerd Knorr 2003-07-21 16:27 ` [RFC/PATCH] 1/2 v4l: sysfs'ify video4linux core Greg KH 2003-07-16 13:33 ` [RFC/PATCH] sysfs'ify video4linux Mark McClelland 2003-07-16 14:10 ` root 2003-07-16 16:23 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).