Skip to contentSkip to conversation

oops in bttv

PATCH Applied Chuck Ebbert 9 participants 24 messages Jul 13, 2006 – Jul 25, 2006 Media (V4L/DVB)

lore
Share & tools
lore

Chuck Ebbert

author
#0 lore raw

In-Reply-To: <[email protected]>

Quoted 39 lines · Alex Riesen

What I did was to call settings of the flashplayer and press on the webcam symbol there. The system didn't crash, just this oops:

BUG: unable to handle kernel NULL pointer dereference at virtual address 00000065
 printing eip:
c01875b0
*pde = 00000000
Oops: 0000 [#1]
PREEMPT SMP 
Modules linked in: tuner tvaudio msp3400 bttv video_buf firmware_class ir_common
 compat_ioctl32 btcx_risc tveeprom videodev v4l2_common nfs vfat fat nfsd export

fs lockd sunrpc snd_pcm_oss snd_mixer_oss snd_seq_oss snd_seq_midi_event snd_seq
snd_seq_device reiserfs rtc microcode radeon drm intel_agp agpgart snd_intel8x0 snd_ac97_codec snd_ac97_bus snd_pcm snd_timer snd soundcore snd_page_alloc sd_m
od usb_storage libusual usbhid uhci_hcd usbcore e100

CPU:    0
EIP:    0060:[<c01875b0>]    Not tainted VLI
EFLAGS: 00010292   (2.6.18-rc1 #80)

EIP is at sysfs_add_file+0x10/0x76
eax: 00000001 ebx: f9bd6d14 ecx: 00000004 edx: f9bd6d14
esi: f9bde920 edi: 00000001 ebp: dc489eac esp: dc489e94
ds: 007b es: 007b ss: 0068
Process modprobe (pid: 11377, ti=dc489000 task=f21ff6b0 task.ti=dc489000)

Stack: f9bde920 f9bde920 00000004 f9bde920 f9bde920 00000000 dc489eb4 c018763c 
       dc489ebc c0220cc3 dc489ec8 f9bc3073 c19b4800 dc489ee4 f9bc35de 00000000 
       20bd7818 f9bd7880 f9bd7880 ffffffed dc489ef0 c01c3c1b c19b4800 dc489f04 
Call Trace:
 [<c018763c>] sysfs_create_file+0x26/0x28
 [<c0220cc3>] class_device_create_file+0x14/0x1a
 [<f9bc3073>] bttv_register_video+0x8c/0x147 [bttv]
 [<f9bc35de>] bttv_probe+0x4ab/0x593 [bttv]
 [<c01c3c1b>] pci_call_probe+0xd/0x10
 [<c01c3c4f>] __pci_device_probe+0x31/0x43
 [<c01c3c82>] pci_device_probe+0x21/0x34
 [<c0220429>] driver_probe_device+0x47/0x94
 [<c0220542>] __driver_attach+0x5e/0x89
 [<c021fada>] bus_for_each_dev+0x38/0x5d
 [<c0220581>] driver_attach+0x14/0x16
 [<c021ff2c>] bus_add_driver+0x5f/0x98
Quoted 9 lines
 [<c02209a8>] driver_register+0x75/0x7a
 [<c01c3e3e>] __pci_register_driver+0x4f/0x5d
 [<f9bc3b3b>] bttv_init_module+0x9f/0xa1 [bttv]
 [<c01325f7>] sys_init_module+0x95/0x1c5
 [<c010291b>] syscall_call+0x7/0xb
 [<b7ea781e>] 0xb7ea781e
Code: 01 00 00 00 6a 00 e8 f9 cf f8 ff 89 d8 e8 2f 40 fe ff 5b 8d 65 f4 5b 5e 5f 5d c3 55 89 e5 57 89 c7 56 53 89 d3 83 ec 0c 89 4d f0 <8b> 40 64 89 45 ec 8b 47 1c 05 84 00 00 00 0f b7 72 08 c7 45 e8 
EIP: [<c01875b0>] sysfs_add_file+0x10/0x76 SS:ESP 0068:dc489e94

int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type) {

OOPS==> struct sysfs_dirent * parent_sd = dir->d_fsdata;
        umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;

dir == 1, which is a strange value for a pointer to have (you can see it in eax.)

called from:

int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)

{
        BUG_ON(!kobj || !kobj->dentry || !attr);

        return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);

}

so kobj->dentry == 1 here.

called from:

int class_device_create_file(struct class_device * class_dev,
const struct class_device_attribute * attr)

{
        int error = -EINVAL;
        if (class_dev)
                error = sysfs_create_file(&class_dev->kobj, &attr->attr);
        return error;
}

so the kobj is in a class_dev.

called from:

static int __devinit bttv_register_video(struct bttv *btv)

{
        if (no_overlay <= 0) {
                bttv_video_template.type |= VID_TYPE_OVERLAY;
        } else {
                printk("bttv: Overlay support disabled.\n");
        }

        /* 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->c.nr,btv->video_dev->minor & 0x1f);

===> video_device_create_file(btv->video_dev, &class_device_attr_card);

video_device_create_file() is a wrapper for class_device_create_file():

video_device_create_file(struct video_device *vfd,
struct class_device_attribute *attr)

{
        class_device_create_file(&vfd->class_dev, attr);
}

The class_dev is created like this, in video_register_device():

        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.devt        = MKDEV(VIDEO_MAJOR, vfd->minor);
        sprintf(vfd->class_dev.class_id, "%s%d", name_base, i - base);
        class_device_register(&vfd->class_dev);
        class_device_create_file(&vfd->class_dev,
                                &class_device_attr_name);

so it looks like class_device_register() is putting the 1 into the dentry? Culprit looks to be class_device_add() in that case.

I tried tracing through that and... gave up.

IAC looks like this isn't a bttv problem.

Signature · 2 lines
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush
#1 lore raw
Quoted 130 lines · Chuck Ebbert

In-Reply-To: <[email protected]>

Quoted 39 lines · Alex Riesen

What I did was to call settings of the flashplayer and press on the webcam symbol there. The system didn't crash, just this oops:

BUG: unable to handle kernel NULL pointer dereference at virtual address 00000065
 printing eip:
c01875b0
*pde = 00000000
Oops: 0000 [#1]
PREEMPT SMP 
Modules linked in: tuner tvaudio msp3400 bttv video_buf firmware_class ir_common
 compat_ioctl32 btcx_risc tveeprom videodev v4l2_common nfs vfat fat nfsd export

fs lockd sunrpc snd_pcm_oss snd_mixer_oss snd_seq_oss snd_seq_midi_event snd_seq
snd_seq_device reiserfs rtc microcode radeon drm intel_agp agpgart snd_intel8x0 snd_ac97_codec snd_ac97_bus snd_pcm snd_timer snd soundcore snd_page_alloc sd_m
od usb_storage libusual usbhid uhci_hcd usbcore e100

CPU:    0
EIP:    0060:[<c01875b0>]    Not tainted VLI
EFLAGS: 00010292   (2.6.18-rc1 #80)

EIP is at sysfs_add_file+0x10/0x76
eax: 00000001 ebx: f9bd6d14 ecx: 00000004 edx: f9bd6d14
esi: f9bde920 edi: 00000001 ebp: dc489eac esp: dc489e94
ds: 007b es: 007b ss: 0068
Process modprobe (pid: 11377, ti=dc489000 task=f21ff6b0 task.ti=dc489000)

Stack: f9bde920 f9bde920 00000004 f9bde920 f9bde920 00000000 dc489eb4 c018763c 
       dc489ebc c0220cc3 dc489ec8 f9bc3073 c19b4800 dc489ee4 f9bc35de 00000000 
       20bd7818 f9bd7880 f9bd7880 ffffffed dc489ef0 c01c3c1b c19b4800 dc489f04 
Call Trace:
 [<c018763c>] sysfs_create_file+0x26/0x28
 [<c0220cc3>] class_device_create_file+0x14/0x1a
 [<f9bc3073>] bttv_register_video+0x8c/0x147 [bttv]
 [<f9bc35de>] bttv_probe+0x4ab/0x593 [bttv]
 [<c01c3c1b>] pci_call_probe+0xd/0x10
 [<c01c3c4f>] __pci_device_probe+0x31/0x43
 [<c01c3c82>] pci_device_probe+0x21/0x34
 [<c0220429>] driver_probe_device+0x47/0x94
 [<c0220542>] __driver_attach+0x5e/0x89
 [<c021fada>] bus_for_each_dev+0x38/0x5d
 [<c0220581>] driver_attach+0x14/0x16
 [<c021ff2c>] bus_add_driver+0x5f/0x98
Quoted 9 lines
 [<c02209a8>] driver_register+0x75/0x7a
 [<c01c3e3e>] __pci_register_driver+0x4f/0x5d
 [<f9bc3b3b>] bttv_init_module+0x9f/0xa1 [bttv]
 [<c01325f7>] sys_init_module+0x95/0x1c5
 [<c010291b>] syscall_call+0x7/0xb
 [<b7ea781e>] 0xb7ea781e
Code: 01 00 00 00 6a 00 e8 f9 cf f8 ff 89 d8 e8 2f 40 fe ff 5b 8d 65 f4 5b 5e 5f 5d c3 55 89 e5 57 89 c7 56 53 89 d3 83 ec 0c 89 4d f0 <8b> 40 64 89 45 ec 8b 47 1c 05 84 00 00 00 0f b7 72 08 c7 45 e8 
EIP: [<c01875b0>] sysfs_add_file+0x10/0x76 SS:ESP 0068:dc489e94

int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type) {

OOPS==> struct sysfs_dirent * parent_sd = dir->d_fsdata;
        umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;

dir == 1, which is a strange value for a pointer to have (you can see it in eax.)

called from:

int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)

{
        BUG_ON(!kobj || !kobj->dentry || !attr);

        return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);

}

so kobj->dentry == 1 here.

called from:

int class_device_create_file(struct class_device * class_dev,
const struct class_device_attribute * attr)

{
        int error = -EINVAL;
        if (class_dev)
                error = sysfs_create_file(&class_dev->kobj, &attr->attr);
        return error;
}

so the kobj is in a class_dev.

called from:

static int __devinit bttv_register_video(struct bttv *btv)

{
        if (no_overlay <= 0) {
                bttv_video_template.type |= VID_TYPE_OVERLAY;
        } else {
                printk("bttv: Overlay support disabled.\n");
        }

        /* 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->c.nr,btv->video_dev->minor & 0x1f);

===> video_device_create_file(btv->video_dev, &class_device_attr_card);

video_device_create_file() is a wrapper for class_device_create_file():

video_device_create_file(struct video_device *vfd,
struct class_device_attribute *attr)

{
        class_device_create_file(&vfd->class_dev, attr);
}

The class_dev is created like this, in video_register_device():

        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.devt        = MKDEV(VIDEO_MAJOR, vfd->minor);
        sprintf(vfd->class_dev.class_id, "%s%d", name_base, i - base);
        class_device_register(&vfd->class_dev);
        class_device_create_file(&vfd->class_dev,
                                &class_device_attr_name);
Quoted 2 lines

so it looks like class_device_register() is putting the 1 into the dentry? Culprit looks to be class_device_add() in that case.

Perhaps we should check the return value of class_device_register() to verify that nothing bad happened there? A dentry of 1 is very odd.

If you enable debugging for the driver core and for kobjects (they both are config options), is there anything interesting in the log right before this happens?

Quoted 1 line

I tried tracing through that and... gave up.

Heh, yeah, it's not exactly a "simple" path...

thanks,

greg k-h

#2 lore raw
Quoted 131 lines · On Wed, 12 Jul 2006 22:05:41 -0700 Greg KH wrote:
Quoted 130 lines · Chuck Ebbert

In-Reply-To: <[email protected]>

Quoted 39 lines · Alex Riesen

What I did was to call settings of the flashplayer and press on the webcam symbol there. The system didn't crash, just this oops:

BUG: unable to handle kernel NULL pointer dereference at virtual address 00000065
 printing eip:
c01875b0
*pde = 00000000
Oops: 0000 [#1]
PREEMPT SMP 
Modules linked in: tuner tvaudio msp3400 bttv video_buf firmware_class ir_common
 compat_ioctl32 btcx_risc tveeprom videodev v4l2_common nfs vfat fat nfsd export

fs lockd sunrpc snd_pcm_oss snd_mixer_oss snd_seq_oss snd_seq_midi_event snd_seq
snd_seq_device reiserfs rtc microcode radeon drm intel_agp agpgart snd_intel8x0 snd_ac97_codec snd_ac97_bus snd_pcm snd_timer snd soundcore snd_page_alloc sd_m
od usb_storage libusual usbhid uhci_hcd usbcore e100

CPU:    0
EIP:    0060:[<c01875b0>]    Not tainted VLI
EFLAGS: 00010292   (2.6.18-rc1 #80)

EIP is at sysfs_add_file+0x10/0x76
eax: 00000001 ebx: f9bd6d14 ecx: 00000004 edx: f9bd6d14
esi: f9bde920 edi: 00000001 ebp: dc489eac esp: dc489e94
ds: 007b es: 007b ss: 0068
Process modprobe (pid: 11377, ti=dc489000 task=f21ff6b0 task.ti=dc489000)

Stack: f9bde920 f9bde920 00000004 f9bde920 f9bde920 00000000 dc489eb4 c018763c 
       dc489ebc c0220cc3 dc489ec8 f9bc3073 c19b4800 dc489ee4 f9bc35de 00000000 
       20bd7818 f9bd7880 f9bd7880 ffffffed dc489ef0 c01c3c1b c19b4800 dc489f04 
Call Trace:
 [<c018763c>] sysfs_create_file+0x26/0x28
 [<c0220cc3>] class_device_create_file+0x14/0x1a
 [<f9bc3073>] bttv_register_video+0x8c/0x147 [bttv]
 [<f9bc35de>] bttv_probe+0x4ab/0x593 [bttv]
 [<c01c3c1b>] pci_call_probe+0xd/0x10
 [<c01c3c4f>] __pci_device_probe+0x31/0x43
 [<c01c3c82>] pci_device_probe+0x21/0x34
 [<c0220429>] driver_probe_device+0x47/0x94
 [<c0220542>] __driver_attach+0x5e/0x89
 [<c021fada>] bus_for_each_dev+0x38/0x5d
 [<c0220581>] driver_attach+0x14/0x16
 [<c021ff2c>] bus_add_driver+0x5f/0x98
Quoted 9 lines
 [<c02209a8>] driver_register+0x75/0x7a
 [<c01c3e3e>] __pci_register_driver+0x4f/0x5d
 [<f9bc3b3b>] bttv_init_module+0x9f/0xa1 [bttv]
 [<c01325f7>] sys_init_module+0x95/0x1c5
 [<c010291b>] syscall_call+0x7/0xb
 [<b7ea781e>] 0xb7ea781e
Code: 01 00 00 00 6a 00 e8 f9 cf f8 ff 89 d8 e8 2f 40 fe ff 5b 8d 65 f4 5b 5e 5f 5d c3 55 89 e5 57 89 c7 56 53 89 d3 83 ec 0c 89 4d f0 <8b> 40 64 89 45 ec 8b 47 1c 05 84 00 00 00 0f b7 72 08 c7 45 e8 
EIP: [<c01875b0>] sysfs_add_file+0x10/0x76 SS:ESP 0068:dc489e94

int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type) {

OOPS==> struct sysfs_dirent * parent_sd = dir->d_fsdata;
        umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;

dir == 1, which is a strange value for a pointer to have (you can see it in eax.)

called from:

int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)

{
        BUG_ON(!kobj || !kobj->dentry || !attr);

        return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);

}

so kobj->dentry == 1 here.

called from:

int class_device_create_file(struct class_device * class_dev,
const struct class_device_attribute * attr)

{
        int error = -EINVAL;
        if (class_dev)
                error = sysfs_create_file(&class_dev->kobj, &attr->attr);
        return error;
}

so the kobj is in a class_dev.

called from:

static int __devinit bttv_register_video(struct bttv *btv)

{
        if (no_overlay <= 0) {
                bttv_video_template.type |= VID_TYPE_OVERLAY;
        } else {
                printk("bttv: Overlay support disabled.\n");
        }

        /* 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->c.nr,btv->video_dev->minor & 0x1f);

===> video_device_create_file(btv->video_dev, &class_device_attr_card);

video_device_create_file() is a wrapper for class_device_create_file():

video_device_create_file(struct video_device *vfd,
struct class_device_attribute *attr)

{
        class_device_create_file(&vfd->class_dev, attr);
}

The class_dev is created like this, in video_register_device():

        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.devt        = MKDEV(VIDEO_MAJOR, vfd->minor);
        sprintf(vfd->class_dev.class_id, "%s%d", name_base, i - base);
        class_device_register(&vfd->class_dev);
        class_device_create_file(&vfd->class_dev,
                                &class_device_attr_name);
Quoted 5 lines
Quoted 2 lines

so it looks like class_device_register() is putting the 1 into the dentry? Culprit looks to be class_device_add() in that case.

Perhaps we should check the return value of class_device_register() to verify that nothing bad happened there? A dentry of 1 is very odd.

Yes. We are in the process of adding such checks. Join the fun.

Although I'm not sure that a return value of 1 would be caught as an error.

Quoted 7 lines

If you enable debugging for the driver core and for kobjects (they both are config options), is there anything interesting in the log right before this happens?

Quoted 1 line

I tried tracing through that and... gave up.

Heh, yeah, it's not exactly a "simple" path...

---
~Randy

#3 lore raw
Quoted 12 lines · On Wed, 12 Jul 2006 22:24:07 -0700 "Randy.Dunlap" <[email protected]> wrote:
Quoted 12 lines
Quoted 12 lines

The class_dev is created like this, in video_register_device():

        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.devt        = MKDEV(VIDEO_MAJOR, vfd->minor);
        sprintf(vfd->class_dev.class_id, "%s%d", name_base, i - base);
        class_device_register(&vfd->class_dev);
        class_device_create_file(&vfd->class_dev,
                                &class_device_attr_name);
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

so it looks like class_device_register() is putting the 1 into the dentry? Culprit looks to be class_device_add() in that case.

Perhaps we should check the return value of class_device_register() to verify that nothing bad happened there? A dentry of 1 is very odd.

Yes. We are in the process of adding such checks. Join the fun.

Chuck got about as far through this as I did. Yes, I'm suspecting an earlier uncaught error of some form. The videodev changes since 2.6.17 are small and look to be unrelated. I'd be suspecting preexisting videodev bugs which have been exposed by sysfs/driver-model changes. But it's very hard to tell.

Going through and adding 1000 missing check-error-then-recover instances is the ideal approach, but I suspect that leaving callers as-is and blurting a message from the driver core when some of these things fail might get us what we need. But right now we have neither, and we see the result.

#4 lore raw

PATCH 1/1 V4L: struct video_device corruption

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted. Include the vidiocgmbuf function pointer in video_device regardless of the V4L version.

Signed-off-by Robert Fitzsimons <[email protected]>
---
 include/media/v4l2-dev.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
include/media/v4l2-dev.h +4 -3
index 62dae1a..69059d8 100644
@@ -194,10 +194,11 @@ struct video_device
 
 
 	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
-#ifdef HAVE_V4L1
-			/* buffer type is struct vidio_mbuf * */
+	/*
+	 * vidiocgmbuf is part of the V4L1 API
+	 * buffer type is struct vidio_mbuf *
+	 */
 	int (*vidiocgmbuf)  (struct file *file, void *fh, struct video_mbuf *p);
-#endif
 	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
 				struct v4l2_framebuffer *a);
 	int (*vidioc_s_fbuf)   (struct file *file, void *fh,
Signature · 1 line
1.4.1.ga3e6
#5 lore raw

Re: [PATCH] V4L: struct video_device corruption

Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

Cheers,
Mauro.

#6 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 7 lines · Mauro Carvalho Chehab

Em S�b, 2006-07-15 �s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

It's not so clear that the problem is with v4l2-dev.h, because if you look that file:

#ifdef CONFIG_VIDEO_V4L1
#include <linux/videodev.h>
#else
#include <linux/videodev2.h>
#endif

linux/videodev.h is where HAVE_V4L1 is defined (always). So, if CONFIG_VIDEO_V4L1 is set, then HAVE_V4L1 must also bet set.

I think that the real problem is that many drivers include the V4L1 API file videodev.h when V4L1 is NOT on. Should drivers be providing V4L1 API functions, or need anything from videodev.h, if V4L1 is not on?

It seems like they either need to depend on VIDEO_V4L1 or only include the V4L1 API header file when V4L1 is turned on. Which also means they would need to #ifdef out any V4L1 code when V4L1 is turned off. The bttv driver for example does not do this. It includes a bunch of V4L1 functions even when V4L1 (and V4L1_COMPAT) are turned off.

BTW, I think the CONFIG_VIDEO_V4L1 check in v4l2-dev.h should instead check for CONFIG_VIDEO_V4L1_COMPAT. For example, cx88-video.c includes videodev.h when CONFIG_VIDEO_V4L1_COMPAT is turned on.

#7 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Em Seg, 2006-07-17 às 17:25 -0700, Trent Piepho escreveu:

Quoted 9 lines · Mauro Carvalho Chehab
Quoted 7 lines

Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

Quoted 8 lines

It's not so clear that the problem is with v4l2-dev.h, because if you look that file:

#ifdef CONFIG_VIDEO_V4L1
#include <linux/videodev.h>
#else
#include <linux/videodev2.h>
#endif

That's true. I think the OOPS is related to this -git patch:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6ab3d5624e172c553004ecc862bfeac16d9d68b7

It removed config.h from bttv-cards (and other places), but we need CONFIG_* symbols before including v4l2-dev.h. Maybe the right solution would be to include autoconf.h or config.h at the top of v4l2-dev.h.

Quoted 14 lines

linux/videodev.h is where HAVE_V4L1 is defined (always). So, if CONFIG_VIDEO_V4L1 is set, then HAVE_V4L1 must also bet set.

I think that the real problem is that many drivers include the V4L1 API file videodev.h when V4L1 is NOT on. Should drivers be providing V4L1 API functions, or need anything from videodev.h, if V4L1 is not on?

It seems like they either need to depend on VIDEO_V4L1 or only include the V4L1 API header file when V4L1 is turned on. Which also means they would need to #ifdef out any V4L1 code when V4L1 is turned off. The bttv driver for example does not do this. It includes a bunch of V4L1 functions even when V4L1 (and V4L1_COMPAT) are turned off.

Quoted 3 lines

BTW, I think the CONFIG_VIDEO_V4L1 check in v4l2-dev.h should instead check for CONFIG_VIDEO_V4L1_COMPAT. For example, cx88-video.c includes videodev.h when CONFIG_VIDEO_V4L1_COMPAT is turned on.

Yes, you are right. Checking for COMPAT covers both cases.

Cheers,
Mauro.

#8 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 17 lines · Mauro Carvalho Chehab

Em Seg, 2006-07-17 às 17:25 -0700, Trent Piepho escreveu:

Quoted 8 lines · Mauro Carvalho Chehab
Quoted 7 lines

Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

Quoted 8 lines

It's not so clear that the problem is with v4l2-dev.h, because if you look that file:

#ifdef CONFIG_VIDEO_V4L1
#include <linux/videodev.h>
#else
#include <linux/videodev2.h>
#endif

Quoted 6 lines

That's true. I think the OOPS is related to this -git patch:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6ab3d5624e172c553004ecc862bfeac16d9d68b7

It removed config.h from bttv-cards (and other places), but we need CONFIG_* symbols before including v4l2-dev.h. Maybe the right solution would be to include autoconf.h or config.h at the top of v4l2-dev.h.

No, that is a red herring.

The #include <linux/config.h> has been removed in favor of command-line -iMACRO inclusion...

In other words, config.h is still being included, but the explicit #include language is no longer needed.

Signature · 1 line
Michael Krufky
#9 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

In reply to Mauro Carvalho Chehab
Quoted 16 lines · Mauro Carvalho Chehab

Em Seg, 2006-07-17 �s 17:25 -0700, Trent Piepho escreveu:

Quoted 15 lines · Mauro Carvalho Chehab
Quoted 4 lines

Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

I think that the real problem is that many drivers include the V4L1 API file videodev.h when V4L1 is NOT on. Should drivers be providing V4L1 API functions, or need anything from videodev.h, if V4L1 is not on?

It seems like they either need to depend on VIDEO_V4L1 or only include the V4L1 API header file when V4L1 is turned on. Which also means they would need to #ifdef out any V4L1 code when V4L1 is turned off. The bttv driver for example does not do this. It includes a bunch of V4L1 functions even when V4L1 (and V4L1_COMPAT) are turned off.

I've looked into this more, and there is still a serious bug here. If you turn off V4L1 and V4L1_COMPAT, many drivers will a big issue with struct video_device.

In some files (saa7134-tvaudio.c, saa6752hs.c, v4l2-common.c, dozens more), V4L1 will be off and HAVE_V4L1 will not be defined. This gives you the struct video_dev _without_ vidiocgmbuf.

In other files (tveeprom.c, tvaudio.c, bttv-driver.c, and many more), V4L1 will still be off (of course) by HAVE_V4L1 _will_ be defined. This gives you the struct viddeo_dev _with_ vidiocgmbuf.

The first thing to solve this that HAVE_V4L1 should die. Why have a define that is supposed to mirror a Kconfig variable? If everyone used CONFIG_VIDEO_V4L1_COMPAT then there wouldn't be this problem, which some code things V4L1 is on, and some code thinks it's off.

The second thing, is that many drivers don't respect CONFIG_VIDEO_V4L1_COMPAT. They include V4L1 code when V4L1 is turned off.

To fix this completely:
1. Find all unnecessary includes of videodev.h and remove them.

2. Any remaining includes of videodef.h in drivers which don't depend on VIDEO_V4L1_COMPAT or VIDEO_V4L1 in Kconfig should be protected with #ifdef CONFIG_VIDEO_V4L1_COMPAT. This will break many drivers.

3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere. This will break many drivers.

4. Any drivers broken by steps 2 and 3 should be fixed by either:
A. Protecting all V4L1 code with #ifdef CONFIG_VIDEO_V4L1_COMPAT B. Making the drivers require V4L1 in Kconfig

#10 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Trent,

Em Qui, 2006-07-20 às 14:57 -0700, Trent Piepho escreveu:

Quoted 18 lines · Mauro Carvalho Chehab
Quoted 16 lines

Em Seg, 2006-07-17 s 17:25 -0700, Trent Piepho escreveu:

Quoted 15 lines · Mauro Carvalho Chehab
Quoted 4 lines

Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

I think that the real problem is that many drivers include the V4L1 API file videodev.h when V4L1 is NOT on. Should drivers be providing V4L1 API functions, or need anything from videodev.h, if V4L1 is not on?

It seems like they either need to depend on VIDEO_V4L1 or only include the V4L1 API header file when V4L1 is turned on. Which also means they would need to #ifdef out any V4L1 code when V4L1 is turned off. The bttv driver for example does not do this. It includes a bunch of V4L1 functions even when V4L1 (and V4L1_COMPAT) are turned off.

Quoted 3 lines

I've looked into this more, and there is still a serious bug here. If you turn off V4L1 and V4L1_COMPAT, many drivers will a big issue with struct video_device.

If you turn V4L and V4L1_COMPAT off, most drivers won't compile.

Quoted 4 lines

In some files (saa7134-tvaudio.c, saa6752hs.c, v4l2-common.c, dozens more), V4L1 will be off and HAVE_V4L1 will not be defined. This gives you the struct video_dev _without_ vidiocgmbuf.

Ok.

Quoted 4 lines

In other files (tveeprom.c, tvaudio.c, bttv-driver.c, and many more), V4L1 will still be off (of course) by HAVE_V4L1 _will_ be defined. This gives you the struct viddeo_dev _with_ vidiocgmbuf.

Hmm... tveeprom shouln't include videodev but, instead, videodev2. Anyway, both aren't dependent of v4l2-dev.h. For bttv, it is not really a complete V4L2 driver and should be disabled. You should also notice that tvaudio is part of bttv stuff (although also not dependent of video_device struct and v4l2-dev.h).

Quoted 2 lines

The first thing to solve this that HAVE_V4L1 should die.

Partially agreed. We should move all in-kernel stuff to use CONFIG_VIDEO_V4L1_COMPAT. For userspace, this flag might be interesting (as well as his counterpart HAVE_V4L2).

Quoted 1 line

Why have a define that is supposed to mirror a Kconfig variable?

Legacy stuff. HAVE_V4L1 came before the Kconfig flag.

Quoted 8 lines

If everyone used CONFIG_VIDEO_V4L1_COMPAT then there wouldn't be this problem, which some code things V4L1 is on, and some code thinks it's off.

The second thing, is that many drivers don't respect CONFIG_VIDEO_V4L1_COMPAT. They include V4L1 code when V4L1 is turned off.

To fix this completely:
1. Find all unnecessary includes of videodev.h and remove them.

Ok.

Quoted 4 lines

2. Any remaining includes of videodef.h in drivers which don't depend on VIDEO_V4L1_COMPAT or VIDEO_V4L1 in Kconfig should be protected with #ifdef CONFIG_VIDEO_V4L1_COMPAT. This will break many drivers.

Instead, we should do the opposite: checking for both flags inside videodev.h. If no V4L1 or V4L1_COMPAT, it should just include videodev2.h.

Quoted 2 lines

3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.

Agreed.

Quoted 1 line

This will break many drivers.

Perhaps I missed the point, but I can't see what else would be broken by replacing the check.

Quoted 4 lines

4. Any drivers broken by steps 2 and 3 should be fixed by either:
A. Protecting all V4L1 code with #ifdef CONFIG_VIDEO_V4L1_COMPAT B. Making the drivers require V4L1 in Kconfig

Broken drivers for V4L2 only should be marked in Kconfig, just like bttv. Of course, we should work to fix it. Nickolay did a patch in the past removing V4L1 code from bttv and make it use v4l1-compat module instead. We should work seriously on such patch.

Cheers,
Mauro.

#11 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

I've trimmed the CCs, they were getting big and it didn't look like anyone else is taking part.

I have a fix for the immediate issue of struct video_device, it doesn't fix everything else, but does fix this bug.
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=f3cc7acae78a;style=gitweb

Quoted 5 lines · Mauro Carvalho Chehab

Em Qui, 2006-07-20 �s 14:57 -0700, Trent Piepho escreveu:

Quoted 3 lines

I've looked into this more, and there is still a serious bug here. If you turn off V4L1 and V4L1_COMPAT, many drivers will a big issue with struct video_device.

If you turn V4L and V4L1_COMPAT off, most drivers won't compile.

There are many drivers which depend on VIDEO_V4L1 in Kconfig. But, there are many drivers that don't depend on it. All those drivers will compile with V4L1 turned all the way off. I was able to compile 101 modules without V4L1 turned on.

Quoted 5 lines
Quoted 3 lines

In some files (saa7134-tvaudio.c, saa6752hs.c, v4l2-common.c, dozens more), V4L1 will be off and HAVE_V4L1 will not be defined. This gives you the struct video_dev _without_ vidiocgmbuf.

Ok.

Quoted 1 line
Quoted 8 lines
Quoted 3 lines

In other files (tveeprom.c, tvaudio.c, bttv-driver.c, and many more), V4L1 will still be off (of course) by HAVE_V4L1 _will_ be defined. This gives you the struct viddeo_dev _with_ vidiocgmbuf.

Hmm... tveeprom shouln't include videodev but, instead, videodev2. Anyway, both aren't dependent of v4l2-dev.h. For bttv, it is not really a complete V4L2 driver and should be disabled. You should also notice that tvaudio is part of bttv stuff (although also not dependent of video_device struct and v4l2-dev.h).

Well, tvaudio.c does include v4l2-dev.h. There are more that do this:

bttv-cards.c   bttv-vbi.c        msp3400-kthreads.c  tuner-core.c
bttv-driver.c  compat_ioctl32.c  saa5249.c           tuner-simple.c
bttv-gpio.c    cpia2_core.c      tda7432.c           tvaudio.c
bttv-i2c.c     cpia2_usb.c       tda9875.c           tveeprom.c
bttv-if.c      cpia2_v4l.c       tda9887.c           wm8739.c
bttv-input.c   cs53l32a.c        tlv320aic23b.c      wm8775.c
bttv-risc.c    msp3400-driver.c  tuner-3036.c

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig. There might be more files which have the problem with the defines but don't include v4l2-dev.h, I haven't checked for that.

Quoted 14 lines
Quoted 1 line

The first thing to solve this that HAVE_V4L1 should die.

Partially agreed. We should move all in-kernel stuff to use CONFIG_VIDEO_V4L1_COMPAT. For userspace, this flag might be interesting (as well as his counterpart HAVE_V4L2).

Quoted 1 line

Why have a define that is supposed to mirror a Kconfig variable?

Legacy stuff. HAVE_V4L1 came before the Kconfig flag.

Quoted 8 lines

If everyone used CONFIG_VIDEO_V4L1_COMPAT then there wouldn't be this problem, which some code things V4L1 is on, and some code thinks it's off.

The second thing, is that many drivers don't respect CONFIG_VIDEO_V4L1_COMPAT. They include V4L1 code when V4L1 is turned off.

To fix this completely:
1. Find all unnecessary includes of videodev.h and remove them.

Quoted 8 lines

Ok.

Quoted 4 lines

2. Any remaining includes of videodef.h in drivers which don't depend on VIDEO_V4L1_COMPAT or VIDEO_V4L1 in Kconfig should be protected with #ifdef CONFIG_VIDEO_V4L1_COMPAT. This will break many drivers.

Instead, we should do the opposite: checking for both flags inside videodev.h. If no V4L1 or V4L1_COMPAT, it should just include videodev2.h.

Would some drivers continue to include videodev2.h directly? Or would it only be included through videodev.h?

Quoted 5 lines
Quoted 1 line

3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.

Agreed.

Quoted 1 line

This will break many drivers.

Perhaps I missed the point, but I can't see what else would be broken by replacing the check.

See my list above of files in which HAVE_V4L1 is defined but CONFIG_VIDEO_V4L1* is not defined. If you change an #ifdef from HAVE_V4L1 to CONFIG_VIDEO_V4L1_COMPAT, then that #ifdef is changing from on to off. This can breaks things. Go ahead and try it, you'll see.

Quoted 7 lines
Quoted 3 lines

4. Any drivers broken by steps 2 and 3 should be fixed by either:
A. Protecting all V4L1 code with #ifdef CONFIG_VIDEO_V4L1_COMPAT B. Making the drivers require V4L1 in Kconfig

Broken drivers for V4L2 only should be marked in Kconfig, just like bttv. Of course, we should work to fix it. Nickolay did a patch in the past removing V4L1 code from bttv and make it use v4l1-compat module instead. We should work seriously on such patch.

Is it possible that a driver might include V4L1 compatilbility code, for functionality beyond that which the v4l-compat module can provide?

#12 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Hi, Trent,

Em Sex, 2006-07-21 às 13:06 -0700, Trent Piepho escreveu:

Quoted 2 lines

I've trimmed the CCs, they were getting big and it didn't look like anyone else is taking part.

Ok.

Quoted 33 lines

I have a fix for the immediate issue of struct video_device, it doesn't fix everything else, but does fix this bug.
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=f3cc7acae78a;style=gitweb

Quoted 5 lines · Mauro Carvalho Chehab

Em Qui, 2006-07-20 s 14:57 -0700, Trent Piepho escreveu:

Quoted 3 lines

I've looked into this more, and there is still a serious bug here. If you turn off V4L1 and V4L1_COMPAT, many drivers will a big issue with struct video_device.

If you turn V4L and V4L1_COMPAT off, most drivers won't compile.

There are many drivers which depend on VIDEO_V4L1 in Kconfig. But, there are many drivers that don't depend on it. All those drivers will compile with V4L1 turned all the way off. I was able to compile 101 modules without V4L1 turned on.

Quoted 5 lines
Quoted 3 lines

In some files (saa7134-tvaudio.c, saa6752hs.c, v4l2-common.c, dozens more), V4L1 will be off and HAVE_V4L1 will not be defined. This gives you the struct video_dev _without_ vidiocgmbuf.

Ok.

Quoted 1 line
Quoted 8 lines
Quoted 3 lines

In other files (tveeprom.c, tvaudio.c, bttv-driver.c, and many more), V4L1 will still be off (of course) by HAVE_V4L1 _will_ be defined. This gives you the struct viddeo_dev _with_ vidiocgmbuf.

Hmm... tveeprom shouln't include videodev but, instead, videodev2. Anyway, both aren't dependent of v4l2-dev.h. For bttv, it is not really a complete V4L2 driver and should be disabled. You should also notice that tvaudio is part of bttv stuff (although also not dependent of video_device struct and v4l2-dev.h).

Well, tvaudio.c does include v4l2-dev.h. There are more that do this:

Quoted 7 lines
bttv-cards.c   bttv-vbi.c        msp3400-kthreads.c  tuner-core.c
bttv-driver.c  compat_ioctl32.c  saa5249.c           tuner-simple.c
bttv-gpio.c    cpia2_core.c      tda7432.c           tvaudio.c
bttv-i2c.c     cpia2_usb.c       tda9875.c           tveeprom.c
bttv-if.c      cpia2_v4l.c       tda9887.c           wm8739.c
bttv-input.c   cs53l32a.c        tlv320aic23b.c      wm8775.c
bttv-risc.c    msp3400-driver.c  tuner-3036.c

config VIDEO_BT848
tristate "BT848 Video For Linux" depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!

Quoted 3 lines

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig.

Some of the above drivers are V4L2, like tda9887, tuner-core, tuner-simple, msp3400, cs53l32a, tveeprom, wm87xx. Maybe they are just including the wrong headers. We should try to change to videodev2.h and see what happens with all those drivers. The ones that break should me marked with the proper requirement on Kconfig.

Some of they need some #ifdef inside. For example, compat_ioctl32 should handle both APIs, since it is a generic code to fix 32 bit calls to 64 bit kernel.

Quoted 23 lines

There might be more files which have
the problem with the defines but don't include v4l2-dev.h, I haven't checked for that.

Quoted 19 lines
Quoted 1 line

The first thing to solve this that HAVE_V4L1 should die.

Partially agreed. We should move all in-kernel stuff to use CONFIG_VIDEO_V4L1_COMPAT. For userspace, this flag might be interesting (as well as his counterpart HAVE_V4L2).

Quoted 1 line

Why have a define that is supposed to mirror a Kconfig variable?

Legacy stuff. HAVE_V4L1 came before the Kconfig flag.

Quoted 8 lines

If everyone used CONFIG_VIDEO_V4L1_COMPAT then there wouldn't be this problem, which some code things V4L1 is on, and some code thinks it's off.

The second thing, is that many drivers don't respect CONFIG_VIDEO_V4L1_COMPAT. They include V4L1 code when V4L1 is turned off.

To fix this completely:
1. Find all unnecessary includes of videodev.h and remove them.

Ok.

Quoted 4 lines

2. Any remaining includes of videodef.h in drivers which don't depend on VIDEO_V4L1_COMPAT or VIDEO_V4L1 in Kconfig should be protected with #ifdef CONFIG_VIDEO_V4L1_COMPAT. This will break many drivers.

Quoted 6 lines
Quoted 3 lines

Instead, we should do the opposite: checking for both flags inside videodev.h. If no V4L1 or V4L1_COMPAT, it should just include videodev2.h.

Would some drivers continue to include videodev2.h directly? Or would it only be included through videodev.h?

The idea is that V4L2 drivers should need to include only videodev2, when V4L1 compat is not defined.

Quoted 11 lines
Quoted 6 lines
Quoted 2 lines

3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.

Agreed.

Quoted 1 line

This will break many drivers.

Perhaps I missed the point, but I can't see what else would be broken by replacing the check.

See my list above of files in which HAVE_V4L1 is defined but CONFIG_VIDEO_V4L1* is not defined. If you change an #ifdef from HAVE_V4L1 to CONFIG_VIDEO_V4L1_COMPAT, then that #ifdef is changing from on to off. This can breaks things. Go ahead and try it, you'll see.

So, we need to fix it. May you work on such patch?

Quoted 11 lines
Quoted 8 lines
Quoted 4 lines

4. Any drivers broken by steps 2 and 3 should be fixed by either:
A. Protecting all V4L1 code with #ifdef CONFIG_VIDEO_V4L1_COMPAT B. Making the drivers require V4L1 in Kconfig

Broken drivers for V4L2 only should be marked in Kconfig, just like bttv. Of course, we should work to fix it. Nickolay did a patch in the past removing V4L1 code from bttv and make it use v4l1-compat module instead. We should work seriously on such patch.

Is it possible that a driver might include V4L1 compatilbility code, for functionality beyond that which the v4l-compat module can provide?

Yes. There is just *one* ioctl that should be done driver by driver, for buffer allocation on V4L1 model.

Cheers,
Mauro.

#13 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 5 lines · Mauro Carvalho Chehab

config VIDEO_BT848
tristate "BT848 Video For Linux" depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!

You can compile and use bt848 without V4L1 turned on. It still has some V4L1 functions defined.

Quoted 9 lines
Quoted 3 lines

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig.

Some of the above drivers are V4L2, like tda9887, tuner-core, tuner-simple, msp3400, cs53l32a, tveeprom, wm87xx. Maybe they are just including the wrong headers. We should try to change to videodev2.h and see what happens with all those drivers. The ones that break should me marked with the proper requirement on Kconfig.

Quoted 3 lines

Some of they need some #ifdef inside. For example, compat_ioctl32 should handle both APIs, since it is a generic code to fix 32 bit calls to 64 bit kernel.

I think this is pretty much what I've been saying. Drivers should:
A. Not include videodev.h, but use videodev2.h
B. Include videodev.h, but be marked V4L1 in Kconfig
C. #ifdef around videodev.h (and code that needs videodev.h), so it
is not included or needed when V4L1 is turned off.

#14 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Ok, I had some time to fix the broken dependencies. Please look at the two commits I did today.

One patch removes HAVE_V4L1 check at drivers. It also include some checks for V4L1_COMPAT on some core files that should implement both calls to provide support for V4L1 drivers.

The other fixes the broken dependencies for drivers that still need V4L1 to work properly.

Cheers,
Mauro.

Em Sex, 2006-07-21 às 15:55 -0700, Trent Piepho escreveu:

Quoted 23 lines · Mauro Carvalho Chehab
Quoted 5 lines

config VIDEO_BT848
tristate "BT848 Video For Linux" depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!

You can compile and use bt848 without V4L1 turned on. It still has some V4L1 functions defined.

Quoted 9 lines
Quoted 3 lines

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig.

Some of the above drivers are V4L2, like tda9887, tuner-core, tuner-simple, msp3400, cs53l32a, tveeprom, wm87xx. Maybe they are just including the wrong headers. We should try to change to videodev2.h and see what happens with all those drivers. The ones that break should me marked with the proper requirement on Kconfig.

Quoted 3 lines

Some of they need some #ifdef inside. For example, compat_ioctl32 should handle both APIs, since it is a generic code to fix 32 bit calls to 64 bit kernel.

Quoted 5 lines

I think this is pretty much what I've been saying. Drivers should:
A. Not include videodev.h, but use videodev2.h
B. Include videodev.h, but be marked V4L1 in Kconfig
C. #ifdef around videodev.h (and code that needs videodev.h), so it
is not included or needed when V4L1 is turned off.

Cheers,
Mauro.

#15 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 2 lines · Mauro Carvalho Chehab

Ok, I had some time to fix the broken dependencies. Please look at the two commits I did today.

Two things I noticed. bttv didn't use to _require_ V4L1. I use bttv and have V4L1 turned off. I guess this is just because bttv ignored the V4L1 setting?

Second, the fix for class_device_create_file() doesn't roll-back properly on failure. I already posted a patch which does this correctly. Attached is the patch against the current Hg, go ahead and import it.

Also attached is patch to bttv-driver.c that has it use class_device_create_file(), with an error message and handling of failure.

Quoted 34 lines

One patch removes HAVE_V4L1 check at drivers. It also include some checks for V4L1_COMPAT on some core files that should implement both calls to provide support for V4L1 drivers.

The other fixes the broken dependencies for drivers that still need V4L1 to work properly.

Cheers,
Mauro.

Em Sex, 2006-07-21 �s 15:55 -0700, Trent Piepho escreveu:

Quoted 23 lines · Mauro Carvalho Chehab
Quoted 5 lines

config VIDEO_BT848
tristate "BT848 Video For Linux" depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!

You can compile and use bt848 without V4L1 turned on. It still has some V4L1 functions defined.

Quoted 12 lines
Quoted 3 lines

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig.

Some of the above drivers are V4L2, like tda9887, tuner-core, tuner-simple, msp3400, cs53l32a, tveeprom, wm87xx. Maybe they are just including the wrong headers. We should try to change to videodev2.h and see what happens with all those drivers. The ones that break should me marked with the proper requirement on Kconfig.

Some of they need some #ifdef inside. For example, compat_ioctl32 should handle both APIs, since it is a generic code to fix 32 bit calls to 64 bit kernel.

Quoted 9 lines
Quoted 5 lines

I think this is pretty much what I've been saying. Drivers should:
A. Not include videodev.h, but use videodev2.h
B. Include videodev.h, but be marked V4L1 in Kconfig
C. #ifdef around videodev.h (and code that needs videodev.h), so it
is not included or needed when V4L1 is turned off.

Cheers,
Mauro.

# HG changeset patch
# User Trent Piepho <[email protected]> # Node ID b900796fdfb4bc87642c470515f88ed7ae92ced3 # Parent e33331856212e707150dc85fda486240eb7feae0 Handle class_device_create_file errors

From: Trent Piepho <[email protected]>

Add proper error checking and roll-back for failure of class_device_create_file() in videodev.c. Print error messages and unroll partially created sysfs entries.

Also, failure of class_device_register() in video_register_device() is handled correctly. It was failing to de-allocate the minor number. This must be done in video_register_device(), since the caller has no way of knowing if failure occurred before or after the class device was registered.

Also added an error message if video_register_device() is called with an unknown type, which should never happen.

Signed-off-by Trent Piepho <[email protected]>
linux/drivers/media/video/videodev.c +23 -10
diff -r e33331856212 -r b900796fdfb4 linux/drivers/media/video/videodev.c
@@ -1557,6 +1557,8 @@ int video_register_device(struct video_d
 			name_base = "radio";
 			break;
 		default:
+			printk(KERN_ERR "%s called with unknown type: %d\n",
+			       __FUNCTION__, type);
 			return -1;
 	}
 

@@ -1597,18 +1599,20 @@ int video_register_device(struct video_d
 	if (ret) {
 		printk(KERN_ERR "%s: class_device_register failed\n",
 		       __FUNCTION__);
-		return ret;
-	}
-        ret = class_device_create_file(&vfd->class_dev, &class_device_attr_name);
-        if (ret < 0) {
-                printk(KERN_WARNING "%s error: %d\n", __FUNCTION__, ret);
-		return ret;
+		goto fail_minor;
+	}
+	ret = class_device_create_file(&vfd->class_dev, &class_device_attr_name);
+	if (ret) {
+		printk(KERN_ERR "%s: class_device_create_file 'name' failed\n",
+		       __FUNCTION__);
+		goto fail_classdev;
 	}
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,12)
-        ret = class_device_create_file(&vfd->class_dev, &class_device_attr_dev);
-        if (ret < 0) {
-                printk(KERN_WARNING "%s error: %d\n", __FUNCTION__, ret);
-		return ret;
+	ret = class_device_create_file(&vfd->class_dev, &class_device_attr_dev);
+	if (ret) {
+		printk(KERN_ERR "%s: class_device_create_file 'dev' failed\n",
+		       __FUNCTION__);
+		goto fail_classdev;
 	}
 #endif
 

@@ -1620,6 +1624,15 @@ int video_register_device(struct video_d
 		       "http://lwn.net/Articles/36850/\n", vfd->name);
 #endif
 	return 0;
+
+fail_classdev:
+	class_device_unregister(&vfd->class_dev);
+fail_minor:
+	mutex_lock(&videodev_lock);
+	video_device[vfd->minor] = NULL;
+	vfd->minor = -1;
+	mutex_unlock(&videodev_lock);
+	return ret;
 }
 
 /**

# HG changeset patch
# User Trent Piepho <[email protected]>
# Node ID 092b04007c0f1aa2d3f9b4467ea8ab3837723196 # Parent b900796fdfb4bc87642c470515f88ed7ae92ced3
bttv: use class_device_create_file and handle errors

From: Trent Piepho <[email protected]>

Revery bttv-driver.c from video_device_create_file() to use class_device_create_file() again. video_device_create_file() is only available when V4L1 is on.

Proper error checking is added for failure of class_device_create_file(). Will print error message and unroll partially created sysfs entries.

Signed-off-by Trent Piepho <[email protected]>
linux/drivers/media/video/bt8xx/bttv-driver.c +6 -2
diff -r b900796fdfb4 -r 092b04007c0f linux/drivers/media/video/bt8xx/bttv-driver.c
@@ -3957,8 +3957,12 @@ static int __devinit bttv_register_video
 	printk(KERN_INFO "bttv%d: registered device video%d\n",
 	       btv->c.nr,btv->video_dev->minor & 0x1f);
 #if LINUX_VERSION_CODE > KERNEL_VERSION(2,5,0)
-
-	video_device_create_file(btv->video_dev, &class_device_attr_card);
+	if (class_device_create_file(&btv->video_dev->class_dev,
+				     &class_device_attr_card)<0) {
+		printk(KERN_ERR "bttv%d: class_device_create_file 'card' "
+		       "failed\n", btv->c.nr);
+		goto err;
+	}
 #endif
 
 	/* vbi */
#16 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Em Dom, 2006-07-23 às 19:16 -0700, Trent Piepho escreveu:

Quoted 7 lines · Mauro Carvalho Chehab
Quoted 2 lines

Ok, I had some time to fix the broken dependencies. Please look at the two commits I did today.

Two things I noticed. bttv didn't use to _require_ V4L1. I use bttv and have V4L1 turned off. I guess this is just because bttv ignored the V4L1 setting?

In fact, as videodev.h were included, all V4L1 stuff were also added. Now, if you try to compile it with V4L1, it will fail. Bttv have support for both V4L1 and V4L2 inside, but still relies on several V4L1 stuff at the code Nickolay did a patch in the past to remove V4L1, using, instead, v4l1_compat mode. At that time, it suffered some troubles, and his patch became obsoleted by several changes (basically at msp3400). We should get his patch again, port to current version and fix.

Quoted 3 lines

Second, the fix for class_device_create_file() doesn't roll-back properly on failure. I already posted a patch which does this correctly. Attached is the patch against the current Hg, go ahead and import it.

On your hg, there are 4 patches to be applied:

1) Fix bug where struct video_device was not defined consistently This one seems to conflict with some previous changes I did. Please remove it from your tree (you need to re-clone it - you may use hg-menu to help on this stuff);

2) Remove of couple of useless lines
Seems ok.

3) Handle class_device_create_file in V4L2 drivers The patch didn't applied well at current tree.

4) bttv: clean up some pre-2.6.0 class device compat stuff the removal of class_device_create_file on compat.h won't break compilation against kernel versions 2.4? Yes, I know that current building system won't allow such compilations, but I don't want to remove compat stuff for 2.4. Maybe someone may work later on make V4L to work well on 2.4. This would be interesting to embedded systems.

Please re-do your tree and ask me to pull after done.

Quoted 48 lines

Also attached is patch to bttv-driver.c that has it use class_device_create_file(), with an error message and handling of failure.

Quoted 43 lines

One patch removes HAVE_V4L1 check at drivers. It also include some checks for V4L1_COMPAT on some core files that should implement both calls to provide support for V4L1 drivers.

The other fixes the broken dependencies for drivers that still need V4L1 to work properly.

Cheers,
Mauro.

Em Sex, 2006-07-21 s 15:55 -0700, Trent Piepho escreveu:

Quoted 23 lines · Mauro Carvalho Chehab
Quoted 5 lines

config VIDEO_BT848
tristate "BT848 Video For Linux" depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!

You can compile and use bt848 without V4L1 turned on. It still has some V4L1 functions defined.

Quoted 9 lines
Quoted 3 lines

All these files include v4l2-dev.h and have HAVE_V4L1 defined when V4L1 is not turned on in Kconfig. There files are all buildable when V4L1 is off; they don't depend on it in Kconfig.

Some of the above drivers are V4L2, like tda9887, tuner-core, tuner-simple, msp3400, cs53l32a, tveeprom, wm87xx. Maybe they are just including the wrong headers. We should try to change to videodev2.h and see what happens with all those drivers. The ones that break should me marked with the proper requirement on Kconfig.

Quoted 3 lines

Some of they need some #ifdef inside. For example, compat_ioctl32 should handle both APIs, since it is a generic code to fix 32 bit calls to 64 bit kernel.

Quoted 5 lines

I think this is pretty much what I've been saying. Drivers should:
A. Not include videodev.h, but use videodev2.h
B. Include videodev.h, but be marked V4L1 in Kconfig
C. #ifdef around videodev.h (and code that needs videodev.h), so it
is not included or needed when V4L1 is turned off.

Cheers,
Mauro.

Cheers,
Mauro.

#17 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 10 lines · Mauro Carvalho Chehab

Em Dom, 2006-07-23 �s 19:16 -0700, Trent Piepho escreveu:

Quoted 3 lines

Second, the fix for class_device_create_file() doesn't roll-back properly on failure. I already posted a patch which does this correctly. Attached is the patch against the current Hg, go ahead and import it.

On your hg, there are 4 patches to be applied:

1) Fix bug where struct video_device was not defined consistently This one seems to conflict with some previous changes I did. Please remove it from your tree (you need to re-clone it - you may use hg-menu to help on this stuff);

My Hg tree was outdated by your patches. If you look at the time, I made that fix on Jul 20th, then your comprehensive patch on Jul 23rd incorporated that part of the fix.

Quoted 2 lines

3) Handle class_device_create_file in V4L2 drivers The patch didn't applied well at current tree.

I know, that's why I attached a version to my email that would. I have now re-done my tree to take into account the current code.

Quoted 3 lines

4) bttv: clean up some pre-2.6.0 class device compat stuff the removal of class_device_create_file on compat.h won't break compilation against kernel versions 2.4? Yes, I know that current

There was a check in bttv-driver.c that would skip class_device_create_file() for older kernels (it got the version wrong, but same idea). I just moved this check into compat.h so that the bttv code wouldn't be as cluttered. My change shouldn't make any difference at all for bttv under 2.4. It is just to clean up the code; put compat stuff in compat.h and not in the drivers when possible.

Other code which called c_d_c_f() will now skip it the same way bttv did under old kernels. All the class_device stuff doesn't exist under 2.4, and there will be a hundred class_device related breakages. So maybe this patch fixes one of those hundred breakages.

Applied
#18 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Em Seg, 2006-07-24 às 15:06 -0700, Trent Piepho escreveu:

Quoted 7 lines · Mauro Carvalho Chehab
Quoted 6 lines

Em Dom, 2006-07-23 s 19:16 -0700, Trent Piepho escreveu:

Quoted 3 lines

Second, the fix for class_device_create_file() doesn't roll-back properly on failure. I already posted a patch which does this correctly. Attached is the patch against the current Hg, go ahead and import it.

On your hg, there are 4 patches to be applied:

Quoted 8 lines
Quoted 4 lines

1) Fix bug where struct video_device was not defined consistently This one seems to conflict with some previous changes I did. Please remove it from your tree (you need to re-clone it - you may use hg-menu to help on this stuff);

My Hg tree was outdated by your patches. If you look at the time, I made that fix on Jul 20th, then your comprehensive patch on Jul 23rd incorporated that part of the fix.

The point is that your commit ask arrived after I've applied my patches.

Quoted 6 lines
Quoted 3 lines

3) Handle class_device_create_file in V4L2 drivers The patch didn't applied well at current tree.

I know, that's why I attached a version to my email that would. I have now re-done my tree to take into account the current code.

Applied. I'm submitting the current stuff to both mainstream and -mm series.

Cheers,
Mauro.

#19 lore raw

Re: [PATCH] V4L: struct video_device corruption

In reply to Mauro Carvalho Chehab
Quoted 8 lines · On Sat, 15 Jul 2006 22:31:04 -0300 Mauro Carvalho Chehab…

Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

So I've lost the plot with all of this. Does the current git-dvb contain the desired fixes?

Do we expect this will fix the various DVB crashes which people (including Alex) have reported?

Thanks.

#20 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 12 lines · Andrew Morton
Quoted 8 lines · On Sat, 15 Jul 2006 22:31:04 -0300 Mauro Carvalho Chehab…

Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

Quoted 5 lines

So I've lost the plot with all of this. Does the current git-dvb contain the desired fixes?

Do we expect this will fix the various DVB crashes which people (including Alex) have reported?

I believe you are referring to the "oops in bttv" thread.

As i understood, the case that Alex reported in was a null pointer issue due to which modprobe bttv crashes, rather than a DVB issue.

Call Trace:
 [<c018763c>] sysfs_create_file+0x26/0x28
 [<c0220cc3>] class_device_create_file+0x14/0x1a
 [<f9bc3073>] bttv_register_video+0x8c/0x147 [bttv]
 [<f9bc35de>] bttv_probe+0x4ab/0x593 [bttv]

Therefore the V4L fixes should probably fix the same as well.

Regards,
Manu

#21 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

In reply to Andrew Morton
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Sat, 15 Jul 2006 22:31:04 -0300 Mauro Carvalho Chehab…

Em S�b, 2006-07-15 �s 23:08 +0000, Robert Fitzsimons escreveu:

Quoted 3 lines

The layout of struct video_device would change depending on whether videodev.h (V4L1) was include or not before v4l2-dev.h, which caused the structure to get corrupted.

Quoted 7 lines
Quoted 4 lines

Hmm... good point! However, I the proper solution would be to trust on CONFIG_VIDEO_V4L1_COMPAT or CONFIG_VIDEO_V4L1 instead. it makes no sense to keep a pointer to an unsupported callback, when V4L1 is not selected.

So I've lost the plot with all of this. Does the current git-dvb contain the desired fixes?

The problem was that the v4l code in general was not using CONFIG_VIDEO_V4L1* like it should, but was detecting V4L1 support based on whether on not the V4L1 header file had been included.

Some drivers didn't depend on V4L1 in Kconfig, and would include the V4L1 header when V4L1 was off. This caused some code to think V4L1 was off and some code to think it was on.

This caused a serious bug with inconsistent defintions of struct video_device, as well as other problems.

I posted a patch that fixed the video_device problem, then Mauro made a comprehensive patch that incorporated this fix and well as all the other code that was using V4L1 when it shouldn't. That patch is in the v4l-dvb Mercurial repository, but not moved on to git yet.

Quoted 2 lines

Do we expect this will fix the various DVB crashes which people (including Alex) have reported?

This problem would only appear if VIDEO_V4L1 was turned off. If it was on, then all the code would agree it was on, and there would be no problems. If the crash is still there when VIDEO_V4L1 = y, then it's not related to this bug.

If VIDEO_V4L1 was turned off, then some drivers (one of which is bttv) would have a different struct video_device than the video core code. This would break things so completely that it could crash just about anywhere.

#22 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Quoted 11 lines · On Tue, 25 Jul 2006 01:42:19 -0700 (PDT) Trent Piepho <[email protected]>…
Quoted 2 lines

Do we expect this will fix the various DVB crashes which people (including Alex) have reported?

This problem would only appear if VIDEO_V4L1 was turned off. If it was on, then all the code would agree it was on, and there would be no problems. If the crash is still there when VIDEO_V4L1 = y, then it's not related to this bug.

If VIDEO_V4L1 was turned off, then some drivers (one of which is bttv) would have a different struct video_device than the video core code. This would break things so completely that it could crash just about anywhere.

OK, thanks. Alex had

# CONFIG_VIDEO_V4L1 is not set
# CONFIG_VIDEO_V4L1_COMPAT is not set
CONFIG_VIDEO_V4L2=y

#23 lore raw

Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption

Andrew,

Em Ter, 2006-07-25 às 02:06 -0700, Andrew Morton escreveu:

Quoted 15 lines
Quoted 11 lines · On Tue, 25 Jul 2006 01:42:19 -0700 (PDT) Trent Piepho <[email protected]>…
Quoted 2 lines

Do we expect this will fix the various DVB crashes which people (including Alex) have reported?

This problem would only appear if VIDEO_V4L1 was turned off. If it was on, then all the code would agree it was on, and there would be no problems. If the crash is still there when VIDEO_V4L1 = y, then it's not related to this bug.

If VIDEO_V4L1 was turned off, then some drivers (one of which is bttv) would have a different struct video_device than the video core code. This would break things so completely that it could crash just about anywhere.

Quoted 5 lines

OK, thanks. Alex had

# CONFIG_VIDEO_V4L1 is not set
# CONFIG_VIDEO_V4L1_COMPAT is not set
CONFIG_VIDEO_V4L2=y

That's explains the bug.

I'll send this patch, together with several other patches (including several V4L1 to V4L2 conversions) today to -git.

Quoted 1 line

Cheers,
Mauro.