linux-kernel - Re: [PATCH] PM / watchdog: iTCO: stop watchdog during system suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2928815.XiLbVYtUVn@vostro.rjw.lan>
Date:	Fri, 03 Apr 2015 00:04:02 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	linux-watchdog@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Wim Van Sebroeck <wim@...ana.be>,
	Aubrey Li <aubrey.li@...el.com>, Borun Fu <borun.fu@...el.com>
Subject: Re: [PATCH] PM / watchdog: iTCO: stop watchdog during system suspend

On Wednesday, April 01, 2015 06:21:40 PM Guenter Roeck wrote:
> On 04/01/2015 05:31 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

[cut]

> Hi Rafael,
> 
> This only covers suspend and resume, but not any of the other
> sleep operations (like SIMPLE_DEV_PM_OPS would do).
> Is that intentional ?

Yes, it is:
 (1) The problemm at hand is strictly specific to system suspend.
 (2) The SIMPLE_DEV_PM_OPS etc macros don't cover the _noirq callback variants
     (and we want to use the _noirq variants, because we want the watchdog to
     be stopped as late and restarted as early as reasonably possible).
 (3) I'm not even sure if adding runtime PM support to this driver would make
     any sense. :-)

> 
> > +};
> > +
> > +#define ITCO_WDT_PM_OPS	&iTCO_wdt_pm
> 
> Checkpatch wants to see (&iTCO_wdt_pm).
> 
> ERROR: Macros with complex values should be enclosed in parentheses

Please find an updated patch below.

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
Subject: PM / watchdog: iTCO: stop watchdog during system suspend

If the target sleep state of the system is not an ACPI sleep state
(S1, S2 or S3), the TCO watchdog needs to be stopped during system
suspend, because it may not be possible to ping it any more after
timekeeping has been suspended (suspend-to-idle does that for
one example).

For this reason, provide ->suspend_noirq and ->resume_noirq
callbacks for the iTCO watchdog driver and use them to stop
and restart the watchdog during system suspend and resume,
respectively, if the system is not going to enter an ACPI
sleep state (in which case the watchdog will be stopped
by the platform firmware before the state is entered).

Reported-by: Borun Fu <borun.fu@...el.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
---
 drivers/watchdog/iTCO_wdt.c |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

Index: linux-pm/drivers/watchdog/iTCO_wdt.c
===================================================================
--- linux-pm.orig/drivers/watchdog/iTCO_wdt.c
+++ linux-pm/drivers/watchdog/iTCO_wdt.c
@@ -51,6 +51,7 @@
 #define DRV_VERSION	"1.11"
 
 /* Includes */
+#include <linux/acpi.h>			/* For ACPI support */
 #include <linux/module.h>		/* For module specific items */
 #include <linux/moduleparam.h>		/* For new moduleparam's */
 #include <linux/types.h>		/* For standard types (like size_t) */
@@ -103,6 +104,8 @@ static struct {		/* this is private data
 	struct platform_device *dev;
 	/* the PCI-device */
 	struct pci_dev *pdev;
+	/* whether or not the watchdog has been suspended */
+	bool suspended;
 } iTCO_wdt_private;
 
 /* module parameters */
@@ -571,12 +574,60 @@ static void iTCO_wdt_shutdown(struct pla
 	iTCO_wdt_stop(NULL);
 }
 
+#ifdef CONFIG_PM_SLEEP
+/*
+ * Suspend-to-idle requires this, because it stops the ticks and timekeeping, so
+ * the watchdog cannot be pinged while in that state.  In ACPI sleep states the
+ * watchdog is stopped by the platform firmware.
+ */
+
+#ifdef CONFIG_ACPI
+static inline bool need_suspend(void)
+{
+	return acpi_target_system_state() == ACPI_STATE_S0;
+}
+#else
+static inline bool need_suspend(void) { return true; }
+#endif
+
+static int iTCO_wdt_suspend_noirq(struct device *dev)
+{
+	int ret = 0;
+
+	iTCO_wdt_private.suspended = false;
+	if (watchdog_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
+		ret = iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+		if (!ret)
+			iTCO_wdt_private.suspended = true;
+	}
+	return ret;
+}
+
+static int iTCO_wdt_resume_noirq(struct device *dev)
+{
+	if (iTCO_wdt_private.suspended)
+		iTCO_wdt_start(&iTCO_wdt_watchdog_dev);
+
+	return 0;
+}
+
+struct dev_pm_ops iTCO_wdt_pm = {
+	.suspend_noirq = iTCO_wdt_suspend_noirq,
+	.resume_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,
 	.remove         = iTCO_wdt_remove,
 	.shutdown       = iTCO_wdt_shutdown,
 	.driver         = {
 		.name   = DRV_NAME,
+		.pm     = ITCO_WDT_PM_OPS,
 	},
 };
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/