[New Driver]: usbvideo2 webcam core + pac207 driver using it.

[Pete Zaitcev]

On Fri, 28 Mar 2008 22:53:39 +0100, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:

> 	u8 users;

Make it int, too easy to overflow.

> static u32
> usbvideo2_request_buffers(struct usbvideo2_device* cam, u32 count)
> {
> 		if ((buff = vmalloc_32_user(cam->nbuffers *
> 						PAGE_ALIGN(cam->imagesize))))

I think that placing buffers into the lower 4GB is a senseless
limitation to build into a library. Although today the only system
which can run DMA between memory above 4GB and USB is Opteron,
with XHCI it's going to change.

> 					spin_unlock(&cam->queue_lock);
> 					wake_up_interruptible(&cam->wait_frame);

Why only interruptible, and why outside of the lock? Bizarre.

> 	urb->dev = cam->usbdev;
> 	ret = usb_submit_urb(urb, GFP_ATOMIC);

If you don't plan on using this code on kernel 2.4, don't restore
urb->dev. This was unnecessary for years. Just set it where you
fill the URB.

> 			urb->transfer_buffer = usb_buffer_alloc(udev,
> 							 (psz + 1) *
> 							 USBVIDEO2_ISO_PACKETS,
> 							 GFP_KERNEL,
> 							 &urb->transfer_dma);

Why are you using usb_buffer_alloc here? Why not use kmalloc?

Secondly, let's suppose you're allocating for 512-byte packets
(actually 513 for some reason). You're above 8KB by a few bytes,
thus making this an order 2 allocation. I am quite certain this
is going to fail on loaded systems.

If you ever run across a device with a bigger maximum packet size
(e.g. a WUSB webcam) this is just going to crash and burn.

So, why do you need to stuff 16 ISO blocks into one URB? Hardware
limitation?

Any why is this affinity to device's declared packet size? The HC
transparently merges transfers for you. So, the only thing you
need to think about is if your buffers are an integral number
of packets.

What is this +1 business, anyway?

> 	usbvideo2_release_buffers(cam);
> 	if (b->count)
> 		b->count = usbvideo2_request_buffers(cam, b->count);

OK, I'm going to skip this. V4L people know that area.

Same for mmap. I only made sure you aren't trying to run DMA
directly off a vmalloc area.

The API between usbvideo2 and camera modules seems ok, nothing
to comment. It'll become clearer if it was done right when you
add more camera modules.

-- Pete