Hi Marek,
On Monday 29 November 2010 16:13:17 Marek Szyprowski wrote:
> On Monday, November 29, 2010 3:37 PM Laurent Pinchart wrote:
> > On Monday 29 November 2010 15:27:00 Marek Szyprowski wrote:
> > > On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
> > > > On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
> > > > > On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
> > > > > > On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
> > > > > > > From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > > > > > >
> > > > > > > Add generic memory handling routines for userspace pointer
> > > > > > > handling, contiguous memory verification and mapping.
> > > > > > >
> > > > > > > Signed-off-by: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > > > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > > > > > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > > > > > > CC: Pawel Osciak <pawel@xxxxxxxxxx>
> > > >
> > > > [snip]
> > > >
> > > > > > > + if (ret) {
> > > > > > > + printk(KERN_ERR "Invalid userspace address\n");
> > > > > > > + goto done;
> > > > > > > + }
> > > > > > > +
> > > > > > > + *paddr = (curr_pfn << PAGE_SHIFT) + offset;
> > > > > > > +
> > > > > > > + for (i = 1; i < num_pages; ++i) {
> > > > > > > + prev_pfn = curr_pfn;
> > > > > > > + vaddr += PAGE_SIZE;
> > > > > > > +
> > > > > > > + ret = follow_pfn(vma, vaddr, &curr_pfn);
> > > > > > > + if (ret || curr_pfn != prev_pfn + 1) {
> > > > > > > + printk(KERN_ERR "Invalid userspace address\n");
> > > > > > > + ret = -EINVAL;
> > > > > >
> > > > > > I would use -EFAULT when curr_pfn != prev_pfn + 1.
> > > > >
> > > > > I don't think that this situation desires for a EFAULT error code.
> > > > > EINVAL is imho enough to let application know that this memory
> > > > > cannot be used for the userptr buffer.
> > > >
> > > > -EINVAL is used quite a lot already to report many different error
> > > > conditions. -EFAULT would let application know that the issue is
> > > > about the pointer address as opposed to the buffer index for
> > > > instance.
> > >
> > > The application will not notice it probably in this case, because a
> > > default EFAULT handler will kill it in most of the cases ;)
> >
> > Will it ? Where ? ioctls can return -EFAULT.
>
> Ok. You are right. I mixed something. Please ignore my previous comment.
>
> > > > > > > +/**
> > > > > > > + * vb2_get_userptr() - acquire an area pointed to by userspace
> > > > > > > addres vaddr + * @vaddr: virtual userspace address to the given
> > > > > > > area + *
> > > > > > > + * This function attempts to acquire an area mapped in the
> > > > > > > userspace for + * the duration of a hardware operation.
> > > > > > > + *
> > > > > > > + * Returns a virtual memory region associated with the given
> > > > > > > vaddr on success + * or NULL.
> > > > > > > + */
> > > > > > > +struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
> > > > > > > +{
> > > > > > > + struct mm_struct *mm = current->mm;
> > > > > > > + struct vm_area_struct *vma;
> > > > > > > + struct vm_area_struct *vma_copy;
> > > > > > > +
> > > > > > > + vma_copy = kmalloc(sizeof(struct vm_area_struct),
> > > > > > > GFP_KERNEL); + if (vma_copy == NULL)
> > > > > > > + return NULL;
> > > > > > > +
> > > > > > > + down_read(&mm->mmap_sem);
> > > > > > > +
> > > > > > > + vma = find_vma(mm, vaddr);
> > > > > > > + if (!vma)
> > > > > > > + goto done;
> > > > > > > +
> > > > > > > + if (vma->vm_ops && vma->vm_ops->open)
> > > > > > > + vma->vm_ops->open(vma);
> > > > > > > +
> > > > > > > + if (vma->vm_file)
> > > > > > > + get_file(vma->vm_file);
> > > > > >
> > > > > > Could you explain what this is for ?
> > > > >
> > > > > This is called when you access physically contiguous memory from
> > > > > different device which has been earlier mmaped by the application.
> > > > > This way applications uses userptr mode with dmacontig. When doing
> > > > > this, you must ensure somehow that this memory won't be
> > > > > released/freed before you finish. This is achieved by calling
> > > > > open() callback and increasing the use count of the mmaped file.
> > > > > Same things happen when your application calls fork() and a new
> > > > > process gets access to your mmaped device.
> > > >
> > > > Good point.
> > > >
> > > > Don't you also need to pin the pages to memory with get_user_pages()
> > > > ?
> > >
> > > Nope. get_user_pages() doesn't support pfn-type memory at all (it just
> > > fails this that case). PFN-type memory cannot be swapped out anyway so
> > > it not a problem here.
> >
> > But vb2_get_userptr() can be used on non-PFN memory, can't it ?
>
> I thought that this function is used only by dma-coherent and cma
> allocators. dma-sg and iommu allocators will definitely call
> get_user_pages().
Then it should probably be renamed to vb2_get_pfn_user_pages().
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
