From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from aer-iport-4.cisco.com ([173.38.203.54]:33619 "EHLO aer-iport-4.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754647AbbFKRdg (ORCPT ); Thu, 11 Jun 2015 13:33:36 -0400 Message-ID: <5579C3FF.8050202@cisco.com> Date: Thu, 11 Jun 2015 19:23:11 +0200 From: Hans Verkuil MIME-Version: 1.0 To: Laurent Pinchart , Mauro Carvalho Chehab CC: Andrew Morton , Jan Kara , Linux Media Mailing List , Mauro Carvalho Chehab , Hans Verkuil , Fabian Frederick , Prabhakar Lad , Tomi Valkeinen Subject: Re: [PATCH 2/9] [media] media: omap_vout: Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns() References: <0bec810973e08df0e66260e84d2dcea055a3fad7.1433927458.git.mchehab@osg.samsung.com> <1439884.SWlnxou8Xt@avalon> In-Reply-To: <1439884.SWlnxou8Xt@avalon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 06/11/15 06:21, Laurent Pinchart wrote: > Hello, > > (CC'ing Tomi Valkeinen) > > On Wednesday 10 June 2015 06:20:45 Mauro Carvalho Chehab wrote: >> From: Jan Kara >> >> Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns() instead of >> hand made mapping of virtual address to physical address. Also the >> function leaked page reference from get_user_pages() so fix that by >> properly release the reference when omap_vout_buffer_release() is >> called. >> >> Signed-off-by: Jan Kara >> Signed-off-by: Hans Verkuil >> [hans.verkuil@cisco.com: remove unused struct omap_vout_device *vout >> variable] >> >> Signed-off-by: Mauro Carvalho Chehab >> >> diff --git a/drivers/media/platform/omap/omap_vout.c >> b/drivers/media/platform/omap/omap_vout.c index f09c5f17a42f..7feb6394f111 >> 100644 >> --- a/drivers/media/platform/omap/omap_vout.c >> +++ b/drivers/media/platform/omap/omap_vout.c >> @@ -195,46 +195,34 @@ static int omap_vout_try_format(struct v4l2_pix_format >> *pix) } >> >> /* >> - * omap_vout_uservirt_to_phys: This inline function is used to convert user >> - * space virtual address to physical address. >> + * omap_vout_get_userptr: Convert user space virtual address to physical >> + * address. >> */ >> -static unsigned long omap_vout_uservirt_to_phys(unsigned long virtp) >> +static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp, >> + u32 *physp) >> { >> - unsigned long physp = 0; >> - struct vm_area_struct *vma; >> - struct mm_struct *mm = current->mm; >> + struct frame_vector *vec; >> + int ret; >> >> /* For kernel direct-mapped memory, take the easy way */ >> - if (virtp >= PAGE_OFFSET) >> - return virt_to_phys((void *) virtp); >> - >> - down_read(¤t->mm->mmap_sem); >> - vma = find_vma(mm, virtp); >> - if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) { >> - /* this will catch, kernel-allocated, mmaped-to-usermode >> - addresses */ >> - physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start); >> - up_read(¤t->mm->mmap_sem); >> - } else { >> - /* otherwise, use get_user_pages() for general userland pages */ >> - int res, nr_pages = 1; >> - struct page *pages; >> + if (virtp >= PAGE_OFFSET) { >> + *physp = virt_to_phys((void *)virtp); > > Lovely. virtp comes from userspace and as far as I know it arrives here > completely unchecked. The problem isn't introduced by this patch, but > omap_vout buffer management seems completely broken to me, and nobody seems to > care about the driver. Given that omapdrm should now provide the video output > capabilities that are missing from omapfb and resulted in the development of > omap_vout, shouldn't we drop the omap_vout driver ? In addition it uses the old videobuf framework which at some point will be reason for removal if nobody cares enough to convert to videobuf2. So I would also be in favor of removal if there are no objections (well, probably first to staging for one or two kernel cycles followed by removal). Hans > > Tomi, any opinion on this ? Do you see any omap_vout capability missing from > omapdrm ? > >> + return 0; >> + } >> >> - res = get_user_pages(current, current->mm, virtp, nr_pages, 1, >> - 0, &pages, NULL); >> - up_read(¤t->mm->mmap_sem); >> + vec = frame_vector_create(1); >> + if (!vec) >> + return -ENOMEM; >> >> - if (res == nr_pages) { >> - physp = __pa(page_address(&pages[0]) + >> - (virtp & ~PAGE_MASK)); >> - } else { >> - printk(KERN_WARNING VOUT_NAME >> - "get_user_pages failed\n"); >> - return 0; >> - } >> + ret = get_vaddr_frames(virtp, 1, true, false, vec); >> + if (ret != 1) { >> + frame_vector_destroy(vec); >> + return -EINVAL; >> } >> + *physp = __pfn_to_phys(frame_vector_pfns(vec)[0]); >> + vb->priv = vec; >> >> - return physp; >> + return 0; >> } >> >> /* >> @@ -784,11 +772,15 @@ static int omap_vout_buffer_prepare(struct >> videobuf_queue *q, * address of the buffer >> */ >> if (V4L2_MEMORY_USERPTR == vb->memory) { >> + int ret; >> + >> if (0 == vb->baddr) >> return -EINVAL; >> /* Physical address */ >> - vout->queued_buf_addr[vb->i] = (u8 *) >> - omap_vout_uservirt_to_phys(vb->baddr); >> + ret = omap_vout_get_userptr(vb, vb->baddr, >> + (u32 *)&vout->queued_buf_addr[vb->i]); >> + if (ret < 0) >> + return ret; >> } else { >> unsigned long addr, dma_addr; >> unsigned long size; >> @@ -834,12 +826,13 @@ static void omap_vout_buffer_queue(struct >> videobuf_queue *q, static void omap_vout_buffer_release(struct >> videobuf_queue *q, >> struct videobuf_buffer *vb) >> { >> - struct omap_vout_device *vout = q->priv_data; >> - >> vb->state = VIDEOBUF_NEEDS_INIT; >> + if (vb->memory == V4L2_MEMORY_USERPTR && vb->priv) { >> + struct frame_vector *vec = vb->priv; >> >> - if (V4L2_MEMORY_MMAP != vout->memory) >> - return; >> + put_vaddr_frames(vec); >> + frame_vector_destroy(vec); >> + } >> } >> >> /* >