2020-07-17 13:32:37

by Tero Kristo

[permalink] [raw]
Subject: [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer

Hi,

V4 of this series only fixes 32bit compile issue with patch #3. Appears
to be compiling now fine with ARM32 allmodconfig.

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


2020-07-17 13:32:37

by Tero Kristo

[permalink] [raw]
Subject: [PATCHv4 1/4] watchdog: use __watchdog_ping in startup

Current watchdog startup functionality does not respect the minimum hw
heartbeat setup and the last watchdog ping timeframe when watchdog is
already running and userspace process attaches to it. Fix this by using
the __watchdog_ping from the startup also. For this code path, we can
also let the __watchdog_ping handle the bookkeeping for the worker and
last keepalive times.

Signed-off-by: Tero Kristo <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/watchdog_dev.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 7e4cd34a8c20..bc1cfa288553 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -275,15 +275,18 @@ static int watchdog_start(struct watchdog_device *wdd)
set_bit(_WDOG_KEEPALIVE, &wd_data->status);

started_at = ktime_get();
- if (watchdog_hw_running(wdd) && wdd->ops->ping)
- err = wdd->ops->ping(wdd);
- else
+ if (watchdog_hw_running(wdd) && wdd->ops->ping) {
+ err = __watchdog_ping(wdd);
+ if (err == 0)
+ set_bit(WDOG_ACTIVE, &wdd->status);
+ } else {
err = wdd->ops->start(wdd);
- if (err == 0) {
- set_bit(WDOG_ACTIVE, &wdd->status);
- wd_data->last_keepalive = started_at;
- wd_data->last_hw_keepalive = started_at;
- watchdog_update_worker(wdd);
+ if (err == 0) {
+ set_bit(WDOG_ACTIVE, &wdd->status);
+ wd_data->last_keepalive = started_at;
+ wd_data->last_hw_keepalive = started_at;
+ watchdog_update_worker(wdd);
+ }
}

return err;
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2020-07-17 13:33:17

by Tero Kristo

[permalink] [raw]
Subject: [PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls

PM runtime should be disabled in the fail path of probe and when
the driver is removed.

Fixes: 2d63908bdbfb ("watchdog: Add K3 RTI watchdog support")
Signed-off-by: Tero Kristo <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/rti_wdt.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 7cbdc178ffe8..705e8f7523e8 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -303,6 +303,7 @@ static int rti_wdt_probe(struct platform_device *pdev)

err_iomap:
pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);

return ret;
}
@@ -313,6 +314,7 @@ static int rti_wdt_remove(struct platform_device *pdev)

watchdog_unregister_device(&wdt->wdd);
pm_runtime_put(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);

return 0;
}
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2020-07-17 13:33:33

by Tero Kristo

[permalink] [raw]
Subject: [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time

Certain watchdogs require the watchdog only to be pinged within a
specific time window, pinging too early or too late cause the watchdog
to fire. In cases where this sort of watchdog has been started before
kernel comes up, we must adjust the watchdog keepalive window to match
the actually running timer, so add a new driver API for this purpose.

Signed-off-by: Tero Kristo <[email protected]>
---
.../watchdog/watchdog-kernel-api.rst | 12 ++++++++
drivers/watchdog/watchdog_dev.c | 30 +++++++++++++++++++
include/linux/watchdog.h | 2 ++
3 files changed, 44 insertions(+)

diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst
index 068a55ee0d4a..baf44e986b07 100644
--- a/Documentation/watchdog/watchdog-kernel-api.rst
+++ b/Documentation/watchdog/watchdog-kernel-api.rst
@@ -336,3 +336,15 @@ an action is taken by a preconfigured pretimeout governor preassigned to
the watchdog device. If watchdog pretimeout governor framework is not
enabled, watchdog_notify_pretimeout() prints a notification message to
the kernel log buffer.
+
+To set the last known HW keepalive time for a watchdog, the following function
+should be used::
+
+ int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+ unsigned int last_ping_ms)
+
+This function must be called immediately after watchdog registration. It
+sets the last known hardware heartbeat to have happened last_ping_ms before
+current time. Calling this is only needed if the watchdog is already running
+when probe is called, and the watchdog can only be pinged after the
+min_hw_heartbeat_ms time has passed from the last ping.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index bc1cfa288553..e74a0c6811b5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1138,6 +1138,36 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
watchdog_cdev_unregister(wdd);
}

+/*
+ * watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
+ * @wdd: watchdog device
+ * @last_ping_ms: time since last HW heartbeat
+ *
+ * Adjusts the last known HW keepalive time for a watchdog timer.
+ * This is needed if the watchdog is already running when the probe
+ * function is called, and it can't be pinged immediately. This
+ * function must be called immediately after watchdog registration,
+ * and min_hw_heartbeat_ms must be set for this to be useful.
+ */
+int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+ unsigned int last_ping_ms)
+{
+ struct watchdog_core_data *wd_data;
+ ktime_t now;
+
+ if (!wdd)
+ return -EINVAL;
+
+ wd_data = wdd->wd_data;
+
+ now = ktime_get();
+
+ wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));
+
+ return __watchdog_ping(wdd);
+}
+EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);
+
/*
* watchdog_dev_init: init dev part of watchdog core
*
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1464ce6ffa31..9b19e6bb68b5 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
extern int watchdog_register_device(struct watchdog_device *);
extern void watchdog_unregister_device(struct watchdog_device *);

+int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
+
/* devres register variant */
int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);

--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2020-07-19 13:55:36

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time

On 7/17/20 6:29 AM, Tero Kristo wrote:
> Certain watchdogs require the watchdog only to be pinged within a
> specific time window, pinging too early or too late cause the watchdog
> to fire. In cases where this sort of watchdog has been started before
> kernel comes up, we must adjust the watchdog keepalive window to match
> the actually running timer, so add a new driver API for this purpose.
>
> Signed-off-by: Tero Kristo <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> .../watchdog/watchdog-kernel-api.rst | 12 ++++++++
> drivers/watchdog/watchdog_dev.c | 30 +++++++++++++++++++
> include/linux/watchdog.h | 2 ++
> 3 files changed, 44 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst
> index 068a55ee0d4a..baf44e986b07 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.rst
> +++ b/Documentation/watchdog/watchdog-kernel-api.rst
> @@ -336,3 +336,15 @@ an action is taken by a preconfigured pretimeout governor preassigned to
> the watchdog device. If watchdog pretimeout governor framework is not
> enabled, watchdog_notify_pretimeout() prints a notification message to
> the kernel log buffer.
> +
> +To set the last known HW keepalive time for a watchdog, the following function
> +should be used::
> +
> + int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
> + unsigned int last_ping_ms)
> +
> +This function must be called immediately after watchdog registration. It
> +sets the last known hardware heartbeat to have happened last_ping_ms before
> +current time. Calling this is only needed if the watchdog is already running
> +when probe is called, and the watchdog can only be pinged after the
> +min_hw_heartbeat_ms time has passed from the last ping.
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index bc1cfa288553..e74a0c6811b5 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1138,6 +1138,36 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
> watchdog_cdev_unregister(wdd);
> }
>
> +/*
> + * watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
> + * @wdd: watchdog device
> + * @last_ping_ms: time since last HW heartbeat
> + *
> + * Adjusts the last known HW keepalive time for a watchdog timer.
> + * This is needed if the watchdog is already running when the probe
> + * function is called, and it can't be pinged immediately. This
> + * function must be called immediately after watchdog registration,
> + * and min_hw_heartbeat_ms must be set for this to be useful.
> + */
> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
> + unsigned int last_ping_ms)
> +{
> + struct watchdog_core_data *wd_data;
> + ktime_t now;
> +
> + if (!wdd)
> + return -EINVAL;
> +
> + wd_data = wdd->wd_data;
> +
> + now = ktime_get();
> +
> + wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));
> +
> + return __watchdog_ping(wdd);
> +}
> +EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);
> +
> /*
> * watchdog_dev_init: init dev part of watchdog core
> *
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 1464ce6ffa31..9b19e6bb68b5 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
> extern int watchdog_register_device(struct watchdog_device *);
> extern void watchdog_unregister_device(struct watchdog_device *);
>
> +int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
> +
> /* devres register variant */
> int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);
>
>