2015-11-03 08:30:19

by Robin Gong

[permalink] [raw]
Subject: [PATCH v3 1/2] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT

Since the watchdog common framework centrialize the IOCTL interfaces of
device driver now, the SETPRETIMEOUT and GETPRETIMEOUT need to be added
in the common code.

Signed-off-by: Robin Gong <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 12 +++++++++
drivers/watchdog/watchdog_dev.c | 37 ++++++++++++++++++++++++++
include/linux/watchdog.h | 11 ++++++++
3 files changed, 60 insertions(+)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index d8b0d33..20aa841 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -51,6 +51,7 @@ struct watchdog_device {
const struct watchdog_ops *ops;
unsigned int bootstatus;
unsigned int timeout;
+ unsigned int pretimeout;
unsigned int min_timeout;
unsigned int max_timeout;
void *driver_data;
@@ -73,6 +74,7 @@ It contains following fields:
additional information about the watchdog timer itself. (Like it's unique name)
* ops: a pointer to the list of watchdog operations that the watchdog supports.
* timeout: the watchdog timer's timeout value (in seconds).
+* pretimeout: The watchdog devices pre_timeout value.
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
* max_timeout: the watchdog timer's maximum timeout value (in seconds).
* bootstatus: status of the device after booting (reported with watchdog
@@ -99,6 +101,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 *);
@@ -163,6 +166,15 @@ they are supported. These optional routines/operations are:
because the watchdog does not necessarily has a 1 second resolution).
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
+* set_pretimeout: this routine check and changes the pre_timeout value of
+ the watchdog, because some watchdog device can trigger the pre_timeout
+ interrupt before watchdog timeout event happened, so that we have chance
+ to save some critical information or something else before watchdog
+ triggered. The pre_timeout value means the number of seconds before
+ watchdog timeout.It returns 0 on success, -EINVAL for "parameter out
+ of range" and and -EIO for "could not write value to the watchdog".
+ (Note: the WDIOF_SETPRETIMEOUT needs to be set in the options field of the
+ watchdog's info structure).
* get_timeleft: this routines returns the time that's left before a reset.
* ref: the operation that calls kref_get on the kref of a dynamically
allocated watchdog_device struct.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefba..f4a02e5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -218,6 +218,37 @@ out_timeout:
}

/*
+ * watchdog_set_pretimeout: set the watchdog timer pretimeout
+ * @wddev: the watchdog device to set the timeout for
+ * @timeout: pretimeout to set in seconds
+ */
+
+static int watchdog_set_pretimeout(struct watchdog_device *wddev,
+ unsigned int timeout)
+{
+ int err;
+
+ if (!wddev->ops->set_pretimeout ||
+ !(wddev->info->options & WDIOF_PRETIMEOUT))
+ return -EOPNOTSUPP;
+ if (watchdog_pretimeout_invalid(wddev, timeout))
+ return -EINVAL;
+
+ mutex_lock(&wddev->lock);
+
+ if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+ err = -ENODEV;
+ goto out_timeout;
+ }
+
+ err = wddev->ops->set_pretimeout(wddev, timeout);
+
+out_timeout:
+ 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
@@ -393,6 +424,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
if (err)
return err;
return put_user(val, p);
+ case WDIOC_SETPRETIMEOUT:
+ if (get_user(val, p))
+ return -EFAULT;
+ return watchdog_set_pretimeout(wdd, val);
+ case WDIOC_GETPRETIMEOUT:
+ return put_user(wdd->pretimeout, p);
default:
return -ENOTTY;
}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index d74a0e9..281b949 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.
* @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 *);
@@ -60,6 +62,7 @@ struct watchdog_ops {
* @ops: Pointer to the list of watchdog operations.
* @bootstatus: Status of the watchdog device at boot.
* @timeout: The watchdog devices timeout value.
+ * @pretimeout: The watchdog devices pre_timeout value.
* @min_timeout:The watchdog devices minimum timeout value.
* @max_timeout:The watchdog devices maximum timeout value.
* @driver-data:Pointer to the drivers private data.
@@ -86,6 +89,7 @@ struct watchdog_device {
const struct watchdog_ops *ops;
unsigned int bootstatus;
unsigned int timeout;
+ unsigned int pretimeout;
unsigned int min_timeout;
unsigned int max_timeout;
void *driver_data;
@@ -123,6 +127,13 @@ 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->timeout && t >= wdd->timeout;
+}
+
/* Use the following functions to manipulate watchdog driver specific data */
static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
{
--
1.9.1


2015-11-03 08:30:37

by Robin Gong

[permalink] [raw]
Subject: [PATCH v3 2/2] watchdog: imx2_wdt: add set_pretimeout interface

Enable set_pretimeout interface and trigger the pretimeout interrupt before
watchdog timeout event happen.

Signed-off-by: Robin Gong <[email protected]>
---
drivers/watchdog/imx2_wdt.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 0bb1a1d..06631cf 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -24,6 +24,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
@@ -52,12 +53,18 @@
#define IMX2_WDT_WRSR 0x04 /* Reset Status Register */
#define IMX2_WDT_WRSR_TOUT (1 << 1) /* -> Reset due to Timeout */

+#define IMX2_WDT_WICR 0x06 /*Interrupt Control Register*/
+#define IMX2_WDT_WICR_WIE (1 << 15) /* -> Interrupt Enable */
+#define IMX2_WDT_WICR_WTIS (1 << 14) /* -> Interrupt Status */
+#define IMX2_WDT_WICR_WICT 0xFF /* Watchdog Interrupt Timeout */
+
#define IMX2_WDT_WMCR 0x08 /* Misc Register */

#define IMX2_WDT_MAX_TIME 128
#define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */

#define WDOG_SEC_TO_COUNT(s) ((s * 2 - 1) << 8)
+#define WDOG_SEC_TO_PRECOUNT(s) ((s) * 2) /* set WDOG pre timeout count*/

struct imx2_wdt_device {
struct clk *clk;
@@ -80,7 +87,8 @@ MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="

static const struct watchdog_info imx2_wdt_info = {
.identity = "imx2+ watchdog",
- .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+ .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE
+ | WDIOF_PRETIMEOUT,
};

static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
@@ -207,12 +215,49 @@ static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
}
}

+static int imx2_wdt_set_pretimeout(struct watchdog_device *wdog,
+ unsigned int new_timeout)
+{
+ struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+ u32 val;
+
+ regmap_read(wdev->regmap, IMX2_WDT_WICR, &val);
+ /* set the new pre-timeout value in the WSR */
+ val &= ~IMX2_WDT_WICR_WICT;
+ val |= WDOG_SEC_TO_PRECOUNT(new_timeout);
+
+ regmap_write(wdev->regmap, IMX2_WDT_WICR, val | IMX2_WDT_WICR_WIE);
+
+ wdog->pretimeout = new_timeout;
+
+ return 0;
+}
+
+static irqreturn_t imx2_wdt_isr(int irq, void *dev_id)
+{
+ struct platform_device *pdev = dev_id;
+ struct watchdog_device *wdog = platform_get_drvdata(pdev);
+ struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+ u32 val;
+
+ regmap_read(wdev->regmap, IMX2_WDT_WICR, &val);
+ if (val & IMX2_WDT_WICR_WTIS) {
+ /*clear interrupt status bit*/
+ regmap_write(wdev->regmap, IMX2_WDT_WICR, val);
+ panic("pre-timeout:%d, %d seconds remained\n", wdog->pretimeout,
+ wdog->timeout - wdog->pretimeout);
+ }
+
+ return IRQ_HANDLED;
+}
+
static const struct watchdog_ops imx2_wdt_ops = {
.owner = THIS_MODULE,
.start = imx2_wdt_start,
.stop = imx2_wdt_stop,
.ping = imx2_wdt_ping,
.set_timeout = imx2_wdt_set_timeout,
+ .set_pretimeout = imx2_wdt_set_pretimeout,
};

static const struct regmap_config imx2_wdt_regmap_config = {
@@ -229,6 +274,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *base;
int ret;
+ int irq;
u32 val;

wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
@@ -294,6 +340,16 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
goto disable_clk;
}

+ irq = platform_get_irq(pdev, 0);
+ if (irq > 0) {
+ ret = devm_request_irq(&pdev->dev, irq, imx2_wdt_isr, 0,
+ dev_name(&pdev->dev), pdev);
+ if (ret) {
+ dev_err(&pdev->dev, "can't get irq %d\n", irq);
+ goto disable_clk;
+ }
+ }
+
wdev->restart_handler.notifier_call = imx2_restart_handler;
wdev->restart_handler.priority = 128;
ret = register_restart_handler(&wdev->restart_handler);
--
1.9.1

2015-11-21 03:30:59

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT

On 11/03/2015 12:28 AM, Robin Gong wrote:
> Since the watchdog common framework centrialize the IOCTL interfaces of
> device driver now, the SETPRETIMEOUT and GETPRETIMEOUT need to be added
> in the common code.
>

Hi Robin,

Sorry for the long delay. I finally found the time to think about this.
I like the approach - it is simple and straightforward - but we'll need
to do a bit more.

First, we need to define the semantics. It must be possible to disable the
pretimeout, which we probably want to do by setting it to 0.

Second, we need to decide what to do when the _timeout_ is updated and the
pretimeout is configured but would be out of range. The easy solution would be
to update pretimeout to "timeout - 1" in that case. That would be tricky, though,
since it would have to be set _before_ updating the timeout, and the actually
selected timeout may differ from the asked for timeout.

Therefore, I think this should be left to the driver: Add a note stating that
the driver is responsible for updating pretimeout to a valid range if the
timeout is changed. Only the driver can implement this without race condition.

Some of the necessary changes outlined below.

Thanks,
Guenter

> Signed-off-by: Robin Gong <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 12 +++++++++
> drivers/watchdog/watchdog_dev.c | 37 ++++++++++++++++++++++++++
> include/linux/watchdog.h | 11 ++++++++
> 3 files changed, 60 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index d8b0d33..20aa841 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -51,6 +51,7 @@ struct watchdog_device {
> const struct watchdog_ops *ops;
> unsigned int bootstatus;
> unsigned int timeout;
> + unsigned int pretimeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> void *driver_data;
> @@ -73,6 +74,7 @@ It contains following fields:
> additional information about the watchdog timer itself. (Like it's unique name)
> * ops: a pointer to the list of watchdog operations that the watchdog supports.
> * timeout: the watchdog timer's timeout value (in seconds).
> +* pretimeout: The watchdog devices pre_timeout value.

... (in seconds)

> * min_timeout: the watchdog timer's minimum timeout value (in seconds).
> * max_timeout: the watchdog timer's maximum timeout value (in seconds).
> * bootstatus: status of the device after booting (reported with watchdog
> @@ -99,6 +101,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 *);
> @@ -163,6 +166,15 @@ they are supported. These optional routines/operations are:
> because the watchdog does not necessarily has a 1 second resolution).
> (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
> watchdog's info structure).

Add note to set_timeout semantics stating that the driver is responsible
for updating pretimeout if supported and enabled, and if the new timeout
conflicts with the old pretimeout.

> +* set_pretimeout: this routine check and changes the pre_timeout value of
> + the watchdog, because some watchdog device can trigger the pre_timeout
> + interrupt before watchdog timeout event happened, so that we have chance
> + to save some critical information or something else before watchdog
> + triggered. The pre_timeout value means the number of seconds before
> + watchdog timeout.It returns 0 on success, -EINVAL for "parameter out
Missing space ^

> + of range" and and -EIO for "could not write value to the watchdog".
> + (Note: the WDIOF_SETPRETIMEOUT needs to be set in the options field of the
> + watchdog's info structure).

Also add a note that setting pretimeout to 0 disables it.

> * get_timeleft: this routines returns the time that's left before a reset.
> * ref: the operation that calls kref_get on the kref of a dynamically
> allocated watchdog_device struct.
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 6aaefba..f4a02e5 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -218,6 +218,37 @@ out_timeout:
> }
>
> /*
> + * watchdog_set_pretimeout: set the watchdog timer pretimeout
> + * @wddev: the watchdog device to set the timeout for
> + * @timeout: pretimeout to set in seconds
> + */
> +
> +static int watchdog_set_pretimeout(struct watchdog_device *wddev,
> + unsigned int timeout)
> +{
> + int err;
> +
> + if (!wddev->ops->set_pretimeout ||
> + !(wddev->info->options & WDIOF_PRETIMEOUT))
> + return -EOPNOTSUPP;
> + if (watchdog_pretimeout_invalid(wddev, timeout))
> + return -EINVAL;
> +
> + mutex_lock(&wddev->lock);
> +
> + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
> + err = -ENODEV;
> + goto out_timeout;
> + }
> +
> + err = wddev->ops->set_pretimeout(wddev, timeout);
> +
> +out_timeout:
> + mutex_unlock(&wddev->lock);
> + return err;
> +}
> +

We'll also want a function watchdog_init_pretimeout(), similar to
watchdog_init_timeout().

> +/*
> * 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
> @@ -393,6 +424,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> if (err)
> return err;
> return put_user(val, p);
> + case WDIOC_SETPRETIMEOUT:
> + if (get_user(val, p))
> + return -EFAULT;
> + return watchdog_set_pretimeout(wdd, val);
> + case WDIOC_GETPRETIMEOUT:
> + return put_user(wdd->pretimeout, p);
> default:
> return -ENOTTY;
> }
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index d74a0e9..281b949 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.
> * @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 *);
> @@ -60,6 +62,7 @@ struct watchdog_ops {
> * @ops: Pointer to the list of watchdog operations.
> * @bootstatus: Status of the watchdog device at boot.
> * @timeout: The watchdog devices timeout value.
> + * @pretimeout: The watchdog devices pre_timeout value.
> * @min_timeout:The watchdog devices minimum timeout value.
> * @max_timeout:The watchdog devices maximum timeout value.
> * @driver-data:Pointer to the drivers private data.
> @@ -86,6 +89,7 @@ struct watchdog_device {
> const struct watchdog_ops *ops;
> unsigned int bootstatus;
> unsigned int timeout;
> + unsigned int pretimeout;
> unsigned int min_timeout;
> unsigned int max_timeout;
> void *driver_data;
> @@ -123,6 +127,13 @@ 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->timeout && t >= wdd->timeout;

0 is valid:

return t && wdd->timeout && t >= wdd->timeout;

> +}
> +
> /* Use the following functions to manipulate watchdog driver specific data */
> static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
> {
>