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
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 <[email protected]>
---
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
Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.
Signed-off-by: Liu Xinpeng <[email protected]>
Reviewed-by: Tzung-Bi Shih <[email protected]>
---
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
On 4/26/22 01:27, [email protected] 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 <[email protected]>
> > Reviewed-by: Tzung-Bi Shih <[email protected]>
> 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
On 4/26/22 01:17, [email protected] 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 <[email protected]>
> > Reviewed-by: Tzung-Bi Shih <[email protected]>
> > ---
> > 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