[PATCH] tm6000: Prevent Kernel Oops changing channel when stream is still on.

[PATCH] tm6000: Prevent Kernel Oops changing channel when stream is still on.

[Bee Hock Goh]

do a streamoff before setting standard to prevent kernel oops by
irq_callback if changing of channel is done while streaming is still
on-going.


Signed-off-by: Bee Hock Goh <beehock@gmail.com>

diff --git a/drivers/staging/tm6000/tm6000-video.c
b/drivers/staging/tm6000/tm6000-video.c
index c53de47..32f625d 100644
--- a/drivers/staging/tm6000/tm6000-video.c
+++ b/drivers/staging/tm6000/tm6000-video.c

@@ -1081,8 +1086,8 @@ static int vidioc_s_std (struct file *file, void
*priv, v4l2_std_id *norm)
 	struct tm6000_fh   *fh=priv;
 	struct tm6000_core *dev = fh->dev;

+	vidioc_streamoff(file, priv, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 	rc=tm6000_set_standard (dev, norm);
-
 	fh->width  = dev->width;
 	fh->height = dev->height;

Re: [PATCH] tm6000: Prevent Kernel Oops changing channel when stream is still on.

[Mauro Carvalho Chehab]

Hi Bee,

Bee Hock Goh wrote:
> do a streamoff before setting standard to prevent kernel oops by
> irq_callback if changing of channel is done while streaming is still
> on-going.
> 
> 
> Signed-off-by: Bee Hock Goh <beehock@gmail.com>
> 
> diff --git a/drivers/staging/tm6000/tm6000-video.c
> b/drivers/staging/tm6000/tm6000-video.c
> index c53de47..32f625d 100644
> --- a/drivers/staging/tm6000/tm6000-video.c
> +++ b/drivers/staging/tm6000/tm6000-video.c
> 
> @@ -1081,8 +1086,8 @@ static int vidioc_s_std (struct file *file, void
> *priv, v4l2_std_id *norm)
>  	struct tm6000_fh   *fh=priv;
>  	struct tm6000_core *dev = fh->dev;
> 
> +	vidioc_streamoff(file, priv, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	rc=tm6000_set_standard (dev, norm);
> -
>  	fh->width  = dev->width;
>  	fh->height = dev->height;

This doesn't seem to be the right thing to do. The problem here is that
changing a video standard takes a long time to happen. As calling an
ioctl is protected by KBL, QBUF/DQBUF won't be called, so, the driver
will run out of the buffers, and *buf will become null. This can eventually
happen during copy_streams().

---

tm6000: Fix a panic if buffer become NULL

Changing a video standard takes a long time to happen on tm6000, since it
needs to load another firmware, and the i2c implementation on this device
is really slow. As calling an ioctl is protected by KBL, QBUF/DQBUF won't 
be called, so, the driver will run out of the buffers, and *buf will become 
NULL. This can eventually happen during copy_streams(). The fix is to leave
the URB copy loop, if there's no more buffers available.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/linux/drivers/staging/tm6000/tm6000-video.c b/linux/drivers/staging/tm6000/tm6000-video.c
--- a/linux/drivers/staging/tm6000/tm6000-video.c
+++ b/linux/drivers/staging/tm6000/tm6000-video.c
@@ -539,7 +539,7 @@ static inline int tm6000_isoc_copy(struc
 				}
 			}
 			copied += len;
-			if (copied>=size)
+			if (copied >= size || !buf)
 				break;
 //		}
 	}

Re: [PATCH] tm6000: Prevent Kernel Oops changing channel when stream is still on.

[Mauro Carvalho Chehab]

Mauro Carvalho Chehab wrote:
> Hi Bee,
> 
> Bee Hock Goh wrote:
>> do a streamoff before setting standard to prevent kernel oops by
>> irq_callback if changing of channel is done while streaming is still
>> on-going.
> 
> This doesn't seem to be the right thing to do. The problem here is that
> changing a video standard takes a long time to happen. As calling an
> ioctl is protected by KBL, QBUF/DQBUF won't be called, so, the driver
> will run out of the buffers, and *buf will become null. This can eventually
> happen during copy_streams().
> 
> ---
> 
> tm6000: Fix a panic if buffer become NULL
> 
> Changing a video standard takes a long time to happen on tm6000, since it
> needs to load another firmware, and the i2c implementation on this device
> is really slow. As calling an ioctl is protected by KBL, QBUF/DQBUF won't 
> be called, so, the driver will run out of the buffers, and *buf will become 
> NULL. This can eventually happen during copy_streams(). The fix is to leave
> the URB copy loop, if there's no more buffers available.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/linux/drivers/staging/tm6000/tm6000-video.c b/linux/drivers/staging/tm6000/tm6000-video.c

Sorry, I sent the wrong one. That's the proper fix. I've also improved
the comments to better express what's happening.

Tested here with HVR-900H and the Panic disappeared.

---

tm6000: Fix a panic if buffer become NULL

Changing a video standard takes a long time to happen on tm6000, since it
needs to load another firmware, and the i2c implementation on this device
is really slow. When the driver tries to change the video standard, a
kernel panic is produced:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffffa0c7b48a>] tm6000_irq_callback+0x57f/0xac2 [tm6000]
...
Kernel panic - not syncing: Fatal exception in interrupt

By inspecting it with gdb:

(gdb) list *tm6000_irq_callback+0x57f
0x348a is in tm6000_irq_callback (drivers/staging/tm6000/tm6000-video.c:202).
197             /* FIXME: move to tm6000-isoc */
198             static int last_line = -2, start_line = -2, last_field = -2;
199
200             /* FIXME: this is the hardcoded window size
201              */
202             unsigned int linewidth = (*buf)->vb.width << 1;
203
204             if (!dev->isoc_ctl.cmd) {
205                     c = (header >> 24) & 0xff;
206

Clearly, it was the trial to access *buf, at line 202 that caused the
Panic.

As ioctl is serialized, While S_STD is handled,QBUF/DQBUF won't be called.
So, the driver will run out of the buffers, and *buf will become NULL. 

As, on tm6000, the same URB can contain more than one video buffer, it is
likely to hit a condition where no new buffer is available whily copying
the streams. The fix is to leave the URB copy loop, if there's no more buffers 
are available.

The same bug could also be produced by an application that is not fast enough
to request new video buffers.

The same bug were reported by Bee Hock Goh <beehock@gmail.com>.

Thanks-to: Bee Hock Goh <beehock@gmail.com> for reporting the bug
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Index: work.x86-64/drivers/staging/tm6000/tm6000-video.c
===================================================================
--- work.x86-64.orig/drivers/staging/tm6000/tm6000-video.c
+++ work.x86-64/drivers/staging/tm6000/tm6000-video.c
@@ -395,6 +395,8 @@ HEADER:
 					jiffies);
 			return rc;
 		}
+		if (!*buf)
+			return 0;
 	}
 
 	return 0;
@@ -528,7 +530,7 @@ static inline int tm6000_isoc_copy(struc
 				}
 			}
 			copied += len;
-			if (copied>=size)
+			if (copied >= size || !buf)
 				break;
 //		}
 	}

Re: [PATCH] tm6000: Prevent Kernel Oops changing channel when stream is still on.

[Mauro Carvalho Chehab]

The two patches fixed the OOPS I was having.

The big problem I'm still suffering with HVR-900H is that tm6000 insists on dying:

hub 1-0:1.0: port 8 disabled by hub (EMI?), re-enabling...
usb 1-8: USB disconnect, address 5
tm6000 tm6000_irq_callback :urb resubmit failed (error=-19)
tm6000 tm6000_irq_callback :urb resubmit failed (error=-19)
tm6000 tm6000_irq_callback :urb resubmit failed (error=-19)
tm6000 tm6000_irq_callback :urb resubmit failed (error=-19)
tm6000 tm6000_irq_callback :urb resubmit failed (error=-19)
tm6000: disconnecting tm6000 #2
xc2028 2-0061: destroying instance

As the chipset stops answering USB, a new, non-fatal bug hits:

------------[ cut here ]------------
WARNING: at lib/list_debug.c:48 list_del+0x30/0x87()
Hardware name:  
list_del corruption. prev->next should be ffff88003df65ec0, but was (null)
Modules linked in: ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder ir_core tm6000(C) v4l2_common videodev v4l1_compat v4l2_compat_ioctl32 videobuf_vmalloc videobuf_core autofs4 hidp rfcomm l2cap crc16 bluetooth iptable_filter ip_tables ip6t_REJECT xt_tcpudp ip6table_filter ip6_tables x_tables ipv6 powernow_k8 dm_multipath scsi_dh sbs sbshc battery acpi_memhotplug ac lp snd_intel8x0 snd_ac97_codec ac97_bus snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device i2c_algo_bit snd_pcm_oss snd_mixer_oss sg snd_pcm nvidia(P) ide_cd_mod snd_timer serio_raw parport_pc cdrom snd button parport floppy i2c_nforce2 k8temp soundcore snd_page_alloc pcspkr shpchp hwmon forcedeth i2c_core dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod sata_nv libata sd_mod scsi_mod ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: tuner_xc2028]
Pid: 12402, comm: mplayer Tainted: P        WC 2.6.33 #4
Call Trace:
 [<ffffffff81179c24>] ? list_del+0x30/0x87
 [<ffffffff8103a43f>] ? warn_slowpath_common+0x77/0x8e
 [<ffffffff8103a4b2>] ? warn_slowpath_fmt+0x51/0x59
 [<ffffffff810c54a5>] ? free_block+0xdf/0xfe
 [<ffffffff8102c9bc>] ? __wake_up_sync_key+0x3a/0x56
 [<ffffffff81179c24>] ? list_del+0x30/0x87
 [<ffffffffa015e9c8>] ? videobuf_queue_cancel+0x48/0xba [videobuf_core]
 [<ffffffffa013e2b3>] ? videobuf_vm_close+0x80/0x14d [videobuf_vmalloc]
 [<ffffffff810b23d1>] ? remove_vma+0x2c/0x72
 [<ffffffff810b2522>] ? exit_mmap+0x10b/0x129
 [<ffffffff8103813c>] ? mmput+0x34/0xa2
 [<ffffffff8103c077>] ? exit_mm+0x109/0x114
 [<ffffffff8103d2b0>] ? do_exit+0x1de/0x66e
 [<ffffffff8103d7ad>] ? do_group_exit+0x6d/0x97
 [<ffffffff8103d7e9>] ? sys_exit_group+0x12/0x16
 [<ffffffff810028ab>] ? system_call_fastpath+0x16/0x1b
---[ end trace fed27d3fe75cb89b ]---
------------[ cut here ]------------
WARNING: at lib/list_debug.c:51 list_del+0x5c/0x87()
Hardware name:  
list_del corruption. next->prev should be ffff88003df65bc0, but was (null)
Modules linked in: ir_sony_decoder ir_jvc_decoder ir_rc6_decoder ir_rc5_decoder ir_nec_decoder ir_core tm6000(C) v4l2_common videodev v4l1_compat v4l2_compat_ioctl32 videobuf_vmalloc videobuf_core autofs4 hidp rfcomm l2cap crc16 bluetooth iptable_filter ip_tables ip6t_REJECT xt_tcpudp ip6table_filter ip6_tables x_tables ipv6 powernow_k8 dm_multipath scsi_dh sbs sbshc battery acpi_memhotplug ac lp snd_intel8x0 snd_ac97_codec ac97_bus snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device i2c_algo_bit snd_pcm_oss snd_mixer_oss sg snd_pcm nvidia(P) ide_cd_mod snd_timer serio_raw parport_pc cdrom snd button parport floppy i2c_nforce2 k8temp soundcore snd_page_alloc pcspkr shpchp hwmon forcedeth i2c_core dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod sata_nv libata sd_mod scsi_mod ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: tuner_xc2028]
Pid: 12402, comm: mplayer Tainted: P        WC 2.6.33 #4
Call Trace:
 [<ffffffff81179c50>] ? list_del+0x5c/0x87
 [<ffffffff8103a43f>] ? warn_slowpath_common+0x77/0x8e
 [<ffffffff8103a4b2>] ? warn_slowpath_fmt+0x51/0x59
 [<ffffffff810c54a5>] ? free_block+0xdf/0xfe
 [<ffffffff81035d58>] ? __wake_up+0x30/0x44
 [<ffffffff81179c50>] ? list_del+0x5c/0x87
 [<ffffffffa015e9c8>] ? videobuf_queue_cancel+0x48/0xba [videobuf_core]
 [<ffffffffa013e2b3>] ? videobuf_vm_close+0x80/0x14d [videobuf_vmalloc]
 [<ffffffff810b23d1>] ? remove_vma+0x2c/0x72
 [<ffffffff810b2522>] ? exit_mmap+0x10b/0x129
 [<ffffffff8103813c>] ? mmput+0x34/0xa2
 [<ffffffff8103c077>] ? exit_mm+0x109/0x114
 [<ffffffff8103d2b0>] ? do_exit+0x1de/0x66e
 [<ffffffff8103d7ad>] ? do_group_exit+0x6d/0x97
 [<ffffffff8103d7e9>] ? sys_exit_group+0x12/0x16
 [<ffffffff810028ab>] ? system_call_fastpath+0x16/0x1b
---[ end trace fed27d3fe75cb89c ]---
usbcore: deregistering interface driver tm6000


Cheers,
Mauro

-- 

Cheers,
Mauro