On Tue, 30 Nov 2004, Sergey Vlasov wrote:
Meelis Roos wrote: Well, it takes the ps->dev->exclusive_access also to see that the ioctl does not exist. Someone who checks ioctls of different kernels to see which succeeds might get into a livelock with this.
I'm not sure if this is really important.
IMHO it is. If it doesn't break anything right now, it certainly may cause troubles in the future. For example a multithreaded application which at first make a ioctl which uses exclusive_access and second tread will do some polling without exclusive_access. But in case the second thread try first a non existing ioctl, it will get into lock and cant send signal to first tread to interrupt his ioctl...
It is not a good example, but I hope you understand what i mean.
And avoiding the lock for an unknown ioctl will lead to lots of duplicated code (locking will be
pushed to the lower level and duplicated for all switch branches) -
We can use a macro, if it is really necessary
will this really be better?
IMHO it is correct to return -ENOIOCTLCMD right away, not waiting for freeing a mutex.
I also attached a patch against 2.4.28, which should guarantee correct behavior. I tested it with modem_run and it worked. It seems that this patch also magically fixed a strange modem_run bug, which fails to reload firmware second time before reboot.
-- Best regards Kaupo Arulo
--- devio.c.orig 2004-11-28 22:24:49.000000000 +0200
+++ devio.c 2004-12-01 12:47:02.000000000 +0200
@@ -1153,45 +1153,62 @@ static int usbdev_ioctl(struct inode *in
if (!(file->f_mode & FMODE_WRITE))
return -EPERM;
- down_read(&ps->devsem);
+ down_read(&ps->devsem); /* FIXME: should we set devsem also per "case"
+ like exclusive_access to avoid
+ blocking nonexistent ioctls ? */
if (!ps->dev) {
up_read(&ps->devsem);
return -ENODEV;
}
-
- /*
- * grab device's exclusive_access mutex to prevent its driver from
- * using this device while it is being accessed by us.
+ /*
+ * Some ioctls don't touch the device and can be called without
+ * grabbing its exclusive_access mutex; they are handled in this
+ * switch. Other ioctls which need exclusive_access are handled in
+ * usbdev_ioctl_exclusive(), so we grab device's exclusive_access
+ * mutex ONLY if needed and WHEN actually needed!!!
*/
- down(&ps->dev->exclusive_access);
-
switch (cmd) {
case USBDEVFS_CONTROL:
- ret = proc_control(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_control(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_BULK:
- ret = proc_bulk(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_bulk(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_RESETEP:
- ret = proc_resetep(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_resetep(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_RESET:
- ret = proc_resetdevice(ps);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_resetdevice(ps);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_CLEAR_HALT:
- ret = proc_clearhalt(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_clearhalt(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_GETDRIVER:
@@ -1203,21 +1220,33 @@ static int usbdev_ioctl(struct inode *in
break;
case USBDEVFS_SETINTERFACE:
- ret = proc_setintf(ps, (void *)arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_setintf(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_SETCONFIGURATION:
- ret = proc_setconfig(ps, (void *)arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_setconfig(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_SUBMITURB:
- ret = proc_submiturb(ps, (void *)arg);
- if (ret >= 0)
- inode->i_mtime = CURRENT_TIME;
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_submiturb(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ if (ret >= 0)
+ inode->i_mtime = CURRENT_TIME;
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_DISCARDURB:
- ret = proc_unlinkurb(ps, (void *)arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_unlinkurb(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_REAPURB:
@@ -1233,18 +1262,26 @@ static int usbdev_ioctl(struct inode *in
break;
case USBDEVFS_CLAIMINTERFACE:
- ret = proc_claiminterface(ps, (void *)arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_claiminterface(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_RELEASEINTERFACE:
- ret = proc_releaseinterface(ps, (void *)arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_releaseinterface(ps, (void *)arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
case USBDEVFS_IOCTL:
- ret = proc_ioctl(ps, (void *) arg);
+ if (down_interruptible(&ps->dev->exclusive_access) == 0) {
+ ret = proc_ioctl(ps, (void *) arg);
+ up(&ps->dev->exclusive_access);
+ } else ret = -ERESTARTSYS;
break;
}
- up(&ps->dev->exclusive_access);
up_read(&ps->devsem);
if (ret >= 0)
inode->i_atime = CURRENT_TIME;
