On Tue, 2011-02-08 at 23:44 -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 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. > > Thanks for the review!
OK. This one's queued up too: http://git.linuxtv.org/awalls/media_tree.git?a=commitdiff;h=23c4e3bb2a53a9a75cbbeb564a791d80c722b8db I changed the return code to -EFAULT as this return code bubbles up as a return code for write(). The V4L2 API spec indicates that -EINVAL was, as you observed, just wrong for this situation: http://linuxtv.org/downloads/v4l-dvb-apis/func-write.html Regards, Andy > drivers/media/video/ivtv/ivtv-yuv.c | 52 > +++++++++++++++++++++++++++-------- > 1 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/video/ivtv/ivtv-yuv.c > b/drivers/media/video/ivtv/ivtv-yuv.c > index c087537..e64b3db 100644 > --- a/drivers/media/video/ivtv/ivtv-yuv.c > +++ b/drivers/media/video/ivtv/ivtv-yuv.c > @@ -77,23 +77,51 @@ 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; > + > + 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); > } > - dma->page_count = 0; > - return -EINVAL; > + 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; > + } > + return rc; > } > > + 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
