Re: requirements for suspend()

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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel