Re: [linux-usb-devel] Re: [OOPS, usbcore, releaseintf] 2.6.0-test10-mm1

[Duncan Sands <baldrick@free.fr>]

On Monday 08 December 2003 17:31, Alan Stern wrote:
> On Mon, 8 Dec 2003, Duncan Sands wrote:
> > On Monday 08 December 2003 17:03, David Brownell wrote:
> > > Duncan Sands wrote:
> > > > Hi Vince, I'm not sure, but it looks like a bug in the USB core.
> > > > I was kind of expecting this :)  My patch causes devio.c to hold
> > > > a reference to the usb_device maybe long after the device has
> > > > been disconnected.  This is supposed to be OK, but from your
> > >
> > > ... no, that's not supposed to be OK.  Returning from disconnect()
> > > means that a device driver is no longer referencing the interface
> > > the driver bound to, or ep0.
> >
> > Well, I thought Greg wanted it to be OK :)  Anyway, I don't use
> > the device after disconnect except to take the semaphore
> > (dev->serialize), check for disconnection (dev->state), and
> > of course to execute a usb_put_dev.  Surely this usage should
> > be OK?
>
> As long as your disconnect routine doesn't do usb_put_dev, so that it
> maintains its reference, I don't see a problem.  But why do you want to
> check dev->state later on?  Once your disconnect routine has returned, you
> should be totally through with the device.  You should no longer care
> whether it's attached or not.

Hi Alan, this is for usbfs, not a normal driver.  Recall that I want to replace
use of ps->devsem with ps->dev->serialize.  Currently ps->dev is set to NULL in
the devio.c usbfs disconnect method (if some interface is claimed) or in
inode.c on device disconnect, making it hard to lock with ps->dev->serialize :)
Thus disconnect should no longer be signalled by setting ps->dev to NULL.
For the same reason ps->dev should not be freed on disconnect.  It follows
that I should hold a reference to ps->dev until ps goes down.  And this is
what I do.  By the way, rather than introducing a new flag to indicate
disconnection, ps->dev->state will do.

> And of course, remember that there are valid reasons for your disconnect
> routine to be called even when the device remains attached.  (rmmod is a
> good example.)

Sure.

All the best,

Duncan.

PS: Here is the patch that fixed the original usbfs Oops, but gained the new
one Vince reported:

diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c	Sun Dec  7 01:20:31 2003
+++ b/drivers/usb/core/devio.c	Sun Dec  7 01:20:31 2003
@@ -87,17 +87,15 @@
 static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 {
 	struct dev_state *ps = (struct dev_state *)file->private_data;
+	struct usb_device *dev = ps->dev;
 	ssize_t ret = 0;
 	unsigned len;
 	loff_t pos;
 	int i;
 
 	pos = *ppos;
-	down_read(&ps->devsem);
-	if (!ps->dev) {
-		ret = -ENODEV;
-		goto err;
-	} else if (pos < 0) {
+	down(&dev->serialize);
+	if (pos < 0) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -106,7 +104,7 @@
 		len = sizeof(struct usb_device_descriptor) - pos;
 		if (len > nbytes)
 			len = nbytes;
-		if (copy_to_user(buf, ((char *)&ps->dev->descriptor) + pos, len)) {
+		if (copy_to_user(buf, ((char *)&dev->descriptor) + pos, len)) {
 			ret = -EFAULT;
 			goto err;
 		}
@@ -118,9 +116,9 @@
 	}
 
 	pos = sizeof(struct usb_device_descriptor);
-	for (i = 0; nbytes && i < ps->dev->descriptor.bNumConfigurations; i++) {
+	for (i = 0; nbytes && i < dev->descriptor.bNumConfigurations; i++) {
 		struct usb_config_descriptor *config =
-			(struct usb_config_descriptor *)ps->dev->rawdescriptors[i];
+			(struct usb_config_descriptor *)dev->rawdescriptors[i];
 		unsigned int length = le16_to_cpu(config->wTotalLength);
 
 		if (*ppos < pos + length) {
@@ -129,7 +127,7 @@
 				len = nbytes;
 
 			if (copy_to_user(buf,
-			    ps->dev->rawdescriptors[i] + (*ppos - pos), len)) {
+			    dev->rawdescriptors[i] + (*ppos - pos), len)) {
 				ret = -EFAULT;
 				goto err;
 			}
@@ -144,7 +142,7 @@
 	}
 
 err:
-	up_read(&ps->devsem);
+	up(&dev->serialize);
 	return ret;
 }
 
@@ -324,22 +322,18 @@
 static void driver_disconnect(struct usb_interface *intf)
 {
 	struct dev_state *ps = usb_get_intfdata (intf);
+	unsigned int ifnum = intf->altsetting->desc.bInterfaceNumber;
 
 	if (!ps)
 		return;
 
-	/* this waits till synchronous requests complete */
-	down_write (&ps->devsem);
-
 	/* prevent new I/O requests */
-	ps->dev = 0;
-	ps->ifclaimed = 0;
 	usb_set_intfdata (intf, NULL);
+	if (ifnum < 8*sizeof(ps->ifclaimed))
+		clear_bit(ifnum, &ps->ifclaimed);
 
 	/* force async requests to complete */
-	destroy_all_async (ps);
-
-	up_write (&ps->devsem);
+	destroy_async_on_interface (ps, ifnum);
 }
 
 struct usb_driver usbdevfs_driver = {
@@ -355,21 +349,18 @@
 	struct usb_interface *iface;
 	int err;
 
-	if (intf >= 8*sizeof(ps->ifclaimed) || !dev
-			|| intf >= dev->actconfig->desc.bNumInterfaces)
+	if (intf >= 8*sizeof(ps->ifclaimed) || intf >= dev->actconfig->desc.bNumInterfaces)
 		return -EINVAL;
-	/* already claimed */
-	if (test_bit(intf, &ps->ifclaimed))
-		return 0;
-	iface = dev->actconfig->interface[intf];
+	if (test_and_set_bit(intf, &ps->ifclaimed))
+		return 0; /* already claimed */
 	err = -EBUSY;
-	lock_kernel();
+	iface = dev->actconfig->interface[intf]; /* change to usb_ifnum_to_if here and elsewhere? */
 	if (!usb_interface_claimed(iface)) {
 		usb_driver_claim_interface(&usbdevfs_driver, iface, ps);
-		set_bit(intf, &ps->ifclaimed);
 		err = 0;
 	}
-	unlock_kernel();
+	if (err)
+		clear_bit(intf, &ps->ifclaimed);
 	return err;
 }
 
@@ -383,13 +374,13 @@
 		return -EINVAL;
 	err = -EINVAL;
 	dev = ps->dev;
-	down(&dev->serialize);
+
+	/* the configuration cannot change under us if we have a claimed interface */
 	if (test_and_clear_bit(intf, &ps->ifclaimed)) {
 		iface = dev->actconfig->interface[intf];
-		usb_driver_release_interface(&usbdevfs_driver, iface);
+		usb_driver_release_interface(&usbdevfs_driver, iface); /* may sleep - beware for BKL! */
 		err = 0;
 	}
-	up(&dev->serialize);
 	return err;
 }
 
@@ -492,7 +483,7 @@
 
 	lock_kernel();
 	ret = -ENOENT;
-	dev = inode->u.generic_ip;
+	dev = usb_get_dev (inode->u.generic_ip);
 	if (!dev) {
 		kfree(ps);
 		goto out;
@@ -504,7 +495,6 @@
 	INIT_LIST_HEAD(&ps->async_pending);
 	INIT_LIST_HEAD(&ps->async_completed);
 	init_waitqueue_head(&ps->wait);
-	init_rwsem(&ps->devsem);
 	ps->discsignr = 0;
 	ps->disctask = current;
 	ps->disccontext = NULL;
@@ -521,18 +511,21 @@
 static int usbdev_release(struct inode *inode, struct file *file)
 {
 	struct dev_state *ps = (struct dev_state *)file->private_data;
+	struct usb_device *dev = ps->dev;
 	unsigned int i;
 
-	lock_kernel();
+	/* the only race is with driver_disconnect */
+	down(&dev->serialize);
 	list_del_init(&ps->list);
 
-	if (ps->dev) {
+	if (dev->state != USB_STATE_NOTATTACHED)
 		for (i = 0; ps->ifclaimed && i < 8*sizeof(ps->ifclaimed); i++)
 			if (test_bit(i, &ps->ifclaimed))
-				releaseintf(ps, i);
-	}
-	unlock_kernel();
+				releaseintf(ps, i); /* may sleep - makes the BKL problematic */
 	destroy_all_async(ps);
+	up(&dev->serialize);
+	usb_put_dev (ps->dev);
+	ps->dev = NULL;
 	kfree(ps);
         return 0;
 }
@@ -712,22 +705,32 @@
 
 static int proc_resetdevice(struct dev_state *ps)
 {
+	struct usb_device *dev = ps->dev;
 	int i, ret;
 
-	ret = usb_reset_device(ps->dev);
+	up(&dev->serialize);
+	ret = usb_reset_device(dev);
+	down(&dev->serialize);
+	if (!ret && dev->state == USB_STATE_NOTATTACHED)
+		ret = -ENODEV;
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < ps->dev->actconfig->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = ps->dev->actconfig->interface[i];
+	for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf = dev->actconfig->interface[i];
 
 		/* Don't simulate interfaces we've claimed */
 		if (test_bit(i, &ps->ifclaimed))
 			continue;
 
 		err ("%s - this function is broken", __FUNCTION__);
-		if (intf->driver && ps->dev) {
+		if (intf->driver) {
+			up(&dev->serialize);
 			usb_probe_interface (&intf->dev);
+			down(&dev->serialize);
+		}
+		if (dev->state == USB_STATE_NOTATTACHED) {
+			return -ENODEV;
 		}
 	}
 
@@ -976,18 +979,19 @@
         DECLARE_WAITQUEUE(wait, current);
 	struct async *as = NULL;
 	void __user *addr;
+	struct usb_device *dev = ps->dev;
 	int ret;
 
 	add_wait_queue(&ps->wait, &wait);
-	while (ps->dev) {
+	while (dev->state != USB_STATE_NOTATTACHED) {
 		__set_current_state(TASK_INTERRUPTIBLE);
 		if ((as = async_getcompleted(ps)))
 			break;
 		if (signal_pending(current))
 			break;
-		up_read(&ps->devsem);
+		up(&dev->serialize);
 		schedule();
-		down_read(&ps->devsem);
+		down(&dev->serialize);
 	}
 	remove_wait_queue(&ps->wait, &wait);
 	set_current_state(TASK_RUNNING);
@@ -1089,57 +1093,53 @@
 		}
 	}
 
-       if (!ps->dev)
-               retval = -ENODEV;
-       else if (!(ifp = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
+       if (!(ifp = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
                retval = -EINVAL;
-       else switch (ctrl.ioctl_code) {
-
-       /* disconnect kernel driver from interface, leaving it unbound.  */
-       /* maybe unbound - you get no guarantee it stays unbound */
-       case USBDEVFS_DISCONNECT:
-		/* this function is misdesigned - retained for compatibility */
+	else {
 		lock_kernel();
-		driver = ifp->driver;
-		if (driver) {
-			dbg ("disconnect '%s' from dev %d interface %d",
-			     driver->name, ps->dev->devnum, ctrl.ifno);
-			usb_unbind_interface(&ifp->dev);
-		} else
-			retval = -ENODATA;
-		unlock_kernel();
-		break;
+		up(&ps->dev->serialize);
+		switch (ctrl.ioctl_code) {
 
-	/* let kernel drivers try to (re)bind to the interface */
-	case USBDEVFS_CONNECT:
-		lock_kernel();
-		retval = usb_probe_interface (&ifp->dev);
-		unlock_kernel();
-		break;
+		/* disconnect kernel driver from interface, leaving it unbound.  */
+		/* maybe unbound - you get no guarantee it stays unbound */
+		case USBDEVFS_DISCONNECT:
+			/* this function is misdesigned - retained for compatibility */
+			driver = ifp->driver;
+			if (driver) {
+				dbg ("disconnect '%s' from dev %d interface %d",
+					driver->name, ps->dev->devnum, ctrl.ifno);
+				usb_unbind_interface(&ifp->dev);
+			} else
+				retval = -ENODATA;
+			break;
 
-	/* talk directly to the interface's driver */
-	default:
-		/* BKL used here to protect against changing the binding
-		 * of this driver to this device, as well as unloading its
-		 * driver module.
-		 */
-		lock_kernel ();
-		driver = ifp->driver;
-		if (driver == 0 || driver->ioctl == 0) {
-			unlock_kernel();
-			retval = -ENOSYS;
-		} else {
-			if (!try_module_get (driver->owner)) {
-				unlock_kernel();
+		/* let kernel drivers try to (re)bind to the interface */
+		case USBDEVFS_CONNECT:
+			retval = usb_probe_interface (&ifp->dev);
+			break;
+
+		/* talk directly to the interface's driver */
+		default:
+			/* BKL used here to protect against changing the binding
+			* of this driver to this device, as well as unloading its
+			* driver module.
+			*/
+			driver = ifp->driver;
+			if (driver == 0 || driver->ioctl == 0) {
 				retval = -ENOSYS;
-				break;
+			} else {
+				if (!try_module_get (driver->owner)) {
+					retval = -ENOSYS;
+					break;
+				}
+				retval = driver->ioctl (ifp, ctrl.ioctl_code, buf);
+				if (retval == -ENOIOCTLCMD)
+					retval = -ENOTTY;
+				module_put (driver->owner);
 			}
-			unlock_kernel ();
-			retval = driver->ioctl (ifp, ctrl.ioctl_code, buf);
-			if (retval == -ENOIOCTLCMD)
-				retval = -ENOTTY;
-			module_put (driver->owner);
 		}
+		down(&ps->dev->serialize);
+		unlock_kernel();
 	}
 
 	/* cleanup and return */
@@ -1161,13 +1161,15 @@
 static int usbdev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct dev_state *ps = (struct dev_state *)file->private_data;
+	struct usb_device *dev = ps->dev;
 	int ret = -ENOTTY;
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EPERM;
-	down_read(&ps->devsem);
-	if (!ps->dev) {
-		up_read(&ps->devsem);
+
+	down(&dev->serialize);
+	if (dev->state == USB_STATE_NOTATTACHED) {
+		up(&dev->serialize);
 		return -ENODEV;
 	}
 	switch (cmd) {
@@ -1212,7 +1214,9 @@
 		break;
 
 	case USBDEVFS_SETCONFIGURATION:
+		up(&dev->serialize);
 		ret = proc_setconfig(ps, (void __user *)arg);
+		down(&dev->serialize);
 		break;
 
 	case USBDEVFS_SUBMITURB:
@@ -1233,6 +1237,10 @@
 		ret = proc_reapurbnonblock(ps, (void __user *)arg);
 		break;
 
+	case USBDEVFS_RELEASEINTERFACE:
+		ret = proc_releaseinterface(ps, (void __user *)arg);
+		break;
+
 	case USBDEVFS_DISCSIGNAL:
 		ret = proc_disconnectsignal(ps, (void __user *)arg);
 		break;
@@ -1241,15 +1249,12 @@
 		ret = proc_claiminterface(ps, (void __user *)arg);
 		break;
 
-	case USBDEVFS_RELEASEINTERFACE:
-		ret = proc_releaseinterface(ps, (void __user *)arg);
-		break;
-
 	case USBDEVFS_IOCTL:
 		ret = proc_ioctl(ps, (void __user *) arg);
-		break;
+	break;
 	}
-	up_read(&ps->devsem);
+	up(&dev->serialize);
+
 	if (ret >= 0)
 		inode->i_atime = CURRENT_TIME;
 	return ret;
@@ -1264,7 +1269,7 @@
 	poll_wait(file, &ps->wait, wait);
 	if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed))
 		mask |= POLLOUT | POLLWRNORM;
-	if (!ps->dev)
+	if (ps->dev->state == USB_STATE_NOTATTACHED)
 		mask |= POLLERR | POLLHUP;
 	return mask;
 }
diff -Nru a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
--- a/drivers/usb/core/inode.c	Sun Dec  7 01:20:31 2003
+++ b/drivers/usb/core/inode.c	Sun Dec  7 01:20:31 2003
@@ -717,9 +717,9 @@
 	while (!list_empty(&dev->filelist)) {
 		ds = list_entry(dev->filelist.next, struct dev_state, list);
 		list_del_init(&ds->list);
-		down_write(&ds->devsem);
-		ds->dev = NULL;
-		up_write(&ds->devsem);
+//		down_write(&ds->devsem);
+//		ds->dev = NULL;
+//		up_write(&ds->devsem);
 		if (ds->discsignr) {
 			sinfo.si_signo = SIGPIPE;
 			sinfo.si_errno = EPIPE;
diff -Nru a/include/linux/usbdevice_fs.h b/include/linux/usbdevice_fs.h
--- a/include/linux/usbdevice_fs.h	Sun Dec  7 01:20:31 2003
+++ b/include/linux/usbdevice_fs.h	Sun Dec  7 01:20:31 2003
@@ -154,7 +154,6 @@
 
 struct dev_state {
 	struct list_head list;      /* state list */
-	struct rw_semaphore devsem; /* protects modifications to dev (dev == NULL indicating disconnect) */ 
 	struct usb_device *dev;
 	struct file *file;
 	spinlock_t lock;            /* protects the async urb lists */