Re: usblp: add dynamic URBs, fix races

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