| Message ID | 1431689090-3125-1-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Guenter, Great thanks for your review, feedback inline below :-) On 15 May 2015 at 21:33, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/15/2015 04:24 AM, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> Reasons: >> (1)kernel already has two watchdog drivers are using "pretimeout": >> drivers/char/ipmi/ipmi_watchdog.c >> drivers/watchdog/kempld_wdt.c(but the definition is different) >> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> drivers/watchdog/watchdog_core.c | 66 >> ++++++++++++++++++++++++++++++++++++++++ >> drivers/watchdog/watchdog_dev.c | 48 +++++++++++++++++++++++++++++ >> include/linux/watchdog.h | 19 ++++++++++++ >> 3 files changed, 133 insertions(+) >> >> diff --git a/drivers/watchdog/watchdog_core.c >> b/drivers/watchdog/watchdog_core.c >> index cec9b55..6ca9578 100644 >> --- a/drivers/watchdog/watchdog_core.c >> +++ b/drivers/watchdog/watchdog_core.c >> @@ -54,6 +54,14 @@ static void watchdog_check_min_max_timeout(struct >> watchdog_device *wdd) >> wdd->min_timeout = 0; >> wdd->max_timeout = 0; >> } >> + if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) >> { >> + pr_info("Invalid min timeout, resetting to min >> pretimeout!\n"); >> + wdd->min_timeout = wdd->min_pretimeout; >> + } >> + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) >> { >> + pr_info("Invalid max timeout, resetting to max >> pretimeout!\n"); >> + wdd->max_timeout = wdd->max_pretimeout; >> + } > > > I am a bit concerned about the context dependency introduced here. If > someone calls > _init_pretimeout after calling init_timeout, this may result in still > invalid timeout > values. yes, that logic is not very clean, so my thought is : maybe we can integrate watchdog_init_timeout and watchdog_init_pretimeout, if maintainer agree to add pretimeout into framework. > > >> } >> >> /** >> @@ -98,6 +106,63 @@ int watchdog_init_timeout(struct watchdog_device *wdd, >> } >> EXPORT_SYMBOL_GPL(watchdog_init_timeout); >> >> +static void watchdog_check_min_max_pretimeout(struct watchdog_device >> *wdd) >> +{ >> + /* >> + * Check that we have valid min and max pretimeout values, if >> + * not reset them both to 0 (=not used or unknown) >> + */ >> + if (wdd->min_pretimeout > wdd->max_pretimeout) { >> + pr_info("Invalid min and max pretimeout, resetting to >> 0!\n"); >> + wdd->min_pretimeout = 0; >> + wdd->max_pretimeout = 0; >> + } >> +} >> + >> +/** >> + * watchdog_init_pretimeout() - initialize the pretimeout field >> + * @pretimeout_parm: pretimeout module parameter >> + * @dev: Device that stores the timeout-sec property >> + * >> + * Initialize the pretimeout field of the watchdog_device struct with >> either >> + * the pretimeout module parameter (if it is valid value) or the >> timeout-sec >> + * property (only if it is a valid value and the timeout_parm is out of >> bounds). >> + * If none of them are valid then we keep the old value (which should >> normally >> + * be the default pretimeout value. >> + * >> + * A zero is returned on success and -EINVAL for failure. >> + */ > > > The new API function also needs to be documented in > Documentation/watchdog/watchdog-kernel-api.txt. good point, thanks will do > >> +int watchdog_init_pretimeout(struct watchdog_device *wdd, >> + unsigned int pretimeout_parm, struct device >> *dev) >> +{ >> + int ret = 0; >> + u32 timeouts[2]; >> + >> + watchdog_check_min_max_pretimeout(wdd); >> + >> + /* try to get the timeout module parameter first */ >> + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) && >> + pretimeout_parm) { >> + wdd->pretimeout = pretimeout_parm; >> + return ret; >> + } >> + if (pretimeout_parm) >> + ret = -EINVAL; >> + >> + /* try to get the timeout_sec property */ >> + if (!dev || !dev->of_node) >> + return ret; >> + ret = of_property_read_u32_array(dev->of_node, >> + "timeout-sec", timeouts, 2); >> + if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1]) > > > Guess we are still waiting for feedback from the devicetree maintainers on > this. yes! > > Both the above synchronization concern and this makes me wonder if we should > introduce > watchdog_init_timeouts() instead, which would take pretimeout as additional > parameter. > What do you think about that ? yes, that is what I am thinking about > > >> + wdd->pretimeout = timeouts[1]; >> + else >> + ret = -EINVAL; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout); >> + >> /** >> * watchdog_register_device() - register a watchdog device >> * @wdd: watchdog device >> @@ -119,6 +184,7 @@ int watchdog_register_device(struct watchdog_device >> *wdd) >> if (wdd->ops->start == NULL || wdd->ops->stop == NULL) >> return -EINVAL; >> >> + watchdog_check_min_max_pretimeout(wdd); >> watchdog_check_min_max_timeout(wdd); >> >> /* >> diff --git a/drivers/watchdog/watchdog_dev.c >> b/drivers/watchdog/watchdog_dev.c >> index 6aaefba..b519257 100644 >> --- a/drivers/watchdog/watchdog_dev.c >> +++ b/drivers/watchdog/watchdog_dev.c >> @@ -218,6 +218,38 @@ out_timeout: >> } >> >> /* >> + * watchdog_set_pretimeout: set the watchdog timer pretimeout >> + * @wddev: the watchdog device to set the timeout for >> + * @pretimeout: pretimeout to set in seconds >> + */ >> + >> +static int watchdog_set_pretimeout(struct watchdog_device *wddev, >> + unsigned int pretimeout) >> +{ >> + int err; >> + >> + if (!wddev->ops->set_pretimeout || >> + !(wddev->info->options & WDIOF_PRETIMEOUT)) >> + return -EOPNOTSUPP; >> + >> + if (watchdog_pretimeout_invalid(wddev, pretimeout)) >> + return -EINVAL; >> + >> + mutex_lock(&wddev->lock); >> + >> + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { >> + err = -ENODEV; >> + goto out_pretimeout; >> + } >> + >> + err = wddev->ops->set_pretimeout(wddev, pretimeout); >> + >> +out_pretimeout: >> + mutex_unlock(&wddev->lock); >> + return err; >> +} >> + >> +/* >> * watchdog_get_timeleft: wrapper to get the time left before a >> reboot >> * @wddev: the watchdog device to get the remaining time from >> * @timeleft: the time that's left >> @@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, >> unsigned int cmd, >> if (wdd->timeout == 0) >> return -EOPNOTSUPP; >> return put_user(wdd->timeout, p); >> + case WDIOC_SETPRETIMEOUT: >> + if (get_user(val, p)) >> + return -EFAULT; >> + err = watchdog_set_pretimeout(wdd, val); >> + if (err < 0) >> + return err; >> + /* If the watchdog is active then we send a keepalive ping >> + * to make sure that the watchdog keep's running (and if > > > s/keep's/keeps/ Great thanks, typo > >> + * possible that it takes the new timeout) */ > > > Please use the common multi-line comment style (that it isn't used above is > not > an argument). yes, my bad, will fixed . > > >> + watchdog_ping(wdd); >> + /* Fall */ >> + case WDIOC_GETPRETIMEOUT: >> + /* timeout == 0 means that we don't use the pretimeout */ >> + if (wdd->pretimeout == 0) >> + return -EOPNOTSUPP; >> + return put_user(wdd->pretimeout, p); >> case WDIOC_GETTIMELEFT: >> err = watchdog_get_timeleft(wdd, &val); >> if (err) >> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h >> index a746bf5..f0a3c5b 100644 >> --- a/include/linux/watchdog.h >> +++ b/include/linux/watchdog.h >> @@ -25,6 +25,7 @@ struct watchdog_device; >> * @ping: The routine that sends a keepalive ping to the watchdog >> device. >> * @status: The routine that shows the status of the watchdog device. >> * @set_timeout:The routine for setting the watchdog devices timeout >> value. >> + * @set_pretimeout:The routine for setting the watchdog devices >> pretimeout value >> * @get_timeleft:The routine that get's the time that's left before a >> reset. >> * @ref: The ref operation for dyn. allocated watchdog_device >> structs >> * @unref: The unref operation for dyn. allocated watchdog_device >> structs >> @@ -44,6 +45,7 @@ struct watchdog_ops { >> int (*ping)(struct watchdog_device *); >> unsigned int (*status)(struct watchdog_device *); >> int (*set_timeout)(struct watchdog_device *, unsigned int); >> + int (*set_pretimeout)(struct watchdog_device *, unsigned int); >> unsigned int (*get_timeleft)(struct watchdog_device *); >> void (*ref)(struct watchdog_device *); >> void (*unref)(struct watchdog_device *); >> @@ -62,6 +64,9 @@ struct watchdog_ops { >> * @timeout: The watchdog devices timeout value. >> * @min_timeout:The watchdog devices minimum timeout value. >> * @max_timeout:The watchdog devices maximum timeout value. >> + * @pretimeout: The watchdog devices pretimeout value. >> + * @min_pretimeout:The watchdog devices minimum pretimeout value. >> + * @max_pretimeout:The watchdog devices maximum pretimeout value. >> * @driver-data:Pointer to the drivers private data. >> * @lock: Lock for watchdog core internal use only. >> * @status: Field that contains the devices internal status bits. >> @@ -86,6 +91,9 @@ struct watchdog_device { >> unsigned int timeout; >> unsigned int min_timeout; >> unsigned int max_timeout; >> + unsigned int pretimeout; >> + unsigned int min_pretimeout; >> + unsigned int max_pretimeout; >> void *driver_data; >> struct mutex lock; >> unsigned long status; >> @@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct >> watchdog_device *wdd, unsigne >> (t < wdd->min_timeout || t > wdd->max_timeout)); >> } >> >> +/* Use the following function to check if a pretimeout value is invalid >> */ >> +static inline bool watchdog_pretimeout_invalid(struct watchdog_device >> *wdd, >> + unsigned int t) >> +{ >> + return ((wdd->max_pretimeout != 0) && >> + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); >> +} >> + >> /* Use the following functions to manipulate watchdog driver specific >> data */ >> static inline void watchdog_set_drvdata(struct watchdog_device *wdd, >> void *data) >> { >> @@ -134,6 +150,9 @@ static inline void *watchdog_get_drvdata(struct >> watchdog_device *wdd) >> /* drivers/watchdog/watchdog_core.c */ >> extern int watchdog_init_timeout(struct watchdog_device *wdd, >> unsigned int timeout_parm, struct device >> *dev); >> +extern int watchdog_init_pretimeout(struct watchdog_device *wdd, >> + unsigned int pretimeout_parm, >> + struct device *dev); > > > Please drop the 'extern'. Guess it may be time to clean up the watchdog core > code ;-). yes, you are right, but in different patchset ?? > > Thanks, > Guenter >
Hi Arnd, Great thanks for your suggestion :-) feedback inline below On 15 May 2015 at 22:04, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 15 May 2015 19:24:48 fu.wei@linaro.org wrote: >> +static void watchdog_check_min_max_pretimeout(struct watchdog_device *wdd) >> +{ >> + /* >> + * Check that we have valid min and max pretimeout values, if >> + * not reset them both to 0 (=not used or unknown) >> + */ >> + if (wdd->min_pretimeout > wdd->max_pretimeout) { >> + pr_info("Invalid min and max pretimeout, resetting to 0!\n"); >> + wdd->min_pretimeout = 0; >> + wdd->max_pretimeout = 0; >> + } >> +} > > I would probably just fold this function into the existing > watchdog_check_min_max_timeout() and check both normal and pre-timeout > there. yes, I can do that , and that is good idea > >> +/** >> + * watchdog_init_pretimeout() - initialize the pretimeout field >> + * @pretimeout_parm: pretimeout module parameter >> + * @dev: Device that stores the timeout-sec property >> + * >> + * Initialize the pretimeout field of the watchdog_device struct with either >> + * the pretimeout module parameter (if it is valid value) or the timeout-sec >> + * property (only if it is a valid value and the timeout_parm is out of bounds). >> + * If none of them are valid then we keep the old value (which should normally >> + * be the default pretimeout value. >> + * >> + * A zero is returned on success and -EINVAL for failure. >> + */ >> +int watchdog_init_pretimeout(struct watchdog_device *wdd, >> + unsigned int pretimeout_parm, struct device *dev) >> +{ >> + int ret = 0; >> + u32 timeouts[2]; >> + >> + watchdog_check_min_max_pretimeout(wdd); >> + >> + /* try to get the timeout module parameter first */ >> + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) && >> + pretimeout_parm) { >> + wdd->pretimeout = pretimeout_parm; >> + return ret; >> + } >> + if (pretimeout_parm) >> + ret = -EINVAL; >> + >> + /* try to get the timeout_sec property */ >> + if (!dev || !dev->of_node) >> + return ret; >> + ret = of_property_read_u32_array(dev->of_node, >> + "timeout-sec", timeouts, 2); >> + if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1]) >> + wdd->pretimeout = timeouts[1]; >> + else >> + ret = -EINVAL; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout); > > Same here: the function is very similar to the watchdog_init_timeout > function, and it reads the same property, so just do both here. > > The easiest way for that is probably to use of_find_property() > and of_prop_next_u32() to read the two numbers. integrate watchdog_init_pretimeout and watchdog_init_timeout will be a little hard, we may need to change this API to : watchdog_init_timeouts(struct watchdog_device *wdd, unsigned int timeout_parm, unsigned int pretimeout_parm, struct device *dev) then we need to update all the watchdog drivers which use this API, maybe we can do this in a individual patchset, after this pretimeout patch is merged. Is that OK ? :-) any thought? > > Arnd >
Hi Guenter, yes, I think it is OK for me, Once this patchset is merged, I will try to make a new patchset just for this integration. On 16 May 2015 at 02:01, Guenter Roeck <linux@roeck-us.net> wrote: > On Fri, May 15, 2015 at 09:49:07PM +0800, Fu Wei wrote: >> Hi Guenter, >> >> Great thanks for your review, >> feedback inline below :-) >> >> On 15 May 2015 at 21:33, Guenter Roeck <linux@roeck-us.net> wrote: > > [ ... ] > >> >> + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) >> >> { >> >> + pr_info("Invalid max timeout, resetting to max >> >> pretimeout!\n"); >> >> + wdd->max_timeout = wdd->max_pretimeout; >> >> + } >> > >> > >> > I am a bit concerned about the context dependency introduced here. If >> > someone calls >> > _init_pretimeout after calling init_timeout, this may result in still >> > invalid timeout >> > values. >> >> yes, that logic is not very clean, so my thought is : >> maybe we can integrate watchdog_init_timeout and watchdog_init_pretimeout, >> if maintainer agree to add pretimeout into framework. >> > I think we should just assume that Wim will accept it, and try to find > the best possible solution (or at least a good one). > > Guenter
Hi Guenter, np, will do so :-) On 19 May 2015 at 01:23, Guenter Roeck <linux@roeck-us.net> wrote: > On Tue, May 19, 2015 at 01:19:22AM +0800, Fu Wei wrote: >> Hi Arnd, >> >> Great thanks for your suggestion :-) >> >> feedback inline below >> >> On 15 May 2015 at 22:04, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Friday 15 May 2015 19:24:48 fu.wei@linaro.org wrote: >> >> +static void watchdog_check_min_max_pretimeout(struct watchdog_device *wdd) >> >> +{ >> >> + /* >> >> + * Check that we have valid min and max pretimeout values, if >> >> + * not reset them both to 0 (=not used or unknown) >> >> + */ >> >> + if (wdd->min_pretimeout > wdd->max_pretimeout) { >> >> + pr_info("Invalid min and max pretimeout, resetting to 0!\n"); >> >> + wdd->min_pretimeout = 0; >> >> + wdd->max_pretimeout = 0; >> >> + } >> >> +} >> > >> > I would probably just fold this function into the existing >> > watchdog_check_min_max_timeout() and check both normal and pre-timeout >> > there. >> >> yes, I can do that , and that is good idea >> >> > >> >> +/** >> >> + * watchdog_init_pretimeout() - initialize the pretimeout field >> >> + * @pretimeout_parm: pretimeout module parameter >> >> + * @dev: Device that stores the timeout-sec property >> >> + * >> >> + * Initialize the pretimeout field of the watchdog_device struct with either >> >> + * the pretimeout module parameter (if it is valid value) or the timeout-sec >> >> + * property (only if it is a valid value and the timeout_parm is out of bounds). >> >> + * If none of them are valid then we keep the old value (which should normally >> >> + * be the default pretimeout value. >> >> + * >> >> + * A zero is returned on success and -EINVAL for failure. >> >> + */ >> >> +int watchdog_init_pretimeout(struct watchdog_device *wdd, >> >> + unsigned int pretimeout_parm, struct device *dev) >> >> +{ >> >> + int ret = 0; >> >> + u32 timeouts[2]; >> >> + >> >> + watchdog_check_min_max_pretimeout(wdd); >> >> + >> >> + /* try to get the timeout module parameter first */ >> >> + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) && >> >> + pretimeout_parm) { >> >> + wdd->pretimeout = pretimeout_parm; >> >> + return ret; >> >> + } >> >> + if (pretimeout_parm) >> >> + ret = -EINVAL; >> >> + >> >> + /* try to get the timeout_sec property */ >> >> + if (!dev || !dev->of_node) >> >> + return ret; >> >> + ret = of_property_read_u32_array(dev->of_node, >> >> + "timeout-sec", timeouts, 2); >> >> + if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1]) >> >> + wdd->pretimeout = timeouts[1]; >> >> + else >> >> + ret = -EINVAL; >> >> + >> >> + return ret; >> >> +} >> >> +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout); >> > >> > Same here: the function is very similar to the watchdog_init_timeout >> > function, and it reads the same property, so just do both here. >> > >> > The easiest way for that is probably to use of_find_property() >> > and of_prop_next_u32() to read the two numbers. >> >> integrate watchdog_init_pretimeout and watchdog_init_timeout will be a >> little hard, >> we may need to change this API to : >> >> watchdog_init_timeouts(struct watchdog_device *wdd, unsigned int timeout_parm, >> unsigned int pretimeout_parm, struct device *dev) >> >> then we need to update all the watchdog drivers which use this API, >> maybe we can do this in a individual patchset, after this pretimeout >> patch is merged. >> >> Is that OK ? :-) any thought? >> > That is what I would recommend. > > Guenter
Hi Arnd, Guenter, yes, that is brilliant idea!! I will try to do so , that solve the compatibility problem , so I guess we can try this time :-) On 19 May 2015 at 04:14, Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, May 18, 2015 at 10:03:52PM +0200, Arnd Bergmann wrote: >> On Monday 18 May 2015 10:23:30 Guenter Roeck wrote: >> > > >> > > integrate watchdog_init_pretimeout and watchdog_init_timeout will be a >> > > little hard, >> > > we may need to change this API to : >> > > >> > > watchdog_init_timeouts(struct watchdog_device *wdd, unsigned int timeout_parm, >> > > unsigned int pretimeout_parm, struct device *dev) >> > > >> > > then we need to update all the watchdog drivers which use this API, >> > > maybe we can do this in a individual patchset, after this pretimeout >> > > patch is merged. >> > > >> > > Is that OK ? any thought? >> > > >> > That is what I would recommend. >> > >> >> The API change is fine, but I don't think you need to change all drivers. >> >> Just add a small wrapper function in the header file doing the conversion: >> >> static inline int watchdog_init_timeout(struct watchdog_device *wdd, >> unsigned int timeout_parm, struct device *dev) >> { >> return watchdog_init_timeouts(wdd, timeout_parm, ~0ul, dev); >> } >> >> Then you can update the drivers that actually use the pretimeout to >> use the new function at some point, and leave all other drivers calling >> the wrapper function. >> > Excellent idea. > > Guenter
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..6ca9578 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -54,6 +54,14 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) wdd->min_timeout = 0; wdd->max_timeout = 0; } + if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) { + pr_info("Invalid min timeout, resetting to min pretimeout!\n"); + wdd->min_timeout = wdd->min_pretimeout; + } + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) { + pr_info("Invalid max timeout, resetting to max pretimeout!\n"); + wdd->max_timeout = wdd->max_pretimeout; + } } /** @@ -98,6 +106,63 @@ int watchdog_init_timeout(struct watchdog_device *wdd, } EXPORT_SYMBOL_GPL(watchdog_init_timeout); +static void watchdog_check_min_max_pretimeout(struct watchdog_device *wdd) +{ + /* + * Check that we have valid min and max pretimeout values, if + * not reset them both to 0 (=not used or unknown) + */ + if (wdd->min_pretimeout > wdd->max_pretimeout) { + pr_info("Invalid min and max pretimeout, resetting to 0!\n"); + wdd->min_pretimeout = 0; + wdd->max_pretimeout = 0; + } +} + +/** + * watchdog_init_pretimeout() - initialize the pretimeout field + * @pretimeout_parm: pretimeout module parameter + * @dev: Device that stores the timeout-sec property + * + * Initialize the pretimeout field of the watchdog_device struct with either + * the pretimeout module parameter (if it is valid value) or the timeout-sec + * property (only if it is a valid value and the timeout_parm is out of bounds). + * If none of them are valid then we keep the old value (which should normally + * be the default pretimeout value. + * + * A zero is returned on success and -EINVAL for failure. + */ +int watchdog_init_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout_parm, struct device *dev) +{ + int ret = 0; + u32 timeouts[2]; + + watchdog_check_min_max_pretimeout(wdd); + + /* try to get the timeout module parameter first */ + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) && + pretimeout_parm) { + wdd->pretimeout = pretimeout_parm; + return ret; + } + if (pretimeout_parm) + ret = -EINVAL; + + /* try to get the timeout_sec property */ + if (!dev || !dev->of_node) + return ret; + ret = of_property_read_u32_array(dev->of_node, + "timeout-sec", timeouts, 2); + if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1]) + wdd->pretimeout = timeouts[1]; + else + ret = -EINVAL; + + return ret; +} +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout); + /** * watchdog_register_device() - register a watchdog device * @wdd: watchdog device @@ -119,6 +184,7 @@ int watchdog_register_device(struct watchdog_device *wdd) if (wdd->ops->start == NULL || wdd->ops->stop == NULL) return -EINVAL; + watchdog_check_min_max_pretimeout(wdd); watchdog_check_min_max_timeout(wdd); /* diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 6aaefba..b519257 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -218,6 +218,38 @@ out_timeout: } /* + * watchdog_set_pretimeout: set the watchdog timer pretimeout + * @wddev: the watchdog device to set the timeout for + * @pretimeout: pretimeout to set in seconds + */ + +static int watchdog_set_pretimeout(struct watchdog_device *wddev, + unsigned int pretimeout) +{ + int err; + + if (!wddev->ops->set_pretimeout || + !(wddev->info->options & WDIOF_PRETIMEOUT)) + return -EOPNOTSUPP; + + if (watchdog_pretimeout_invalid(wddev, pretimeout)) + return -EINVAL; + + mutex_lock(&wddev->lock); + + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_pretimeout; + } + + err = wddev->ops->set_pretimeout(wddev, pretimeout); + +out_pretimeout: + mutex_unlock(&wddev->lock); + return err; +} + +/* * watchdog_get_timeleft: wrapper to get the time left before a reboot * @wddev: the watchdog device to get the remaining time from * @timeleft: the time that's left @@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, if (wdd->timeout == 0) return -EOPNOTSUPP; return put_user(wdd->timeout, p); + case WDIOC_SETPRETIMEOUT: + if (get_user(val, p)) + return -EFAULT; + err = watchdog_set_pretimeout(wdd, val); + if (err < 0) + return err; + /* If the watchdog is active then we send a keepalive ping + * to make sure that the watchdog keep's running (and if + * possible that it takes the new timeout) */ + watchdog_ping(wdd); + /* Fall */ + case WDIOC_GETPRETIMEOUT: + /* timeout == 0 means that we don't use the pretimeout */ + if (wdd->pretimeout == 0) + return -EOPNOTSUPP; + return put_user(wdd->pretimeout, p); case WDIOC_GETTIMELEFT: err = watchdog_get_timeleft(wdd, &val); if (err) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index a746bf5..f0a3c5b 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -25,6 +25,7 @@ struct watchdog_device; * @ping: The routine that sends a keepalive ping to the watchdog device. * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. + * @set_pretimeout:The routine for setting the watchdog devices pretimeout value * @get_timeleft:The routine that get's the time that's left before a reset. * @ref: The ref operation for dyn. allocated watchdog_device structs * @unref: The unref operation for dyn. allocated watchdog_device structs @@ -44,6 +45,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); + int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -62,6 +64,9 @@ struct watchdog_ops { * @timeout: The watchdog devices timeout value. * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. + * @pretimeout: The watchdog devices pretimeout value. + * @min_pretimeout:The watchdog devices minimum pretimeout value. + * @max_pretimeout:The watchdog devices maximum pretimeout value. * @driver-data:Pointer to the drivers private data. * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. @@ -86,6 +91,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne (t < wdd->min_timeout || t > wdd->max_timeout)); } +/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd, + unsigned int t) +{ + return ((wdd->max_pretimeout != 0) && + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); +} + /* Use the following functions to manipulate watchdog driver specific data */ static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data) { @@ -134,6 +150,9 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) /* drivers/watchdog/watchdog_core.c */ extern int watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, struct device *dev); +extern int watchdog_init_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + struct device *dev); extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *);