linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] watchdog: refactor init_timeout and update users
@ 2019-04-16 10:24 Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout Wolfram Sang
                   ` (15 more replies)
  0 siblings, 16 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:24 UTC (permalink / raw)
  To: linux-watchdog; +Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang

When trying to add DT support to the DA9063 WDT, I didn't want to add yet
another error message when watchdog_init_timeout fails. The core could do this
with more detail, and also much more consistent. So, I refactored this routine
and removed error strings from its callers. Note that DA9063 support is not
added here but was sent seperately[1]. This series has been tested using the
renesas_wdt driver on a Renesas Salvator-XS board (R-Car M3N) and build bot is
happy, too.

Changes since V1:

* return -EINVAL if DT supplied a value of '0' [patch 1]
* update kdoc to mention that DT cannot be '0' but module_param can be
  [patch 1]
* add Guenter's Rev-by to all patches except patch 1
* add driver maintainers to CC this time

Looking forward to comments.

Thanks,

   Wolfram

[1] https://patchwork.kernel.org/patch/10899801/

Wolfram Sang (16):
  watchdog: refactor watchdog_init_timeout
  watchdog: add error messages when initializing timeout fails
  watchdog: cadence_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: cadence_wdt: still probe if user supplied timeout is invalid
  watchdog: ebc-c384_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  watchdog: i6300esb: drop warning after calling watchdog_init_timeout
  watchdog: imx_sc_wdt: drop warning after calling watchdog_init_timeout
  watchdog: ni903x_wdt: drop warning after calling watchdog_init_timeout
  watchdog: nic7018_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: renesas_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: sp5100_tco: drop warning after calling watchdog_init_timeout
  watchdog: st_lpc_wdt: drop warning after calling watchdog_init_timeout
  watchdog: stm32_iwdg: drop warning after calling watchdog_init_timeout
  watchdog: xen_wdt: drop warning after calling watchdog_init_timeout
  watchdog: ziirave_wdt: drop warning after calling
    watchdog_init_timeout

 drivers/watchdog/cadence_wdt.c   |  7 +------
 drivers/watchdog/ebc-c384_wdt.c  |  5 +----
 drivers/watchdog/hpwdt.c         |  3 +--
 drivers/watchdog/i6300esb.c      |  5 +----
 drivers/watchdog/imx_sc_wdt.c    |  5 +----
 drivers/watchdog/ni903x_wdt.c    |  4 +---
 drivers/watchdog/nic7018_wdt.c   |  5 +----
 drivers/watchdog/renesas_wdt.c   |  4 +---
 drivers/watchdog/sp5100_tco.c    |  4 +---
 drivers/watchdog/st_lpc_wdt.c    |  4 +---
 drivers/watchdog/stm32_iwdg.c    |  6 +-----
 drivers/watchdog/watchdog_core.c | 39 +++++++++++++++++++++++++--------------
 drivers/watchdog/xen_wdt.c       |  4 +---
 drivers/watchdog/ziirave_wdt.c   |  6 +-----
 14 files changed, 38 insertions(+), 63 deletions(-)

-- 
2.11.0


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 13:23   ` Guenter Roeck
  2019-04-16 10:25 ` [PATCH v2 02/16] watchdog: add error messages when initializing timeout fails Wolfram Sang
                   ` (14 subsequent siblings)
  15 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The function is not easy to read and has a problem: -EINVAL is returned
when the module parameter is invalid but the DT parameter is OK.

Refactor the code to have the same pattern of checks for the module
parameter and DT. Further ones can be easily added in the future if the
need arises. The above mentioned problem is fixed, too.

Some documentation is added to describe the different handlings of '0'
for the module parameter and the DT property.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/watchdog_core.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index eb8fa25f8eb2..21e53cc49977 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -105,9 +105,12 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
  * timeout 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 timeout value).
+ * be the default timeout value). Note that for the module parameter, '0' means
+ * 'use default' while it is an invalid value for the timeout-sec property.
+ * It should simply be dropped if you want to use the default value then.
  *
- * A zero is returned on success and -EINVAL for failure.
+ * A zero is returned on success or -EINVAL if all provided values are out of
+ * bounds.
  */
 int watchdog_init_timeout(struct watchdog_device *wdd,
 				unsigned int timeout_parm, struct device *dev)
@@ -117,22 +120,24 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 
 	watchdog_check_min_max_timeout(wdd);
 
-	/* try to get the timeout module parameter first */
-	if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
-		wdd->timeout = timeout_parm;
-		return ret;
-	}
-	if (timeout_parm)
+	/* check the driver supplied value (likely a module parameter) first */
+	if (timeout_parm) {
+		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
+			wdd->timeout = timeout_parm;
+			return 0;
+		}
 		ret = -EINVAL;
+	}
 
 	/* try to get the timeout_sec property */
-	if (dev == NULL || dev->of_node == NULL)
-		return ret;
-	of_property_read_u32(dev->of_node, "timeout-sec", &t);
-	if (!watchdog_timeout_invalid(wdd, t) && t)
-		wdd->timeout = t;
-	else
+	if (dev && dev->of_node &&
+	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
+		if (t && !watchdog_timeout_invalid(wdd, t)) {
+			wdd->timeout = t;
+			return 0;
+		}
 		ret = -EINVAL;
+	}
 
 	return ret;
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 02/16] watchdog: add error messages when initializing timeout fails
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 03/16] watchdog: cadence_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

This not only removes boilerplate code from watchdog drivers, it can
also be more specific which of the supplied value actually fails. Also,
the loglevel becomes now consistent across drivers.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/watchdog_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 21e53cc49977..cd3ca6b366ef 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -126,6 +126,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = timeout_parm;
 			return 0;
 		}
+		dev_err(dev, "driver supplied timeout (%u) out of range\n",
+			timeout_parm);
 		ret = -EINVAL;
 	}
 
@@ -136,9 +138,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = t;
 			return 0;
 		}
+		dev_err(dev, "DT supplied timeout (%u) out of range\n", t);
 		ret = -EINVAL;
 	}
 
+	if (ret < 0 && wdd->timeout)
+		dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 03/16] watchdog: cadence_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 02/16] watchdog: add error messages when initializing timeout fails Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 04/16] watchdog: cadence_wdt: still probe if user supplied timeout is invalid Wolfram Sang
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/cadence_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index c5051907df00..e6eaeaf3dfb1 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -329,10 +329,8 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	cdns_wdt_device->parent = dev;
 
 	ret = watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
-	if (ret) {
-		dev_err(dev, "unable to set timeout value\n");
+	if (ret)
 		return ret;
-	}
 
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
 	watchdog_stop_on_reboot(cdns_wdt_device);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 04/16] watchdog: cadence_wdt: still probe if user supplied timeout is invalid
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (2 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 03/16] watchdog: cadence_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

We have a default timeout value in the driver which we will fall back to
if the user supplied values are out of bounce.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/cadence_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index e6eaeaf3dfb1..a22f2d431a35 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -328,10 +328,7 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	/* Initialize the members of cdns_wdt structure */
 	cdns_wdt_device->parent = dev;
 
-	ret = watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
-	if (ret)
-		return ret;
-
+	watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
 	watchdog_stop_on_reboot(cdns_wdt_device);
 	watchdog_set_drvdata(cdns_wdt_device, wdt);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (3 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 04/16] watchdog: cadence_wdt: still probe if user supplied timeout is invalid Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:28   ` William Breathitt Gray
  2019-04-16 10:25 ` [PATCH v2 06/16] watchdog: hpwdt: " Wolfram Sang
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang,
	William Breathitt Gray, Wim Van Sebroeck, linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ebc-c384_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
index 4c4c8ce78021..c176f59fea28 100644
--- a/drivers/watchdog/ebc-c384_wdt.c
+++ b/drivers/watchdog/ebc-c384_wdt.c
@@ -117,10 +117,7 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
 	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
 
 	watchdog_set_nowayout(wdd, nowayout);
-
-	if (watchdog_init_timeout(wdd, timeout, dev))
-		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
-			timeout, WATCHDOG_TIMEOUT);
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	return devm_watchdog_register_device(dev, wdd);
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (4 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 20:34   ` Jerry Hoemann
  2019-04-16 10:25 ` [PATCH v2 07/16] watchdog: i6300esb: " Wolfram Sang
                   ` (9 subsequent siblings)
  15 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Jerry Hoemann,
	Wim Van Sebroeck, linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/hpwdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ef30c7e9728d..db1bf6f546ae 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
 		goto error_init_nmi_decoding;
 
 	watchdog_set_nowayout(&hpwdt_dev, nowayout);
-	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
-		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
+	watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
 
 	if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
 		dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout to zero\n");
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 07/16] watchdog: i6300esb: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (5 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 06/16] watchdog: hpwdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 08/16] watchdog: imx_sc_wdt: " Wolfram Sang
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/i6300esb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index e312a2aeecad..17941c03996b 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -311,10 +311,7 @@ static int esb_probe(struct pci_dev *pdev,
 	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
 	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
 	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
-	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
-		dev_info(&pdev->dev,
-			"heartbeat value must be " ESB_HEARTBEAT_RANGE
-			", using %u\n", edev->wdd.timeout);
+	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
 	watchdog_set_nowayout(&edev->wdd, nowayout);
 	watchdog_stop_on_reboot(&edev->wdd);
 	watchdog_stop_on_unregister(&edev->wdd);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 08/16] watchdog: imx_sc_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (6 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 07/16] watchdog: i6300esb: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 09/16] watchdog: ni903x_wdt: " Wolfram Sang
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel, linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/imx_sc_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
index 86c2722f2a09..6dc24ceb1b2c 100644
--- a/drivers/watchdog/imx_sc_wdt.c
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	imx_sc_wdd->parent = &pdev->dev;
 	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
 
-	ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev, "Failed to set timeout value, using default\n");
-
+	watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev);
 	watchdog_stop_on_reboot(imx_sc_wdd);
 	watchdog_stop_on_unregister(imx_sc_wdd);
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 09/16] watchdog: ni903x_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (7 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 08/16] watchdog: imx_sc_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 10/16] watchdog: nic7018_wdt: " Wolfram Sang
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ni903x_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/ni903x_wdt.c b/drivers/watchdog/ni903x_wdt.c
index dc67742e9018..fbc1df86c6cc 100644
--- a/drivers/watchdog/ni903x_wdt.c
+++ b/drivers/watchdog/ni903x_wdt.c
@@ -217,9 +217,7 @@ static int ni903x_acpi_add(struct acpi_device *device)
 	wdd->parent = dev;
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, nowayout);
-	ret = watchdog_init_timeout(wdd, timeout, dev);
-	if (ret)
-		dev_err(dev, "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	ret = watchdog_register_device(wdd);
 	if (ret) {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 10/16] watchdog: nic7018_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (8 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 09/16] watchdog: ni903x_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 11/16] watchdog: renesas_wdt: " Wolfram Sang
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/nic7018_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/nic7018_wdt.c b/drivers/watchdog/nic7018_wdt.c
index dcd265685837..82843abe38f8 100644
--- a/drivers/watchdog/nic7018_wdt.c
+++ b/drivers/watchdog/nic7018_wdt.c
@@ -211,10 +211,7 @@ static int nic7018_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, nowayout);
-
-	ret = watchdog_init_timeout(wdd, timeout, dev);
-	if (ret)
-		dev_warn(dev, "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	/* Unlock WDT register */
 	outb(UNLOCK, wdt->io_base + WDT_REG_LOCK);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 11/16] watchdog: renesas_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (9 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 10/16] watchdog: nic7018_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 12/16] watchdog: sp5100_tco: " Wolfram Sang
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 8d1086b45c94..37d757288b22 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -236,9 +236,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_unregister(&priv->wdev);
 
 	/* This overrides the default timeout only if DT configuration was found */
-	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev, "Specified timeout value invalid, using default\n");
+	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
 
 	if (csra & RWTCSRA_TME) {
 		/* Ensure properly initialized dividers */
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 12/16] watchdog: sp5100_tco: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (10 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 11/16] watchdog: renesas_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 13/16] watchdog: st_lpc_wdt: " Wolfram Sang
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/sp5100_tco.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 41aaae2d5287..553735b256e2 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -395,9 +395,7 @@ static int sp5100_tco_probe(struct platform_device *pdev)
 	wdd->min_timeout = 1;
 	wdd->max_timeout = 0xffff;
 
-	if (watchdog_init_timeout(wdd, heartbeat, NULL))
-		dev_info(dev, "timeout value invalid, using %d\n",
-			 wdd->timeout);
+	watchdog_init_timeout(wdd, heartbeat, NULL);
 	watchdog_set_nowayout(wdd, nowayout);
 	watchdog_stop_on_reboot(wdd);
 	watchdog_stop_on_unregister(wdd);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 13/16] watchdog: st_lpc_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (11 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 12/16] watchdog: sp5100_tco: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 14/16] watchdog: stm32_iwdg: " Wolfram Sang
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Patrice Chotard,
	Wim Van Sebroeck, linux-arm-kernel, linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/st_lpc_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/st_lpc_wdt.c b/drivers/watchdog/st_lpc_wdt.c
index 9a5ed95c3403..7a90184eb950 100644
--- a/drivers/watchdog/st_lpc_wdt.c
+++ b/drivers/watchdog/st_lpc_wdt.c
@@ -224,10 +224,8 @@ static int st_wdog_probe(struct platform_device *pdev)
 
 	/* Init Watchdog timeout with value in DT */
 	ret = watchdog_init_timeout(&st_wdog_dev, 0, dev);
-	if (ret) {
-		dev_err(dev, "Unable to initialise watchdog timeout\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = devm_watchdog_register_device(dev, &st_wdog_dev);
 	if (ret) {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 14/16] watchdog: stm32_iwdg: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (12 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 13/16] watchdog: st_lpc_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 15/16] watchdog: xen_wdt: " Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 16/16] watchdog: ziirave_wdt: " Wolfram Sang
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	Maxime Coquelin, Alexandre Torgue, linux-stm32, linux-arm-kernel,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/stm32_iwdg.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index 309563e002b8..50ce762bddb2 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -235,11 +235,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
-
-	ret = watchdog_init_timeout(wdd, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev,
-			 "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, 0, &pdev->dev);
 
 	ret = watchdog_register_device(wdd);
 	if (ret) {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 15/16] watchdog: xen_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (13 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 14/16] watchdog: stm32_iwdg: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  2019-04-16 10:25 ` [PATCH v2 16/16] watchdog: ziirave_wdt: " Wolfram Sang
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/xen_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index f1c016d015b3..d54da2e0f2e1 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -135,9 +135,7 @@ static int xen_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
-		dev_info(&pdev->dev, "timeout value invalid, using %d\n",
-			xen_wdt_dev.timeout);
+	watchdog_init_timeout(&xen_wdt_dev, timeout, NULL);
 	watchdog_set_nowayout(&xen_wdt_dev, nowayout);
 	watchdog_stop_on_reboot(&xen_wdt_dev);
 	watchdog_stop_on_unregister(&xen_wdt_dev);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 16/16] watchdog: ziirave_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
                   ` (14 preceding siblings ...)
  2019-04-16 10:25 ` [PATCH v2 15/16] watchdog: xen_wdt: " Wolfram Sang
@ 2019-04-16 10:25 ` Wolfram Sang
  15 siblings, 0 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 10:25 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux-renesas-soc, Guenter Roeck, Wolfram Sang, Wim Van Sebroeck,
	linux-kernel

The core will print out details now.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ziirave_wdt.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index d3594aa3a374..43e6b575c32c 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -658,11 +658,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	w_priv->wdd.parent = &client->dev;
 	w_priv->wdd.groups = ziirave_wdt_groups;
 
-	ret = watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
-	if (ret) {
-		dev_info(&client->dev,
-			 "Unable to select timeout value, using default\n");
-	}
+	watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
 
 	/*
 	 * The default value set in the watchdog should be perfectly valid, so
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:25 ` [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
@ 2019-04-16 10:28   ` William Breathitt Gray
  0 siblings, 0 replies; 33+ messages in thread
From: William Breathitt Gray @ 2019-04-16 10:28 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-watchdog, linux-renesas-soc, Guenter Roeck,
	Wim Van Sebroeck, linux-kernel

On Tue, Apr 16, 2019 at 12:25:04PM +0200, Wolfram Sang wrote:
> The core will print out details now.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/watchdog/ebc-c384_wdt.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
> index 4c4c8ce78021..c176f59fea28 100644
> --- a/drivers/watchdog/ebc-c384_wdt.c
> +++ b/drivers/watchdog/ebc-c384_wdt.c
> @@ -117,10 +117,7 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>  	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
>  
>  	watchdog_set_nowayout(wdd, nowayout);
> -
> -	if (watchdog_init_timeout(wdd, timeout, dev))
> -		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
> -			timeout, WATCHDOG_TIMEOUT);
> +	watchdog_init_timeout(wdd, timeout, dev);
>  
>  	return devm_watchdog_register_device(dev, wdd);
>  }
> -- 
> 2.11.0

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout
  2019-04-16 10:25 ` [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout Wolfram Sang
@ 2019-04-16 13:23   ` Guenter Roeck
  0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2019-04-16 13:23 UTC (permalink / raw)
  To: Wolfram Sang, linux-watchdog
  Cc: linux-renesas-soc, Wim Van Sebroeck, linux-kernel

On 4/16/19 3:25 AM, Wolfram Sang wrote:
> The function is not easy to read and has a problem: -EINVAL is returned
> when the module parameter is invalid but the DT parameter is OK.
> 
> Refactor the code to have the same pattern of checks for the module
> parameter and DT. Further ones can be easily added in the future if the
> need arises. The above mentioned problem is fixed, too.
> 
> Some documentation is added to describe the different handlings of '0'
> for the module parameter and the DT property.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/watchdog_core.c | 33 +++++++++++++++++++--------------
>   1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index eb8fa25f8eb2..21e53cc49977 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -105,9 +105,12 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>    * timeout 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 timeout value).
> + * be the default timeout value). Note that for the module parameter, '0' means
> + * 'use default' while it is an invalid value for the timeout-sec property.
> + * It should simply be dropped if you want to use the default value then.
>    *
> - * A zero is returned on success and -EINVAL for failure.
> + * A zero is returned on success or -EINVAL if all provided values are out of
> + * bounds.
>    */
>   int watchdog_init_timeout(struct watchdog_device *wdd,
>   				unsigned int timeout_parm, struct device *dev)
> @@ -117,22 +120,24 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>   
>   	watchdog_check_min_max_timeout(wdd);
>   
> -	/* try to get the timeout module parameter first */
> -	if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
> -		wdd->timeout = timeout_parm;
> -		return ret;
> -	}
> -	if (timeout_parm)
> +	/* check the driver supplied value (likely a module parameter) first */
> +	if (timeout_parm) {
> +		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
> +			wdd->timeout = timeout_parm;
> +			return 0;
> +		}
>   		ret = -EINVAL;
> +	}
>   
>   	/* try to get the timeout_sec property */
> -	if (dev == NULL || dev->of_node == NULL)
> -		return ret;
> -	of_property_read_u32(dev->of_node, "timeout-sec", &t);
> -	if (!watchdog_timeout_invalid(wdd, t) && t)
> -		wdd->timeout = t;
> -	else
> +	if (dev && dev->of_node &&
> +	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0) {
> +		if (t && !watchdog_timeout_invalid(wdd, t)) {
> +			wdd->timeout = t;
> +			return 0;
> +		}
>   		ret = -EINVAL;
> +	}
>   
>   	return ret;
>   }
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 10:25 ` [PATCH v2 06/16] watchdog: hpwdt: " Wolfram Sang
@ 2019-04-16 20:34   ` Jerry Hoemann
  2019-04-16 20:48     ` Guenter Roeck
  2019-04-16 20:50     ` Wolfram Sang
  0 siblings, 2 replies; 33+ messages in thread
From: Jerry Hoemann @ 2019-04-16 20:34 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-watchdog, linux-renesas-soc, Guenter Roeck,
	Wim Van Sebroeck, linux-kernel

On Tue, Apr 16, 2019 at 12:25:05PM +0200, Wolfram Sang wrote:
> The core will print out details now.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/watchdog/hpwdt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index ef30c7e9728d..db1bf6f546ae 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
>  		goto error_init_nmi_decoding;
>  
>  	watchdog_set_nowayout(&hpwdt_dev, nowayout);
> -	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> -		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
> +	watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);

I applied patches 1,2 & 6 in testing.

Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout().

The second patch in this series is using "dev" as input to dev_err and dev_warn.

This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin:


[   80.848160] (NULL device *): driver supplied timeout (4294967295) out of range
[   80.855429] (NULL device *): falling back to default timeout (30)


if the call in hpwdt driver is changed to:

	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev))


We see the message like we'd desire:

[ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range
[ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30)



watchdog_init_timeout() uses dev to "try to get the timeout_sec property"

I am not familiar with this part of the core and what effect having the hpwdt
driver pass in dev to watchdog_init_timeout would have.  (I.e. is the
change safe?)

Guenter, can you help on this question?

Note, hpwdt isn't the only watch dog device that is passing NULL to
watchdog_init_timeout.

Thanks,


Jerry


>  
>  	if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
>  		dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout to zero\n");
> -- 
> 2.11.0

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 20:34   ` Jerry Hoemann
@ 2019-04-16 20:48     ` Guenter Roeck
  2019-04-16 20:55       ` Wolfram Sang
  2019-04-16 20:50     ` Wolfram Sang
  1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2019-04-16 20:48 UTC (permalink / raw)
  To: Jerry Hoemann
  Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Tue, Apr 16, 2019 at 02:34:31PM -0600, Jerry Hoemann wrote:
> On Tue, Apr 16, 2019 at 12:25:05PM +0200, Wolfram Sang wrote:
> > The core will print out details now.
> > 
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  drivers/watchdog/hpwdt.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index ef30c7e9728d..db1bf6f546ae 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
> >  		goto error_init_nmi_decoding;
> >  
> >  	watchdog_set_nowayout(&hpwdt_dev, nowayout);
> > -	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> > -		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
> > +	watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
> 
> I applied patches 1,2 & 6 in testing.
> 
> Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout().
> 
> The second patch in this series is using "dev" as input to dev_err and dev_warn.
> 
> This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin:
> 
> 
> [   80.848160] (NULL device *): driver supplied timeout (4294967295) out of range
> [   80.855429] (NULL device *): falling back to default timeout (30)
> 
Good find. Thanks a lot for testing!

We'll have to address this. Wolfram - it looks like we'll need
separate error message handling for situations where dev is NULL.
We may have to leave it up to the parent after all to display
a message in that case (since we do want to see the driver name).

> 
> if the call in hpwdt driver is changed to:
> 
> 	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev))
> 
> 
> We see the message like we'd desire:
> 
> [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range
> [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30)
> 
> 
> 
> watchdog_init_timeout() uses dev to "try to get the timeout_sec property"
> 
> I am not familiar with this part of the core and what effect having the hpwdt
> driver pass in dev to watchdog_init_timeout would have.  (I.e. is the
> change safe?)
> 
Yes, in general it is safe. watchdog_init_timeout() only uses dev to extract
a timeout value from devicetree (and now to display error messages).

> Guenter, can you help on this question?
> 
> Note, hpwdt isn't the only watch dog device that is passing NULL to
> watchdog_init_timeout.
> 
That is indeed a problem: the pointer will be NULL if there is no parent
device (such as in softdog.c). Otherwise it should never be NULL.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 20:34   ` Jerry Hoemann
  2019-04-16 20:48     ` Guenter Roeck
@ 2019-04-16 20:50     ` Wolfram Sang
  2019-04-16 21:14       ` Guenter Roeck
  1 sibling, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 20:50 UTC (permalink / raw)
  To: Jerry Hoemann
  Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc, Guenter Roeck,
	Wim Van Sebroeck, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

Hi Jerry,

> I applied patches 1,2 & 6 in testing.
> 
> Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout().
> 
> The second patch in this series is using "dev" as input to dev_err and dev_warn.
> 
> This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin:
> 
> 
> [   80.848160] (NULL device *): driver supplied timeout (4294967295) out of range
> [   80.855429] (NULL device *): falling back to default timeout (30)

Thank you for this report. Yes, using 'dev' blindly is a bug.

> if the call in hpwdt driver is changed to:
> 
> 	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev))
> 
> 
> We see the message like we'd desire:
> 
> [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range
> [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30)

The above observation makes sense, but I think we should fix the core
code and not the hpwdt driver. My suggestion would be to add something
like this to watchdog_init_timeout():

	struct device *err_dev = dev ?: wdd->parent;

And then use err_dev for all the printing. Guenter?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 20:48     ` Guenter Roeck
@ 2019-04-16 20:55       ` Wolfram Sang
  2019-04-16 21:20         ` Guenter Roeck
  0 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 20:55 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 275 bytes --]


> That is indeed a problem: the pointer will be NULL if there is no parent
> device (such as in softdog.c). Otherwise it should never be NULL.

Okay, this spoils my err_dev solution. So, we probably go this route
then:

	pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id);

?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 20:50     ` Wolfram Sang
@ 2019-04-16 21:14       ` Guenter Roeck
  0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2019-04-16 21:14 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Tue, Apr 16, 2019 at 10:50:03PM +0200, Wolfram Sang wrote:
> Hi Jerry,
> 
> > I applied patches 1,2 & 6 in testing.
> > 
> > Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout().
> > 
> > The second patch in this series is using "dev" as input to dev_err and dev_warn.
> > 
> > This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin:
> > 
> > 
> > [   80.848160] (NULL device *): driver supplied timeout (4294967295) out of range
> > [   80.855429] (NULL device *): falling back to default timeout (30)
> 
> Thank you for this report. Yes, using 'dev' blindly is a bug.
> 
> > if the call in hpwdt driver is changed to:
> > 
> > 	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev))
> > 
> > 
> > We see the message like we'd desire:
> > 
> > [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range
> > [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30)
> 
> The above observation makes sense, but I think we should fix the core
> code and not the hpwdt driver. My suggestion would be to add something
> like this to watchdog_init_timeout():
> 
> 	struct device *err_dev = dev ?: wdd->parent;
> 
> And then use err_dev for all the printing. Guenter?
> 

That is a good idea, and we should do that. Unfortunately, wdd->parent can also
be NULL, either because there is no parent device (e.g. softdog.c), or because
the driver author forgot to set ->parent. So we still need a fallback.
Does it make sense to print watchdog_info->identity if both dev and wdd->parent
are NULL ?

Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 20:55       ` Wolfram Sang
@ 2019-04-16 21:20         ` Guenter Roeck
  2019-04-16 22:17           ` Wolfram Sang
  0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2019-04-16 21:20 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote:
> 
> > That is indeed a problem: the pointer will be NULL if there is no parent
> > device (such as in softdog.c). Otherwise it should never be NULL.
> 
> Okay, this spoils my err_dev solution. So, we probably go this route
> then:
> 
> 	pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id);
> 

I don't like it because it doesn't show the driver name, and watchdog%d
can change with each reboot. How about something like this ?

static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err)
{
	if (wdd->parent)
		dev_err(wdd->parent, "%s: %d\n", text, err);
	else
		pr_err("%s: %s: %d\n", wdd->info->identity, text, err);
}

We could then use the same mechanism to generate error messages for
watchdog_register_device().

Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 21:20         ` Guenter Roeck
@ 2019-04-16 22:17           ` Wolfram Sang
  2019-04-16 22:38             ` Guenter Roeck
  0 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-16 22:17 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On Tue, Apr 16, 2019 at 02:20:46PM -0700, Guenter Roeck wrote:
> On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote:
> > 
> > > That is indeed a problem: the pointer will be NULL if there is no parent
> > > device (such as in softdog.c). Otherwise it should never be NULL.
> > 
> > Okay, this spoils my err_dev solution. So, we probably go this route
> > then:
> > 
> > 	pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id);
> > 
> 
> I don't like it because it doesn't show the driver name, and watchdog%d
> can change with each reboot. How about something like this ?
> 
> static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err)
> {
> 	if (wdd->parent)
> 		dev_err(wdd->parent, "%s: %d\n", text, err);
> 	else
> 		pr_err("%s: %s: %d\n", wdd->info->identity, text, err);
> }
> 
> We could then use the same mechanism to generate error messages for
> watchdog_register_device().

'text' is a constant string then. Supporting a format string will make
this much more complicated. Yet, printing out the wrong timeout is
useful, I think.

What about:

	dev_str = wdd->parent ? dev_name(wdd->parent) : wdd->info->identity;
	pr_<errlvl>("%s: <errstr>\n", dev_str, ...);

?

This can be easily copied for watchdog_register_device, not much
overhead there.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 22:17           ` Wolfram Sang
@ 2019-04-16 22:38             ` Guenter Roeck
  2019-04-17 19:45               ` Wolfram Sang
  0 siblings, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2019-04-16 22:38 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Wed, Apr 17, 2019 at 12:17:02AM +0200, Wolfram Sang wrote:
> On Tue, Apr 16, 2019 at 02:20:46PM -0700, Guenter Roeck wrote:
> > On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote:
> > > 
> > > > That is indeed a problem: the pointer will be NULL if there is no parent
> > > > device (such as in softdog.c). Otherwise it should never be NULL.
> > > 
> > > Okay, this spoils my err_dev solution. So, we probably go this route
> > > then:
> > > 
> > > 	pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id);
> > > 
> > 
> > I don't like it because it doesn't show the driver name, and watchdog%d
> > can change with each reboot. How about something like this ?
> > 
> > static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err)
> > {
> > 	if (wdd->parent)
> > 		dev_err(wdd->parent, "%s: %d\n", text, err);
> > 	else
> > 		pr_err("%s: %s: %d\n", wdd->info->identity, text, err);
> > }
> > 
> > We could then use the same mechanism to generate error messages for
> > watchdog_register_device().
> 
> 'text' is a constant string then. Supporting a format string will make
> this much more complicated. Yet, printing out the wrong timeout is
> useful, I think.
> 
> What about:
> 
> 	dev_str = wdd->parent ? dev_name(wdd->parent) : wdd->info->identity;
> 	pr_<errlvl>("%s: <errstr>\n", dev_str, ...);
> 
Yes, that works as well. Note that it will actually print something like
"watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c.
I guess that should be ok.

Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-16 22:38             ` Guenter Roeck
@ 2019-04-17 19:45               ` Wolfram Sang
  2019-04-17 20:17                 ` Guenter Roeck
  2019-04-24  8:38                 ` Geert Uytterhoeven
  0 siblings, 2 replies; 33+ messages in thread
From: Wolfram Sang @ 2019-04-17 19:45 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]


> Yes, that works as well. Note that it will actually print something like
> "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c.
> I guess that should be ok.

I have the following diff applied on top of patch 2. Works with and
without a parent device. I am not super happy casting 'identity' but
since its u8-type is exported to userspace, I think we can't avoid it.
Guenter, is this cast safe? Here is the diff:

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cd3ca6b366ef..62be9e52a4de 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
 int watchdog_init_timeout(struct watchdog_device *wdd,
 				unsigned int timeout_parm, struct device *dev)
 {
+	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
+			      (const char *)wdd->info->identity;
 	unsigned int t = 0;
 	int ret = 0;
 
@@ -126,8 +128,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = timeout_parm;
 			return 0;
 		}
-		dev_err(dev, "driver supplied timeout (%u) out of range\n",
-			timeout_parm);
+		pr_err("%s: driver supplied timeout (%u) out of range\n",
+			dev_str, timeout_parm);
 		ret = -EINVAL;
 	}
 
@@ -138,12 +140,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = t;
 			return 0;
 		}
-		dev_err(dev, "DT supplied timeout (%u) out of range\n", t);
+		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
 		ret = -EINVAL;
 	}
 
 	if (ret < 0 && wdd->timeout)
-		dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout);
+		pr_warn("%s: falling back to default timeout (%u)\n", dev_str,
+			wdd->timeout);
 
 	return ret;
 }


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-17 19:45               ` Wolfram Sang
@ 2019-04-17 20:17                 ` Guenter Roeck
  2019-04-17 21:18                   ` Wolfram Sang
  2019-04-24  8:38                 ` Geert Uytterhoeven
  1 sibling, 1 reply; 33+ messages in thread
From: Guenter Roeck @ 2019-04-17 20:17 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Wed, Apr 17, 2019 at 09:45:28PM +0200, Wolfram Sang wrote:
> 
> > Yes, that works as well. Note that it will actually print something like
> > "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c.
> > I guess that should be ok.
> 
> I have the following diff applied on top of patch 2. Works with and
> without a parent device. I am not super happy casting 'identity' but
> since its u8-type is exported to userspace, I think we can't avoid it.
> Guenter, is this cast safe? Here is the diff:
> 
I would think it is safe, but question is if it is it needed - ie do you see
a warning if it isn't there ? Presumably yes; if so, let's just do it.

Thanks,
Guenter

> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cd3ca6b366ef..62be9e52a4de 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>  int watchdog_init_timeout(struct watchdog_device *wdd,
>  				unsigned int timeout_parm, struct device *dev)
>  {
> +	const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
> +			      (const char *)wdd->info->identity;
>  	unsigned int t = 0;
>  	int ret = 0;
>  
> @@ -126,8 +128,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  			wdd->timeout = timeout_parm;
>  			return 0;
>  		}
> -		dev_err(dev, "driver supplied timeout (%u) out of range\n",
> -			timeout_parm);
> +		pr_err("%s: driver supplied timeout (%u) out of range\n",
> +			dev_str, timeout_parm);
>  		ret = -EINVAL;
>  	}
>  
> @@ -138,12 +140,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  			wdd->timeout = t;
>  			return 0;
>  		}
> -		dev_err(dev, "DT supplied timeout (%u) out of range\n", t);
> +		pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t);
>  		ret = -EINVAL;
>  	}
>  
>  	if (ret < 0 && wdd->timeout)
> -		dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout);
> +		pr_warn("%s: falling back to default timeout (%u)\n", dev_str,
> +			wdd->timeout);
>  
>  	return ret;
>  }
> 



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-17 20:17                 ` Guenter Roeck
@ 2019-04-17 21:18                   ` Wolfram Sang
  2019-04-17 22:00                     ` Guenter Roeck
  0 siblings, 1 reply; 33+ messages in thread
From: Wolfram Sang @ 2019-04-17 21:18 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]


> > I have the following diff applied on top of patch 2. Works with and
> > without a parent device. I am not super happy casting 'identity' but
> > since its u8-type is exported to userspace, I think we can't avoid it.
> > Guenter, is this cast safe? Here is the diff:
> > 
> I would think it is safe, but question is if it is it needed - ie do you see
> a warning if it isn't there ? Presumably yes; if so, let's just do it.

Yes, the compiler rightfully complains otherwise.

Do you prefer resending only patch 2 or the whole series?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-17 21:18                   ` Wolfram Sang
@ 2019-04-17 22:00                     ` Guenter Roeck
  0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2019-04-17 22:00 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, linux-watchdog, linux-renesas-soc,
	Wim Van Sebroeck, linux-kernel

On Wed, Apr 17, 2019 at 11:18:32PM +0200, Wolfram Sang wrote:
> 
> > > I have the following diff applied on top of patch 2. Works with and
> > > without a parent device. I am not super happy casting 'identity' but
> > > since its u8-type is exported to userspace, I think we can't avoid it.
> > > Guenter, is this cast safe? Here is the diff:
> > > 
> > I would think it is safe, but question is if it is it needed - ie do you see
> > a warning if it isn't there ? Presumably yes; if so, let's just do it.
> 
> Yes, the compiler rightfully complains otherwise.
> 
> Do you prefer resending only patch 2 or the whole series?
> 
Please resend the whole series; that simplifies tracking.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-17 19:45               ` Wolfram Sang
  2019-04-17 20:17                 ` Guenter Roeck
@ 2019-04-24  8:38                 ` Geert Uytterhoeven
  2019-04-24 12:57                   ` Guenter Roeck
  1 sibling, 1 reply; 33+ messages in thread
From: Geert Uytterhoeven @ 2019-04-24  8:38 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Guenter Roeck, Jerry Hoemann, Wolfram Sang,
	Linux Watchdog Mailing List, Linux-Renesas, Wim Van Sebroeck,
	Linux Kernel Mailing List

Hi Wolfram,

On Wed, Apr 17, 2019 at 9:46 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > Yes, that works as well. Note that it will actually print something like
> > "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c.
> > I guess that should be ok.
>
> I have the following diff applied on top of patch 2. Works with and
> without a parent device. I am not super happy casting 'identity' but
> since its u8-type is exported to userspace, I think we can't avoid it.
> Guenter, is this cast safe? Here is the diff:
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cd3ca6b366ef..62be9e52a4de 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>  int watchdog_init_timeout(struct watchdog_device *wdd,
>                                 unsigned int timeout_parm, struct device *dev)
>  {
> +       const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
> +                             (const char *)wdd->info->identity;

    struct watchdog_info {
            ...
            __u8  identity[32];     /* Identity of the board */
    };

Is identity[] guaranteed to be NUL-terminated?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  2019-04-24  8:38                 ` Geert Uytterhoeven
@ 2019-04-24 12:57                   ` Guenter Roeck
  0 siblings, 0 replies; 33+ messages in thread
From: Guenter Roeck @ 2019-04-24 12:57 UTC (permalink / raw)
  To: Geert Uytterhoeven, Wolfram Sang
  Cc: Jerry Hoemann, Wolfram Sang, Linux Watchdog Mailing List,
	Linux-Renesas, Wim Van Sebroeck, Linux Kernel Mailing List

On 4/24/19 1:38 AM, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Wed, Apr 17, 2019 at 9:46 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>>> Yes, that works as well. Note that it will actually print something like
>>> "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c.
>>> I guess that should be ok.
>>
>> I have the following diff applied on top of patch 2. Works with and
>> without a parent device. I am not super happy casting 'identity' but
>> since its u8-type is exported to userspace, I think we can't avoid it.
>> Guenter, is this cast safe? Here is the diff:
>>
>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>> index cd3ca6b366ef..62be9e52a4de 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>>   int watchdog_init_timeout(struct watchdog_device *wdd,
>>                                  unsigned int timeout_parm, struct device *dev)
>>   {
>> +       const char *dev_str = wdd->parent ? dev_name(wdd->parent) :
>> +                             (const char *)wdd->info->identity;
> 
>      struct watchdog_info {
>              ...
>              __u8  identity[32];     /* Identity of the board */
>      };
> 
> Is identity[] guaranteed to be NUL-terminated?
> 

I would hope so, because we export its contents via sysfs to userspace
with
	return sprintf(buf, "%s\n", wdd->info->identity);

Also, there are already several pr_err() assuming that it is
a string,

Guenter

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2019-04-24 12:57 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16 10:24 [PATCH v2 00/16] watchdog: refactor init_timeout and update users Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 01/16] watchdog: refactor watchdog_init_timeout Wolfram Sang
2019-04-16 13:23   ` Guenter Roeck
2019-04-16 10:25 ` [PATCH v2 02/16] watchdog: add error messages when initializing timeout fails Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 03/16] watchdog: cadence_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 04/16] watchdog: cadence_wdt: still probe if user supplied timeout is invalid Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout Wolfram Sang
2019-04-16 10:28   ` William Breathitt Gray
2019-04-16 10:25 ` [PATCH v2 06/16] watchdog: hpwdt: " Wolfram Sang
2019-04-16 20:34   ` Jerry Hoemann
2019-04-16 20:48     ` Guenter Roeck
2019-04-16 20:55       ` Wolfram Sang
2019-04-16 21:20         ` Guenter Roeck
2019-04-16 22:17           ` Wolfram Sang
2019-04-16 22:38             ` Guenter Roeck
2019-04-17 19:45               ` Wolfram Sang
2019-04-17 20:17                 ` Guenter Roeck
2019-04-17 21:18                   ` Wolfram Sang
2019-04-17 22:00                     ` Guenter Roeck
2019-04-24  8:38                 ` Geert Uytterhoeven
2019-04-24 12:57                   ` Guenter Roeck
2019-04-16 20:50     ` Wolfram Sang
2019-04-16 21:14       ` Guenter Roeck
2019-04-16 10:25 ` [PATCH v2 07/16] watchdog: i6300esb: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 08/16] watchdog: imx_sc_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 09/16] watchdog: ni903x_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 10/16] watchdog: nic7018_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 11/16] watchdog: renesas_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 12/16] watchdog: sp5100_tco: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 13/16] watchdog: st_lpc_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 14/16] watchdog: stm32_iwdg: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 15/16] watchdog: xen_wdt: " Wolfram Sang
2019-04-16 10:25 ` [PATCH v2 16/16] watchdog: ziirave_wdt: " Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).