From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1946010AbbEONeF (ORCPT ); Fri, 15 May 2015 09:34:05 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:32867 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030276AbbEONeD (ORCPT ); Fri, 15 May 2015 09:34:03 -0400 Message-ID: <5555F5C5.8050806@roeck-us.net> Date: Fri, 15 May 2015 06:33:57 -0700 From: Guenter Roeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: fu.wei@linaro.org, Suravee.Suthikulpanit@amd.com, linaro-acpi@lists.linaro.org, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org CC: tekkamanninja@gmail.com, graeme.gregory@linaro.org, al.stone@linaro.org, hanjun.guo@linaro.org, timur@codeaurora.org, ashwin.chaugule@linaro.org, arnd@arndb.de, vgandhi@codeaurora.org, wim@iguana.be, jcm@redhat.com, leo.duran@amd.com, corbet@lwn.net Subject: Re: [PATCH 4/6] Watchdog: introdouce "pretimeout" into framework References: <=fu.wei@linaro.org> <1431689090-3125-1-git-send-email-fu.wei@linaro.org> In-Reply-To: <1431689090-3125-1-git-send-email-fu.wei@linaro.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/15/2015 04:24 AM, fu.wei@linaro.org wrote: > From: Fu Wei > > 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 > --- > 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. > } > > /** > @@ -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. > +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. 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 ? > + 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/ > + * 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). > + 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 ;-). Thanks, Guenter