Am Dienstag, 8. Mai 2007 20:33 schrieb Alan Stern:
> > > > +int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, int
> > > > timeout)
> > > > +{
> > > > + int res;
> > > > +
> > > > + res = wait_event_timeout(anchor->wait, list_empty(&anchor->urb_list),
> > > > timeout);
> > >
> > > Maybe make this an interruptible wait?
> >
> > For use in disconnect(), suspend() and pre_reset()? Theses can't handle
> > signals.
>
> What do you mean? They don't have to handle signals. They just have to
> cut the timeout short when a signal arrives and then kill the outstanding
> URBs.
Who would send a signal? If this is called with pending URBs, aren't
tasks frozen?
> Also, you don't want to contribute to the load average while you're
> waiting.
Why? This is a system function.
> > Ok. I've followed all other your suggestions.
>
> The usbcore part looks good (except that your kerneldoc needs to be
> updated and you should try to observe the 80-column limit).
OK.
> The usb-skeleton part is a little more questionable.
>
> > --- a/drivers/usb/usb-skeleton.c 2007-05-04 13:07:30.000000000 +0200
> > +++ b/drivers/usb/usb-skeleton.c 2007-05-08 13:41:34.000000000 +0200
> > @@ -54,10 +54,13 @@ struct usb_skel {
> > struct usb_device *udev; /* the usb device for
> > this device */
> > struct usb_interface *interface; /* the interface for
> > this device */
> > struct semaphore limit_sem; /* limiting the
> > number of writes in progress */
> > + struct usb_anchor submitted; /* in case we need to
> > retract our submissions */
> > unsigned char *bulk_in_buffer; /* the buffer to
> > receive data */
> > size_t bulk_in_size; /* the size of the
> > receive buffer */
> > __u8 bulk_in_endpointAddr; /* the address of the
> > bulk in endpoint */
> > __u8 bulk_out_endpointAddr; /* the address of the
> > bulk out endpoint */
> > + int errors; /* the last request
> > tanked */
> > + spinlock_t err_lock; /* lock for errors */
>
> Here we go, adding yet another lock. Isn't it starting to become excessive?
Why? One normal lock and a spin lock.
> > @@ -213,6 +219,15 @@ static ssize_t skel_write(struct file *f
> > goto exit;
> > }
> >
> > + spin_lock_irq(&dev->err_lock);
> > + if (dev->errors < 0) {
> > + dev->errors = 0;
> > + retval = -EIO;
> > + }
> > + spin_unlock_irq(&dev->err_lock);
> > + if (retval < 0)
> > + goto error;
>
> Is this really correct? You might end up reporting one user's errors
> to a different user. But maybe that's the standard approach; I don't
> know...
We are mixing writes anyway.
> You forgot to unanchor the URB if usb_submit_urb() failed.
OK.
> For now, don't bother with io_mutex. After all, the code above won't
> prevent more URBs from being submitted after skel_suspend() returns.
OK.
Regards
Oliver
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
