bug in changeset 13239:54535665f94b ?

bug in changeset 13239:54535665f94b ?

[e9hack]

Hi,

something is wrong in changeset 13239:54535665f94b. After applying it, I get page faults
in various applications:
...
Oct 31 17:36:35 vdr dhcpcd[3280]: wlan1: adding default route via 192.168.23.1 metric 0
Oct 31 17:36:35 vdr dhcpcd[3280]: wlan1: adding route to 169.254.0.0/16 metric 0
Oct 31 17:36:36 vdr kernel: [   25.759549] DEBI rx: 0(0)kB/s size=188/188/188 cnt=0/s, tx:
0(0)kB/s size=0/0/0 cnt=0/s
Oct 31 17:36:36 vdr kernel: [   25.787398] video directory[3249]: segfault at 7f8e8be1139d
ip 00007f8e8be125ce sp 0000000042312280 error 7 in libc-2.8.so[7f8e8bdbb000+14f000]
Oct 31 17:36:36 vdr modify_resolvconf: Service dhcpcd modified /etc/resolv.conf. See info
block in this file
Oct 31 17:36:36 vdr dhcpcd[3280]: wlan1: exiting
Oct 31 17:36:36 vdr kernel: [   25.858000] killproc[3380]: segfault at a18 ip
00007f2441b1b0b7 sp 00007fffbee296c0 error 6 in libc-2.8.so[7f2441ac4000+14f000]
Oct 31 17:36:36 vdr kernel: [   25.860567] killproc[3381]: segfault at a15 ip
00007fc02b4ad0b7 sp 00007fff554387e0 error 6 in libc-2.8.so[7fc02b456000+14f000]
Oct 31 17:36:36 vdr kernel: [   25.862552] killproc[3382]: segfault at a18 ip
00007f2016d9f0b7 sp 00007fff6e366b90 error 6 in libc-2.8.so[7f2016d48000+14f000]
Oct 31 17:36:36 vdr kernel: [   25.864523] killproc[3383]: segfault at a18 ip
00007f91d85df0b7 sp 00007fff8b13f2c0 error 6 in libc-2.8.so[7f91d8588000+14f000]
Oct 31 17:36:36 vdr ifdown:     wlan1
Oct 31 17:36:36 vdr kernel: [   25.942528] killproc[3416]: segfault at 1 ip
00007fdcdeccb0b7 sp 00007fff33c2ff00 error 6 in libc-2.8.so[7fdcdec74000+14f000]
Oct 31 17:36:36 vdr kernel: [   25.965127] ip[3423]: segfault at 0 ip 00007fb0dc50b47e sp
00007fffbf6d2790 error 6 in libc-2.8.so[7fb0dc4b4000+14f000]
Oct 31 17:36:36 vdr ifup:     wlan1
Oct 31 17:36:36 vdr SuSEfirewall2: /var/lock/SuSEfirewall2.booting exists which means
system boot in progress, exit.
Oct 31 17:36:36 vdr ifup-dhcp: IP/Netmask: '192.168.23.6'
Oct 31 17:36:36 vdr ifup-dhcp:  / '255.255.255.0'
Oct 31 17:36:36 vdr ifup-dhcp:  ('vdr')
Oct 31 17:36:36 vdr ifup-dhcp:
Oct 31 17:36:36 vdr kernel: [   26.567896] ip[3551]: segfault at 0 ip 00007f8c9b00f47e sp
00007fff71063240 error 6 in libc-2.8.so[7f8c9afb8000+14f000]
Oct 31 17:36:36 vdr kernel: [   26.664260] startproc[3587]: segfault at 99f ip
00007fa6c35790b7 sp 00007fffc6d713c0 error 6 in libc-2.8.so[7fa6c3522000+14f000]
...


If I remove the call to release_all_pagetables() in buffer_release(), I don't see this
page faults.

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

Hi,

the BUG is in vidioc_streamoff() for the saa7146. This function
releases all buffers first, and stops the capturing and dma tranfer of
the saa7146 as second. If the page table, which is currently used by
the saa7146, is modified by another thread, the saa7146 writes
anywhere to the physical RAM. IMHO vidioc_streamoff() must stop the
saa7146 first and may then release the buffers.

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[Mauro Carvalho Chehab]

Hi Hartmut,

Em Sun, 01 Nov 2009 16:59:26 +0100
e9hack <e9hack@googlemail.com> escreveu:

> Hi,
> 
> something is wrong in changeset 13239:54535665f94b. After applying it, I get page faults
> in various applications:
> ...
> 
> If I remove the call to release_all_pagetables() in buffer_release(), I don't see this
> page faults.


Em Mon, 2 Nov 2009 21:27:44 +0100
e9hack@googlemail.com escreveu:

> the BUG is in vidioc_streamoff() for the saa7146. This function
> releases all buffers first, and stops the capturing and dma tranfer of
> the saa7146 as second. If the page table, which is currently used by
> the saa7146, is modified by another thread, the saa7146 writes
> anywhere to the physical RAM. IMHO vidioc_streamoff() must stop the
> saa7146 first and may then release the buffers.

I agree. We need first to stop DMA activity, and then release the page tables.

Could you please test if the enclosed patch fixes the issue?

Cheers,
Mauro

saa7146: stop DMA before de-allocating DMA scatter/gather page buffers

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

diff --git a/linux/drivers/media/common/saa7146_video.c b/linux/drivers/media/common/saa7146_video.c
--- a/linux/drivers/media/common/saa7146_video.c
+++ b/linux/drivers/media/common/saa7146_video.c
@@ -1334,9 +1334,9 @@ static void buffer_release(struct videob
 
 	DEB_CAP(("vbuf:%p\n",vb));
 
+	saa7146_dma_free(dev,q,buf);
+
 	release_all_pagetables(dev, buf);
-
-	saa7146_dma_free(dev,q,buf);
 }
 
 static struct videobuf_queue_ops video_qops = {

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

Mauro Carvalho Chehab schrieb:

> I agree. We need first to stop DMA activity, and then release the page tables.
> 
> Could you please test if the enclosed patch fixes the issue?

Hi Mauro,

your patch doesn't solve the problem, because saa7146_dma_free() doesn't stop a running
dma transfer of the saa7146. Since last weekend, I'm using the attached patch. I'm not
sure, if the functionality of video_end() must be split. Maybe the last part of
video_end() must be execute at the end of vidioc_streamoff().

Regards,
Hartmut





[-- Attachment #2: saa7146_video.c.diff --]
[-- Type: text/x-diff, Size: 3565 bytes --]

vdr:/usr/src/v4l-dvb/linux/drivers/media/common # hg diff saa7146_video.c
diff -r 40705fec2fb2 linux/drivers/media/common/saa7146_video.c
--- a/linux/drivers/media/common/saa7146_video.c        Fri Nov 06 15:54:49 2009 -0200
+++ b/linux/drivers/media/common/saa7146_video.c        Sat Nov 07 15:02:32 2009 +0100
@@ -1093,22 +1093,18 @@ static int vidioc_streamoff(struct file
                return 0;
        }

-       if (vv->video_fh != fh) {
-               DEB_S(("capturing, but in another open.\n"));
-               return -EBUSY;
-       }
+       err = video_end(fh, file);
+       if (err != 0)
+               return err;

        err = -EINVAL;
        if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
                err = videobuf_streamoff(&fh->video_q);
        else if (type == V4L2_BUF_TYPE_VBI_CAPTURE)
                err = videobuf_streamoff(&fh->vbi_q);
-       if (0 != err) {
+       if (0 != err)
                DEB_D(("warning: videobuf_streamoff() failed.\n"));
-               video_end(fh, file);
-       } else {
-               err = video_end(fh, file);
-       }
+
        return err;
 }

@@ -1332,7 +1328,44 @@ static void buffer_release(struct videob
        struct saa7146_dev *dev = fh->dev;
        struct saa7146_buf *buf = (struct saa7146_buf *)vb;

+       int i;
+       u32 mc1;
+
        DEB_CAP(("vbuf:%p\n",vb));
+
+       mc1 = saa7146_read(dev, MC1);
+       for(i = 0; i < 3; i++)
+       {
+               u32 base_even;
+               u32 base_odd;
+               u32 prot_addr;
+               u32 base_page;
+               u32 pitch;
+               u32 num;
+               u32 vptr;
+
+               if (buf->pt[i].cpu && buf->pt[i].dma) {
+                       const u32 dma_mask[] = {MASK_06, MASK_05, MASK_04};
+                       vptr = saa7146_read(dev, PCI_VDP1 + i * (PCI_VDP2 - PCI_VDP1));
+                       base_even = saa7146_read(dev, BASE_EVEN1 + i * (BASE_EVEN2 - BASE_EVEN1));
+                       base_odd = saa7146_read(dev, BASE_ODD1 + i * (BASE_ODD2 - BASE_ODD1));
+                       prot_addr = saa7146_read(dev, PROT_ADDR1 + i * (PROT_ADDR2 - PROT_ADDR1));
+                       base_page = saa7146_read(dev, BASE_PAGE1 + i *(BASE_PAGE2 - BASE_PAGE1));
+                       pitch = saa7146_read(dev, PITCH1 + i * (PITCH2 - PITCH1));
+                       num = saa7146_read(dev, NUM_LINE_BYTE1 + i * (NUM_LINE_BYTE2 - NUM_LINE_BYTE1));
+
+                       if ((base_page & (0xfffff000 | ME1)) == (buf->pt[i].dma | ME1) && (mc1 & dma_mask[i])) {
+                               printk("(%s:%d) vdma%d.base_even:     %08x\n", __FILE__, __LINE__, i + 1, base_even);
+                               printk("(%s:%d) vdma%d.base_odd:      %08x\n", __FILE__, __LINE__, i + 1, base_odd);
+                               printk("(%s:%d) vdma%d.prot_addr:     %08x\n", __FILE__, __LINE__, i + 1, prot_addr);
+                               printk("(%s:%d) vdma%d.base_page:     %08x\n", __FILE__, __LINE__, i + 1, base_page);
+                               printk("(%s:%d) vdma%d.pitch:         %08x\n", __FILE__, __LINE__, i + 1, pitch);
+                               printk("(%s:%d) vdma%d.num_line_byte: %08x\n", __FILE__, __LINE__, i + 1, num);
+                               printk("(%s:%d) vdma%d.vptr:          %08x\n", __FILE__, __LINE__, i + 1, vptr);
+                               printk("(%s:%d) MC1:                 %08x\n", __FILE__, __LINE__, mc1);
+                       }
+               }
+       }

        release_all_pagetables(dev, buf);

Re: bug in changeset 13239:54535665f94b ?

[Mauro Carvalho Chehab]

Em Sat, 07 Nov 2009 15:05:02 +0100
e9hack <e9hack@googlemail.com> escreveu:

> Mauro Carvalho Chehab schrieb:
> 
> > I agree. We need first to stop DMA activity, and then release the page tables.
> > 
> > Could you please test if the enclosed patch fixes the issue?
> 
> Hi Mauro,
> 
> your patch doesn't solve the problem, because saa7146_dma_free() doesn't stop a running
> dma transfer of the saa7146.

Well, it should be stopping it. The logic is to wait for an incoming dma
transfer and then disable dma transfers:

void saa7146_dma_free(struct saa7146_dev *dev,struct videobuf_queue *q,
                                                struct saa7146_buf *buf)
{
        struct videobuf_dmabuf *dma=videobuf_to_dma(&buf->vb);
        DEB_EE(("dev:%p, buf:%p\n",dev,buf));

        BUG_ON(in_interrupt());

        videobuf_waiton(&buf->vb,0,0);
        videobuf_dma_unmap(q, dma);
        videobuf_dma_free(dma);
        buf->vb.state = VIDEOBUF_NEEDS_INIT;
}

Maybe the code for dma_unmap is incomplete?

> Since last weekend, I'm using the attached patch. I'm not
> sure, if the functionality of video_end() must be split. Maybe the last part of
> video_end() must be execute at the end of vidioc_streamoff().

It is not safe to stop at streamoff(), since applications may close the device
without calling streamoff. The kernel driver should be able to handle such
situations as well.

So, if my patch doesn't work, we'll need to add more bits at saa7146_dma_free().

-- 

Cheers,
Mauro

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

Mauro Carvalho Chehab schrieb:
> Em Sat, 07 Nov 2009 15:05:02 +0100
> e9hack <e9hack@googlemail.com> escreveu:
> 
>> Mauro Carvalho Chehab schrieb:
>>
>>> I agree. We need first to stop DMA activity, and then release the page tables.
>>>
>>> Could you please test if the enclosed patch fixes the issue?
>> Hi Mauro,
>>
>> your patch doesn't solve the problem, because saa7146_dma_free() doesn't stop a running
>> dma transfer of the saa7146.
> 
> Well, it should be stopping it. The logic is to wait for an incoming dma
> transfer and then disable dma transfers:
> 
> void saa7146_dma_free(struct saa7146_dev *dev,struct videobuf_queue *q,
>                                                 struct saa7146_buf *buf)
> {
>         struct videobuf_dmabuf *dma=videobuf_to_dma(&buf->vb);
>         DEB_EE(("dev:%p, buf:%p\n",dev,buf));
> 
>         BUG_ON(in_interrupt());
> 
>         videobuf_waiton(&buf->vb,0,0);
>         videobuf_dma_unmap(q, dma);
>         videobuf_dma_free(dma);
>         buf->vb.state = VIDEOBUF_NEEDS_INIT;
> }

In my case, videobuf_queue_cancel() is called previously. videobuf_queue_cancel() wakes up
all buffers, but it doesn't handle the currently by the saa7146 used buffer. queue->curr
points to this buffer. Waiting for an incoming dma transfer in saa7146_dma_free() has no
effect for such a buffer.

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

e9hack schrieb:
>> I agree. We need first to stop DMA activity, and then release the page tables.
>>
>> Could you please test if the enclosed patch fixes the issue?
> 
> your patch doesn't solve the problem, because saa7146_dma_free() doesn't stop a running
> dma transfer of the saa7146. 

Sorry, that was wrong. I did only look, if my additional messages were print or not, but I
didn't wait for the memory corruption, because it may corrupt the file system. With your
patch, I get the messages, but the protection address is set to 0, which disables the dma
transfer even if the dma transfer is enabled within the MC1 register.

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[Johann Friedrichs]

Mauro Carvalho Chehab schrieb:
> Hi Hartmut,
> 
> Em Sun, 01 Nov 2009 16:59:26 +0100
> e9hack <e9hack@googlemail.com> escreveu:
> 
>> Hi,
>>
>> something is wrong in changeset 13239:54535665f94b. After applying it, I get page faults
>> in various applications:
>> ...
>>
>> If I remove the call to release_all_pagetables() in buffer_release(), I don't see this
>> page faults.
> 
> 
> Em Mon, 2 Nov 2009 21:27:44 +0100
> e9hack@googlemail.com escreveu:
> 
>> the BUG is in vidioc_streamoff() for the saa7146. This function
>> releases all buffers first, and stops the capturing and dma tranfer of
>> the saa7146 as second. If the page table, which is currently used by
>> the saa7146, is modified by another thread, the saa7146 writes
>> anywhere to the physical RAM. IMHO vidioc_streamoff() must stop the
>> saa7146 first and may then release the buffers.
> 
> I agree. We need first to stop DMA activity, and then release the page tables.
> 
> Could you please test if the enclosed patch fixes the issue?
> 
> Cheers,
> Mauro
> 
> saa7146: stop DMA before de-allocating DMA scatter/gather page buffers
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/linux/drivers/media/common/saa7146_video.c b/linux/drivers/media/common/saa7146_video.c
> --- a/linux/drivers/media/common/saa7146_video.c
> +++ b/linux/drivers/media/common/saa7146_video.c
> @@ -1334,9 +1334,9 @@ static void buffer_release(struct videob
>  
>  	DEB_CAP(("vbuf:%p\n",vb));
>  
> +	saa7146_dma_free(dev,q,buf);
> +
>  	release_all_pagetables(dev, buf);
> -
> -	saa7146_dma_free(dev,q,buf);
>  }
>  
>  static struct videobuf_queue_ops video_qops = {
> 
Hi Mauro,
I cannot easily catch any memory overwriting done by 
saa7146-capture-dma, but Hartmut is right that there is quite a chance.
I would prefer calling video_end() before videobuf_streamoff() in 
vidioc_streamoff(), because this physically shuts down the capture 
immediately at the hardware source. By the way, this is done in the same 
sequence in video_close, where videobuf_stop (which calls 
videobuf_streamoff) also gets called after video_end.
I have no negative impact with your patch and it might shutdown the dma 
as well, but as said before, I don't see any immediate errors even with 
the released version.
Cheers
Johann

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

Mauro Carvalho Chehab schrieb:
> Could you please test if the enclosed patch fixes the issue?
> 
> saa7146: stop DMA before de-allocating DMA scatter/gather page buffers
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/linux/drivers/media/common/saa7146_video.c b/linux/drivers/media/common/saa7146_video.c
> --- a/linux/drivers/media/common/saa7146_video.c
> +++ b/linux/drivers/media/common/saa7146_video.c
> @@ -1334,9 +1334,9 @@ static void buffer_release(struct videob
>  
>  	DEB_CAP(("vbuf:%p\n",vb));
>  
> +	saa7146_dma_free(dev,q,buf);
> +
>  	release_all_pagetables(dev, buf);
> -
> -	saa7146_dma_free(dev,q,buf);
>  }
>  
>  static struct videobuf_queue_ops video_qops = {

Hi Mauro,

can you please commit this patch? It's perfect just I wrote here
(http://news.gmane.org/gmane.linux.drivers.video-input-infrastructure).

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[e9hack]

e9hack schrieb:
> can you please commit this patch? It's perfect just I wrote here
> (http://news.gmane.org/gmane.linux.drivers.video-input-infrastructure).

Ooops, the link wasn't correct.
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/11846

Regards,
Hartmut

Re: bug in changeset 13239:54535665f94b ?

[Mauro Carvalho Chehab]

e9hack wrote:
> e9hack schrieb:
>> can you please commit this patch? It's perfect just I wrote here
>> (http://news.gmane.org/gmane.linux.drivers.video-input-infrastructure).
> 
> Ooops, the link wasn't correct.
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/11846
> 
> Regards,
> Hartmut
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Hartmut,

There's no patch at the link you've pointed me.

Please re-send me the patch that worked.

Cheers,
Mauro.