Re: [PATCH 11/13] [hwmon] changed ioctls to unlocked

[Hans de Goede <hdegoede@redhat.com>]

Hi,

2 Notes:

On 03/24/2009 10:12 PM, stoyboyker@gmail.com wrote:
> From: Stoyan Gaydarov<stoyboyker@gmail.com>
>
> Signed-off-by: Stoyan Gaydarov<stoyboyker@gmail.com>
> ---
>   drivers/hwmon/fschmd.c |   15 +++++++++++----
>   1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/fschmd.c b/drivers/hwmon/fschmd.c
> index d07f4ef..79efb7b 100644
> --- a/drivers/hwmon/fschmd.c
> +++ b/drivers/hwmon/fschmd.c
> @@ -40,6 +40,7 @@
>   #include<linux/hwmon-sysfs.h>
>   #include<linux/err.h>
>   #include<linux/mutex.h>
> +#include<linux/smp_lock.h>
>   #include<linux/sysfs.h>
>   #include<linux/dmi.h>
>   #include<linux/fs.h>
> @@ -769,15 +770,17 @@ static ssize_t watchdog_write(struct file *filp, const char __user *buf,
>   	return count;
>   }
>
> -static int watchdog_ioctl(struct inode *inode, struct file *filp,
> -	unsigned int cmd, unsigned long arg)
> +static long watchdog_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   {
>   	static struct watchdog_info ident = {
>   		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
>   				WDIOF_CARDRESET,
>   		.identity = "FSC watchdog"
>   	};
> -	int i, ret = 0;
> +
> +	lock_kernel();
> +
> +	int i, ret;
>   	struct fschmd_data *data = filp->private_data;
>
>   	switch (cmd) {
> @@ -837,6 +840,10 @@ static int watchdog_ioctl(struct inode *inode, struct file *filp,
>   		ret = -ENOTTY;
>   	}
>
> +	if(!ret)
> +		ret = -ENOTTY;
> +
> +	unlock_kernel();
>   	return ret;
>   }
>

1) Why on earth do you add that check a return value of 0 here means success, now the ioctl
no longer returns success ever ????

This does not inspire trust for your other 12 patches (which I did not check). Also please do
not ever, *never* hide bug-fixes like this one in other patches. If you believe you've found
a bug while working on something else, send the something else and the bugfix as 2 separate
patches! Otherwise little (potentially wrong) changes like these might slip under the radar
of the reviewer.

> @@ -846,7 +853,7 @@ static struct file_operations watchdog_fops = {
>   	.open = watchdog_open,
>   	.release = watchdog_release,
>   	.write = watchdog_write,
> -	.ioctl = watchdog_ioctl,
> +	.unlocked_ioctl = watchdog_ioctl,
>   };
>
>

2) I'm not completely familiar with char device locking semantics, but the
    watchdog_ioctl function is safe for being called multiple times simultaneous
    already (including being called together with other functions like write).
    So assuming the char device core kernel code is smart enough to not call
    watchdog_release while other operations such as ioctl are still being
    executed, then this above chunk is all that is needed. I guess this is my
    bad and I should have used unlocked_ioctl to begin with.

Regards,

Hans