Am Donnerstag, 19. Dezember 2002 22:04 schrieb [EMAIL PROTECTED]:
> Oliver, could you take a couple of minutes and review the usbvision NT1003
> code. I would love to have a list
> of bad code. I'm not the most experienced person with the Linux USB API.
>
> heheh.... You look like the perfect person to point out some faults in the
> code.
Well, if you ask for it.
static void *usbvideo_rvmalloc(unsigned long size)
- use the usbvideo library code
if (waitqueue_active(&frame->wq)) {
wake_up_interruptible(&frame->wq);
- always do wake_up unconditionally
static int usbvision_read_reg(struct usb_usbvision *usbvision, unsigned char reg)
static int usbvision_write_reg(struct usb_usbvision *usbvision, unsigned char reg
- they race with disconnect. use a semaphore. and check for disconnection under it
for (retries = 5;;) {
rc = usb_control_msg(usbvision->dev,
usb_sndctrlpipe(usbvision->dev, 1),
USBVISION_OP_CODE,
- races with disconnect. see above
static int usbvision_set_compress_params(struct usb_usbvision *usbvision)
- same race
urb = usb_alloc_urb(FRAMES_PER_DESC);
if (urb == NULL) {
printk(KERN_ERR
"usbvision_init_isoc: usb_init_isoc() failed.\n");
return -ENOMEM;
}
- memory leak in error case. you have to free the already allocated urbs
/* Submit all URBs */
for (i = 0; i < USBVISION_NUMSBUF; i++) {
err = usb_submit_urb(usbvision->sbuf[i].urb);
if (err)
printk(KERN_ERR
"usbvision_init_isoc: usb_run_isoc(%d) ret %d\n",
i, err);
}
- won't do. if one fails, you have to unlink the others and return an error
/* Set packet size to 0 */
j = usb_set_interface(usbvision->dev, usbvision->iface,
usbvision->ifaceAltInactive);
- races with disconnect. see above.
if ((usbvision->fbuf == NULL) || (usbvision->scratch == NULL)) {
printk(KERN_ERR "usbvision_open: unable to allocate memory for
frame buffer and scratch\n");
err = -ENOMEM;
- memory leak in error case
if (usbvision->sbuf[i].data == NULL) {
err = -ENOMEM;
break;
}
- memory leak in error case
case VIDIOCGCHAN:
{
- not threadsafe. it needs a semaphore
case VIDIOCSPICT:
case VIDIOCMCAPTURE:
- see above
current->state = TASK_INTERRUPTIBLE;
- use set_current_state(). Note: if you have pending signals, your state
already is TASK_RUNNING
interruptible_sleep_on((void *) &frame->wq);
- never use this. always schedule
/* Code below may sleep, need to lock module while we are here */
MOD_INC_USE_COUNT;
- No longer true in 2.5
> I would like to fix the code that's broken or outdated. I would post to
> the dev list, but I would be killed before Christmas comes.
>
> Yes, I remember the last web cam driver discussion. The poor guy was
> pounded dead...
Your driver is not so bad. It's impossible to understand it fully in so little
time, but it has no larger problems.
You should think about converting it to V4L2 and you need to change
to the new urb completion signature.
Then you should submit it for inclusion into the kernel.
Regards
Oliver
-------------------------------------------------------
This SF.NET email is sponsored by: Geek Gift Procrastinating?
Get the perfect geek gift now! Before the Holidays pass you by.
T H I N K G E E K . C O M http://www.thinkgeek.com/sf/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
