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