| Message ID | 20240213070203.489846-1-jerry.hoemann@hpe.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog/hpwdt: Support Suspend and Resume | expand |
On Tue, Feb 13, 2024 at 12:02:03AM -0700, Jerry Hoemann wrote: > Add call backs to support suspend and resume. > That makes me wonder if we should add something like watchdog_stop_on_suspend() to the watchdog core. Almost every watchdog driver supporting suspend/resume repeats the same sequence (except for the debug message). That is a separate question, though. > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > drivers/watchdog/hpwdt.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index 138dc8d8ca3d..6565cfaa8e57 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -378,11 +378,38 @@ static void hpwdt_exit(struct pci_dev *dev) > pci_disable_device(dev); > } > > +static int hpwdt_suspend(struct device *dev) > +{ > + dev_dbg(dev, "Suspend watchdog\n"); > + Doesn't the suspend / resume code already display such debug messages ? > + if (watchdog_active(&hpwdt_dev)) > + hpwdt_stop(); > + > + return 0; > +} > + > +static int hpwdt_resume(struct device *dev) > +{ > + dev_dbg(dev, "Resume watchdog\n"); > + > + if (watchdog_active(&hpwdt_dev)) > + hpwdt_start(&hpwdt_dev); > + > + return 0; > +} > + > +static DEFINE_SIMPLE_DEV_PM_OPS(hpwdt_pm_ops, hpwdt_suspend, hpwdt_resume); That disables / enables the watchdog as part of regular suspend/resume handling, meaning any hang during suspend/resume that happens later will hang the system. Sure you don't want to use SET_LATE_SYSTEM_SLEEP_PM_OPS() instead ? Never mind, though. Your call, obviously. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter
On Tue, Feb 13, 2024 at 08:12:57AM -0800, Guenter Roeck wrote: > On Tue, Feb 13, 2024 at 12:02:03AM -0700, Jerry Hoemann wrote: > > Add call backs to support suspend and resume. > > > > That makes me wonder if we should add something like > watchdog_stop_on_suspend() > to the watchdog core. Almost every watchdog driver supporting > suspend/resume repeats the same sequence (except for the debug > message). That is a separate question, though. I think that is a good idea. > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > drivers/watchdog/hpwdt.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > index 138dc8d8ca3d..6565cfaa8e57 100644 > > --- a/drivers/watchdog/hpwdt.c > > +++ b/drivers/watchdog/hpwdt.c > > @@ -378,11 +378,38 @@ static void hpwdt_exit(struct pci_dev *dev) > > pci_disable_device(dev); > > } > > > > +static int hpwdt_suspend(struct device *dev) > > +{ > > + dev_dbg(dev, "Suspend watchdog\n"); > > + > > Doesn't the suspend / resume code already display such debug messages ? It displays some. I mostly was using to make sure that the callback was being called. I can drop it. > > > + if (watchdog_active(&hpwdt_dev)) > > + hpwdt_stop(); > > + > > + return 0; > > +} > > + > > +static int hpwdt_resume(struct device *dev) > > +{ > > + dev_dbg(dev, "Resume watchdog\n"); > > + > > + if (watchdog_active(&hpwdt_dev)) > > + hpwdt_start(&hpwdt_dev); > > + > > + return 0; > > +} > > + > > +static DEFINE_SIMPLE_DEV_PM_OPS(hpwdt_pm_ops, hpwdt_suspend, hpwdt_resume); > > That disables / enables the watchdog as part of regular suspend/resume > handling, meaning any hang during suspend/resume that happens later will > hang the system. Sure you don't want to use SET_LATE_SYSTEM_SLEEP_PM_OPS() > instead ? > > Never mind, though. Your call, obviously. Your suggestion would be an improvement. Will change in version 2 of patch. Thanks for the help. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Thanks, > Guenter
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 138dc8d8ca3d..6565cfaa8e57 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -378,11 +378,38 @@ static void hpwdt_exit(struct pci_dev *dev) pci_disable_device(dev); } +static int hpwdt_suspend(struct device *dev) +{ + dev_dbg(dev, "Suspend watchdog\n"); + + if (watchdog_active(&hpwdt_dev)) + hpwdt_stop(); + + return 0; +} + +static int hpwdt_resume(struct device *dev) +{ + dev_dbg(dev, "Resume watchdog\n"); + + if (watchdog_active(&hpwdt_dev)) + hpwdt_start(&hpwdt_dev); + + return 0; +} + +static DEFINE_SIMPLE_DEV_PM_OPS(hpwdt_pm_ops, hpwdt_suspend, hpwdt_resume); + static struct pci_driver hpwdt_driver = { .name = "hpwdt", .id_table = hpwdt_devices, .probe = hpwdt_init_one, .remove = hpwdt_exit, + + .driver = { + .name = "hpwdt", + .pm = &hpwdt_pm_ops, + } }; MODULE_AUTHOR("Tom Mingarelli");
Add call backs to support suspend and resume. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/watchdog/hpwdt.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)