Re: oops in bttv

Re: oops in bttv

[Chuck Ebbert]

In-Reply-To: <20060711204940.GA11497@steel.home>

On Tue, 11 Jul 2006 22:49:40 +0200, Alex Riesen wrote:

> 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
>  [<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.

-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: oops in bttv

[Greg KH]

On Thu, Jul 13, 2006 at 12:43:49AM -0400, Chuck Ebbert wrote:
> In-Reply-To: <20060711204940.GA11497@steel.home>
> 
> On Tue, 11 Jul 2006 22:49:40 +0200, Alex Riesen wrote:
> 
> > 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
> >  [<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.

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?

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

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

thanks,

greg k-h

Re: oops in bttv

[Randy.Dunlap]

On Wed, 12 Jul 2006 22:05:41 -0700 Greg KH wrote:

> On Thu, Jul 13, 2006 at 12:43:49AM -0400, Chuck Ebbert wrote:
> > In-Reply-To: <20060711204940.GA11497@steel.home>
> > 
> > On Tue, 11 Jul 2006 22:49:40 +0200, Alex Riesen wrote:
> > 
> > > 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
> > >  [<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.
> 
> 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.

> 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?
> 
> > I tried tracing through that and... gave up.
> 
> Heh, yeah, it's not exactly a "simple" path...

---
~Randy

Re: oops in bttv

[Andrew Morton]

On Wed, 12 Jul 2006 22:24:07 -0700
"Randy.Dunlap" <rdunlap@xenotime.net> wrote:

> > > 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.
> > 
> > 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.

[PATCH] V4L: struct video_device corruption

[Robert Fitzsimons]

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 <robfitz@273k.net>
---
 include/media/v4l2-dev.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 62dae1a..69059d8 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -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,
-- 
1.4.1.ga3e6

Re: [PATCH] V4L: struct video_device corruption

[Mauro Carvalho Chehab]

Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:
> 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.

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1581 bytes --]

On Sat, 15 Jul 2006, Mauro Carvalho Chehab wrote:
> Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:
> > 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.

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

[Mauro Carvalho Chehab]

Em Seg, 2006-07-17 às 17:25 -0700, Trent Piepho escreveu:
> On Sat, 15 Jul 2006, Mauro Carvalho Chehab wrote:
> > Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:
> > > 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
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.

> 
> 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.
Yes, you are right. Checking for COMPAT covers both cases.

Cheers, 
Mauro.

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

[Michael Krufky]

Mauro Carvalho Chehab wrote:
> Em Seg, 2006-07-17 às 17:25 -0700, Trent Piepho escreveu:
>> On Sat, 15 Jul 2006, Mauro Carvalho Chehab wrote:
>>> Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:
>>>> 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
> 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.
-- 
Michael Krufky

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 2545 bytes --]

On Wed, 19 Jul 2006, Mauro Carvalho Chehab wrote:
> Em Seg, 2006-07-17 às 17:25 -0700, Trent Piepho escreveu:
> > On Sat, 15 Jul 2006, Mauro Carvalho Chehab wrote:
> > > Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:
> > > > 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

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

[Mauro Carvalho Chehab]

Trent,

Em Qui, 2006-07-20 às 14:57 -0700, Trent Piepho escreveu:
> On Wed, 19 Jul 2006, Mauro Carvalho Chehab wrote:
> > Em Seg, 2006-07-17 s 17:25 -0700, Trent Piepho escreveu:
> > > On Sat, 15 Jul 2006, Mauro Carvalho Chehab wrote:
> > > > Em Sb, 2006-07-15 s 23:08 +0000, Robert Fitzsimons escreveu:
> > > > > 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.
If you turn V4L and V4L1_COMPAT off, most drivers won't compile.
> 
> 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.
> 
> 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).
> 
> 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).
> Why have a define that is supposed to mirror a Kconfig variable?
Legacy stuff. HAVE_V4L1 came before the Kconfig flag.
> 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.
>
> 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. 
> 
> 3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.
Agreed.
>    This will break many drivers.
Perhaps I missed the point, but I can't see what else would be broken by
replacing the check.
> 
> 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.

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 4781 bytes --]

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

On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> Em Qui, 2006-07-20 às 14:57 -0700, Trent Piepho escreveu:
> > 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.

> > 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.
> >
> > 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.

> > 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).
> > Why have a define that is supposed to mirror a Kconfig variable?
> Legacy stuff. HAVE_V4L1 came before the Kconfig flag.
> > 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.
> >
> > 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?

> > 3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.
> Agreed.
> >    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.

> > 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?

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

[Mauro Carvalho Chehab]

Hi, Trent,

Em Sex, 2006-07-21 às 13:06 -0700, Trent Piepho escreveu:
> I've trimmed the CCs, they were getting big and it didn't look like anyone
> else is taking part.
Ok.
> 
> 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
> 
> On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> > Em Qui, 2006-07-20 s 14:57 -0700, Trent Piepho escreveu:
> > > 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.
> 
> > > 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.
> > >
> > > 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
config VIDEO_BT848
        tristate "BT848 Video For Linux"
        depends on VIDEO_DEV && PCI && I2C && VIDEO_V4L2

Argh! it should be V4L1 instead!
 

> 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.

>  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.
> 
> > > 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).
> > > Why have a define that is supposed to mirror a Kconfig variable?
> > Legacy stuff. HAVE_V4L1 came before the Kconfig flag.
> > > 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.
> > >
> > > 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?
The idea is that V4L2 drivers should need to include only videodev2,
when V4L1 compat is not defined.
> 
> > > 3. Replace HAVE_V4L1 with CONFIG_VIDEO_V4L1_COMPAT everywhere.
> > Agreed.
> > >    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?
> 
> > > 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.

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

[Trent Piepho]

On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> 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.

> > 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.

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.

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

[Mauro Carvalho Chehab]

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:
> On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> > 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.
> 
> > > 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.
> 
> 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.

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 2501 bytes --]

On Sun, 23 Jul 2006, Mauro Carvalho Chehab wrote:
> 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.


> 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:
> > On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> > > 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.
> >
> > > > 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.
> >
> > 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.
>
>

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2823 bytes --]

# HG changeset patch
# User Trent Piepho <xyzzy@speakeasy.org>
# Node ID b900796fdfb4bc87642c470515f88ed7ae92ced3
# Parent  e33331856212e707150dc85fda486240eb7feae0
Handle class_device_create_file errors

From: Trent Piepho <xyzzy@speakeasy.org>

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 <xyzzy@speakeasy.org>

diff -r e33331856212 -r b900796fdfb4 linux/drivers/media/video/videodev.c
--- a/linux/drivers/media/video/videodev.c	Sun Jul 23 18:36:01 2006 -0700
+++ b/linux/drivers/media/video/videodev.c	Sun Jul 23 19:08:21 2006 -0700
@@ -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;
 }
 
 /**

[-- Attachment #3: Type: TEXT/PLAIN, Size: 1424 bytes --]

# HG changeset patch
# User Trent Piepho <xyzzy@speakeasy.org>
# Node ID 092b04007c0f1aa2d3f9b4467ea8ab3837723196
# Parent  b900796fdfb4bc87642c470515f88ed7ae92ced3
bttv: use class_device_create_file and handle errors

From: Trent Piepho <xyzzy@speakeasy.org>

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 <xyzzy@speakeasy.org>

diff -r b900796fdfb4 -r 092b04007c0f linux/drivers/media/video/bt8xx/bttv-driver.c
--- a/linux/drivers/media/video/bt8xx/bttv-driver.c	Sun Jul 23 19:08:21 2006 -0700
+++ b/linux/drivers/media/video/bt8xx/bttv-driver.c	Sun Jul 23 19:10:52 2006 -0700
@@ -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 */

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

[Mauro Carvalho Chehab]

Em Dom, 2006-07-23 às 19:16 -0700, Trent Piepho escreveu:
> On Sun, 23 Jul 2006, Mauro Carvalho Chehab wrote:
> > 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.
> 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.
> 
> Also attached is patch to bttv-driver.c that has it use
> class_device_create_file(), with an error message and handling of failure.
> 
> 
> > 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:
> > > On Fri, 21 Jul 2006, Mauro Carvalho Chehab wrote:
> > > > 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.
> > >
> > > > > 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.
> > >
> > > 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.

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1887 bytes --]

On Mon, 24 Jul 2006, Mauro Carvalho Chehab wrote:
> Em Dom, 2006-07-23 às 19:16 -0700, Trent Piepho escreveu:
> > 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.

> 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.

> 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.

Re: [PATCH] V4L: struct video_device corruption

[Andrew Morton]

On Sat, 15 Jul 2006 22:31:04 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:

> Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:
> > 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.

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

[Manu Abraham]

Andrew Morton wrote:
> On Sat, 15 Jul 2006 22:31:04 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> 
>> Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:
>>> 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?

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

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

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 2026 bytes --]

On Mon, 24 Jul 2006, Andrew Morton wrote:
> On Sat, 15 Jul 2006 22:31:04 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
>
> > Em Sáb, 2006-07-15 às 23:08 +0000, Robert Fitzsimons escreveu:
> > > 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?

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.

> 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.

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

[Andrew Morton]

On Tue, 25 Jul 2006 01:42:19 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> > 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

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

[Mauro Carvalho Chehab]

Andrew,

Em Ter, 2006-07-25 às 02:06 -0700, Andrew Morton escreveu:
> On Tue, 25 Jul 2006 01:42:19 -0700 (PDT)
> Trent Piepho <xyzzy@speakeasy.org> wrote:
> 
> > > 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
That's explains the bug. 

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

> 
Cheers, 
Mauro.

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

[Mauro Carvalho Chehab]

Em Seg, 2006-07-24 às 15:06 -0700, Trent Piepho escreveu:
> On Mon, 24 Jul 2006, Mauro Carvalho Chehab wrote:
> > Em Dom, 2006-07-23 s 19:16 -0700, Trent Piepho escreveu:
> > > 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.
The point is that your commit ask arrived after I've applied my patches.
> 
> > 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.