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

[Kyle Roeschley <kyle.roeschley@ni.com>]

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.

> > 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.

-- 
Kyle Roeschley
Software Engineer
National Instruments