Am Freitag, 22. Juni 2007 schrieb Pete Zaitcev: > 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().
I see. > > > + 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. Are you sure that no printing app will merge print jobs? > > > + /* 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? CPU A CPU B kfree(urb->transfer_buffer); if (urb->transfer_buffer) something(); urb->transfer_buffer = NULL; If you want to do this I'd suggest: t = urb->transfer_buffer; urb->transfer_buffer = NULL; kfree(t); Generally, I would oppose such nullings. There's no way to do this cleanly. > > > 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. OK. > > > 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? No, because here it bothers me for another reason. It is just a few cycles. But it makes the code harder to maintain. The next one to touch this will look at it, scratch his head and use GFP_ATOMIC just in case or do some funny stuff just in case somebody might call this function with a spinlock held. Needlessly coding for interrupts off is a bad thing. You should make it very clear whether something can be called with interrupts off or not. Agnosticism is a vice. On a related note I find kerneldoc deficient in that regard. Every more or less obvious parameter is documented but there's no tag to describe which locks must, may or may not be held. > > > + 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. Which is obviously legitimate. > 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. I'll ask. > 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? No harm. I just wondered whether there's a reason which would have meant auditing other drivers for it. 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/ _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel