* Re: oops in bttv
@ 2006-07-13 4:43 Chuck Ebbert
2006-07-13 5:05 ` Greg KH
0 siblings, 1 reply; 24+ messages in thread
From: Chuck Ebbert @ 2006-07-13 4:43 UTC (permalink / raw)
To: Alex Riesen
Cc: linux-kernel, Mauro Carvalho Chehab, Greg KH, Andrew Morton,
Stephen Hemminger, video4linux-list, v4l-dvb-maintainer
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: oops in bttv 2006-07-13 4:43 oops in bttv Chuck Ebbert @ 2006-07-13 5:05 ` Greg KH 2006-07-13 5:24 ` Randy.Dunlap 0 siblings, 1 reply; 24+ messages in thread From: Greg KH @ 2006-07-13 5:05 UTC (permalink / raw) To: Chuck Ebbert Cc: Alex Riesen, linux-kernel, Mauro Carvalho Chehab, Andrew Morton, Stephen Hemminger, video4linux-list, v4l-dvb-maintainer 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: oops in bttv 2006-07-13 5:05 ` Greg KH @ 2006-07-13 5:24 ` Randy.Dunlap 2006-07-13 5:44 ` Andrew Morton 0 siblings, 1 reply; 24+ messages in thread From: Randy.Dunlap @ 2006-07-13 5:24 UTC (permalink / raw) To: Greg KH Cc: 76306.1226, fork0, linux-kernel, mchehab, akpm, shemminger, video4linux-list, v4l-dvb-maintainer 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: oops in bttv 2006-07-13 5:24 ` Randy.Dunlap @ 2006-07-13 5:44 ` Andrew Morton 2006-07-15 23:08 ` [PATCH] V4L: struct video_device corruption Robert Fitzsimons 0 siblings, 1 reply; 24+ messages in thread From: Andrew Morton @ 2006-07-13 5:44 UTC (permalink / raw) To: Randy.Dunlap Cc: greg, 76306.1226, fork0, linux-kernel, mchehab, shemminger, video4linux-list, v4l-dvb-maintainer 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] V4L: struct video_device corruption 2006-07-13 5:44 ` Andrew Morton @ 2006-07-15 23:08 ` Robert Fitzsimons 2006-07-16 1:31 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Robert Fitzsimons @ 2006-07-15 23:08 UTC (permalink / raw) To: Andrew Morton Cc: Randy.Dunlap, greg, 76306.1226, fork0, linux-kernel, mchehab, shemminger, video4linux-list, v4l-dvb-maintainer 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] V4L: struct video_device corruption 2006-07-15 23:08 ` [PATCH] V4L: struct video_device corruption Robert Fitzsimons @ 2006-07-16 1:31 ` Mauro Carvalho Chehab 2006-07-18 0:25 ` [v4l-dvb-maintainer] " Trent Piepho 2006-07-25 3:08 ` Andrew Morton 0 siblings, 2 replies; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-16 1:31 UTC (permalink / raw) To: Robert Fitzsimons Cc: Andrew Morton, Randy.Dunlap, greg, 76306.1226, fork0, linux-kernel, shemminger, video4linux-list, v4l-dvb-maintainer 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-16 1:31 ` Mauro Carvalho Chehab @ 2006-07-18 0:25 ` Trent Piepho 2006-07-19 11:54 ` Mauro Carvalho Chehab 2006-07-25 3:08 ` Andrew Morton 1 sibling, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-18 0:25 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Robert Fitzsimons, Andrew Morton, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, Randy.Dunlap, v4l-dvb maintainer list, shemminger [-- 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-18 0:25 ` [v4l-dvb-maintainer] " Trent Piepho @ 2006-07-19 11:54 ` Mauro Carvalho Chehab 2006-07-19 15:36 ` Michael Krufky 2006-07-20 21:57 ` Trent Piepho 0 siblings, 2 replies; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-19 11:54 UTC (permalink / raw) To: Trent Piepho Cc: Robert Fitzsimons, Andrew Morton, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, Randy.Dunlap, v4l-dvb maintainer list, shemminger 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-19 11:54 ` Mauro Carvalho Chehab @ 2006-07-19 15:36 ` Michael Krufky 2006-07-20 21:57 ` Trent Piepho 1 sibling, 0 replies; 24+ messages in thread From: Michael Krufky @ 2006-07-19 15:36 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Trent Piepho, Robert Fitzsimons, Andrew Morton, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, Randy.Dunlap, v4l-dvb maintainer list, shemminger 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-19 11:54 ` Mauro Carvalho Chehab 2006-07-19 15:36 ` Michael Krufky @ 2006-07-20 21:57 ` Trent Piepho 2006-07-21 12:26 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-20 21:57 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Robert Fitzsimons, Andrew Morton, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, Randy.Dunlap, v4l-dvb maintainer list, shemminger [-- 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-20 21:57 ` Trent Piepho @ 2006-07-21 12:26 ` Mauro Carvalho Chehab 2006-07-21 20:06 ` Trent Piepho 0 siblings, 1 reply; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-21 12:26 UTC (permalink / raw) To: Trent Piepho Cc: Robert Fitzsimons, Andrew Morton, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, Randy.Dunlap, v4l-dvb maintainer list, shemminger, Nickolay V. Shmyrev 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-21 12:26 ` Mauro Carvalho Chehab @ 2006-07-21 20:06 ` Trent Piepho 2006-07-21 20:30 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-21 20:06 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel [-- 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-21 20:06 ` Trent Piepho @ 2006-07-21 20:30 ` Mauro Carvalho Chehab 2006-07-21 22:55 ` Trent Piepho 0 siblings, 1 reply; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-21 20:30 UTC (permalink / raw) To: Trent Piepho Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-21 20:30 ` Mauro Carvalho Chehab @ 2006-07-21 22:55 ` Trent Piepho 2006-07-23 9:35 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-21 22:55 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-21 22:55 ` Trent Piepho @ 2006-07-23 9:35 ` Mauro Carvalho Chehab 2006-07-24 2:16 ` Trent Piepho 0 siblings, 1 reply; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-23 9:35 UTC (permalink / raw) To: Trent Piepho Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-23 9:35 ` Mauro Carvalho Chehab @ 2006-07-24 2:16 ` Trent Piepho 2006-07-24 12:34 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-24 2:16 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel [-- 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 */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-24 2:16 ` Trent Piepho @ 2006-07-24 12:34 ` Mauro Carvalho Chehab 2006-07-24 22:06 ` Trent Piepho 0 siblings, 1 reply; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-24 12:34 UTC (permalink / raw) To: Trent Piepho Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-24 12:34 ` Mauro Carvalho Chehab @ 2006-07-24 22:06 ` Trent Piepho 2006-07-25 17:59 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-24 22:06 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: v4l-dvb maintainer list, Linux and Kernel Video, linux-kernel [-- 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-24 22:06 ` Trent Piepho @ 2006-07-25 17:59 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-25 17:59 UTC (permalink / raw) To: Linux and Kernel Video; +Cc: v4l-dvb maintainer list, linux-kernel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] V4L: struct video_device corruption 2006-07-16 1:31 ` Mauro Carvalho Chehab 2006-07-18 0:25 ` [v4l-dvb-maintainer] " Trent Piepho @ 2006-07-25 3:08 ` Andrew Morton 2006-07-25 7:57 ` [v4l-dvb-maintainer] " Manu Abraham 2006-07-25 8:42 ` Trent Piepho 1 sibling, 2 replies; 24+ messages in thread From: Andrew Morton @ 2006-07-25 3:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: robfitz, rdunlap, greg, 76306.1226, fork0, linux-kernel, shemminger, video4linux-list, v4l-dvb-maintainer 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-25 3:08 ` Andrew Morton @ 2006-07-25 7:57 ` Manu Abraham 2006-07-25 8:42 ` Trent Piepho 1 sibling, 0 replies; 24+ messages in thread From: Manu Abraham @ 2006-07-25 7:57 UTC (permalink / raw) To: Andrew Morton Cc: Mauro Carvalho Chehab, robfitz, video4linux-list, 76306.1226, fork0, greg, linux-kernel, rdunlap, v4l-dvb-maintainer, shemminger 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-25 3:08 ` Andrew Morton 2006-07-25 7:57 ` [v4l-dvb-maintainer] " Manu Abraham @ 2006-07-25 8:42 ` Trent Piepho 2006-07-25 9:06 ` Andrew Morton 1 sibling, 1 reply; 24+ messages in thread From: Trent Piepho @ 2006-07-25 8:42 UTC (permalink / raw) To: Andrew Morton Cc: Mauro Carvalho Chehab, robfitz, Linux and Kernel Video, 76306.1226, fork0, greg, linux-kernel, rdunlap, v4l-dvb maintainer list, shemminger [-- 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-25 8:42 ` Trent Piepho @ 2006-07-25 9:06 ` Andrew Morton 2006-07-25 12:28 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 24+ messages in thread From: Andrew Morton @ 2006-07-25 9:06 UTC (permalink / raw) To: Trent Piepho Cc: mchehab, robfitz, video4linux-list, 76306.1226, fork0, greg, linux-kernel, rdunlap, v4l-dvb-maintainer, shemminger 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v4l-dvb-maintainer] Re: [PATCH] V4L: struct video_device corruption 2006-07-25 9:06 ` Andrew Morton @ 2006-07-25 12:28 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 24+ messages in thread From: Mauro Carvalho Chehab @ 2006-07-25 12:28 UTC (permalink / raw) To: Andrew Morton Cc: Trent Piepho, robfitz, video4linux-list, 76306.1226, fork0, greg, linux-kernel, rdunlap, v4l-dvb-maintainer, shemminger 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2006-07-25 18:00 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-07-13 4:43 oops in bttv Chuck Ebbert 2006-07-13 5:05 ` Greg KH 2006-07-13 5:24 ` Randy.Dunlap 2006-07-13 5:44 ` Andrew Morton 2006-07-15 23:08 ` [PATCH] V4L: struct video_device corruption Robert Fitzsimons 2006-07-16 1:31 ` Mauro Carvalho Chehab 2006-07-18 0:25 ` [v4l-dvb-maintainer] " Trent Piepho 2006-07-19 11:54 ` Mauro Carvalho Chehab 2006-07-19 15:36 ` Michael Krufky 2006-07-20 21:57 ` Trent Piepho 2006-07-21 12:26 ` Mauro Carvalho Chehab 2006-07-21 20:06 ` Trent Piepho 2006-07-21 20:30 ` Mauro Carvalho Chehab 2006-07-21 22:55 ` Trent Piepho 2006-07-23 9:35 ` Mauro Carvalho Chehab 2006-07-24 2:16 ` Trent Piepho 2006-07-24 12:34 ` Mauro Carvalho Chehab 2006-07-24 22:06 ` Trent Piepho 2006-07-25 17:59 ` Mauro Carvalho Chehab 2006-07-25 3:08 ` Andrew Morton 2006-07-25 7:57 ` [v4l-dvb-maintainer] " Manu Abraham 2006-07-25 8:42 ` Trent Piepho 2006-07-25 9:06 ` Andrew Morton 2006-07-25 12:28 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).