Re: [Linaro-acpi] [PATCH 4/6] Watchdog: introdouce "pretimeout" into framework

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