Re: [2/2] niwatchdog: add support for custom ioctls

[Guenter Roeck <linux@roeck-us.net>]

On 01/25/2016 03:31 PM, Kyle Roeschley wrote:
> Hi Guenter,
>
> On Sat, Jan 16, 2016 at 08:29:41PM -0800, Guenter Roeck wrote:
>> On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
>>> Add custom ioctls for features specific to the NI watchdog, including
>>> disabling the watchdog, changing timeout behavior, and setting timeouts
>>> with sub-second resolution.
>>>
>> I don't really agree with any of the added functionality.
>> Most of the added ioctls duplicate standard functionality.
>>
>>
>
> The main thing that I think could be useful is the increased timeout
> resolution. Aside from that, the other changes are specifically implemented for
> the sake of maintaining compatability with our watchdog API--I think carrying
> some (hopefully not all) of those changes out-of-tree is reasonable.
>
We have been discussing to increase the timeout resolution to milli-seconds for a while.
A patch supporting a hardware timeout maximum in milli-seconds is in the works. I could
imagine adding milli-second resolution to the watchdog core. Nanoseconds is a bit of
an overkill, though.

Guenter

>>> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
>>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>>> ---
>>>   drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
>>>   include/uapi/linux/niwatchdog.h |  10 ++++
>>>   2 files changed, 123 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
>>> index 3908846..fe637a3 100644
>>> --- a/drivers/watchdog/niwatchdog.c
>>> +++ b/drivers/watchdog/niwatchdog.c
>>> @@ -131,6 +131,35 @@ out_unlock:
>>>   	return ret;
>>>   }
>>>
>>> +static int niwatchdog_check_action(u32 action)
>>> +{
>>> +	int err = 0;
>>> +
>>> +	switch (action) {
>>> +	case NIWD_CONTROL_PROC_INTERRUPT:
>>> +	case NIWD_CONTROL_PROC_RESET:
>>> +		break;
>>> +	default:
>>> +		err = -ENOTSUPP;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
>>> +{
>>> +	u8 control;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&niwatchdog->lock, flags);
>>> +
>>> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
>>> +	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
>>> +			      NIWD_CONTROL_PROC_RESET);
>>> +
>>> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
>>> +}
>>> +
>>>   static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
>>>   {
>>>   	u8 action_mask;
>>> @@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
>>>   {
>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>>   	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
>>> +	u8 actions;
>>>
>>> +	niwatchdog_get_actions(niwatchdog, &actions);
>>>   	niwatchdog_reset(niwatchdog);
>>>   	niwatchdog_counter_set(niwatchdog, counter);
>>> -	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>>
>>> -	return niwatchdog_start(niwatchdog);
>>> +	if (actions & NIWD_CONTROL_PROC_RESET)
>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>> +	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
>>> +
>>> +	niwatchdog_start(niwatchdog);
>>> +
>>> +	return 0;
>>>   }
>>>
>>>   static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
>>> @@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
>>>   	return 0;
>>>   }
>>>
>>> +static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
>>> +				 unsigned long arg)
>>> +{
>>> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>> +	int err = 0;
>>> +
>>> +	switch (cmd) {
>>> +	case NIWATCHDOG_IOCTL_PERIOD_NS: {
>>> +		__u32 period = NIWD_PERIOD_NS;
>>> +
>>> +		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
>>> +		break;
>>> +	}
>>> +	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
>>> +		__u32 counter = NIWD_MAX_COUNTER;
>>> +
>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Those are constants and can as well be defined in some documentation.
>>
>
> True. The only reason they're here is that our API expects this function to be
> implemented for all of our watchdog timers. As such, carrying this out-of-tree
> would be reasonable.
>
>>> +	case NIWATCHDOG_IOCTL_COUNTER_SET: {
>>> +		__u32 counter;
>>> +
>>> +		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_counter_set(niwatchdog, counter);
>>> +		break;
>>> +	}
>> Duplicates standard functionality
>>
>
> It duplicates set_timeout(), but with much greater resolution. What would be an
> appropriate way to expose this capability to users?
>
>>> +	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
>>> +		__u32 action;
>>> +
>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_check_action(action);
>>> +		break;
>>> +	}
>>> +	case NIWATCHDOG_IOCTL_ADD_ACTION: {
>>> +		__u32 action;
>>> +
>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_add_action(niwatchdog, action);
>>> +		break;
>>> +	}
>>
>> Use a module parameter or sysfs attribute for those.
>>
>
> I'm a fan of using a sysfs attribute, specifically because the options can be
> changed after opening the watchdog and you can have one, both, or neither of
> the two timeout actions set.
>
>>> +	case NIWATCHDOG_IOCTL_START: {
>>> +		niwatchdog_start(niwatchdog);
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>>> +	case NIWATCHDOG_IOCTL_PET: {
>>> +		__u32 state;
>>> +
>>> +		niwatchdog_pet(niwatchdog, &state);
>>> +		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>>> +	case NIWATCHDOG_IOCTL_RESET: {
>>> +		niwatchdog_reset(niwatchdog);
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>
> These are just maps from old, non-standard iotcls to the standard ones. They
> can be removed in favor of an out-of-tree commit.
>
>>> +	case NIWATCHDOG_IOCTL_COUNTER_GET: {
>>> +		__u32 counter;
>>> +
>>> +		niwatchdog_counter_get(niwatchdog, &counter);
>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>
> Yes, but again I'm not sure how we should give the user a more accurate
> remaining time.
>
>>> +	default:
>>> +		err = -ENOIOCTLCMD;
>>> +		break;
>>> +	};
>>> +
>>> +	return err;
>>> +}
>>> +
>>>   static int niwatchdog_ping(struct watchdog_device *wdd)
>>>   {
>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>> @@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
>>>   	.start		= niwatchdog_wdd_open,
>>>   	.stop		= niwatchdog_wdd_release,
>>>   	.ping		= niwatchdog_ping,
>>> +	.ioctl		= niwatchdog_wdd_ioctl,
>>>   	.set_timeout	= niwatchdog_wdd_set_timeout,
>>>   	.get_timeleft	= niwatchdog_wdd_get_timeleft,
>>>   };
>>> diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
>>> index 9d3613d..4fb8d47 100644
>>> --- a/include/uapi/linux/niwatchdog.h
>>> +++ b/include/uapi/linux/niwatchdog.h
>>> @@ -25,6 +25,16 @@
>>>   #define NIWATCHDOG_STATE_EXPIRED	1
>>>   #define NIWATCHDOG_STATE_DISABLED	2
>>>
>>> +#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
>>> +#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
>>> +#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
>>> +#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
>>> +#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
>>> +#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
>>> +#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
>>> +#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
>>> +#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
>>> +
>>>   #define NIWATCHDOG_NAME			"niwatchdog"
>>>
>>>   #endif /* _LINUX_NIWATCHDOG_H_ */
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Thanks again for the reviews.
>