changeset 14351:2eda2bcc8d6f

PATCH e9hack 3 participants 10 messages Mar 3, 2010 – Apr 21, 2010 Media (V4L/DVB)

Share & tools

Hans Verkuil

2010-03-13 16:27 UTC #1 lore raw

Hi Hartmut,

After reviewing your patch I realized that more needed to be done to get this to work properly. The core problem is that the probe function does not have a counterpart that can do a cleanup.

So if the probe fails, then v4l2_device_unregister will never be called, which is a particular problem on mxb.

I've made a new patch that basically reverts my original patch and instead modifies the mxb driver. I realized that mxb doesn't need to do a separate probe. Instead attach can just call probe and return an error if it fails.

This way I can avoid the devinit and the cleanup is also much more straightforward.

If there are no further comments, then I'll post a pull request in a few days.

Tested with the mxb board. It would be nice if you can verify this with the av7110.

Regards,

Hans

drivers/media/common/saa7146_fops.c +5 -6

index fd8e1f4..7364b96 100644
@@ -423,15 +423,14 @@ static void vv_callback(struct saa7146_dev *dev, unsigned long status)
 	}
 }
 
-int saa7146_vv_devinit(struct saa7146_dev *dev)
-{
-	return v4l2_device_register(&dev->pci->dev, &dev->v4l2_dev);
-}
-EXPORT_SYMBOL_GPL(saa7146_vv_devinit);
-
 int saa7146_vv_init(struct saa7146_dev* dev, struct saa7146_ext_vv *ext_vv)
 {
 	struct saa7146_vv *vv;
+	int err;
+
+	err = v4l2_device_register(&dev->pci->dev, &dev->v4l2_dev);
+	if (err)
+		return err;
 
 	vv = kzalloc(sizeof(struct saa7146_vv), GFP_KERNEL);
 	if (vv == NULL) {

drivers/media/video/hexium_gemini.c +0 -3

index e620a3a..ad2c232 100644
@@ -356,9 +356,6 @@ static int hexium_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_d
 
 	DEB_EE((".\n"));
 
-	ret = saa7146_vv_devinit(dev);
-	if (ret)
-		return ret;
 	hexium = kzalloc(sizeof(struct hexium), GFP_KERNEL);
 	if (NULL == hexium) {
 		printk("hexium_gemini: not enough kernel memory in hexium_attach().\n");

drivers/media/video/hexium_orion.c +0 -4

index fe596a1..938a1f8 100644
@@ -216,10 +216,6 @@ static int hexium_probe(struct saa7146_dev *dev)
 		return -EFAULT;
 	}
 
-	err = saa7146_vv_devinit(dev);
-	if (err)
-		return err;
-
 	hexium = kzalloc(sizeof(struct hexium), GFP_KERNEL);
 	if (NULL == hexium) {
 		printk("hexium_orion: hexium_probe: not enough kernel memory.\n");

drivers/media/video/mxb.c +8 -9

index 9f01f14..ef0c817 100644
@@ -169,11 +169,7 @@ static struct saa7146_extension extension;
 static int mxb_probe(struct saa7146_dev *dev)
 {
 	struct mxb *mxb = NULL;
-	int err;
 
-	err = saa7146_vv_devinit(dev);
-	if (err)
-		return err;
 	mxb = kzalloc(sizeof(struct mxb), GFP_KERNEL);
 	if (mxb == NULL) {
 		DEB_D(("not enough kernel memory.\n"));

@@ -699,14 +695,17 @@ static struct saa7146_ext_vv vv_data;
 /* this function only gets called when the probing was successful */
 static int mxb_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_data *info)
 {
-	struct mxb *mxb = (struct mxb *)dev->ext_priv;
+	struct mxb *mxb;
 
 	DEB_EE(("dev:%p\n", dev));
 
-	/* checking for i2c-devices can be omitted here, because we
-	   already did this in "mxb_vl42_probe" */
-
 	saa7146_vv_init(dev, &vv_data);
+	if (mxb_probe(dev)) {
+		saa7146_vv_release(dev);
+		return -1;
+	}
+	mxb = (struct mxb *)dev->ext_priv;
+
 	vv_data.ops.vidioc_queryctrl = vidioc_queryctrl;
 	vv_data.ops.vidioc_g_ctrl = vidioc_g_ctrl;
 	vv_data.ops.vidioc_s_ctrl = vidioc_s_ctrl;

@@ -726,6 +725,7 @@ static int mxb_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_data
 	vv_data.ops.vidioc_default = vidioc_default;
 	if (saa7146_register_device(&mxb->video_dev, dev, "mxb", VFL_TYPE_GRABBER)) {
 		ERR(("cannot register capture v4l2 device. skipping.\n"));
+		saa7146_vv_release(dev);
 		return -1;
 	}

@@ -846,7 +846,6 @@ static struct saa7146_extension extension = {
 	.pci_tbl	= &pci_tbl[0],
 	.module		= THIS_MODULE,
 
-	.probe		= mxb_probe,
 	.attach		= mxb_attach,
 	.detach		= mxb_detach,

include/media/saa7146_vv.h +0 -1

index b9da1f5..4aeff96 100644
@@ -188,7 +188,6 @@ void saa7146_buffer_timeout(unsigned long data);
 void saa7146_dma_free(struct saa7146_dev* dev,struct videobuf_queue *q,
 						struct saa7146_buf *buf);
 
-int saa7146_vv_devinit(struct saa7146_dev *dev);
 int saa7146_vv_init(struct saa7146_dev* dev, struct saa7146_ext_vv *ext_vv);
 int saa7146_vv_release(struct saa7146_dev* dev);

Quoted 58 lines · On Wednesday 03 March 2010 12:39:27 e9hack wrote:

Hi,
changeset 14351:2eda2bcc8d6f is incomplete. If the init function is split in two function, the deinit function shall consider this. The changes shall be apply also to av7110_init_v4l().
diff -r 58ae12f18e80 linux/drivers/media/common/saa7146_fops.c
linux/drivers/media/common/saa7146_fops.c Wed Mar 03 12:15:23 2010 +0100 renamed from linux/drivers/media/common/saa7146_fops.c Tue Mar 02 23:52:36 2010 -0300 +4 -2
@@ -481,8 +481,10 @@ int saa7146_vv_release(struct saa7146_de
        DEB_EE(("dev:%p\n",dev));
 
        v4l2_device_unregister(&dev->v4l2_dev);
-       pci_free_consistent(dev->pci, SAA7146_CLIPPING_MEM, vv->d_clipping.cpu_addr,
 vv->d_clipping.dma_handle);
-       kfree(vv);
+       if (vv) {
+               pci_free_consistent(dev->pci, SAA7146_CLIPPING_MEM,
 vv->d_clipping.cpu_addr, vv->d_clipping.dma_handle);
+               kfree(vv);
+       }
        dev->vv_data = NULL;
        dev->vv_callback = NULL;
diff -r 58ae12f18e80 linux/drivers/media/dvb/ttpci/av7110_v4l.c
linux/drivers/media/dvb/ttpci/av7110_v4l.c Wed Mar 03 12:15:23 2010 +0100 renamed from linux/drivers/media/dvb/ttpci/av7110_v4l.c Tue Mar 02 23:52:36 2010 -0300 +10 -2
@@ -790,12 +790,20 @@ int av7110_init_v4l(struct av7110 *av711
                vv_data = &av7110_vv_data_c;
        else
                vv_data = &av7110_vv_data_st;
+       ret = saa7146_vv_devinit(dev);
+
+       if (ret < 0) {
+               ERR(("cannot init device. skipping.\n"));
+               return ret;
+       }
+
        ret = saa7146_vv_init(dev, vv_data);
-
-       if (ret) {
+       if (ret < 0) {
                ERR(("cannot init capture device. skipping.\n"));
+               saa7146_vv_release(dev);
                return ret;
        }
+
        vv_data->ops.vidioc_enum_input = vidioc_enum_input;
        vv_data->ops.vidioc_g_input = vidioc_g_input;
        vv_data->ops.vidioc_s_input = vidioc_s_input;
Regards,
Hartmut
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG

e9hack

author

2010-03-16 19:30 UTC #2 lore raw

Am 13.3.2010 17:27, schrieb Hans Verkuil:

Quoted 4 lines

If there are no further comments, then I'll post a pull request in a few days.
Tested with the mxb board. It would be nice if you can verify this with the av7110.

Hi hans,

it works with my TT-C2300 perfectly. The main problem of your changes was: It wasn't possible to unload the module for the TT-C2300.

Regards,
Hartmut

Oliver Endriss

2010-03-20 14:07 UTC #3 lore raw

av7110 and budget_av are broken! (was: Re: changeset 14351:2eda2bcc8d6f)

Quoted 10 lines · e9hack wrote:

Am 13.3.2010 17:27, schrieb Hans Verkuil:
Quoted 4 lines
If there are no further comments, then I'll post a pull request in a few days.
Tested with the mxb board. It would be nice if you can verify this with the av7110.
Hi hans,
it works with my TT-C2300 perfectly. The main problem of your changes was: It wasn't possible to unload the module for the TT-C2300.

Guys, when will you finally apply this fix?

As Hartmut pointed out, changeset 14351:2eda2bcc8d6f broke the av7110 driver (and also budget-av).

It is time to fix it. This bug must not go into the kernel!

CU
Oliver

Signature · 5 lines

----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------

Hans Verkuil

2010-03-20 14:20 UTC #4 lore raw

Re: av7110 and budget_av are broken! (was: Re: changeset 14351:2eda2bcc8d6f)

Quoted 7 lines · On Saturday 20 March 2010 15:07:08 Oliver Endriss wrote:

Quoted 6 lines · e9hack wrote:
Am 13.3.2010 17:27, schrieb Hans Verkuil:
Quoted 4 lines
If there are no further comments, then I'll post a pull request in a few days.
Tested with the mxb board. It would be nice if you can verify this with the av7110.

Quoted 6 lines

Quoted 4 lines
Hi hans,
it works with my TT-C2300 perfectly. The main problem of your changes was: It wasn't possible to unload the module for the TT-C2300.
Guys, when will you finally apply this fix?

Thanks for reminding me, I frankly forgot about this.

Hartmut, is the problem with unloading the module something that my patch caused? Or was that there as well before changeset 14351:2eda2bcc8d6f? Are there any kernel messages indicating why it won't unload?

Regards,

Hans

Quoted 9 lines

As Hartmut pointed out, changeset 14351:2eda2bcc8d6f broke the av7110 driver (and also budget-av).
It is time to fix it. This bug must not go into the kernel!
CU
Oliver

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG

Oliver Endriss

2010-03-20 15:14 UTC #5 lore raw

Re: av7110 and budget_av are broken! (was: Re: changeset 14351:2eda2bcc8d6f)

Quoted 13 lines · Hans Verkuil

Quoted 12 lines · On Saturday 20 March 2010 15:07:08 Oliver Endriss wrote:
Quoted 10 lines · e9hack wrote:
Am 13.3.2010 17:27, schrieb Hans Verkuil:
Quoted 4 lines
If there are no further comments, then I'll post a pull request in a few days.
Tested with the mxb board. It would be nice if you can verify this with the av7110.
Hi hans,
it works with my TT-C2300 perfectly. The main problem of your changes was: It wasn't possible to unload the module for the TT-C2300.

Quoted 7 lines

Quoted 1 line
Guys, when will you finally apply this fix?
Thanks for reminding me, I frankly forgot about this.
Hartmut, is the problem with unloading the module something that my patch caused? Or was that there as well before changeset 14351:2eda2bcc8d6f? Are there any kernel messages indicating why it won't unload?

The patch caused the problem.

You moved v4l2_device_register() from saa7146_vv_init() to saa7146_vv_devinit(), but you did not modify av7110_v4l.c and budget-av.c accordingly.

$ grep saa7146_vv_init v4l/*c
v4l/av7110_v4l.c:       ret = saa7146_vv_init(dev, vv_data);
v4l/budget-av.c:                if (0 != saa7146_vv_init(dev, &vv_data)) {
v4l/hexium_gemini.c:    saa7146_vv_init(dev, &vv_data);
v4l/hexium_orion.c:     saa7146_vv_init(dev, &vv_data);
v4l/mxb.c:      saa7146_vv_init(dev, &vv_data);
v4l/saa7146_fops.c:int saa7146_vv_init(struct saa7146_dev* dev, struct saa7146_ext_vv *ext_vv)
v4l/saa7146_fops.c:EXPORT_SYMBOL_GPL(saa7146_vv_init);
v4l/saa7146_fops.c:static int __init saa7146_vv_init_module(void)
v4l/saa7146_fops.c:module_init(saa7146_vv_init_module);

$ grep saa7146_vv_devinit v4l/*c
v4l/hexium_gemini.c:    ret = saa7146_vv_devinit(dev);
v4l/hexium_orion.c:     err = saa7146_vv_devinit(dev);
v4l/mxb.c:      err = saa7146_vv_devinit(dev);
v4l/saa7146_fops.c:int saa7146_vv_devinit(struct saa7146_dev *dev)
v4l/saa7146_fops.c:EXPORT_SYMBOL_GPL(saa7146_vv_devinit);

CU
Oliver

Signature · 5 lines

----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------

e9hack

author

2010-03-20 16:03 UTC #6 lore raw

Re: av7110 and budget_av are broken!

In reply to Hans Verkuil

Am 20.3.2010 15:20, schrieb Hans Verkuil:

Quoted 3 lines

Hartmut, is the problem with unloading the module something that my patch caused? Or was that there as well before changeset 14351:2eda2bcc8d6f? Are there any kernel messages indicating why it won't unload?

Changset 14351:2eda2bcc8d6f causes a kernel oops during unload of the module for my dvb cards. The call trace points to dvb_ttpci. I assumed, that the FF card is affected only. It may be possible, that budget-av does crash also, if it is unload as second.

Regards,
Hartmut

Mar  3 11:42:33 vdr kernel: [ 3291.915576] saa7146: unregister extension 'budget_av'.
Mar  3 11:42:33 vdr kernel: [ 3291.916426] budget_av 0000:04:07.0: PCI INT A disabled
Mar  3 11:42:33 vdr kernel: [ 3291.918565] saa7146: unregister extension 'dvb'.
Mar  3 11:42:33 vdr kernel: [ 3291.918750] BUG: unable to handle kernel NULL pointer
dereference at (null)
Mar  3 11:42:33 vdr kernel: [ 3291.918966] IP: [<ffffffffa0286aac>]
v4l2_device_unregister+0x1c/0x60 [videodev]
Mar  3 11:42:33 vdr kernel: [ 3291.919013] PGD 3db10067 PUD 37a3d067 PMD 0
Mar  3 11:42:33 vdr kernel: [ 3291.919013] Oops: 0000 [#1] PREEMPT SMP
Mar  3 11:42:33 vdr kernel: [ 3291.919013] last sysfs file:
/sys/devices/platform/w83627ehf.2576/cpu0_vid
Mar  3 11:42:33 vdr kernel: [ 3291.919013] CPU 0
Mar  3 11:42:33 vdr kernel: [ 3291.919013] Pid: 15305, comm: rmmod Not tainted
2.6.33-64-suse-11.0 #4 MS-7207PV/MS-7207PV
Mar  3 11:42:33 vdr kernel: [ 3291.919013] RIP: 0010:[<ffffffffa0286aac>]
[<ffffffffa0286aac>] v4l2_device_unregister+0x1c/0x60 [videodev]
Mar  3 11:42:33 vdr kernel: [ 3291.919013] RSP: 0018:ffff88003da55cc8  EFLAGS: 00010203
Mar  3 11:42:33 vdr kernel: [ 3291.919013] RAX: 0000000000000000 RBX: 0000000000000000
RCX: ffff88003d64d3e0
Mar  3 11:42:34 vdr kernel: [ 3291.919013] RDX: 0000000000000001 RSI: 0000000000000000
RDI: 0000000000000000
Mar  3 11:42:34 vdr kernel: [ 3291.919013] RBP: ffff880037fd7018 R08: 0000000080808081
R09: 0000000000000000
Mar  3 11:42:34 vdr kernel: [ 3291.919013] R10: 0000000000000000 R11: ffffffffa02812a0
R12: ffff880037fd7020
Mar  3 11:42:34 vdr kernel: [ 3291.919013] R13: 0000000000000000 R14: 0000000000000001
R15: ffff88003fb98888
Mar  3 11:42:34 vdr kernel: [ 3291.919013] FS:  00007f2def23b6f0(0000)
GS:ffff880001800000(0000) knlGS:0000000000000000
Mar  3 11:42:34 vdr kernel: [ 3291.919013] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
Mar  3 11:42:34 vdr kernel: [ 3291.919013] CR2: 0000000000000000 CR3: 0000000037abd000
CR4: 00000000000006f0
Mar  3 11:42:34 vdr kernel: [ 3291.919013] DR0: 0000000000000000 DR1: 0000000000000000
DR2: 0000000000000000
Mar  3 11:42:34 vdr kernel: [ 3291.919013] DR3: 0000000000000000 DR6: 00000000ffff0ff0
DR7: 0000000000000400
Mar  3 11:42:34 vdr kernel: [ 3291.919013] Process rmmod (pid: 15305, threadinfo
ffff88003da54000, task ffff880009830080)
Mar  3 11:42:34 vdr kernel: [ 3291.919013] Stack:
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  ffff88003edc0108 ffff880037fd7000
ffff88003db47000 ffffffffa0396ca5
Mar  3 11:42:34 vdr kernel: [ 3291.919013] <0> ffffffffa03df158 ffff88003edc0108
ffff880037fd7118 ffff880037fd7000
Mar  3 11:42:34 vdr kernel: [ 3291.919013] <0> ffffffffa03df158 0000000000000001
ffff88003fb98888 ffffffffa03cb2c5
Mar  3 11:42:34 vdr kernel: [ 3291.919013] Call Trace:
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffffa0396ca5>] ?
saa7146_vv_release+0x45/0x140 [saa7146_vv]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffffa03cb2c5>] ?
av7110_exit_v4l+0x45/0x60 [dvb_ttpci]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffffa03d9748>] ?
av7110_detach+0x138/0x2a0 [dvb_ttpci]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffffa02dd322>] ?
saa7146_remove_one+0xb2/0x230 [saa7146]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff811dd37f>] ? pci_device_remove+0x2f/0x60
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff81270c88>] ?
__device_release_driver+0x78/0xf0
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff81270db0>] ? driver_detach+0xb0/0xc0
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff8126fca4>] ? bus_remove_driver+0x84/0xe0
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff811dd69f>] ?
pci_unregister_driver+0x3f/0xc0
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffffa02dd1a7>] ?
saa7146_unregister_extension+0x27/0x60 [saa7146]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff81084db6>] ?
sys_delete_module+0x1f6/0x2d0
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff81344255>] ?
_raw_spin_unlock_irqrestore+0x15/0x40
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  [<ffffffff81002e6b>] ?
system_call_fastpath+0x16/0x1b
Mar  3 11:42:34 vdr kernel: [ 3291.919013] Code: fe e0 48 c7 03 00 00 00 00 5b c3 0f 1f 40
00 41 54 48 85 ff 55 48 89 fd 53 74 4a e8 cf ff ff ff 48 8b 5d 08 4c 8d 65 08 4c 39 e3 <
Mar  3 11:42:34 vdr kernel: [ 3291.919013] RIP  [<ffffffffa0286aac>]
v4l2_device_unregister+0x1c/0x60 [videodev]
Mar  3 11:42:34 vdr kernel: [ 3291.919013]  RSP <ffff88003da55cc8>
Mar  3 11:42:34 vdr kernel: [ 3291.919013] CR2: 0000000000000000
Mar  3 11:42:34 vdr kernel: [ 3291.934087] ---[ end trace 685f9ceb9b064f25 ]---
Mar  3 11:42:52 vdr logger: rcvdr: stop
Mar  3 11:43:34 vdr shutdown[5610]: shutting down for system reboot

Hans Verkuil

2010-03-20 21:37 UTC #7 lore raw

Re: av7110 and budget_av are broken!

Quoted 7 lines · On Saturday 20 March 2010 17:03:01 e9hack wrote:

Am 20.3.2010 15:20, schrieb Hans Verkuil:
Quoted 3 lines
Hartmut, is the problem with unloading the module something that my patch caused? Or was that there as well before changeset 14351:2eda2bcc8d6f? Are there any kernel messages indicating why it won't unload?
Changset 14351:2eda2bcc8d6f causes a kernel oops during unload of the module for my dvb cards.

OK, I know that. But does the patch I mailed you last time fix this problem without causing new ones? If so, then I'll post that patch to the list.

Regards,

Hans

Quoted 2 lines

The call trace points to dvb_ttpci. I assumed, that the FF card is affected only. It may be possible, that budget-av does crash also, if it is unload as second.

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG

e9hack

author

2010-03-22 19:34 UTC #8 lore raw

Re: av7110 and budget_av are broken!

Am 20.3.2010 22:37, schrieb Hans Verkuil:

Quoted 3 lines · On Saturday 20 March 2010 17:03:01 e9hack wrote:

OK, I know that. But does the patch I mailed you last time fix this problem without causing new ones? If so, then I'll post that patch to the list.

With your last patch, I've no problems. I'm using a a TT-C2300 and a Budget card. If my VDR does start, currently I've no chance to determine which module is load first, but it works. If I unload all modules and load it again, I've no problem. In this case, the modules for the budget card is load first and the modules for the FF loads as second one.

Regards,
Hartmut

e9hack

author

2010-04-21 06:29 UTC #9 lore raw

Re: av7110 and budget_av are broken!

Am 22.3.2010 20:34, schrieb e9hack:

Quoted 9 lines

Am 20.3.2010 22:37, schrieb Hans Verkuil:
Quoted 3 lines · On Saturday 20 March 2010 17:03:01 e9hack wrote:
OK, I know that. But does the patch I mailed you last time fix this problem without causing new ones? If so, then I'll post that patch to the list.
With your last patch, I've no problems. I'm using a a TT-C2300 and a Budget card. If my VDR does start, currently I've no chance to determine which module is load first, but it works. If I unload all modules and load it again, I've no problem. In this case, the modules for the budget card is load first and the modules for the FF loads as second one.

Ping!!!!!!