[PATCH v4 0/4] Some impovements about watchdog

[PATCH v4 0/4] Some impovements about watchdog

[Liu Xinpeng]

Changelog:
v1->v2 Update the commit messages
v2->v3 - Add the context about why using watchdog_timeout_invalid.
       - Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code for
       iTCO watchdog.
v3->v4 - For patch 1, update commit message, rename WDAT_TIMEOUT_MIN 
       to WDAT_MIN_TIMEOUT, keeps consistent with WDAT_DEFAULT_TIMEOUT.
       - For patch 4, iTCO_wdt_suspend_noirq and iTCO_wdt_resume_noirq
       are possible unused, so keep "ifdef CONFIG_PM_SLEEP ... #endif".

       Thanks Guenter Roeck and Tzung-Bi Shih's suggestions.

Liu Xinpeng (4):
  watchdog: wdat_wdg: Using the existed function to check parameter
    timeout
  watchdog: wdat_wdg: Stop watchdog when rebooting the system
  watchdog: wdat_wdg: Stop watchdog when uninstalling module
  watchdog: iTCO_wdg: Make code more clearly with macro definition

 drivers/watchdog/iTCO_wdt.c | 12 +++---------
 drivers/watchdog/wdat_wdt.c |  7 +++++--
 2 files changed, 8 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH v4 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Liu Xinpeng]

Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout
is not limited by it. The limit check in this driver therefore doesn't make
much sense. Similar, the watchdog core ensures that minimum timeout limits
are met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid()
makes more sense because it takes this into account.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/watchdog/wdat_wdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 195c8c004b69..9db01d165310 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 #define WDAT_DEFAULT_TIMEOUT	30
+#define WDAT_MIN_TIMEOUT     1
 
 static int timeout = WDAT_DEFAULT_TIMEOUT;
 module_param(timeout, int, 0);
@@ -344,6 +345,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	wdat->period = tbl->timer_period;
 	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
 	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
+	wdat->wdd.min_timeout = WDAT_MIN_TIMEOUT;
 	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
 	wdat->wdd.info = &wdat_wdt_info;
 	wdat->wdd.ops = &wdat_wdt_ops;
@@ -450,8 +452,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	 * watchdog properly after it has opened the device. In some cases
 	 * the BIOS default is too short and causes immediate reboot.
 	 */
-	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
-	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
+	if (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
 		dev_warn(dev, "Invalid timeout %d given, using %d\n",
 			 timeout, WDAT_DEFAULT_TIMEOUT);
 		timeout = WDAT_DEFAULT_TIMEOUT;
-- 
2.23.0

[PATCH v4 2/4] watchdog: wdat_wdg: Stop watchdog when rebooting the system

[Liu Xinpeng]

Executing reboot command several times on the machine "Dell
PowerEdge R740", UEFI security detection stopped machine
with the following prompt:

UEFI0082: The system was reset due to a timeout from the watchdog
timer. Check the System Event Log (SEL) or crash dumps from
Operating Sysstem to identify the source that triggered the
watchdog timer reset. Update the firmware or driver for the
identified device.

iDRAC has warning event: "The watchdog timer reset the system".

This patch fixes this issue by adding the reboot notifier.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
---
 drivers/watchdog/wdat_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 9db01d165310..0ef2b918364a 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -463,6 +463,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
+	watchdog_stop_on_reboot(&wdat->wdd);
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
 
-- 
2.23.0

[PATCH v4 3/4] watchdog: wdat_wdg: Stop watchdog when uninstalling module

[Liu Xinpeng]

Test shows that wachdog still reboots machine after the module
is removed. Use watchdog_stop_on_unregister to stop the watchdog
on removing.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
---
 drivers/watchdog/wdat_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 0ef2b918364a..6732d7fc4f94 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -464,6 +464,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
 	watchdog_stop_on_reboot(&wdat->wdd);
+	watchdog_stop_on_unregister(&wdat->wdd);
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
 
-- 
2.23.0

[PATCH v4 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Liu Xinpeng]

Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/watchdog/iTCO_wdt.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3f2f4343644f..8d24cf4f4ca3 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -635,22 +635,18 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
 
 	return 0;
 }
+#endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops iTCO_wdt_pm = {
-	.suspend_noirq = iTCO_wdt_suspend_noirq,
-	.resume_noirq = iTCO_wdt_resume_noirq,
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
+				      iTCO_wdt_resume_noirq)
 };
 
-#define ITCO_WDT_PM_OPS	(&iTCO_wdt_pm)
-#else
-#define ITCO_WDT_PM_OPS	NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static struct platform_driver iTCO_wdt_driver = {
 	.probe          = iTCO_wdt_probe,
 	.driver         = {
 		.name   = DRV_NAME,
-		.pm     = ITCO_WDT_PM_OPS,
+		.pm     = &iTCO_wdt_pm,
 	},
 };
 
-- 
2.23.0

Re: [PATCH v4 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Guenter Roeck]

On 4/26/22 00:10, Liu Xinpeng wrote:
> Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>   drivers/watchdog/iTCO_wdt.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3f2f4343644f..8d24cf4f4ca3 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -635,22 +635,18 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
>   
>   	return 0;
>   }
> +#endif /* CONFIG_PM_SLEEP */
>   
>   static const struct dev_pm_ops iTCO_wdt_pm = {
> -	.suspend_noirq = iTCO_wdt_suspend_noirq,
> -	.resume_noirq = iTCO_wdt_resume_noirq,
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
> +				      iTCO_wdt_resume_noirq)
>   };
>   
> -#define ITCO_WDT_PM_OPS	(&iTCO_wdt_pm)
> -#else
> -#define ITCO_WDT_PM_OPS	NULL
> -#endif /* CONFIG_PM_SLEEP */
> -
>   static struct platform_driver iTCO_wdt_driver = {
>   	.probe          = iTCO_wdt_probe,
>   	.driver         = {
>   		.name   = DRV_NAME,
> -		.pm     = ITCO_WDT_PM_OPS,
> +		.pm     = &iTCO_wdt_pm,
>   	},
>   };
>   
Sorry, I fail to see how this is an improvement.

Guenter

Re: [PATCH v4 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Guenter Roeck]

On 4/26/22 00:10, Liu Xinpeng wrote:
> Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout

Drop "Context:".

Also, in the subject, it should be "existing".

> is not limited by it. The limit check in this driver therefore doesn't make
> much sense. Similar, the watchdog core ensures that minimum timeout limits
> are met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid()
> makes more sense because it takes this into account.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

And you really should not make up Reviewed-by: tags. This is completely
inappropriate.

Guenter

> ---
>   drivers/watchdog/wdat_wdt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 195c8c004b69..9db01d165310 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
>   #define WDAT_DEFAULT_TIMEOUT	30
> +#define WDAT_MIN_TIMEOUT     1
>   
>   static int timeout = WDAT_DEFAULT_TIMEOUT;
>   module_param(timeout, int, 0);
> @@ -344,6 +345,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>   	wdat->period = tbl->timer_period;
>   	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
>   	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
> +	wdat->wdd.min_timeout = WDAT_MIN_TIMEOUT;
>   	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
>   	wdat->wdd.info = &wdat_wdt_info;
>   	wdat->wdd.ops = &wdat_wdt_ops;
> @@ -450,8 +452,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>   	 * watchdog properly after it has opened the device. In some cases
>   	 * the BIOS default is too short and causes immediate reboot.
>   	 */
> -	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
> -	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
> +	if (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
>   		dev_warn(dev, "Invalid timeout %d given, using %d\n",
>   			 timeout, WDAT_DEFAULT_TIMEOUT);
>   		timeout = WDAT_DEFAULT_TIMEOUT;

Re: [PATCH v4 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Guenter Roeck]

On 4/26/22 00:10, Liu Xinpeng wrote:
> Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Also, Tzung-Bi never sent a Reviewed-by: tag. As with the other patches,
this is completely inappropriate. Really, who gave you that idea ?
You adding such tags without receiving them means we can never trust
your patches again since you are making things up.

Guenter

> ---
>   drivers/watchdog/iTCO_wdt.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3f2f4343644f..8d24cf4f4ca3 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -635,22 +635,18 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
>   
>   	return 0;
>   }
> +#endif /* CONFIG_PM_SLEEP */
>   
>   static const struct dev_pm_ops iTCO_wdt_pm = {
> -	.suspend_noirq = iTCO_wdt_suspend_noirq,
> -	.resume_noirq = iTCO_wdt_resume_noirq,
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
> +				      iTCO_wdt_resume_noirq)
>   };
>   
> -#define ITCO_WDT_PM_OPS	(&iTCO_wdt_pm)
> -#else
> -#define ITCO_WDT_PM_OPS	NULL
> -#endif /* CONFIG_PM_SLEEP */
> -
>   static struct platform_driver iTCO_wdt_driver = {
>   	.probe          = iTCO_wdt_probe,
>   	.driver         = {
>   		.name   = DRV_NAME,
> -		.pm     = ITCO_WDT_PM_OPS,
> +		.pm     = &iTCO_wdt_pm,
>   	},
>   };
>   

Re: [PATCH v4 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Guenter Roeck]

On 4/26/22 01:17, liuxp11@chinatelecom.cn wrote:
>     On 4/26/22 00:10, Liu Xinpeng wrote:
>      > Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.
>      >
>      > Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
>      > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
>      > ---
>      >   drivers/watchdog/iTCO_wdt.c | 12 ++++--------
>      >   1 file changed, 4 insertions(+), 8 deletions(-)
>      >
>      > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>      > index 3f2f4343644f..8d24cf4f4ca3 100644
>      > --- a/drivers/watchdog/iTCO_wdt.c
>      > +++ b/drivers/watchdog/iTCO_wdt.c
>      > @@ -635,22 +635,18 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
>      >
>      >   return 0;
>      >   }
>      > +#endif /* CONFIG_PM_SLEEP */
>      >
>      >   static const struct dev_pm_ops iTCO_wdt_pm = {
>      > - .suspend_noirq = iTCO_wdt_suspend_noirq,
>      > - .resume_noirq = iTCO_wdt_resume_noirq,
>      > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
>      > +       iTCO_wdt_resume_noirq)
>      >   };
>      >
>      > -#define ITCO_WDT_PM_OPS (&iTCO_wdt_pm)
>      > -#else
>      > -#define ITCO_WDT_PM_OPS NULL
>      > -#endif /* CONFIG_PM_SLEEP */
>      > -
>      >   static struct platform_driver iTCO_wdt_driver = {
>      >   .probe          = iTCO_wdt_probe,
>      >   .driver         = {
>      >   .name   = DRV_NAME,
>      > - .pm     = ITCO_WDT_PM_OPS,
>      > + .pm     = &iTCO_wdt_pm,
>      >   },
>      >   };
>      >
>     Sorry, I fail to see how this is an improvement.
> 
>     Guenter
>     --->
>     #define NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>              .suspend_noirq = pm_sleep_ptr(suspend_fn), \
>              .resume_noirq = pm_sleep_ptr(resume_fn), \
>              .freeze_noirq = pm_sleep_ptr(suspend_fn), \
>              .thaw_noirq = pm_sleep_ptr(resume_fn), \
>              .poweroff_noirq = pm_sleep_ptr(suspend_fn), \
>              .restore_noirq = pm_sleep_ptr(resume_fn),
> 
>     NOIRQ_SYSTEM_SLEEP_PM_OPS, defined for CONFIG_PM_SLEEP, will
>       point ->suspend_noirq, ->freeze_noirq and ->poweroff_noirq to the same
>       function. Vice versa happens for ->resume_noirq, ->thaw_noirq and ->restore_noirq.
>       (commit 020af89a41c41fd2c92d0da524968dfaba6269f0)
> 
>     Some power management occasion needs freeze_noirq/thaw_noirq /poweroff_noirq /restore_noirq.
> 

That isn't what the commit description and subject say. Change description
and subject to describe what is actually done, use __maybe_unused instead
of #ifdef as suggested, and do not invent Reviewed-by: tags.

Also, this is an independent change, and should not be sent together
with the other patches.

Guenter

Re: [PATCH v4 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Guenter Roeck]

On 4/26/22 01:27, liuxp11@chinatelecom.cn wrote:
>     On 4/26/22 00:10, Liu Xinpeng wrote:
>      > Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.
>      >
>      > Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
>      > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
>     Also, Tzung-Bi never sent a Reviewed-by: tag. As with the other patches,
>     this is completely inappropriate. Really, who gave you that idea ?
>     You adding such tags without receiving them means we can never trust
>     your patches again since you are making things up.
>     Guenter
>     -->Very sorry about this. I am not clearly when need to add "Reviewed-by: tag",
>     Just expressing thanks.
> 

If you get a Reviewed-by: tag for a patch, you keep it in the next version of
that patch unless there are substantial changes. You _never_ add one yourself.

Guenter

[PATCH v4 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Liu Xinpeng]

Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout
is not limited by it. The limit check in this driver therefore doesn't make
much sense. Similar, the watchdog core ensures that minimum timeout limits
are met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid()
makes more sense because it takes this into account.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/watchdog/wdat_wdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 195c8c004b69..9db01d165310 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 #define WDAT_DEFAULT_TIMEOUT	30
+#define WDAT_MIN_TIMEOUT     1
 
 static int timeout = WDAT_DEFAULT_TIMEOUT;
 module_param(timeout, int, 0);
@@ -344,6 +345,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	wdat->period = tbl->timer_period;
 	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
 	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
+	wdat->wdd.min_timeout = WDAT_MIN_TIMEOUT;
 	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
 	wdat->wdd.info = &wdat_wdt_info;
 	wdat->wdd.ops = &wdat_wdt_ops;
@@ -450,8 +452,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	 * watchdog properly after it has opened the device. In some cases
 	 * the BIOS default is too short and causes immediate reboot.
 	 */
-	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
-	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
+	if (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
 		dev_warn(dev, "Invalid timeout %d given, using %d\n",
 			 timeout, WDAT_DEFAULT_TIMEOUT);
 		timeout = WDAT_DEFAULT_TIMEOUT;
-- 
2.23.0