Re: requirements for suspend()

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