Re: [lm-sensors] PATCH: add watchdog support to the fschmd driver

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

Jean Delvare wrote:
> Hi Hans,
> 
> On Tue, 11 Nov 2008 10:15:30 +0100, Hans de Goede wrote:
>> This patch adds support for the watchdog part found in _all_ supported FSC
>> sensor chips.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Looks OK to me except for two remaining issues:
> 
>> +static int watchdog_open(struct inode *inode, struct file *filp)
>> +{
>> +	int ret = 0;
>> +	struct fschmd_data *pos, *data = NULL;
>> +
>> +	/* We get called from drivers/char/misc.c with misc_mtx hold, and we
>> +	   call misc_register() from fschmd_probe() with watchdog_data_mutex
>> +	   hold, as misc_register() takes the misc_mtx lock, this is a possible
>> +	   deadlock, so we use mutex_trylock here. */
>> +	if (!mutex_trylock(&watchdog_data_mutex))
>> +		return -ERESTARTSYS;
>> +	list_for_each_entry(pos, &watchdog_data_list, list) {
>> +		if (pos->watchdog_miscdev.minor = iminor(inode)) {
>> +			data = pos;
>> +			break;
>> +		}
>> +	}
>> +	/* Note we can never not have found data, so we don't check for this */
>> +	kref_get(&data->kref);
>> +	mutex_unlock(&watchdog_data_mutex);
>> +
>> +	mutex_lock(&data->watchdog_lock);
>> +	if (data->watchdog_open_count)
>> +		ret = -EBUSY;
>> +	data->watchdog_open_count++;
>> +	mutex_unlock(&data->watchdog_lock);
>> +	if (ret)
>> +		return ret;
> 
> This looks wrong to me. Did you test concurrent open?

No

> If you return
> -EBUSY then caller will see its attempt to open the device node fail,
> so it will never get to close it. That means that you've incremented
> data->watchdog_open_count but it will never get decremented so it will
> be >= 1 forever and the watchdog device is no longer usable. I think
> this should be:
> 
> 	if (data->watchdog_open_count)
> 		ret = -EBUSY;
> 	else
> 		data->watchdog_open_count++;
> 

Agreed, will fix.

> Or am I missing something? You may want to use
> test_and_set_bit()/clear_bit() as many other watchdog drivers are doing.
> 

Yes even better will do.

>> +
>> +	/* Start the watchdog */
>> +	watchdog_trigger(data);
>> +	filp->private_data = data;
>> +
>> +	return nonseekable_open(inode, filp);
>> +}
> 
>> (...)
>> +static int watchdog_ioctl(struct inode *inode, 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;
>> +	struct fschmd_data *data = filp->private_data;
>> +
>> +	switch (cmd) {
>> +	case WDIOC_GETSUPPORT:
>> +		ident.firmware_version = data->revision;
>> +		if (!nowayout)
>> +			ident.options |= WDIOF_MAGICCLOSE;
>> +		if (copy_to_user((void __user *)arg, &ident, sizeof(ident)))
>> +			ret = -EFAULT;
>> +		break;
>> +
>> +	case WDIOC_GETSTATUS:
>> +		ret = put_user(0, (int __user *)arg);
>> +		break;
>> +
>> +	case WDIOC_GETBOOTSTATUS:
>> +		if (data->watchdog_state & FSCHMD_WDOG_STATE_CARDRESET)
>> +			ret = put_user(WDIOF_CARDRESET, (int __user *)arg);
>> +		else
>> +			ret = put_user(0, (int __user *)arg);
>> +		break;
>> +
>> +	case WDIOC_KEEPALIVE:
>> +		ret = watchdog_trigger(data);
>> +		break;
>> +
>> +	case WDIOC_GETTIMEOUT:
>> +		i = watchdog_get_timeout(data);
>> +		ret = put_user(i, (int __user *)arg);
>> +		break;
>> +
>> +	case WDIOC_SETTIMEOUT:
>> +		if (get_user(i, (int __user *)arg)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +		ret = watchdog_set_timeout(data, i);
>> +		if (ret > 0)
>> +			ret = put_user(i, (int __user *)arg);
> 
> Shouldn't you push "ret" rather than "i" back to the caller? As I
> understand it, "i" is what the user asked for, while "ret" is what the
> watchdog will actually do.
> 

And another good catch!

>> +		break;
>> +
>> +	case WDIOC_SETOPTIONS:
>> +		if (get_user(i, (int __user *)arg)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		if (i & WDIOS_DISABLECARD)
>> +			ret = watchdog_stop(data);
>> +		else if (i & WDIOS_ENABLECARD)
>> +			ret = watchdog_trigger(data);
>> +		else
>> +			ret = -EINVAL;
>> +
>> +		break;
>> +	default:
>> +		ret = -ENOTTY;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> Other than that, it looks OK to me (but remember I am no watchdog
> expert,

Well you seem to be doing a good job!

> and getting Wim Van Sebroeck to review your patch would make
> sense.

Ok I'll fix the 2 things you've found and then send an updated patch to Wim Van 
Sebroeck for checking.

> Can you please:
> * Address the remaining issues.

Will do asap and then send the patch to Wim Van Sebroeck

> * Test again.

That will take some day (I won't have access to the hw till next friday).

> * Send an updated version of the patch.

Will do once tested.

> 
> And finally:
> 
> * Send a 3rd patch deprecating the fscher and fscpos drivers, planing
>   them for removal in, say, 6 months?

Ah yes, will do also (gladly even) :)

Thans & Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors