Hello,
On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
> Hi Marek,
>
> 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 ;)
<snip>
> > > > +/**
> > > > + * 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.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
