* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-02 12:59 cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h Michael Krufky
@ 2008-05-02 13:47 ` Andy Walls
2008-05-02 15:22 ` Steven Toth
2008-05-02 15:00 ` Steven Toth
2008-05-03 2:43 ` [PATCH] Fix potential cx18_cards[] entry leaks Andy Walls
2 siblings, 1 reply; 15+ messages in thread
From: Andy Walls @ 2008-05-02 13:47 UTC (permalink / raw)
To: Michael Krufky; +Cc: Steven Toth, Linux and Kernel Video
On Fri, 2008-05-02 at 08:59 -0400, Michael Krufky wrote:
> 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.
> 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
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
> [ 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-02 13:47 ` Andy Walls
@ 2008-05-02 15:22 ` Steven Toth
2008-05-12 14:42 ` Steven Toth
0 siblings, 1 reply; 15+ messages in thread
From: Steven Toth @ 2008-05-02 15:22 UTC (permalink / raw)
To: Andy Walls; +Cc: Steven Toth, Michael Krufky, Linux and Kernel Video
> if (cx->dev)
> cx18_iounmap(cx);
This doesn't feel right.
- Steve
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-02 15:22 ` Steven Toth
@ 2008-05-12 14:42 ` Steven Toth
2008-05-12 14:48 ` mkrufky
2008-05-12 14:48 ` Hans Verkuil
0 siblings, 2 replies; 15+ messages in thread
From: Steven Toth @ 2008-05-12 14:42 UTC (permalink / raw)
To: Andy Walls, Hans Verkuil
Cc: Steven Toth, Michael Krufky, Linux and Kernel Video
Steven Toth wrote:
>> if (cx->dev)
>> cx18_iounmap(cx);
>
> This doesn't feel right.
Hans / Andy,
Any comments?
- Steve
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-12 14:42 ` Steven Toth
@ 2008-05-12 14:48 ` mkrufky
2008-05-15 2:24 ` Andy Walls
2008-05-12 14:48 ` Hans Verkuil
1 sibling, 1 reply; 15+ messages in thread
From: mkrufky @ 2008-05-12 14:48 UTC (permalink / raw)
To: stoth; +Cc: Stoth, video4linux-list
Steven Toth wrote:
> Steven Toth wrote:
>>> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-12 14:48 ` mkrufky
@ 2008-05-15 2:24 ` Andy Walls
2008-05-15 12:57 ` Michael Krufky
0 siblings, 1 reply; 15+ messages in thread
From: Andy Walls @ 2008-05-15 2:24 UTC (permalink / raw)
To: mkrufky; +Cc: video4linux-list, Stoth
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
On Mon, 2008-05-12 at 10:48 -0400, mkrufky@linuxtv.org wrote:
> Steven Toth wrote:
> > Steven Toth wrote:
> >>> 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.
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
> Regards,
>
> Mike
>
[-- Attachment #2: cx18-iounmap-debug.patch --]
[-- Type: text/x-patch, Size: 1198 bytes --]
diff -r d87638488880 linux/drivers/media/video/cx18/cx18-driver.c
--- a/linux/drivers/media/video/cx18/cx18-driver.c Thu May 01 03:23:23 2008 -0400
+++ b/linux/drivers/media/video/cx18/cx18-driver.c Wed May 14 22:09:15 2008 -0400
@@ -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);
[-- Attachment #3: Type: text/plain, Size: 164 bytes --]
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-15 2:24 ` Andy Walls
@ 2008-05-15 12:57 ` Michael Krufky
2008-05-15 23:53 ` Andy Walls
0 siblings, 1 reply; 15+ messages in thread
From: Michael Krufky @ 2008-05-15 12:57 UTC (permalink / raw)
To: Andy Walls; +Cc: video4linux-list, mkrufky, Stoth
Andy Walls wrote:
> On Mon, 2008-05-12 at 10:48 -0400, mkrufky@linuxtv.org wrote:
>
>> Steven Toth wrote:
>>
>>> Steven Toth wrote:
>>>
>>>>> 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.
>>
>
> 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
>
>
>> Regards,
>>
>> Mike
>>
>>
>> ------------------------------------------------------------------------
>>
>> --
>> video4linux-list mailing list
>> Unsubscribe mailto:video4linux-list-request@redhat.com?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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-15 12:57 ` Michael Krufky
@ 2008-05-15 23:53 ` Andy Walls
0 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2008-05-15 23:53 UTC (permalink / raw)
To: Michael Krufky; +Cc: video4linux-list, Stoth
On Thu, 2008-05-15 at 08:57 -0400, Michael Krufky wrote:
> Andy Walls wrote:
> > On Mon, 2008-05-12 at 10:48 -0400, mkrufky@linuxtv.org wrote:
> >
> >> Steven Toth wrote:
> >>
> >>> Steven Toth wrote:
> >>>
> >>>>> 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.
> >>
> >
> > 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
> >
> >
> >> Regards,
> >>
> >> Mike
> >>
> >>
>
>
> 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
> -Mike
>
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-12 14:42 ` Steven Toth
2008-05-12 14:48 ` mkrufky
@ 2008-05-12 14:48 ` Hans Verkuil
2008-05-12 23:20 ` Andy Walls
1 sibling, 1 reply; 15+ messages in thread
From: Hans Verkuil @ 2008-05-12 14:48 UTC (permalink / raw)
To: Steven Toth; +Cc: Steven Toth, Michael Krufky, Linux and Kernel Video
On Monday 12 May 2008 16:42:28 Steven Toth wrote:
> Steven Toth wrote:
> >> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-12 14:48 ` Hans Verkuil
@ 2008-05-12 23:20 ` Andy Walls
0 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2008-05-12 23:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux and Kernel Video, Steven Toth, Michael Krufky
On Mon, 2008-05-12 at 16:48 +0200, Hans Verkuil wrote:
> On Monday 12 May 2008 16:42:28 Steven Toth wrote:
> > Steven Toth wrote:
> > >> 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
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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h
2008-05-02 12:59 cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h Michael Krufky
2008-05-02 13:47 ` Andy Walls
@ 2008-05-02 15:00 ` Steven Toth
2008-05-03 2:43 ` [PATCH] Fix potential cx18_cards[] entry leaks Andy Walls
2 siblings, 0 replies; 15+ messages in thread
From: Steven Toth @ 2008-05-02 15:00 UTC (permalink / raw)
To: Michael Krufky; +Cc: Steven Toth, Linux and Kernel Video
Michael Krufky wrote:
> I had this issue before dtv support was properly added to the cx18 driver.
The DTV disabling is a red herring.
>
> 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?
>
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Fix potential cx18_cards[] entry leaks
2008-05-02 12:59 cx18-0: ioremap failed, perhaps increasing __VMALLOC_RESERVE in page.h Michael Krufky
2008-05-02 13:47 ` Andy Walls
2008-05-02 15:00 ` Steven Toth
@ 2008-05-03 2:43 ` Andy Walls
2008-05-03 13:33 ` [PATCH] 2nd try: " Andy Walls
2008-05-12 15:05 ` [PATCH] " Hans Verkuil
2 siblings, 2 replies; 15+ messages in thread
From: Andy Walls @ 2008-05-03 2:43 UTC (permalink / raw)
To: ivtv-devel; +Cc: Linux and Kernel Video, Michael Krufky
[-- Attachment #1: Type: text/plain, Size: 940 bytes --]
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
[-- Attachment #2: cx18-cards-leak2.patch --]
[-- Type: text/x-patch, Size: 5666 bytes --]
# HG changeset patch
# User Andy Walls <awalls@radix.net>
# Date 1209781272 14400
# Node ID cb8788a7039efb818fd8299d1fcbe8c8f20d3093
# Parent 4c4fd6b8755cc9918255876ff1010bc77374a310
Fix cx18_cards[] entry leak, and possible NULL dereference.
From: Andy Walls <awalls@radix.net>
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.
Signed-off-by: Andy Walls <awalls@radix.net>
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-driver.c
--- a/linux/drivers/media/video/cx18/cx18-driver.c Fri May 02 07:51:27 2008 -0300
+++ b/linux/drivers/media/video/cx18/cx18-driver.c Fri May 02 22:21:12 2008 -0400
@@ -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);
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-driver.h
--- a/linux/drivers/media/video/cx18/cx18-driver.h Fri May 02 07:51:27 2008 -0300
+++ b/linux/drivers/media/video/cx18/cx18-driver.h Fri May 02 22:21:12 2008 -0400
@@ -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;
diff -r 4c4fd6b8755c -r cb8788a7039e linux/drivers/media/video/cx18/cx18-fileops.c
--- a/linux/drivers/media/video/cx18/cx18-fileops.c Fri May 02 07:51:27 2008 -0300
+++ b/linux/drivers/media/video/cx18/cx18-fileops.c Fri May 02 22:21:12 2008 -0400
@@ -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];
[-- Attachment #3: Type: text/plain, Size: 164 bytes --]
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] 2nd try: Fix potential cx18_cards[] entry leaks
2008-05-03 2:43 ` [PATCH] Fix potential cx18_cards[] entry leaks Andy Walls
@ 2008-05-03 13:33 ` Andy Walls
2008-05-12 15:05 ` [PATCH] " Hans Verkuil
1 sibling, 0 replies; 15+ messages in thread
From: Andy Walls @ 2008-05-03 13:33 UTC (permalink / raw)
To: ivtv-devel; +Cc: Linux and Kernel Video, Michael Krufky
[-- Attachment #1: Type: text/plain, Size: 862 bytes --]
On Fri, 2008-05-02 at 22:43 -0400, 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.
> 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
[-- Attachment #2: cx18-cards-leak3.patch --]
[-- Type: text/x-patch, Size: 2414 bytes --]
# HG changeset patch
# User Andy Walls <awalls@radix.net>
# Date 1209820624 14400
# Node ID 2f883fedfb85c1979d9352ffdcf97a2d901dfbeb
# Parent 4c4fd6b8755cc9918255876ff1010bc77374a310
Prevent cx18_cards[] leak and possible bad pointer dereference
From: Andy Walls <awalls@radix.net>
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.
Signed-off-by: Andy Walls <awalls@radix.net>
diff -r 4c4fd6b8755c -r 2f883fedfb85 linux/drivers/media/video/cx18/cx18-driver.c
--- a/linux/drivers/media/video/cx18/cx18-driver.c Fri May 02 07:51:27 2008 -0300
+++ b/linux/drivers/media/video/cx18/cx18-driver.c Sat May 03 09:17:04 2008 -0400
@@ -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);
diff -r 4c4fd6b8755c -r 2f883fedfb85 linux/drivers/media/video/cx18/cx18-fileops.c
--- a/linux/drivers/media/video/cx18/cx18-fileops.c Fri May 02 07:51:27 2008 -0300
+++ b/linux/drivers/media/video/cx18/cx18-fileops.c Sat May 03 09:17:04 2008 -0400
@@ -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];
[-- Attachment #3: Type: text/plain, Size: 164 bytes --]
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] Fix potential cx18_cards[] entry leaks
2008-05-03 2:43 ` [PATCH] Fix potential cx18_cards[] entry leaks Andy Walls
2008-05-03 13:33 ` [PATCH] 2nd try: " Andy Walls
@ 2008-05-12 15:05 ` Hans Verkuil
2008-05-12 23:30 ` Andy Walls
1 sibling, 1 reply; 15+ messages in thread
From: Hans Verkuil @ 2008-05-12 15:05 UTC (permalink / raw)
To: Andy Walls; +Cc: Linux and Kernel Video, Michael Krufky, ivtv-devel
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).
>
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix potential cx18_cards[] entry leaks
2008-05-12 15:05 ` [PATCH] " Hans Verkuil
@ 2008-05-12 23:30 ` Andy Walls
0 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2008-05-12 23:30 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux and Kernel Video, Michael Krufky, ivtv-devel
On Mon, 2008-05-12 at 17:05 +0200, Hans Verkuil wrote:
> 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).
> >
> > 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
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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 15+ messages in thread