Prev: [PATCH -staging] slicoss: Remove net_device_stats from the driver's private.
Next: hwmon: Add support for W83667HG-B to w83627ehf driver
From: Sam Ravnborg on 4 Jul 2010 07:10
>
> 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 mixer_ioctl(file, cmd, arg);
> }
> return -EINVAL;
> }
>
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 4 Jul 2010 17:00
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Takashi Iwai on 12 Jul 2010 16:40
At Mon, 12 Jul 2010 19:53:18 +0200,
Arnd Bergmann wrote:
>
> These are the final conversions for the ioctl file operation so we can remove
> it in the next merge window.
>
> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>
> Cc: Jaroslav Kysela <perex(a)perex.cz>
> Cc: Takashi Iwai <tiwai(a)suse.de>
> Cc: alsa-devel(a)alsa-project.org
> ---
> On Monday 12 July 2010 17:53:32 Takashi Iwai wrote:
> >
> > BTW, do you have an updated patch for native_ioctl conversion wrt
> > sound/*?
>
> Almost forgot that, thanks for reminding me!

Thanks, applied now.

Now the rest is eliminating each lock_kernel() in sound/oss/*.c :)


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 12 Jul 2010 17:20
On Monday 12 July 2010 22:38:25 Takashi Iwai wrote:
> Now the rest is eliminating each lock_kernel() in sound/oss/*.c :)

For other files, I've used a script (see below) to do this, it probably
works with the OSS files as well, although I have not tried yet.

Of course, another option for OSS device drivers would be to
remove the entire driver ;). Either way, my feeling is that the
OSS drivers are not stopping anyone from building a kernel without
CONFIG_BKL once we have introduced that symbol and made the drivers
depend on it.

Arnd

---
#!/bin/bash
file=$1
name=$2
if grep -q lock_kernel ${file} ; then
if grep -q 'include.*linux.mutex.h' ${file} ; then
sed -i '/include.*<linux\/smp_lock.h>/d' ${file}
else
sed -i 's/include.*<linux\/smp_lock.h>.*$/include <linux\/mutex.h>/g' ${file}
fi
sed -i ${file} \
-e "/^#include.*linux.mutex.h/,$ {
1,/^\(static\|int\|long\)/ {
/^\(static\|int\|long\)/istatic DEFINE_MUTEX(${name}_mutex);

} }" \
-e "s/\(un\)*lock_kernel\>[ ]*()/mutex_\1lock(\&${name}_mutex)/g" \
-e '/[ ]*cycle_kernel_lock();/d'
else
sed -i -e '/include.*\<smp_lock.h\>/d' ${file} \
-e '/cycle_kernel_lock()/d'
fi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH -staging] slicoss: Remove net_device_stats from the driver's private.
Next: hwmon: Add support for W83667HG-B to w83627ehf driver