Skip to contentSkip to conversation

cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h

PATCH In review Michael Krufky 4 participants 15 messages May 2, 2008 – May 15, 2008 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

I had this issue before dtv support was properly added to the cx18 driver.

With digital working fine, the issue had disappeared.

Now, using cx18 in the master branch, whose dtv side is crippled due to lack of the mxl driver, the error is back.

The cx18 driver can be loaded on my system once. If I unload it, then I get this every time I try to modprobe it again.

First, this is what it looks like the first time -- this is OK.

[   45.515441] Linux video capture interface: v2.00
[   45.840402] cx18:  Start initialization, version 1.0.0
[   45.840478] cx18-0: Initializing card #0
[   45.840484] cx18-0: Autodetected Hauppauge card
[   45.840510] ACPI: PCI Interrupt 0000:02:07.0[A] -> GSI 19 (level,
low) -> IRQ 17
[   45.840769] cx18-0: cx23418 revision 01010000 (B)
[   45.977540] tveeprom 1-0050: Hauppauge model 74041, rev C6B2, serial#
899541
[   45.977547] tveeprom 1-0050: MAC address is 00-0D-FE-0D-B9-D5
[   45.977551] tveeprom 1-0050: tuner model is TCL M2523_5N_E (idx 112,
type 50)
[   45.977555] tveeprom 1-0050: TV standards NTSC(M) (eeprom 0x08)
[   45.977559] tveeprom 1-0050: audio processor is CX23418 (idx 38)
[   45.977562] tveeprom 1-0050: decoder processor is CX23418 (idx 31)
[   45.977566] tveeprom 1-0050: has no radio, has IR receiver, has IR
transmitter
[   45.977570] cx18-0: Autodetected Hauppauge HVR-1600
[   45.977574] cx18-0: DVB & VBI are not yet supported
[   46.186774] tuner 2-0061: chip found @ 0xc2 (cx18 i2c driver #0-1)
[   46.186834] cs5345 1-004c: chip found @ 0x98 (cx18 i2c driver #0-0)
[   46.188571] cx18-0: Disabled encoder IDX device
[   46.188639] cx18-0: Registered device video0 for encoder MPEG (2 MB)
[   46.188688] cx18-0: Registered device video32 for encoder YUV (2 MB)
[   46.188737] cx18-0: Registered device video24 for encoder PCM audio
(1 MB)
[   46.318804] Driver 'sr' needs updating - please use bus_type methods
[   46.333007] sr0: scsi3-mmc drive: 48x/48x writer cd/rw xa/form2 cdda tray
[   46.333014] Uniform CD-ROM driver Revision: 3.20
[   46.333143] sr 2:0:0:0: Attached scsi CD-ROM sr0
[   46.461691] tuner-simple 2-0061: creating new instance
[   46.461697] tuner-simple 2-0061: type set to 50 (TCL 2002N)
[   46.740100] parport_pc 00:0a: reported by Plug and Play ACPI
[   46.740140] parport0: PC-style at 0x378, irq 7 [PCSPP,TRISTATE]
[   48.183476] cx18-0: loaded v4l-cx23418-apu.fw firmware V00120000
(141200 bytes)
[   48.283518] cx18-0: loaded v4l-cx23418-cpu.fw firmware (174716 bytes)
[   48.289808] cx18-0: FW version: 0.0.71.0 (Release 2006/12/29)
[   48.850871] cx18-0: loaded v4l-cx23418-dig.fw firmware (16382 bytes)
[   48.852862] cx18-0: Initialized card #0: Hauppauge HVR-1600
[   48.852938] ACPI: PCI Interrupt 0000:00:14.5[B] -> GSI 17 (level,
low) -> IRQ 16
[   48.853583] cx18:  End initialization

...but after unloading it once:

[ 2132.526778] tuner-simple 2-0061: destroying instance
[ 2132.531551] ACPI: PCI interrupt for device 0000:02:07.0 disabled
[ 2132.531562] cx18-0: Removed Hauppauge HVR-1600, card #0

every time I try to load cx18 again, I get this:

[ 2198.627071] Linux video capture interface: v2.00
[ 2198.644995] cx18:  Start initialization, version 1.0.0
[ 2198.645061] cx18-0: Initializing card #0
[ 2198.645065] cx18-0: Autodetected Hauppauge card
[ 2198.645092] ACPI: PCI Interrupt 0000:02:07.0[A] -> GSI 19 (level,
low) -> IRQ 17
[ 2198.645115] allocation failed: out of vmalloc space - use
vmalloc=<size> to increase size.
[ 2198.645119] cx18-0: ioremap failed, perhaps increasing
__VMALLOC_RESERVE in page.h
[ 2198.645124] cx18-0: or disabling CONFIG_HIGHMEM4G into the kernel
would help
[ 2198.645130] cx18-0: Error -12 on initialization
[ 2198.645184] cx18: probe of 0000:02:07.0 failed with error -12
[ 2198.645202] cx18:  End initialization
[ 2236.317472] Linux video capture interface: v2.00
[ 2236.344161] cx18:  Start initialization, version 1.0.0
[ 2236.344232] cx18-0: Initializing card #0
[ 2236.344236] cx18-0: Autodetected Hauppauge card
[ 2236.344265] allocation failed: out of vmalloc space - use
vmalloc=<size> to increase size.
[ 2236.344269] cx18-0: ioremap failed, perhaps increasing
__VMALLOC_RESERVE in page.h
[ 2236.344273] cx18-0: or disabling CONFIG_HIGHMEM4G into the kernel
would help
[ 2236.344278] cx18-0: Error -12 on initialization
[ 2236.344287] cx18: probe of 0000:02:07.0 failed with error -12
[ 2236.344304] cx18:  End initialization

-Mike

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#1 lore raw
Quoted 8 lines · Michael Krufky

I had this issue before dtv support was properly added to the cx18 driver.

With digital working fine, the issue had disappeared.

Now, using cx18 in the master branch, whose dtv side is crippled due to lack of the mxl driver, the error is back.

The cx18 driver can be loaded on my system once.

Mike,

On the modprobes , could you use

# modprobe cx18 debug=3

So we can see the debug messages about releasing encoder memory, base address and attempting ioremap.

Quoted 68 lines

If I unload it, then I
get this every time I try to modprobe it again.

First, this is what it looks like the first time -- this is OK.

[   45.515441] Linux video capture interface: v2.00
[   45.840402] cx18:  Start initialization, version 1.0.0
[   45.840478] cx18-0: Initializing card #0
[   45.840484] cx18-0: Autodetected Hauppauge card
[   45.840510] ACPI: PCI Interrupt 0000:02:07.0[A] -> GSI 19 (level,
low) -> IRQ 17
[   45.840769] cx18-0: cx23418 revision 01010000 (B)
[   45.977540] tveeprom 1-0050: Hauppauge model 74041, rev C6B2, serial#
899541
[   45.977547] tveeprom 1-0050: MAC address is 00-0D-FE-0D-B9-D5
[   45.977551] tveeprom 1-0050: tuner model is TCL M2523_5N_E (idx 112,
type 50)
[   45.977555] tveeprom 1-0050: TV standards NTSC(M) (eeprom 0x08)
[   45.977559] tveeprom 1-0050: audio processor is CX23418 (idx 38)
[   45.977562] tveeprom 1-0050: decoder processor is CX23418 (idx 31)
[   45.977566] tveeprom 1-0050: has no radio, has IR receiver, has IR
transmitter
[   45.977570] cx18-0: Autodetected Hauppauge HVR-1600
[   45.977574] cx18-0: DVB & VBI are not yet supported
[   46.186774] tuner 2-0061: chip found @ 0xc2 (cx18 i2c driver #0-1)
[   46.186834] cs5345 1-004c: chip found @ 0x98 (cx18 i2c driver #0-0)
[   46.188571] cx18-0: Disabled encoder IDX device
[   46.188639] cx18-0: Registered device video0 for encoder MPEG (2 MB)
[   46.188688] cx18-0: Registered device video32 for encoder YUV (2 MB)
[   46.188737] cx18-0: Registered device video24 for encoder PCM audio
(1 MB)
[   46.318804] Driver 'sr' needs updating - please use bus_type methods
[   46.333007] sr0: scsi3-mmc drive: 48x/48x writer cd/rw xa/form2 cdda tray
[   46.333014] Uniform CD-ROM driver Revision: 3.20
[   46.333143] sr 2:0:0:0: Attached scsi CD-ROM sr0
[   46.461691] tuner-simple 2-0061: creating new instance
[   46.461697] tuner-simple 2-0061: type set to 50 (TCL 2002N)
[   46.740100] parport_pc 00:0a: reported by Plug and Play ACPI
[   46.740140] parport0: PC-style at 0x378, irq 7 [PCSPP,TRISTATE]
[   48.183476] cx18-0: loaded v4l-cx23418-apu.fw firmware V00120000
(141200 bytes)
[   48.283518] cx18-0: loaded v4l-cx23418-cpu.fw firmware (174716 bytes)
[   48.289808] cx18-0: FW version: 0.0.71.0 (Release 2006/12/29)
[   48.850871] cx18-0: loaded v4l-cx23418-dig.fw firmware (16382 bytes)
[   48.852862] cx18-0: Initialized card #0: Hauppauge HVR-1600
[   48.852938] ACPI: PCI Interrupt 0000:00:14.5[B] -> GSI 17 (level,
low) -> IRQ 16
[   48.853583] cx18:  End initialization

...but after unloading it once:

[ 2132.526778] tuner-simple 2-0061: destroying instance
[ 2132.531551] ACPI: PCI interrupt for device 0000:02:07.0 disabled
[ 2132.531562] cx18-0: Removed Hauppauge HVR-1600, card #0

every time I try to load cx18 again, I get this:

[ 2198.627071] Linux video capture interface: v2.00
[ 2198.644995] cx18:  Start initialization, version 1.0.0
[ 2198.645061] cx18-0: Initializing card #0
[ 2198.645065] cx18-0: Autodetected Hauppauge card
[ 2198.645092] ACPI: PCI Interrupt 0000:02:07.0[A] -> GSI 19 (level,
low) -> IRQ 17
[ 2198.645115] allocation failed: out of vmalloc space - use
vmalloc=<size> to increase size.
[ 2198.645119] cx18-0: ioremap failed, perhaps increasing
Quoted 1 line

__VMALLOC_RESERVE in page.h

I can't reproduce this using the, mxl5005 crippled (and now gone) ~hverkuil/v4l-dvb-cx18 repository I pulled last week.

Hans or Steve,

I noticed we do a conditional iounmap here:

static void cx18_remove(struct pci_dev *pci_dev)

{
        struct cx18 *cx = pci_get_drvdata(pci_dev);

        CX18_DEBUG_INFO("Removing Card #%d\n", cx->num);

        /* Stop all captures */
        CX18_DEBUG_INFO("Stopping all streams\n");
        if (atomic_read(&cx->capturing) > 0)
                cx18_stop_all_captures(cx);

        /* Interrupts */
        sw1_irq_disable(IRQ_CPU_TO_EPU | IRQ_APU_TO_EPU);
        sw2_irq_disable(IRQ_CPU_TO_EPU_ACK | IRQ_APU_TO_EPU_ACK);

        cx18_halt_firmware(cx);

        cx18_streams_cleanup(cx);

        exit_cx18_i2c(cx);

        free_irq(cx->dev->irq, (void *)cx);

        if (cx->dev)
                cx18_iounmap(cx);

        release_mem_region(cx->base_addr, CX18_MEM_SIZE);

        pci_disable_device(cx->dev);

        CX18_INFO("Removed %s, card #%d\n", cx->card_name, cx->num);
}

Why the conditional? Since it is safe to dereference cx->dev->irq in the call to free_irq, why is it then dereferencing cx->dev would not be safe in calling cx18_iounmap()?

-Andy

Quoted 20 lines
[ 2198.645124] cx18-0: or disabling CONFIG_HIGHMEM4G into the kernel
would help
[ 2198.645130] cx18-0: Error -12 on initialization
[ 2198.645184] cx18: probe of 0000:02:07.0 failed with error -12
[ 2198.645202] cx18:  End initialization
[ 2236.317472] Linux video capture interface: v2.00
[ 2236.344161] cx18:  Start initialization, version 1.0.0
[ 2236.344232] cx18-0: Initializing card #0
[ 2236.344236] cx18-0: Autodetected Hauppauge card
[ 2236.344265] allocation failed: out of vmalloc space - use
vmalloc=<size> to increase size.
[ 2236.344269] cx18-0: ioremap failed, perhaps increasing
__VMALLOC_RESERVE in page.h
[ 2236.344273] cx18-0: or disabling CONFIG_HIGHMEM4G into the kernel
would help
[ 2236.344278] cx18-0: Error -12 on initialization
[ 2236.344287] cx18: probe of 0000:02:07.0 failed with error -12
[ 2236.344304] cx18:  End initialization
Quoted 3 lines

-Mike

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#10 lore raw
In reply to Michael Krufky
Quoted 1 line · Michael Krufky

I had this issue before dtv support was properly added to the cx18 driver.

The DTV disabling is a red herring.

Quoted 5 lines

With digital working fine, the issue had disappeared.

Now, using cx18 in the master branch, whose dtv side is crippled due to lack of the mxl driver, the error is back.

Try the hg/~stoth/merge tree, with the feature re-enabled and the newer mxl5005 driver. Does this help?

Quoted 3 lines

The cx18 driver can be loaded on my system once. If I unload it, then I get this every time I try to modprobe it again.

I can't repro the issue, but the unwind code (at least at first glance) looked OK to me.

Hans or Andy should comment on this, it's really their code.

...

Regards,

Steve

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#11 lore raw

PATCH 1/1 Fix potential cx18_cards[] entry leaks

In reply to Michael Krufky

Hans,

When investigating Mike Krufky's report of module reload problems, I ran across problems with the management of the cx18_cards[] array. They're corner cases and not likely to be the cause of Mike problems though.

Upon error conditions in cx18_probe(), the code at the 'err:' label could leak cx18_cards[] entries. Not a big problem since there are 32 of them, but they could have caused a NULL pointer de-reference in cx18_v4l2_open().

The attached patch fixes these and reworks the management of the cx18_cards[] entries. The cx18_active_cards variable is replaced with cx18_highest_cards_index (because that's essentially what cx18_active_cards_was doing +1), and cleanup of entries happens a little more pedantically (obtaining the lock, and removing each entry on a pci remove, instead of waiting until module unload).

The attached patch was made against the latest v4l-dvb hg repository.

Comments welcome.

Regards,
Andy

# HG changeset patch
# User Andy Walls <[email protected]>
# Date 1209781272 14400
# Node ID cb8788a7039efb818fd8299d1fcbe8c8f20d3093 # Parent 4c4fd6b8755cc9918255876ff1010bc77374a310 Fix cx18_cards[] entry leak, and possible NULL dereference.

From: Andy Walls <[email protected]>

The code at the err: label of cx18_probe() could leak cx18_card[] entries. In cx18_v4l2_open() the leaked entries could cause NULL pointer derefernces. Fixed these two problems and modified managment of the cx18_cards[] entries. In the process, replaced the misnamed cx18_cards_active variable with a cx18_highest_card_index variable, which was essentially the function of cx18_cards_active anyway.

linux/drivers/media/video/cx18/cx18-driver.c +43 -15
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-driver.c
@@ -38,9 +38,6 @@
 #include <media/tveeprom.h>
 
 
-/* var to keep track of the number of array elements in use */
-int cx18_cards_active;
-
 /* If you have already X v4l cards, then set this to X. This way
    the device numbers stay matched. Example: you have a WinTV card
    without radio and a Compro H900 with. Normally this would give a
@@ -51,7 +48,10 @@ int cx18_first_minor;
 /* Master variable for all cx18 info */
 struct cx18 *cx18_cards[CX18_MAX_CARDS];
 
-/* Protects cx18_cards_active */
+/* Highest index used so far in cx18_cards[] */
+int cx18_highest_card_index = -1;
+
+/* Protects cx18_cards[] */
 DEFINE_SPINLOCK(cx18_cards_lock);
 
 /* add your revision and whatnot here */
@@ -594,20 +594,40 @@ static void cx18_load_and_init_modules(s
 	hw = cx->hw_flags;
 }
 
+static void cx18_cards_free_entry(struct cx18 *cx)
+{
+	int i = cx->num;
+
+	spin_lock(&cx18_cards_lock);
+
+	kfree(cx18_cards[i]); /* the passed in cx pointer is now invalid! */
+	cx18_cards[i] = NULL;
+
+	cx18_highest_card_index = -1;
+	for (i = 0; i < CX18_MAX_CARDS; i++)
+		if (cx18_cards[i] != NULL)
+			cx18_highest_card_index = i;
+
+	spin_unlock(&cx18_cards_lock);
+}
+
 static int __devinit cx18_probe(struct pci_dev *dev,
 				const struct pci_device_id *pci_id)
 {
 	int retval = 0;
 	int vbi_buf_size;
+	int i;
 	u32 devtype;
 	struct cx18 *cx;
 
 	spin_lock(&cx18_cards_lock);
 
 	/* Make sure we've got a place for this card */
-	if (cx18_cards_active == CX18_MAX_CARDS) {
-		printk(KERN_ERR "cx18:  Maximum number of cards detected (%d).\n",
-			      cx18_cards_active);
+	for (i = 0; i < CX18_MAX_CARDS && cx18_cards[i] != NULL; i++) {}
+
+	if (i == CX18_MAX_CARDS) {
+		printk(KERN_ERR
+		       "cx18:  Maximum number of cards detected (%d).\n", i);
 		spin_unlock(&cx18_cards_lock);
 		return -ENOMEM;
 	}
@@ -617,11 +637,15 @@ static int __devinit cx18_probe(struct p
 		spin_unlock(&cx18_cards_lock);
 		return -ENOMEM;
 	}
-	cx18_cards[cx18_cards_active] = cx;
+	cx18_cards[i] = cx;
 	cx->dev = dev;
-	cx->num = cx18_cards_active++;
+	cx->num = i;
 	snprintf(cx->name, sizeof(cx->name) - 1, "cx18-%d", cx->num);
 	CX18_INFO("Initializing card #%d\n", cx->num);
+
+	for (; i < CX18_MAX_CARDS; i++)
+		if (cx18_cards[i] != NULL)
+			cx18_highest_card_index = i;
 
 	spin_unlock(&cx18_cards_lock);
 
@@ -687,8 +711,6 @@ static int __devinit cx18_probe(struct p
 		CX18_ERR("Could not initialize i2c\n");
 		goto free_map;
 	}
-
-	CX18_DEBUG_INFO("Active card count: %d.\n", cx18_cards_active);
 
 	if (cx->card->hw_all & CX18_HW_TVEEPROM) {
 		/* Based on the model number the cardtype may be changed.
@@ -816,8 +838,7 @@ err:
 		retval = -ENODEV;
 	CX18_ERR("Error %d on initialization\n", retval);
 
-	kfree(cx18_cards[cx18_cards_active]);
-	cx18_cards[cx18_cards_active] = NULL;
+	cx18_cards_free_entry(cx);
 	return retval;
 }
 
@@ -918,6 +939,9 @@ static void cx18_remove(struct pci_dev *
 	pci_disable_device(cx->dev);
 
 	CX18_INFO("Removed %s, card #%d\n", cx->card_name, cx->num);
+
+	pci_set_drvdata(cx->dev, NULL);
+	cx18_cards_free_entry(cx);
 }
 
 /* define a pci_driver for card detection */
@@ -960,11 +984,15 @@ static void module_cleanup(void)
 
 	pci_unregister_driver(&cx18_pci_driver);
 
-	for (i = 0; i < cx18_cards_active; i++) {
+	spin_lock(&cx18_cards_lock);
+	for (i = 0; i <= cx18_highest_card_index; i++) {
 		if (cx18_cards[i] == NULL)
 			continue;
 		kfree(cx18_cards[i]);
-	}
+		cx18_cards[i] = NULL;
+	}
+	cx18_highest_card_index = -1;
+	spin_unlock(&cx18_cards_lock);
 }
 
 module_init(module_start);
linux/drivers/media/video/cx18/cx18-driver.h +1 -1
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-driver.h
@@ -449,7 +449,7 @@ struct cx18 {
 
 /* Globals */
 extern struct cx18 *cx18_cards[];
-extern int cx18_cards_active;
+extern int cx18_highest_card_index;
 extern int cx18_first_minor;
 extern spinlock_t cx18_cards_lock;
 
linux/drivers/media/video/cx18/cx18-fileops.c +3 -1
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-fileops.c
@@ -694,7 +694,9 @@ int cx18_v4l2_open(struct inode *inode, 
 
 	/* Find which card this open was on */
 	spin_lock(&cx18_cards_lock);
-	for (x = 0; cx == NULL && x < cx18_cards_active; x++) {
+	for (x = 0; cx == NULL && x <= cx18_highest_card_index; x++) {
+		if (cx18_cards[x] == NULL)
+			continue;
 		/* find out which stream this open was on */
 		for (y = 0; y < CX18_MAX_STREAMS; y++) {
 			s = &cx18_cards[x]->streams[y];

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#12 lore raw

PATCH 1/1 2nd try: Fix potential cx18_cards[] entry leaks

Quoted 8 lines · Andy Walls

Hans,

When investigating Mike Krufky's report of module reload problems, I ran across problems with the management of the cx18_cards[] array. They're corner cases and not likely to be the cause of Mike problems though.

The attached patch was made against the latest v4l-dvb hg repository.

This is a new version of the patch. The previous version would change card minor number ordering upon errors, which is not what users with a number of cards and multiple video sources wired up would want to happen.

This new patch is almost a minimal set of changes to fix the cx18_cards[] leak and possible bad pointers being left in cx18_cards[] on error. It does include some additional lines to obtain the cx18_cards_lock when accessing cx18_cards[] and not just cx18_cards_active.

Regards,
Andy

# HG changeset patch
# User Andy Walls <[email protected]>
# Date 1209820624 14400
# Node ID 2f883fedfb85c1979d9352ffdcf97a2d901dfbeb
# Parent 4c4fd6b8755cc9918255876ff1010bc77374a310 Prevent cx18_cards[] leak and possible bad pointer dereference

From: Andy Walls <[email protected]>

The code at label 'err:' in cx18_probe() would try and free the wrong cx18_cards[] entry on error exit, and leave a bad pointer in place.

cx18_v4l2_open() could have derefernced this bad pointer or NULL pointer after the fix, so fixed that as well.

Obtained spin lock in all places where cx18_cards[] is accessed, not just where cx18_cards_active is accessed.

linux/drivers/media/video/cx18/cx18-driver.c +10 -3
diff -r 4c4fd6b8755c -r 2f883fedfb85 linux/drivers/media/video/cx18/cx18-driver.c
@@ -598,6 +598,7 @@ static int __devinit cx18_probe(struct p
 				const struct pci_device_id *pci_id)
 {
 	int retval = 0;
+	int i;
 	int vbi_buf_size;
 	u32 devtype;
 	struct cx18 *cx;
@@ -816,8 +817,11 @@ err:
 		retval = -ENODEV;
 	CX18_ERR("Error %d on initialization\n", retval);
 
-	kfree(cx18_cards[cx18_cards_active]);
-	cx18_cards[cx18_cards_active] = NULL;
+	spin_lock(&cx18_cards_lock);
+	i = cx->num;
+	kfree(cx18_cards[i]);
+	cx18_cards[i] = NULL;
+	spin_unlock(&cx18_cards_lock);
 	return retval;
 }
 
@@ -960,11 +964,14 @@ static void module_cleanup(void)
 
 	pci_unregister_driver(&cx18_pci_driver);
 
+	spin_lock(&cx18_cards_lock);
 	for (i = 0; i < cx18_cards_active; i++) {
 		if (cx18_cards[i] == NULL)
 			continue;
 		kfree(cx18_cards[i]);
-	}
+		cx18_cards[i] = NULL;
+	}
+	spin_unlock(&cx18_cards_lock);
 }
 
 module_init(module_start);
linux/drivers/media/video/cx18/cx18-fileops.c +2 -0
diff -r 4c4fd6b8755c -r 2f883fedfb85 linux/drivers/media/video/cx18/cx18-fileops.c
@@ -695,6 +695,8 @@ int cx18_v4l2_open(struct inode *inode, 
 	/* Find which card this open was on */
 	spin_lock(&cx18_cards_lock);
 	for (x = 0; cx == NULL && x < cx18_cards_active; x++) {
+		if (cx18_cards[x] == NULL)
+			continue;
 		/* find out which stream this open was on */
 		for (y = 0; y < CX18_MAX_STREAMS; y++) {
 			s = &cx18_cards[x]->streams[y];

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#4 lore raw
Quoted 9 lines · Steven Toth
Quoted 4 lines · Steven Toth
Quoted 2 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Any comments?

For the record, I've tested again with today's tip ( d87638488880 ) -- same exact behavior.

When I load the modules for the first time, everything is fine.

If I unload the cx18 module, I am unable to load it again, the same error is displayed as I posted in my original message.

Regards,

Mike

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#8 lore raw
In reply to Steven Toth
Quoted 11 lines · On Monday 12 May 2008 16:42:28 Steven Toth wrote:
Quoted 4 lines · Steven Toth
Quoted 2 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Any comments?

- Steve

This conditional is indeed bogus. I've just removed it in my v4l-dvb tree.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#13 lore raw

Re: [PATCH] Fix potential cx18_cards[] entry leaks

In reply to Andy Walls
Quoted 19 lines · On Saturday 03 May 2008 04:43:27 Andy Walls wrote:

Hans,

When investigating Mike Krufky's report of module reload problems, I ran across problems with the management of the cx18_cards[] array. They're corner cases and not likely to be the cause of Mike problems though.

Upon error conditions in cx18_probe(), the code at the 'err:' label could leak cx18_cards[] entries. Not a big problem since there are 32 of them, but they could have caused a NULL pointer de-reference in cx18_v4l2_open().

The attached patch fixes these and reworks the management of the cx18_cards[] entries. The cx18_active_cards variable is replaced with cx18_highest_cards_index (because that's essentially what cx18_active_cards_was doing +1), and cleanup of entries happens a little more pedantically (obtaining the lock, and removing each entry on a pci remove, instead of waiting until module unload).

Quoted 6 lines

The attached patch was made against the latest v4l-dvb hg repository.

Comments welcome.

Regards,
Andy

Hi Andy,

Thanks for looking into this. I've copied the open() fix into the cx18 and ivtv drivers, but not the additional changes: in my opinion they do not actually add anything useful. The potential NULL pointer dereference is however an important fix and definitely should go into 2.6.26.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#9 lore raw
In reply to Hans Verkuil
Quoted 14 lines · Hans Verkuil
Quoted 11 lines · On Monday 12 May 2008 16:42:28 Steven Toth wrote:
Quoted 4 lines · Steven Toth
Quoted 2 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Any comments?

- Steve

Quoted 6 lines

This conditional is indeed bogus. I've just removed it in my v4l-dvb tree.

Regards,

Hans

I noticed it was bogus a while ago. See my comment near the end of this:

https://www.redhat.com/mailman/private/video4linux-list/2008-May/msg00026.html

Given that "cx->dev->irq" dereference would cause a kernel oops before the "if(cx->dev)" reference prevents an iounmap() from happening, I don't have high hopes it will solve Mike's problem. Even if compiler optimization reorders calls, the oops should happen if cx->dev == NULL. (It's still worth getting rid of the bogus check though.)

It could very well be the case however, that the contents of cx->enc_mem has been corrupted, in which case the iounmap() won't free the proper memory region. Sooo....

Mike,

Could you add a log statement to output the value of "cx->enc_mem" before and after the call to cx18_iounmap() in cx18_driver.c:cx18_remove() ?

It should match the memory address displayed when the driver was loaded with debug "info" enabled.

Hans,

In researching Mike's problem, I noticed that the cx18_probe() function leaks struct cx18 allocations and cx18_cards[] entries on error exits. I posted 2 patches to the ivtv-devel list. The 2nd one is the preferred fix; the first was too complex. See

http://www.ivtvdriver.org/pipermail/ivtv-devel/2008-May/005530.html

Also, I've run across a corruption problem with cx->capturing being greater than 0 when no captures were going on, on 4 separate occasions. (I can't reproduce it on demand though.) I'm still trying to track the source, but the indication is MythTV getting an EBUSY returned when it tries to do a VIDIOC_S_FMT when trying to start cx18 analog capture. The EBUSY is at cx18-ioctl.c:cx18_try_or_set_fmt() at line 257.

Regards,
Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#14 lore raw

Re: [PATCH] Fix potential cx18_cards[] entry leaks

In reply to Hans Verkuil
Quoted 38 lines · Hans Verkuil
Quoted 10 lines · On Saturday 03 May 2008 04:43:27 Andy Walls wrote:

Hans,

When investigating Mike Krufky's report of module reload problems, I ran across problems with the management of the cx18_cards[] array. They're corner cases and not likely to be the cause of Mike problems though.

Upon error conditions in cx18_probe(), the code at the 'err:' label could leak cx18_cards[] entries. Not a big problem since there are 32 of them, but they could have caused a NULL pointer de-reference

in

Quoted 7 lines

cx18_v4l2_open().

The attached patch fixes these and reworks the management of the cx18_cards[] entries. The cx18_active_cards variable is replaced with cx18_highest_cards_index (because that's essentially what cx18_active_cards_was doing +1), and cleanup of entries happens a little more pedantically (obtaining the lock, and removing each

entry

Quoted 3 lines

on a pci remove, instead of waiting until module unload).

The attached patch was made against the latest v4l-dvb hg

repository.

Quoted 5 lines

Comments welcome.

Regards,
Andy

Hi Andy,

Thanks for looking into this. I've copied the open() fix into the cx18 and ivtv drivers, but not the additional changes: in my opinion they do not actually add anything useful. The potential NULL pointer dereference is however an important fix and definitely should go into 2.6.26.

Quoted 3 lines

Regards,

Hans

Hans,

Thanks.

Please also review the second, less extensive version of the patch. I think calling kfree() at the end of cx18_probe() with a good pointer, to avoid leaking a struct cx18 allocation, is important too.

When the err exit is executed, cx18_cards_active has already been incremented at least once, since the pointer was stored in what was

cx18_cards[cx18_cards_active]

but is now

cx18_cards[cx18_cards_active-(something >= 1)]

It is better to call

kfree(cx18_cards[cx->num])

to make sure the dynamically allocated memory is actually freed.

-Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#5 lore raw
In reply to [email protected]
Quoted 11 lines · On Mon, 2008-05-12 at 10:48 -0400, [email protected] wrote:
Quoted 9 lines · Steven Toth
Quoted 4 lines · Steven Toth
Quoted 2 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Any comments?

Quoted 7 lines

For the record, I've tested again with today's tip ( d87638488880 ) -- same exact behavior.

When I load the modules for the first time, everything is fine.

If I unload the cx18 module, I am unable to load it again, the same error is displayed as I posted in my original message.

Mike,

Could you apply the attached patch and run this test

(precondition: cx18.ko hasn't been loaded once yet) # cat /proc/iomem /proc/meminfo > ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe -r cx18
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats

and provide the contents of dmesg (or /var/log/messages) and memstats?

The patch will let me see if the contents of cx->enc_mem are bogus on iounmap() and if iounmap() is even being called.

I also want to verify that "cx18 encoder" doesn't get removed from /proc/iomem and that "VmallocUsed" doesn't return to it's previous size when the module is unloaded. That would show that the iounmap() fails.

I'd also want to ensure there is no overlap in /proc/iomem with "cx18 encoder" and something else. The kernel should prevent it, but I want to make sure.

(Hopefully the patch applies cleanly, the line numbers won't quite match up with the latest hg version.)

Regards,
Andy

Quoted 4 lines

Regards,

Mike

linux/drivers/media/video/cx18/cx18-driver.c +5 -3
diff -r d87638488880 linux/drivers/media/video/cx18/cx18-driver.c
@@ -186,7 +192,8 @@ static void cx18_iounmap(struct cx18 *cx
 
 	/* Release io memory */
 	if (cx->enc_mem != NULL) {
-		CX18_DEBUG_INFO("releasing enc_mem\n");
+		CX18_DEBUG_INFO("releasing enc_mem at virt addr %p\n",
+				cx->enc_mem);
 		iounmap(cx->enc_mem);
 		cx->enc_mem = NULL;
 	}
@@ -647,6 +661,8 @@ static int __devinit cx18_probe(struct p
 		   cx->base_addr + CX18_MEM_OFFSET, CX18_MEM_SIZE);
 	cx->enc_mem = ioremap_nocache(cx->base_addr + CX18_MEM_OFFSET,
 				       CX18_MEM_SIZE);
+	CX18_DEBUG_INFO("ioremaped enc_mem at virt addr %p\n", 
+			cx->enc_mem);
 	if (!cx->enc_mem) {
 		CX18_ERR("ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h\n");
 		CX18_ERR("or disabling CONFIG_HIGHMEM4G into the kernel would help\n");
@@ -904,8 +920,7 @@ static void cx18_remove(struct pci_dev *
 
 	free_irq(cx->dev->irq, (void *)cx);
 
-	if (cx->dev)
-		cx18_iounmap(cx);
+	cx18_iounmap(cx);
 
 	release_mem_region(cx->base_addr, CX18_MEM_SIZE);
 

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#6 lore raw
Quoted 50 lines · Andy Walls
Quoted 11 lines · On Mon, 2008-05-12 at 10:48 -0400, [email protected] wrote:
Quoted 9 lines · Steven Toth
Quoted 5 lines · Steven Toth
Quoted 3 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Quoted 8 lines
Quoted 2 lines

Any comments?

For the record, I've tested again with today's tip ( d87638488880 ) -- same exact behavior.

When I load the modules for the first time, everything is fine.

If I unload the cx18 module, I am unable to load it again, the same error is displayed as I posted in my original message.

Mike,

Could you apply the attached patch and run this test

(precondition: cx18.ko hasn't been loaded once yet) # cat /proc/iomem /proc/meminfo > ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe -r cx18
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats

and provide the contents of dmesg (or /var/log/messages) and memstats?

The patch will let me see if the contents of cx->enc_mem are bogus on iounmap() and if iounmap() is even being called.

I also want to verify that "cx18 encoder" doesn't get removed from /proc/iomem and that "VmallocUsed" doesn't return to it's previous size when the module is unloaded. That would show that the iounmap() fails.

I'd also want to ensure there is no overlap in /proc/iomem with "cx18 encoder" and something else. The kernel should prevent it, but I want to make sure.

Quoted 18 lines

(Hopefully the patch applies cleanly, the line numbers won't quite match up with the latest hg version.)

Regards,
Andy

Quoted 11 lines

Regards,

Mike

------------------------------------------------------------------------

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Andy,

I'm out of town right now, and things will be hectic when I get back.... I most likely won't get to this until the middle of the week, perhaps even next weekend.

-Mike

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#7 lore raw
Quoted 68 lines · Michael Krufky
Quoted 52 lines · Andy Walls
Quoted 13 lines · On Mon, 2008-05-12 at 10:48 -0400, [email protected] wrote:
Quoted 11 lines · Steven Toth
Quoted 5 lines · Steven Toth
Quoted 3 lines
        if (cx->dev)
                cx18_iounmap(cx);

This doesn't feel right.

Hans / Andy,

Any comments?

Quoted 8 lines

For the record, I've tested again with today's tip ( d87638488880 ) -- same exact behavior.

When I load the modules for the first time, everything is fine.

If I unload the cx18 module, I am unable to load it again, the same error is displayed as I posted in my original message.

Mike,

Could you apply the attached patch and run this test

(precondition: cx18.ko hasn't been loaded once yet) # cat /proc/iomem /proc/meminfo > ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe -r cx18
# cat /proc/iomem /proc/meminfo >> ~/memstats # modprobe cx18 debug=3
# cat /proc/iomem /proc/meminfo >> ~/memstats

and provide the contents of dmesg (or /var/log/messages) and memstats?

The patch will let me see if the contents of cx->enc_mem are bogus on iounmap() and if iounmap() is even being called.

I also want to verify that "cx18 encoder" doesn't get removed from /proc/iomem and that "VmallocUsed" doesn't return to it's previous size when the module is unloaded. That would show that the iounmap() fails.

I'd also want to ensure there is no overlap in /proc/iomem with "cx18 encoder" and something else. The kernel should prevent it, but I want to make sure.

Quoted 12 lines

(Hopefully the patch applies cleanly, the line numbers won't quite match up with the latest hg version.)

Regards,
Andy

Quoted 5 lines

Regards,

Mike

Quoted 5 lines

Andy,

I'm out of town right now, and things will be hectic when I get back.... I most likely won't get to this until the middle of the week, perhaps even next weekend.

That's OK my next week is booked with travel to a few places as well.

FYI, some amplifying information on the problem. Your original dmesg log showed an error that comes from

mm/vmalloc.c:__get_vm_area_node()

that can only be reached by two "goto out;" statements.

I'll be reading The last half of Chapter 7 of _Understanding_the_Linux_Kernel_ while away next week to try and grok what's going on, since the Chapter still looks applicable to the newer kernel code.

-Andy

Quoted 2 lines

-Mike

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list