On Tue, 8 May 2007, Oliver Neukum wrote:
> Am Montag, 7. Mai 2007 21:23 schrieb Alan Stern:
> > On Mon, 7 May 2007, Oliver Neukum wrote:
>
> > > +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.
Also, you don't want to contribute to the load average while you're
waiting.
> 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).
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?
> @@ -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...
You forgot to unanchor the URB if usb_submit_urb() failed.
> @@ -391,10 +409,31 @@ static void skel_disconnect(struct usb_i
> info("USB Skeleton #%d now disconnected", minor);
> }
>
> +static int skel_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct usb_skel *dev= usb_get_intfdata(intf);
> + int time;
> +
> + mutex_lock(&dev->io_mutex);
> + time = usb_wait_anchor_empty_timeout(&dev->submitted, 1 * HZ);
> + if (!time)
> + usb_kill_anchored_urbs(&dev->submitted);
> + mutex_unlock(&dev->io_mutex);
> +
> + return 0;
> +}
I don't think you should use dev->io_mutex here. It is intended for
synchronizing I/O with disconnect, so that the driver doesn't try to
access the device after it has been unbound. But an unbind can't happen
during a suspend method call, so you shouldn't need the mutex.
Of course, you face a starvation problem if the user keeps writing data as
fast as you keep killing URBs. That will be fixed once the autosuspend
stuff is added; the usb_autopm_get_interface() call will block until
skel_suspend() has returned.
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.
Alan Stern
-------------------------------------------------------------------------
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/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
