On Sat, 27 Sep 2003 18:35:22 +0200
Oliver Neukum <[EMAIL PROTECTED]> wrote:

> Am Samstag, 27. September 2003 14:56 schrieb Luca Risolia:
> 
> Hi,
> 
> congratulations to the driver, it is looking good.
> However, I've found one somewhat obscure bug.
> This piece of code has a small race condition in the error case
> on SMP.
> If a submission fails after another URB has been transmitted
> successfully, such an URB may be just executing its completion
> handler. In this case usb_unlink_urb will fail and already freed
> memory may be accessed.
> 
> > +       /* Submit the URBs */
> > +       for (i = 0; i < W9968CF_URBS; i++) {
> > +               err = usb_submit_urb(cam->urb[i]);
> > +               if (err) {
> > +                       for (j = i-1; j >= 0; j--)
> > +                               usb_unlink_urb(cam->urb[j]);
> > +                       DBG(1, "Couldn't send a transfer request to the "
> > +                              "USB core (error #%d, %s).", err, 
> > +                           symbolic(urb_errlist, err))
> > +                       goto free_urbs;
> > +               }
> > +       }
> > +

Yes, it is a bug. I've found it in other drivers too, when stopping the
isoc transfer, although they work "almost" correctly when an urb is submitted
and the next submission fails: in this case urbs are never freed.
Given that that solution seems to be accepted, I can fix the bug the same 
way, by freeing the urbs "in the best case":

        /* Submit the URBs */
        for (i = 0; i < W9968CF_URBS; i++) {
                err = usb_submit_urb(cam->urb[i]);
                if (err) {
                        for (j = i-1; j >= 0; j--)
                                if (!usb_unlink_urb(cam->urb[j]))
                                        usb_free_urb(cam->urb[j])
                        DBG(1, "Couldn't send a transfer request to the "
                               "USB core (error #%d, %s).", err, 
                            symbolic(urb_errlist, err))
                        return err;
                }
        }
 
The same in stop_transfer():

        /* This avoids race conditions with usb_submit_urb() 
           in the URB completition handler */
        spin_lock_irqsave(&cam->urb_lock, lock_flags);
        cam->streaming = 0;
        spin_unlock_irqrestore(&cam->urb_lock, lock_flags);

        for (i = W9968CF_URBS-1; i >= 0; i--)
                if (cam->urb[i]) {
                        if (!usb_unlink_urb(cam->urb[i])) {
                                usb_free_urb(cam->urb[i]);
                                cam->urb[i] = NULL;
                        }
                }

Thanks for the help.

Luca


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to