> + 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 ?
> + 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.
> + /* 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.
> 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?
>
> static int usblp_release(struct inode *inode, struct file *file)
> @@ -455,10 +475,18 @@ static int usblp_release(struct inode *i
> /* No kernel lock - fine */
> static unsigned int usblp_poll(struct file *file, struct poll_table_struct
> *wait)
> {
> + int ret;
> + unsigned long flags;
We are called with interrupts on.
> +
> struct usblp *usblp = file->private_data;
> - poll_wait(file, &usblp->wait, wait);
> - return ((!usblp->bidir || !usblp->rcomplete) ? 0 : POLLIN | POLLRDNORM)
> + /* Should we check file->f_mode & FMODE_WRITE before poll_wait()? */
> + poll_wait(file, &usblp->rwait, wait);
> + poll_wait(file, &usblp->wwait, wait);
> + 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?
> @@ -656,168 +685,303 @@ done:
> static ssize_t usblp_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> struct usblp *usblp = file->private_data;
> - int timeout, intr, rv, err = 0, transfer_length = 0;
> - size_t writecount = 0;
> + char *writebuf;
> + struct urb *writeurb;
> + int rv;
> + int transfer_length;
> + ssize_t writecount = 0;
> +
> + if (mutex_lock_interruptible(&usblp->wmut)) {
> + rv = -EINTR;
Why not -ERESTARTSYS ?
> +static int usblp_rtest(struct usblp *usblp, int nonblock)
> +{
> + unsigned long flags;
We know interrupts are enabled.
Otherwise it looks very good. Thanks for taking up this old code.
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
