From patchwork Tue Dec 4 15:48:40 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans Verkuil X-Patchwork-Id: 1838981 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id DAAFF3FC64 for ; Tue, 4 Dec 2012 15:49:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752047Ab2LDPta (ORCPT ); Tue, 4 Dec 2012 10:49:30 -0500 Received: from smtp-vbr7.xs4all.nl ([194.109.24.27]:3452 "EHLO smtp-vbr7.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751835Ab2LDPt3 (ORCPT ); Tue, 4 Dec 2012 10:49:29 -0500 Received: from alastor.dyndns.org (166.80-203-20.nextgentel.com [80.203.20.166] (may be forged)) (authenticated bits=0) by smtp-vbr7.xs4all.nl (8.13.8/8.13.8) with ESMTP id qB4FmkNw096044 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Tue, 4 Dec 2012 16:48:46 +0100 (CET) (envelope-from hverkuil@xs4all.nl) Received: from tschai.localnet (tschai.lan [192.168.1.10]) (Authenticated sender: hans) by alastor.dyndns.org (Postfix) with ESMTPSA id C68FB65A0009; Tue, 4 Dec 2012 16:48:39 +0100 (CET) To: LMML Subject: [RFC PATCH] vb2: force output buffers to fault into memory Cc: Marek Szyprowski , Laurent Pinchart , Pawel Osciak From: Hans Verkuil Date: Tue, 4 Dec 2012 16:48:40 +0100 MIME-Version: 1.0 Message-Id: <201212041648.40108.hverkuil@xs4all.nl> X-Virus-Scanned: by XS4ALL Virus Scanner Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org (repost after accidentally using HTML formatting) This needs a good review. The change is minor, but as I am not a mm expert, I'd like to get some more feedback on this. The dma-sg change has been successfully tested on our hardware, but I don't have any hardware to test the vmalloc change. Note that the 'write' attribute is still stored internally and used to tell whether set_page_dirty_lock() should be called during put_userptr. It is my understanding that that still makes sense, so I didn't change that. Regards, Hans --- start patch --- When calling get_user_pages for output buffers, the 'write' argument is set to 0 (since the DMA isn't writing to memory), This can cause unexpected results: If you calloc() buffer memory and do not fill that memory afterwards, then the kernel assigns most of that memory to one single physical 'zero' page. If you queue that buffer to the V4L2 driver, then it will call get_user_pages and store the results. Next you dequeue it, fill the buffer and queue it again. Now the V4L2 core code sees the same userptr and length and expects it to be the same buffer that it got before and it will reuse the results of the previous get_user_pages call. But that still points to zero pages, whereas userspace filled it up and so changed the buffer to use different pages. In other words, the pages the V4L2 core knows about are no longer correct. The solution is to always set 'write' to 1 as this will force the kernel to fault in proper pages. We do this for videobuf2-dma-sg.c and videobuf2-vmalloc.c, but not for videobuf2-dma-contig.c since the userptr there is already supposed to point to contiguous memory and shouldn't use the zero page at all. Signed-off-by: Hans Verkuil --- drivers/media/v4l2-core/videobuf2-dma-sg.c | 3 ++- drivers/media/v4l2-core/videobuf2-vmalloc.c | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c index 25c3b36..c29f159 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c @@ -143,7 +143,8 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, num_pages_from_user = get_user_pages(current, current->mm, vaddr & PAGE_MASK, buf->sg_desc.num_pages, - write, + 1, /* always set write to force + faulting all pages */ 1, /* force */ buf->pages, NULL); diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c index a47fd4f..c8d8519 100644 --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c @@ -107,7 +107,9 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr, /* current->mm->mmap_sem is taken by videobuf2 core */ n_pages = get_user_pages(current, current->mm, vaddr & PAGE_MASK, buf->n_pages, - write, 1, /* force */ + 1, /* always set write to force + faulting all pages */ + 1, /* force */ buf->pages, NULL); if (n_pages != buf->n_pages) goto fail_get_user_pages;