On Sat, 2011-02-05 at 22:53 -0800, Paul Cassella wrote:
> From: Paul Cassella <[email protected]>
>
> get_user_pages() may return -errno, such as -EFAULT. So don't blindly use
> its return value as an offset into dma->map[] for the next get_user_pages()
> call. Since we'll give up and return an error if either fails, don't even
> make the second call if the first failed to give us exactly what we were
> looking for.
>
> The old code would also call put_page() on as many elements of dma->map[]
> as we'd asked for, regardless of how many were valid.
>
> Signed-off-by: Paul Cassella <[email protected]>
>
> ---
> I'm running with this on 2.6.37, though haven't triggered either
> condition. This patch is against staging/for_v2.6.39, which compiles
> cleanly with it.
I'm still reviewing this one, but I found one problem. See below.
> I'm not sure -EINVAL is the best return code vs -EFAULT or -ENOMEM, but
> this mod doesn't change it except when one of the get_user_pages() calls
> actually returned a -errno.
>
> drivers/media/video/ivtv/ivtv-yuv.c | 53
> +++++++++++++++++++++++++++--------
> 1 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/video/ivtv/ivtv-yuv.c
> b/drivers/media/video/ivtv/ivtv-yuv.c
> index c087537..521a235 100644
> --- a/drivers/media/video/ivtv/ivtv-yuv.c
> +++ b/drivers/media/video/ivtv/ivtv-yuv.c
> @@ -77,23 +77,52 @@ static int ivtv_yuv_prep_user_dma(struct ivtv *itv,
> struct ivtv_user_dma *dma,
> /* Get user pages for DMA Xfer */
> down_read(¤t->mm->mmap_sem);
> y_pages = get_user_pages(current, current->mm, y_dma.uaddr,
> y_dma.page_count, 0, 1, &dma->map[0], NULL);
> - uv_pages = get_user_pages(current, current->mm, uv_dma.uaddr,
> uv_dma.page_count, 0, 1, &dma->map[y_pages], NULL);
> + uv_pages = 0; /* silence gcc. value is set and consumed only if: */
> + if (y_pages == y_dma.page_count) {
> + uv_pages = get_user_pages(current, current->mm,
> + uv_dma.uaddr, uv_dma.page_count, 0, 1,
> + &dma->map[y_pages], NULL);
> + }
> up_read(¤t->mm->mmap_sem);
>
> - dma->page_count = y_dma.page_count + uv_dma.page_count;
> -
> - if (y_pages + uv_pages != dma->page_count) {
> - IVTV_DEBUG_WARN
> - ("failed to map user pages, returned %d instead of %d\n",
> - y_pages + uv_pages, dma->page_count);
> -
> - for (i = 0; i < dma->page_count; i++) {
> - put_page(dma->map[i]);
> + if (y_pages != y_dma.page_count || uv_pages != uv_dma.page_count) {
> + int rc = -EINVAL;
rc has scope of only this 'if' block, but ...
> +
> + if (y_pages == y_dma.page_count) {
> + IVTV_DEBUG_WARN
> + ("failed to map uv user pages, returned %d "
> + "expecting %d\n", uv_pages, uv_dma.page_count);
> +
> + if (uv_pages >= 0) {
> + for (i = 0; i < uv_pages; i++) {
> + put_page(dma->map[y_pages + i]);
> + }
> + rc = -EINVAL;
> + } else {
> + rc = uv_pages;
> + }
> + } else {
> + IVTV_DEBUG_WARN
> + ("failed to map y user pages, returned %d "
> + "expecting %d\n", y_pages, y_dma.page_count);
> + }
> + if (y_pages >= 0) {
> + for (i = 0; i < y_pages; i++) {
> + put_page(dma->map[i]);
> + }
> + /*
> + * Inherit the -EINVAL from rc's
> + * initialization, but allow it to be
> + * overriden by uv_pages above if it was an
> + * actual errno.
> + */
> + } else {
> + rc = y_pages;
> }
> - dma->page_count = 0;
> - return -EINVAL;
> }
rc doesn't appear to be return'ed.
Regards,
Andy
> + dma->page_count = y_pages + uv_pages;
> +
> /* Fill & map SG List */
> if (ivtv_udma_fill_sg_list (dma, &uv_dma, ivtv_udma_fill_sg_list (dma,
> &y_dma, 0)) < 0) {
> IVTV_DEBUG_WARN("could not allocate bounce buffers for highmem
> userspace buffers\n");
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
