Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: ivtv: devel

[PATCH] ivtv-yuv.c: handle get_user_pages() -errno returns

 

 

ivtv devel RSS feed   Index | Next | Previous | View Threaded


pwc at bigw

Feb 5, 2011, 10:53 PM

Post #1 of 3 (991 views)
Permalink
[PATCH] ivtv-yuv.c: handle get_user_pages() -errno returns

From: Paul Cassella <fortytwo-ivtv [at] manetheren>

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 <fortytwo-ivtv [at] manetheren>

---
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.

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(&current->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(&current->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);
+ }
+ 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;
}

+ 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");
--
1.7.2.3


_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel


awalls at md

Feb 8, 2011, 4:54 PM

Post #2 of 3 (900 views)
Permalink
Re: [PATCH] ivtv-yuv.c: handle get_user_pages() -errno returns [In reply to]

On Sat, 2011-02-05 at 22:53 -0800, Paul Cassella wrote:
> From: Paul Cassella <fortytwo-ivtv [at] manetheren>
>
> 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 <fortytwo-ivtv [at] manetheren>
>
> ---
> 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(&current->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(&current->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
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel


pwc at bigw

Feb 8, 2011, 9:51 PM

Post #3 of 3 (899 views)
Permalink
Re: [PATCH] ivtv-yuv.c: handle get_user_pages() -errno returns [In reply to]

On Tue, 8 Feb 2011, Andy Walls wrote:

> I'm still reviewing this one, but I found one problem. See below.

> > + 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 ...

> > - dma->page_count = 0;
> > - return -EINVAL;
> > }
>
> rc doesn't appear to be return'ed.

Oops. I'd gone back and forth between setting rc and returning the error
code in-place. I went only halfway forth the last time.


I'll regenerate the patch with the return, and the two added for loops
de-braced.


--
Paul Cassella

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

ivtv devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.