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@xxxxxx>
> > Signed-off-by: Jeff Westfahl <jeff.westfahl@xxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks again for the reviews.
--
Kyle Roeschley
Software Engineer
National Instruments
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
