Re: [alsa-devel] [PATCH 2/6] sound/oss: convert to unlocked_ioctl
diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c index c1070e3..3547450 100644 --- a/sound/oss/au1550_ac97.c +++ b/sound/oss/au1550_ac97.c @@ -824,22 +824,26 @@ mixdev_ioctl(struct ac97_codec *codec, unsigned int cmd, return codec->mixer_ioctl(codec, cmd, arg); }
-static int -au1550_ioctl_mixdev(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg)
+static long +au1550_ioctl_mixdev(struct file *file, unsigned int cmd, unsigned long arg) { struct au1550_state *s = (struct au1550_state *)file->private_data; struct ac97_codec *codec = s->codec;
- int ret;
- lock_kernel();
- ret = mixdev_ioctl(codec, cmd, arg);
- unlock_kernel();
- return mixdev_ioctl(codec, cmd, arg);
- return ret;
}
General comment... In several places you declare a local variable of type "int", but the function return a long.
I know that mixdev_ioctl() return an int so nothing is lost but it just looks wrong.
diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c index 3f3c3f7..b5464fb 100644 --- a/sound/oss/dmasound/dmasound_core.c +++ b/sound/oss/dmasound/dmasound_core.c @@ -337,8 +337,8 @@ static int mixer_release(struct inode *inode, struct file *file) unlock_kernel(); return 0; } -static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
u_long arg)
+static long mixer_ioctl(struct file *file, u_int cmd, u_long arg) { if (_SIOC_DIR(cmd) & _SIOC_WRITE) mixer.modify_counter++; @@ -362,11 +362,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd, return -EINVAL; }
+static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg) +{
- int ret;
- lock_kernel();
- ret = mixer_ioctl(file, cmd, arg);
- unlock_kernel();
- return ret;
+}
Here it looks like a potential but. mixer_ioctl() return a long which is stored in an int and then we return a long.
-static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
u_long arg)
+static long sq_ioctl(struct file *file, u_int cmd, u_long arg) { int val, result; u_long fmt; @@ -1114,18 +1124,29 @@ static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd, return IOCTL_OUT(arg,val);
default:
return mixer_ioctl(inode, file, cmd, arg);
} return -EINVAL;return mixer_ioctl(file, cmd, arg);
}
+static long sq_unlocked_ioctl(struct file *file, u_int cmd, u_long arg) +{
- int ret;
- lock_kernel();
- ret = sq_ioctl(file, cmd, arg);
- unlock_kernel();
- return ret;
+}
ditto: long -> int -> long
Sam
On Sunday 04 July 2010 13:08:35 Sam Ravnborg wrote:
General comment... In several places you declare a local variable of type "int", but the function return a long.
I know that mixdev_ioctl() return an int so nothing is lost but it just looks wrong.
This is completely intentional and follows what we have done in lots of other drivers that are already converted the same way.
The idea is that it was a bad choice to make the 'unlocked_ioctl' operation return 'long' in the first place. I remember Al ranting about this a few years ago. The ioctl syscall always returns an 'int' to user space, the only reason we have a 'long' return code in the definition of sys_ioctl and most other syscalls is to avoid problems for 32 bit emulation on some architectures.
Maybe one day someone will rename all unlocked_ioctl calls back to ioctl and change the return type back to int.
diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c index 3f3c3f7..b5464fb 100644 --- a/sound/oss/dmasound/dmasound_core.c +++ b/sound/oss/dmasound/dmasound_core.c @@ -337,8 +337,8 @@ static int mixer_release(struct inode *inode, struct file *file) unlock_kernel(); return 0; } -static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
u_long arg)
+static long mixer_ioctl(struct file *file, u_int cmd, u_long arg) { if (_SIOC_DIR(cmd) & _SIOC_WRITE) mixer.modify_counter++; @@ -362,11 +362,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd, return -EINVAL; }
+static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg) +{
- int ret;
- lock_kernel();
- ret = mixer_ioctl(file, cmd, arg);
- unlock_kernel();
- return ret;
+}
Here it looks like a potential but. mixer_ioctl() return a long which is stored in an int and then we return a long.
Right. I should probably have left the return value of mixer_ioctl as an int instead, to follow the scheme used in other drivers.
Arnd
participants (2)
-
Arnd Bergmann
-
Sam Ravnborg