On Fri, 22 Jun 2007 10:52:18 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > + if (urb->status < 0)
> > + usblp->rstatus = urb->status;
> > + else
> > + usblp->rstatus = urb->actual_length;
> > usblp->rcomplete = 1;
> > -unplug:
> > - wake_up_interruptible(&usblp->wait);
> > + wake_up(&usblp->rwait);
>
> Why no longer _interruptible ?
Because I want to wake anything that gets into this wait list.
Currently, every waiter is interruptible, but I don't want to keep
track. It's the same thing as using spin_lock_irqsave().
> > + spin_lock(&usblp->lock);
> > + if (urb->status < 0)
> > + usblp->wstatus = urb->status;
> > + else
> > + usblp->wstatus = urb->actual_length;
>
> This raises the question why an error would mean that no payload
> has been transfered.
In the context of usblp it's pretty much the same thing. If the
stream is corrupt, you might as well abort the whole job: it's going
to be illegible. There's no reason to return the actual_length to
the application.
> > + /* XXX Use usb_setup_bulk_urb when available. Talk to Marcel. */
> > + kfree(urb->transfer_buffer);
> > + urb->transfer_buffer = NULL; /* Not refcounted, so to be safe... */
> > + usb_free_urb(urb);
>
> If you want to be safe, do it without a race.
What race?
> > static void usblp_unlink_urbs(struct usblp *usblp)
> > {
> > - usb_kill_urb(usblp->writeurb);
> > - if (usblp->bidir)
> > - usb_kill_urb(usblp->readurb);
> > + usb_kill_anchored_urbs(&usblp->urbs);
> > }
>
> What good does encapsulation here?
Believe it or not, this thought has crossed my mind.
I just wanted to preserve the area in case something else
would be needed. Observe that the usb-skeleton waits for the URBs
to complete. We do NOT do it on purpose, but this is unusual.
> > static unsigned int usblp_poll(struct file *file, struct poll_table_struct
> > *wait)
> > {
> > + int ret;
> > + unsigned long flags;
>
> We are called with interrupts on.
We discussed it a few times before. I guess my use of spin_lock in
callbacks wasn't enough to appease you, eh?
> > + spin_lock_irqsave(&usblp->lock, flags);
> > + ret = ((!usblp->bidir || !usblp->rcomplete) ? 0 : POLLIN | POLLRDNORM)
> > | (!usblp->wcomplete ? 0 : POLLOUT | POLLWRNORM);
> > + spin_unlock_irqrestore(&usblp->lock, flags);
> > + return ret;
> > }
>
> Shouldn't disconnect cause an error to be reported?
Good question. Old driver didn't do it. Since URBs fail on disconnect,
it caused a momentary spin of the application and a EIO from next write.
If you have good contacts with CUPS people or other application writers,
it may help to ask them if they want anything. It's easy to add.
N.B., it may be useless, depending who schedules earlier. If the writer
wakes up before the khubd does, everything would happen just as it
always did (e.g. a quick loop-around and a EIO).
> > + if (mutex_lock_interruptible(&usblp->wmut)) {
> > + rv = -EINTR;
>
> Why not -ERESTARTSYS ?
Hmm. Yes, the syscall is restartable at this point. I guess I inherited
it from the old driver, where the loop was unified, so it didn't try
to distinguish the cases when something was written or nothing was written,
and returned -EINTR always. On the other hand, what's the harm?
-- Pete
-------------------------------------------------------------------------
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
