[PATCH] usbfront: do not assume sequentially mapped pages

[PATCH] usbfront: do not assume sequentially mapped pages

[Ian Jackson]

xenhcd_gnttab_map in usbfront-q.c looks up the mfn of the start of the
usb transfer buffer.  But the buffer may span several pages, and the
current code simply increments the obtained mfn.  Needless to say this
is an unwarranted assumption.  It causes large transfers to be
corrupted and/or to overwrite other parts of memory.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff -r 87c84f7dd850 drivers/xen/usbfront/usbfront-q.c
--- a/drivers/xen/usbfront/usbfront-q.c	Fri Mar 20 09:00:58 2009 +0000
+++ b/drivers/xen/usbfront/usbfront-q.c	Fri Mar 27 17:53:12 2009 +0100
@@ -106,12 +106,15 @@ static inline void xenhcd_gnttab_map(str
 	unsigned int bytes;
 	int i;
 
-	page = virt_to_page(addr);
-	buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
-	offset = offset_in_page(addr);
 	len = length;
 
 	for(i = 0;i < nr_pages;i++){
+		BUG_ON(!len);
+
+		page = virt_to_page(addr);
+		buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
+		offset = offset_in_page(addr);
+
 		bytes = PAGE_SIZE - offset;
 		if(bytes > len)
 			bytes = len;
@@ -123,9 +126,8 @@ static inline void xenhcd_gnttab_map(str
 		seg[i].offset = (uint16_t)offset;
 		seg[i].length = (uint16_t)bytes;
 
-		buffer_pfn++;
+		addr += bytes;
 		len -= bytes;
-		offset = 0;
 	}
 }

Re: [PATCH] usbfront: do not assume sequentially mapped pages

[Noboru Iwamatsu]

Hi,

Oh, I had a misunderstanding about the continuity of buffer pages.
This caused webcam's image to corrupt.

PVUSB applied to this change can work well with the following webcams.

* Creative Labs: WebCam 3 USB (with ov511 driver)
* Creative Labs: WebCam NX pro (with gspca driver)

Now, Webcam works!

Thanks,

Noboru

Ian Jackson wrote:
> xenhcd_gnttab_map in usbfront-q.c looks up the mfn of the start of the
> usb transfer buffer.  But the buffer may span several pages, and the
> current code simply increments the obtained mfn.  Needless to say this
> is an unwarranted assumption.  It causes large transfers to be
> corrupted and/or to overwrite other parts of memory.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff -r 87c84f7dd850 drivers/xen/usbfront/usbfront-q.c
> --- a/drivers/xen/usbfront/usbfront-q.c	Fri Mar 20 09:00:58 2009 +0000
> +++ b/drivers/xen/usbfront/usbfront-q.c	Fri Mar 27 17:53:12 2009 +0100
> @@ -106,12 +106,15 @@ static inline void xenhcd_gnttab_map(str
>  	unsigned int bytes;
>  	int i;
>  
> -	page = virt_to_page(addr);
> -	buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
> -	offset = offset_in_page(addr);
>  	len = length;
>  
>  	for(i = 0;i < nr_pages;i++){
> +		BUG_ON(!len);
> +
> +		page = virt_to_page(addr);
> +		buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
> +		offset = offset_in_page(addr);
> +
>  		bytes = PAGE_SIZE - offset;
>  		if(bytes > len)
>  			bytes = len;
> @@ -123,9 +126,8 @@ static inline void xenhcd_gnttab_map(str
>  		seg[i].offset = (uint16_t)offset;
>  		seg[i].length = (uint16_t)bytes;
>  
> -		buffer_pfn++;
> +		addr += bytes;
>  		len -= bytes;
> -		offset = 0;
>  	}
>  }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] usbfront: do not assume sequentially mapped pages

[Ian Jackson]

Noboru Iwamatsu writes ("Re: [Xen-devel] [PATCH] usbfront: do not assume sequentially mapped pages"):
> Oh, I had a misunderstanding about the continuity of buffer pages.
> This caused webcam's image to corrupt.
> 
> PVUSB applied to this change can work well with the following webcams.

That's great.  I've been trying it out with a Logitech Quickcam Sphere
AF (with the Linux uvc driver from v4l) and there still seem to be
problems, which look like they're related to the v4l driver's attempts
to do direct access to dma pages and/or mmap pages into client process
address space.

> * Creative Labs: WebCam 3 USB (with ov511 driver)
> * Creative Labs: WebCam NX pro (with gspca driver)

You might like to try a v4l uvc driver and see if it works for you.

Ian.

Re: [PATCH] usbfront: do not assume sequentially mapped pages

[Noboru Iwamatsu]

Ian,

 > You might like to try a v4l uvc driver and see if it works for you.

Sorry for the late reply.

I've been trying USB2.0 UVC webcam too, but I have not succeeded yet.
I dumped urbs and saw no errors in isoc-urbs.

My testing environment is the following,

UVC driver:
I checked out the recent driver from 
http://linuxtv.org/hg/~pinchartl/uvcvideo/

Webcam:
* Logitech QuickCam S5500 (046d:09a1)
* Logitech QuickCam E3500 (046d:09a4)

App:
* luvcview (ver. 20070512)

Results:
luvcview outputs "Ignoring empty buffer ..." and exits with errors.

Regards,

Noboru

Re: [PATCH] usbfront: do not assume sequentially mapped pages

[Ian Jackson]

Noboru Iwamatsu writes ("Re: [Xen-devel] [PATCH] usbfront: do not assume sequentially mapped pages"):
> Ian,
>  > You might like to try a v4l uvc driver and see if it works for you.
> 
> Sorry for the late reply.
> 
> I've been trying USB2.0 UVC webcam too, but I have not succeeded yet.
> I dumped urbs and saw no errors in isoc-urbs.

I didn't see errors.  I ran uvccapture (as in Debian lenny), which
seems to be the most simple test program available, and it segfaults
while accessing what is supposed to be a buffer full of video data
provided by mmapping the kernel's UVC driver.

Ian.

Re: [PATCH] usbfront: do not assume sequentially mapped pages

[Noboru Iwamatsu]

Ian,

I tried out uvccapture-0.5, and I'm certain that we have the
same problem.

When uvccapture had started memcpy-ing from the v4l2_buffer, the 
v4l2_buffer was not filled yet.
(in line 294 of v4l2uvc.c, vd->buf.bytesused is less than HEADERFRAME1)

I'm uncertain whether this problem results from the isoc-transfer timing 
or v4l relating.

Regards,

Noboru

Ian Jackson wrote:
> Noboru Iwamatsu writes ("Re: [Xen-devel] [PATCH] usbfront: do not assume sequentially mapped pages"):
>> Ian,
>>  > You might like to try a v4l uvc driver and see if it works for you.
>>
>> Sorry for the late reply.
>>
>> I've been trying USB2.0 UVC webcam too, but I have not succeeded yet.
>> I dumped urbs and saw no errors in isoc-urbs.
> 
> I didn't see errors.  I ran uvccapture (as in Debian lenny), which
> seems to be the most simple test program available, and it segfaults
> while accessing what is supposed to be a buffer full of video data
> provided by mmapping the kernel's UVC driver.
> 
> Ian.