[PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Zhang Rui]

On some Intel client platforms like SKL/KBL/CNL/CML, there is a
PCH thermal sensor that monitors the PCH temperature and blocks the system
from entering S0ix in case it overheats.

Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
temperature above threshold") introduces a delay loop to cool the
temperature down for this purpose.

However, in practice, we found that the time it takes to cool the PCH down
below threshold highly depends on the initial PCH temperature when the
delay starts, as well as the ambient temperature.

For example, on a Dell XPS 9360 laptop, the problem can be triggered 
1. when it is suspended with heavy workload running.
or
2. when it is moved from New Hampshire to Florida.

In these cases, the 1 second delay is not sufficient. As a result, the
system stays in a shallower power state like PCx instead of S0ix, and
drains the battery power, without user' notice.

In this patch series, we first fix the problem in patch 1/7 ~ 3/7, by
1. expand the default overall cooling delay timeout to 60 seconds.
2. make sure the temperature is below threshold rather than equal to it.
3. move the delay to .suspend_noirq phase instead, in order to
   a) do the cooling when the system is in a more quiescent state
   b) be aware of wakeup events during the long delay, because some wakeup
      events (ACPI Power button Press, USB mouse, etc) become valid only
      in .suspend_noirq phase and later.

However, this potential long delay introduces a problem to our suspend
stress automation test, because the delay makes it hard to predict how
much time it takes to suspend the system.
As we want to do as much suspend iterations as possible in limited time,
setting a 60+ seconds rtc alarm for suspend which usually takes shorter
than 1 second is far beyond overkill.

Thus, in patch 4/7 ~ 7/7, a rtc driver hook is introduced, which cancels
the armed rtc alarm in the beginning of suspend and then rearm the rtc
alarm with a short interval (say, 2 second) right before system suspended.

By running
 # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
before suspend, the system can be resumed by RTC alarm right after it is
suspended, no matter how much time the suspend really takes.

This patch series has been tested on the same Dell XPS 9360 laptop and
S0ix is 100% achieved across 1000+ s2idle iterations.

thanks,
rui

[PATCH 1/7] PM: wakeup: expose pm_wakeup_pending to modules

[Zhang Rui]

intel_pch_thermal driver needs a long delay to cool itself (60 seconds
in maximum) during suspend. When a wakeup event occures during the
delay, it is better for the intel_pch_thermal driver to detect this and
quit cooling because the suspend is likely to abort anyway.

Thus expose pm_wakeup_pending to modules so that intel_pch_thermal
driver can be aware of the wakeup events.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/base/power/wakeup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a57d469676ca..11a4ffe91367 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -930,6 +930,7 @@ bool pm_wakeup_pending(void)
 
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
+EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 
 void pm_system_wakeup(void)
 {
-- 
2.17.1

[PATCH 2/7] thermal: intel: pch: enhance overheat handling

[Zhang Rui]

Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
temperature above threshold") introduces delay loop mechanism that allows
PCH temperature to go down below threshold during suspend so it won't
block S0ix. And the default overall delay timeout is 1 second.

However, in practice, we found that the time it takes to cool the PCH down
below threshold highly depends on the initial PCH temperature when the
delay starts, as well as the ambient temperature.
And in some cases, the 1 second delay is not sufficient. As a result, the
system stays in a shallower power state like PCx instead of S0ix, and
drains the battery power, without user' notice.

To make sure S0ix is not blocked by the PCH overheating, we
1. expand the default overall timeout to 60 seconds.
2. make sure the temperature is below threshold rather than equal to it.
3. move the delay to .suspend_noirq phase instead, in order to
   a) do cooling delay with a more quiescent system
   b) be aware of wakeup events during the long delay, because some wakeup
      events (ACPI Power button Press, USB mouse, etc) become valid only
      in .suspend_noirq phase and later.

This may introduce longer suspend time, but only in the cases when the
system overheats and Linux used to enter a shallower S2idle state, say,
PCx instead of S0ix.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index 527c91f5960b..b7b32e2f5ae2 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -70,8 +70,8 @@ static unsigned int delay_timeout = 100;
 module_param(delay_timeout, int, 0644);
 MODULE_PARM_DESC(delay_timeout, "amount of time delay for each iteration.");
 
-/* Number of iterations for cooling delay, 10 counts by default for now */
-static unsigned int delay_cnt = 10;
+/* Number of iterations for cooling delay, 600 counts by default for now */
+static unsigned int delay_cnt = 600;
 module_param(delay_cnt, int, 0644);
 MODULE_PARM_DESC(delay_cnt, "total number of iterations for time delay.");
 
@@ -193,10 +193,11 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp)
 	return 0;
 }
 
+/* Cool the PCH when it's overheat in .suspend_noirq phase */
 static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 {
 	u8 tsel;
-	u8 pch_delay_cnt = 1;
+	int pch_delay_cnt = 1;
 	u16 pch_thr_temp, pch_cur_temp;
 
 	/* Shutdown the thermal sensor if it is not enabled by BIOS */
@@ -233,7 +234,10 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 	 * which helps to indentify the reason why S0ix entry was rejected.
 	 */
 	while (pch_delay_cnt <= delay_cnt) {
-		if (pch_cur_temp <= pch_thr_temp)
+		if (pch_cur_temp < pch_thr_temp)
+			break;
+
+		if (pm_wakeup_pending())
 			break;
 
 		dev_warn(&ptd->pdev->dev,
@@ -245,7 +249,7 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 		pch_delay_cnt++;
 	}
 
-	if (pch_cur_temp > pch_thr_temp)
+	if (pch_cur_temp >= pch_thr_temp)
 		dev_warn(&ptd->pdev->dev,
 			"CPU-PCH is hot [%dC] even after delay, continue to suspend. S0ix might fail\n",
 			pch_cur_temp);
@@ -455,7 +459,7 @@ static void intel_pch_thermal_remove(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 }
 
-static int intel_pch_thermal_suspend(struct device *device)
+static int intel_pch_thermal_suspend_noirq(struct device *device)
 {
 	struct pch_thermal_device *ptd = dev_get_drvdata(device);
 
@@ -495,7 +499,7 @@ static const struct pci_device_id intel_pch_thermal_id[] = {
 MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
 
 static const struct dev_pm_ops intel_pch_pm_ops = {
-	.suspend = intel_pch_thermal_suspend,
+	.suspend_noirq = intel_pch_thermal_suspend_noirq,
 	.resume = intel_pch_thermal_resume,
 };
 
-- 
2.17.1

[PATCH 3/7] thermal: intel: pch: improve the cooling delay log

[Zhang Rui]

Previously, during suspend, intel_pch_thermal driver logs for every
cooling iteration, about the current PCH temperature and number of cooling
iterations that have been tried, like below

[  100.955526] intel_pch_thermal 0000:00:14.2: CPU-PCH current temp [53C] higher than the threshold temp [50C], sleep 1 times for 100 ms duration
[  101.064156] intel_pch_thermal 0000:00:14.2: CPU-PCH current temp [53C] higher than the threshold temp [50C], sleep 2 times for 100 ms duration

After changing the default delay_cnt to 600, in practice, it is common to
see tens of the above messages if the system is suspended when PCH
overheats. Thus, change this log message from dev_warn to dev_dbg because
it is only useful when we want to check the temperature trend.

At the same time, there is always a one-line message given by the driver
with the patch applied, with below four possibilities.

1. PCH is cool, no cooling delay needed
[ 1791.902853] intel_pch_thermal 0000:00:12.0: CPU-PCH is cool [48C]

2. PCH overheats and becomes cool after the cooling delays
[ 1475.511617] intel_pch_thermal 0000:00:12.0: CPU-PCH is cool [49C] after 30700 ms delay

3. PCH still overheats after the overall cooling timeout
[ 2250.157487] intel_pch_thermal 0000:00:12.0: CPU-PCH is hot [60C] after 60000 ms delay. S0ix might fail

4. PCH aborts cooling because of wakeup event detected during the delay
[ 1933.639509] intel_pch_thermal 0000:00:12.0: Wakeup event detected, abort cooling

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c | 31 +++++++++++++++--------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index b7b32e2f5ae2..c1fa2b29b153 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -197,7 +197,7 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp)
 static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 {
 	u8 tsel;
-	int pch_delay_cnt = 1;
+	int pch_delay_cnt = 0;
 	u16 pch_thr_temp, pch_cur_temp;
 
 	/* Shutdown the thermal sensor if it is not enabled by BIOS */
@@ -233,29 +233,38 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 	 * temperature stays above threshold, notify the warning message
 	 * which helps to indentify the reason why S0ix entry was rejected.
 	 */
-	while (pch_delay_cnt <= delay_cnt) {
+	while (pch_delay_cnt < delay_cnt) {
 		if (pch_cur_temp < pch_thr_temp)
 			break;
 
-		if (pm_wakeup_pending())
-			break;
+		if (pm_wakeup_pending()) {
+			dev_warn(&ptd->pdev->dev, "Wakeup event detected, abort cooling\n");
+			return 0;
+		}
 
-		dev_warn(&ptd->pdev->dev,
+		pch_delay_cnt++;
+		dev_dbg(&ptd->pdev->dev,
 			"CPU-PCH current temp [%dC] higher than the threshold temp [%dC], sleep %d times for %d ms duration\n",
 			pch_cur_temp, pch_thr_temp, pch_delay_cnt, delay_timeout);
 		msleep(delay_timeout);
 		/* Read the PCH current temperature for next cycle. */
 		pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
-		pch_delay_cnt++;
 	}
 
 	if (pch_cur_temp >= pch_thr_temp)
 		dev_warn(&ptd->pdev->dev,
-			"CPU-PCH is hot [%dC] even after delay, continue to suspend. S0ix might fail\n",
-			pch_cur_temp);
-	else
-		dev_info(&ptd->pdev->dev,
-			"CPU-PCH is cool [%dC], continue to suspend\n", pch_cur_temp);
+			"CPU-PCH is hot [%dC] after %d ms delay. S0ix might fail\n",
+			pch_cur_temp, pch_delay_cnt * delay_timeout);
+	else {
+		if (pch_delay_cnt)
+			dev_info(&ptd->pdev->dev,
+				"CPU-PCH is cool [%dC] after %d ms delay\n",
+				pch_cur_temp, pch_delay_cnt * delay_timeout);
+		else
+			dev_info(&ptd->pdev->dev,
+				"CPU-PCH is cool [%dC]\n",
+				pch_cur_temp);
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH 4/7] ACPI: video: improve PM notifer callback

[Zhang Rui]

PM notifier callbacks should check for supported events rather than filter
out the unsupported events. So that it won't break when a new event is
introduced.

No functional change in this patch.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/acpi/acpi_video.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 990ff5b0aeb8..e07782b1fbb6 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1707,24 +1707,23 @@ static int acpi_video_resume(struct notifier_block *nb,
 	int i;
 
 	switch (val) {
-	case PM_HIBERNATION_PREPARE:
-	case PM_SUSPEND_PREPARE:
-	case PM_RESTORE_PREPARE:
-		return NOTIFY_DONE;
-	}
-
-	video = container_of(nb, struct acpi_video_bus, pm_nb);
-
-	dev_info(&video->device->dev, "Restoring backlight state\n");
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+	case PM_POST_RESTORE:
+		video = container_of(nb, struct acpi_video_bus, pm_nb);
+
+		dev_info(&video->device->dev, "Restoring backlight state\n");
+
+		for (i = 0; i < video->attached_count; i++) {
+			video_device = video->attached_array[i].bind_info;
+			if (video_device && video_device->brightness)
+				acpi_video_device_lcd_set_level(video_device,
+						video_device->brightness->curr);
+		}
 
-	for (i = 0; i < video->attached_count; i++) {
-		video_device = video->attached_array[i].bind_info;
-		if (video_device && video_device->brightness)
-			acpi_video_device_lcd_set_level(video_device,
-					video_device->brightness->curr);
+		return NOTIFY_OK;
 	}
-
-	return NOTIFY_OK;
+	return NOTIFY_DONE;
 }
 
 static acpi_status
-- 
2.17.1

[PATCH 5/7] wil6210: remove debug message for unsupported PM event

[Zhang Rui]

Remove the useless debug message for unsupported PM event because it is
noop in current code, and it gives a warning when a new event is
introduced, which it doesn't care.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/net/wireless/ath/wil6210/pcie_bus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index ce40d94909ad..1d6c4e926004 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -600,7 +600,6 @@ static int wil6210_pm_notify(struct notifier_block *notify_block,
 						      evt);
 		break;
 	default:
-		wil_dbg_pm(wil, "unhandled notify mode %ld\n", mode);
 		break;
 	}
 
-- 
2.17.1

[PATCH 6/7] PM: suspend: introduce PM_SUSPEND_LATE event

[Zhang Rui]

In some cases, special handling is needed after the .suspend_noirq phase.

Introduce a new suspend event PM_SUSPEND_LATE and call the notifier chain
for this purpose.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 300273ff40cc..01ec171e8f60 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -477,6 +477,7 @@ static inline int is_hibernate_resume_dev(dev_t dev) { return 0; }
 #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
+#define PM_SUSPEND_LATE		0x0007 /* Late suspend phase */
 
 extern struct mutex system_transition_mutex;
 
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 6fcdee7e87a5..3c662acc908f 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -415,6 +415,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
+	pm_notifier_call_chain(PM_SUSPEND_LATE);
+
 	if (state == PM_SUSPEND_TO_IDLE) {
 		s2idle_loop();
 		goto Platform_wake;
-- 
2.17.1

[PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

Automated suspend/resume testing uses the RTC for wakeup.
A short rtcwake period is desirable, so that more suspend/resume
cycles can be completed, while the machine is available for testing.

But if too short a wake interval is specified, the event can occur,
while still suspending, and then no event wakes the suspended system
until the user notices that testing has stalled, and manually intervenes.

Here we add a hook to the rtc-cmos driver to
a) remove the alarm timer in the beginning of suspend, if there is any
b) arm the wakeup in PM notifier callback, which is in the very late stage
   before the system really suspends
The remaining part of suspend is usually measured under 10 ms,
and so arming the timer at this point could be as fast as the minimum
time of 1-second.

But there is a 2nd race.  The RTC has 1-second granularity, and unless
you are timing the timer with a higher resolution timer,
there is no telling if the current time + 1 will occur immediately,
or a full second in the future.  And so 2-seconds is the safest minimum:

Run 1,000 s2idle cycles with (max of) 2 second wakeup period:

 # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
 # sleepgraph.py -m freeze -multi 1000 0 -skiphtml -gzip

Clear the timer override, to not interfere with subsequent
real use of the machine's suspend/resume feature:

 # echo 0 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec

Originally-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/rtc/interface.c |  1 +
 drivers/rtc/rtc-cmos.c  | 45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 9edd662c69ac..fb93aa2dc99c 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -1020,6 +1020,7 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
 		rtc_timer_remove(rtc, timer);
 	mutex_unlock(&rtc->ops_lock);
 }
+EXPORT_SYMBOL_GPL(rtc_timer_cancel);
 
 /**
  * rtc_read_offset - Read the amount of rtc offset in parts per billion
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 7c006c2b125f..9590c40fa9d8 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -32,6 +32,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
+#include <linux/suspend.h>
 #include <linux/platform_device.h>
 #include <linux/log2.h>
 #include <linux/pm.h>
@@ -70,6 +71,9 @@ static inline int cmos_use_acpi_alarm(void)
 }
 #endif
 
+static int rtc_wake_override_sec;
+module_param(rtc_wake_override_sec, int, 0644);
+
 struct cmos_rtc {
 	struct rtc_device	*rtc;
 	struct device		*dev;
@@ -89,6 +93,7 @@ struct cmos_rtc {
 	u8			century;
 
 	struct rtc_wkalrm	saved_wkalrm;
+	struct notifier_block	pm_nb;
 };
 
 /* both platform and pnp busses use negative numbers for invalid irqs */
@@ -744,6 +749,42 @@ static irqreturn_t cmos_interrupt(int irq, void *p)
 		return IRQ_NONE;
 }
 
+static int cmos_pm_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
+{
+	struct cmos_rtc *cmos = container_of(nb, struct cmos_rtc, pm_nb);
+	struct rtc_device       *rtc = cmos->rtc;
+	unsigned long           now;
+	struct rtc_wkalrm       alm;
+
+	if (rtc_wake_override_sec == 0)
+		return NOTIFY_OK;
+
+	switch (mode) {
+	case PM_SUSPEND_PREPARE:
+		/*
+		 * Cancel the timer to make sure it won't fire
+		 * before rtc is rearmed later.
+		 */
+		rtc_timer_cancel(rtc, &rtc->aie_timer);
+		break;
+	case PM_SUSPEND_LATE:
+		if (rtc_read_time(rtc, &alm.time))
+			return NOTIFY_BAD;
+
+		now = rtc_tm_to_time64(&alm.time);
+		memset(&alm, 0, sizeof(alm));
+		rtc_time64_to_tm(now + rtc_wake_override_sec, &alm.time);
+		alm.enabled = true;
+		if (rtc_set_alarm(rtc, &alm))
+			return NOTIFY_BAD;
+		if (cmos->wake_on)
+			cmos->wake_on(cmos->dev);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
 #ifdef	CONFIG_PNP
 #define	INITSECTION
 
@@ -937,6 +978,9 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 		 nvmem_cfg.size,
 		 use_hpet_alarm() ? ", hpet irqs" : "");
 
+	cmos_rtc.pm_nb.notifier_call = cmos_pm_notify;
+	register_pm_notifier(&cmos_rtc.pm_nb);
+
 	return 0;
 
 cleanup2:
@@ -965,6 +1009,7 @@ static void cmos_do_remove(struct device *dev)
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 	struct resource *ports;
 
+	unregister_pm_notifier(&cmos_rtc.pm_nb);
 	cmos_do_shutdown(cmos->irq);
 
 	if (is_valid_irq(cmos->irq)) {
-- 
2.17.1

Re: [PATCH 5/7] wil6210: remove debug message for unsupported PM event

[Kalle Valo]

Zhang Rui <rui.zhang@intel.com> writes:

> Remove the useless debug message for unsupported PM event because it is
> noop in current code, and it gives a warning when a new event is
> introduced, which it doesn't care.

It's a debug message, not a warning, and only visible when debug
messages are enabled. Why do you want to remove it?

> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>

Is this really tested on a wil6210 device? Not that it matters, just
surprised to see a Tested-by for a wil6210 patch. It's not really common
hardware.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 5/7] wil6210: remove debug message for unsupported PM event

[Zhang Rui]

Hi, Kalle,

thanks for the quick response.

On Thu, 2022-05-05 at 07:38 +0300, Kalle Valo wrote:
> Zhang Rui <rui.zhang@intel.com> writes:
> 
> > Remove the useless debug message for unsupported PM event because
> > it is
> > noop in current code, and it gives a warning when a new event is
> > introduced, which it doesn't care.
> 
> It's a debug message, not a warning, and only visible when debug
> messages are enabled. Why do you want to remove it?

I'm concerning that people will report problems when they see new
messages which never shows up previously.

Deleting or keeping this message are both okay to me. But patch 6/7
indeed introduces a change to this piece of code and it's better for
you to be aware of it before people starts to complain.

> 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> 
> Is this really tested on a wil6210 device? Not that it matters, just
> surprised to see a Tested-by for a wil6210 patch. It's not really
> common
> hardware.

No, we just tested the whole patch series on a Dell 9360 laptop, and a
series of internal test machines. I didn't check if any of them has
this device or not. Maybe I should remove the tested by in this case?

thanks,
rui

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Oliver Neukum]

On 05.05.22 03:58, Zhang Rui wrote:
> On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> PCH thermal sensor that monitors the PCH temperature and blocks the system
> from entering S0ix in case it overheats.
>
> Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> temperature above threshold") introduces a delay loop to cool the
> temperature down for this purpose.
>
> However, in practice, we found that the time it takes to cool the PCH down
> below threshold highly depends on the initial PCH temperature when the
> delay starts, as well as the ambient temperature.

>
> This patch series has been tested on the same Dell XPS 9360 laptop and
> S0ix is 100% achieved across 1000+ s2idle iterations.
>
Hi,

what is the user experience if this ever triggers? At that stage the
system will appear to be suspended to an external observer, won't it?
So in effect you'd have a system that spontaneously wakes up, won't you?

    Regards
        Oliver

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Rafael J. Wysocki]

On Thu, May 5, 2022 at 10:23 AM Oliver Neukum <oneukum@suse.com> wrote:
>
>
>
> On 05.05.22 03:58, Zhang Rui wrote:
> > On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> > PCH thermal sensor that monitors the PCH temperature and blocks the system
> > from entering S0ix in case it overheats.
> >
> > Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> > temperature above threshold") introduces a delay loop to cool the
> > temperature down for this purpose.
> >
> > However, in practice, we found that the time it takes to cool the PCH down
> > below threshold highly depends on the initial PCH temperature when the
> > delay starts, as well as the ambient temperature.
>
> >
> > This patch series has been tested on the same Dell XPS 9360 laptop and
> > S0ix is 100% achieved across 1000+ s2idle iterations.
> >
> Hi,
>
> what is the user experience if this ever triggers? At that stage the
> system will appear to be suspended to an external observer, won't it?
> So in effect you'd have a system that spontaneously wakes up, won't you?

No, you won't.

It will just go ahead and reach S0ix when it can.  It will only wake
up if there's a legitimate wakeup even in the meantime.

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Zhang Rui]

Hi, Neukum,

Thanks for your response, I missed your original reply in my Inbox.

On Thu, 2022-05-05 at 14:02 +0200, Rafael J. Wysocki wrote:
> On Thu, May 5, 2022 at 10:23 AM Oliver Neukum <oneukum@suse.com>
> wrote:
> > 
> > 
> > 
> > On 05.05.22 03:58, Zhang Rui wrote:
> > > On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> > > PCH thermal sensor that monitors the PCH temperature and blocks
> > > the system
> > > from entering S0ix in case it overheats.
> > > 
> > > Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due
> > > to PCH
> > > temperature above threshold") introduces a delay loop to cool the
> > > temperature down for this purpose.
> > > 
> > > However, in practice, we found that the time it takes to cool the
> > > PCH down
> > > below threshold highly depends on the initial PCH temperature
> > > when the
> > > delay starts, as well as the ambient temperature.
> > > 
> > > This patch series has been tested on the same Dell XPS 9360
> > > laptop and
> > > S0ix is 100% achieved across 1000+ s2idle iterations.
> > > 
> > 
> > Hi,
> > 
> > what is the user experience if this ever triggers? At that stage
> > the
> > system will appear to be suspended to an external observer, won't
> > it?
> > So in effect you'd have a system that spontaneously wakes up, won't
> > you?
> 
> No, you won't.
> 
> It will just go ahead and reach S0ix when it can.  It will only wake
> up if there's a legitimate wakeup even in the meantime.

Please correct me if I misunderstand your question, Oliver.

Without the patch, the system becomes suspended and stays in PCx.
With the patch, the system first stays in PCx during suspending (in the
intel_pch_thermal driver' cooling delays), and then becomes suspended
and stays in S0ix.

thanks,
rui

Re: [PATCH 5/7] wil6210: remove debug message for unsupported PM event

[Kalle Valo]

Zhang Rui <rui.zhang@intel.com> writes:

> Hi, Kalle,
>
> thanks for the quick response.
>
> On Thu, 2022-05-05 at 07:38 +0300, Kalle Valo wrote:
>> Zhang Rui <rui.zhang@intel.com> writes:
>> 
>> > Remove the useless debug message for unsupported PM event because
>> > it is
>> > noop in current code, and it gives a warning when a new event is
>> > introduced, which it doesn't care.
>> 
>> It's a debug message, not a warning, and only visible when debug
>> messages are enabled. Why do you want to remove it?
>
> I'm concerning that people will report problems when they see new
> messages which never shows up previously.
>
> Deleting or keeping this message are both okay to me. But patch 6/7
> indeed introduces a change to this piece of code and it's better for
> you to be aware of it before people starts to complain.
>
>> 
>> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> > Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
>> 
>> Is this really tested on a wil6210 device? Not that it matters, just
>> surprised to see a Tested-by for a wil6210 patch. It's not really
>> common
>> hardware.
>
> No, we just tested the whole patch series on a Dell 9360 laptop, and a
> series of internal test machines. I didn't check if any of them has
> this device or not. Maybe I should remove the tested by in this case?

I think it's best to drop this wil6210 patch. The driver is orphaned
anyway and if anyone complains, they will do that to me :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Alexandre Belloni]

Hello,

I assume I can ignore this patch as this seems to be only for testing
I'm not even sure why this is needed as this completely breaks setting
the alarm time.

On 05/05/2022 09:58:14+0800, Zhang Rui wrote:
> Automated suspend/resume testing uses the RTC for wakeup.
> A short rtcwake period is desirable, so that more suspend/resume
> cycles can be completed, while the machine is available for testing.
> 
> But if too short a wake interval is specified, the event can occur,
> while still suspending, and then no event wakes the suspended system
> until the user notices that testing has stalled, and manually intervenes.
> 
> Here we add a hook to the rtc-cmos driver to
> a) remove the alarm timer in the beginning of suspend, if there is any
> b) arm the wakeup in PM notifier callback, which is in the very late stage
>    before the system really suspends
> The remaining part of suspend is usually measured under 10 ms,
> and so arming the timer at this point could be as fast as the minimum
> time of 1-second.
> 
> But there is a 2nd race.  The RTC has 1-second granularity, and unless
> you are timing the timer with a higher resolution timer,
> there is no telling if the current time + 1 will occur immediately,
> or a full second in the future.  And so 2-seconds is the safest minimum:
> 
> Run 1,000 s2idle cycles with (max of) 2 second wakeup period:
> 
>  # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
>  # sleepgraph.py -m freeze -multi 1000 0 -skiphtml -gzip
> 
> Clear the timer override, to not interfere with subsequent
> real use of the machine's suspend/resume feature:
> 
>  # echo 0 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> 
> Originally-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> ---
>  drivers/rtc/interface.c |  1 +
>  drivers/rtc/rtc-cmos.c  | 45 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 9edd662c69ac..fb93aa2dc99c 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -1020,6 +1020,7 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
>  		rtc_timer_remove(rtc, timer);
>  	mutex_unlock(&rtc->ops_lock);
>  }
> +EXPORT_SYMBOL_GPL(rtc_timer_cancel);
>  
>  /**
>   * rtc_read_offset - Read the amount of rtc offset in parts per billion
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 7c006c2b125f..9590c40fa9d8 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -32,6 +32,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/spinlock.h>
> +#include <linux/suspend.h>
>  #include <linux/platform_device.h>
>  #include <linux/log2.h>
>  #include <linux/pm.h>
> @@ -70,6 +71,9 @@ static inline int cmos_use_acpi_alarm(void)
>  }
>  #endif
>  
> +static int rtc_wake_override_sec;
> +module_param(rtc_wake_override_sec, int, 0644);
> +
>  struct cmos_rtc {
>  	struct rtc_device	*rtc;
>  	struct device		*dev;
> @@ -89,6 +93,7 @@ struct cmos_rtc {
>  	u8			century;
>  
>  	struct rtc_wkalrm	saved_wkalrm;
> +	struct notifier_block	pm_nb;
>  };
>  
>  /* both platform and pnp busses use negative numbers for invalid irqs */
> @@ -744,6 +749,42 @@ static irqreturn_t cmos_interrupt(int irq, void *p)
>  		return IRQ_NONE;
>  }
>  
> +static int cmos_pm_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
> +{
> +	struct cmos_rtc *cmos = container_of(nb, struct cmos_rtc, pm_nb);
> +	struct rtc_device       *rtc = cmos->rtc;
> +	unsigned long           now;
> +	struct rtc_wkalrm       alm;
> +
> +	if (rtc_wake_override_sec == 0)
> +		return NOTIFY_OK;
> +
> +	switch (mode) {
> +	case PM_SUSPEND_PREPARE:
> +		/*
> +		 * Cancel the timer to make sure it won't fire
> +		 * before rtc is rearmed later.
> +		 */
> +		rtc_timer_cancel(rtc, &rtc->aie_timer);
> +		break;
> +	case PM_SUSPEND_LATE:
> +		if (rtc_read_time(rtc, &alm.time))
> +			return NOTIFY_BAD;
> +
> +		now = rtc_tm_to_time64(&alm.time);
> +		memset(&alm, 0, sizeof(alm));
> +		rtc_time64_to_tm(now + rtc_wake_override_sec, &alm.time);
> +		alm.enabled = true;
> +		if (rtc_set_alarm(rtc, &alm))
> +			return NOTIFY_BAD;
> +		if (cmos->wake_on)
> +			cmos->wake_on(cmos->dev);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef	CONFIG_PNP
>  #define	INITSECTION
>  
> @@ -937,6 +978,9 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>  		 nvmem_cfg.size,
>  		 use_hpet_alarm() ? ", hpet irqs" : "");
>  
> +	cmos_rtc.pm_nb.notifier_call = cmos_pm_notify;
> +	register_pm_notifier(&cmos_rtc.pm_nb);
> +
>  	return 0;
>  
>  cleanup2:
> @@ -965,6 +1009,7 @@ static void cmos_do_remove(struct device *dev)
>  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
>  	struct resource *ports;
>  
> +	unregister_pm_notifier(&cmos_rtc.pm_nb);
>  	cmos_do_shutdown(cmos->irq);
>  
>  	if (is_valid_irq(cmos->irq)) {
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 5/7] wil6210: remove debug message for unsupported PM event

[Zhang Rui]

On Fri, 2022-05-06 at 17:04 +0300, Kalle Valo wrote:
> Zhang Rui <rui.zhang@intel.com> writes:
> 
> > Hi, Kalle,
> > 
> > thanks for the quick response.
> > 
> > On Thu, 2022-05-05 at 07:38 +0300, Kalle Valo wrote:
> > > Zhang Rui <rui.zhang@intel.com> writes:
> > > 
> > > > Remove the useless debug message for unsupported PM event
> > > > because
> > > > it is
> > > > noop in current code, and it gives a warning when a new event
> > > > is
> > > > introduced, which it doesn't care.
> > > 
> > > It's a debug message, not a warning, and only visible when debug
> > > messages are enabled. Why do you want to remove it?
> > 
> > I'm concerning that people will report problems when they see new
> > messages which never shows up previously.
> > 
> > Deleting or keeping this message are both okay to me. But patch 6/7
> > indeed introduces a change to this piece of code and it's better
> > for
> > you to be aware of it before people starts to complain.
> > 
> > > 
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> > > 
> > > Is this really tested on a wil6210 device? Not that it matters,
> > > just
> > > surprised to see a Tested-by for a wil6210 patch. It's not really
> > > common
> > > hardware.
> > 
> > No, we just tested the whole patch series on a Dell 9360 laptop,
> > and a
> > series of internal test machines. I didn't check if any of them has
> > this device or not. Maybe I should remove the tested by in this
> > case?
> 
> I think it's best to drop this wil6210 patch. The driver is orphaned
> anyway and if anyone complains, they will do that to me :)
> 
Sure, I will drop this patch.
Thanks for reviewing.

-rui

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

Hi, Alexandre,

Thanks for reviewing the patch.

On Fri, 2022-05-06 at 23:46 +0200, Alexandre Belloni wrote:
> Hello,
> 
> I assume I can ignore this patch as this seems to be only for testing

The main purpose of this patch is for automate testing.
But this doesn't mean it cannot be part of upstream code, right?

> I'm not even sure why this is needed as this completely breaks
> setting
> the alarm time.

Or overrides the alarm time, :)

People rely on the rtc alarm in the automated suspend stress test,
which suspend and resume the system for over 1000 iterations.
As I mentioned in the cover letter of this patch series, if the system
suspend time varies from under 1 second to over 60 seconds, how to
alarm the RTC before suspend?
This feature is critical in this scenario.

Plus, the current solution is transparent to people who don't known/use
this "rtc_wake_override_sec" parameter. And for people who use this,
they should know that the previous armed RTC alarm will be overrode
whenever a system suspend is triggered. I can add a message when the
parameter is set, if needed.

thanks,
rui

> 
> On 05/05/2022 09:58:14+0800, Zhang Rui wrote:
> > Automated suspend/resume testing uses the RTC for wakeup.
> > A short rtcwake period is desirable, so that more suspend/resume
> > cycles can be completed, while the machine is available for
> > testing.
> > 
> > But if too short a wake interval is specified, the event can occur,
> > while still suspending, and then no event wakes the suspended
> > system
> > until the user notices that testing has stalled, and manually
> > intervenes.
> > 
> > Here we add a hook to the rtc-cmos driver to
> > a) remove the alarm timer in the beginning of suspend, if there is
> > any
> > b) arm the wakeup in PM notifier callback, which is in the very
> > late stage
> >    before the system really suspends
> > The remaining part of suspend is usually measured under 10 ms,
> > and so arming the timer at this point could be as fast as the
> > minimum
> > time of 1-second.
> > 
> > But there is a 2nd race.  The RTC has 1-second granularity, and
> > unless
> > you are timing the timer with a higher resolution timer,
> > there is no telling if the current time + 1 will occur immediately,
> > or a full second in the future.  And so 2-seconds is the safest
> > minimum:
> > 
> > Run 1,000 s2idle cycles with (max of) 2 second wakeup period:
> > 
> >  # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> >  # sleepgraph.py -m freeze -multi 1000 0 -skiphtml -gzip
> > 
> > Clear the timer override, to not interfere with subsequent
> > real use of the machine's suspend/resume feature:
> > 
> >  # echo 0 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> > 
> > Originally-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> > ---
> >  drivers/rtc/interface.c |  1 +
> >  drivers/rtc/rtc-cmos.c  | 45
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> > index 9edd662c69ac..fb93aa2dc99c 100644
> > --- a/drivers/rtc/interface.c
> > +++ b/drivers/rtc/interface.c
> > @@ -1020,6 +1020,7 @@ void rtc_timer_cancel(struct rtc_device *rtc,
> > struct rtc_timer *timer)
> >  		rtc_timer_remove(rtc, timer);
> >  	mutex_unlock(&rtc->ops_lock);
> >  }
> > +EXPORT_SYMBOL_GPL(rtc_timer_cancel);
> >  
> >  /**
> >   * rtc_read_offset - Read the amount of rtc offset in parts per
> > billion
> > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> > index 7c006c2b125f..9590c40fa9d8 100644
> > --- a/drivers/rtc/rtc-cmos.c
> > +++ b/drivers/rtc/rtc-cmos.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/suspend.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/log2.h>
> >  #include <linux/pm.h>
> > @@ -70,6 +71,9 @@ static inline int cmos_use_acpi_alarm(void)
> >  }
> >  #endif
> >  
> > +static int rtc_wake_override_sec;
> > +module_param(rtc_wake_override_sec, int, 0644);
> > +
> >  struct cmos_rtc {
> >  	struct rtc_device	*rtc;
> >  	struct device		*dev;
> > @@ -89,6 +93,7 @@ struct cmos_rtc {
> >  	u8			century;
> >  
> >  	struct rtc_wkalrm	saved_wkalrm;
> > +	struct notifier_block	pm_nb;
> >  };
> >  
> >  /* both platform and pnp busses use negative numbers for invalid
> > irqs */
> > @@ -744,6 +749,42 @@ static irqreturn_t cmos_interrupt(int irq,
> > void *p)
> >  		return IRQ_NONE;
> >  }
> >  
> > +static int cmos_pm_notify(struct notifier_block *nb, unsigned long
> > mode, void *_unused)
> > +{
> > +	struct cmos_rtc *cmos = container_of(nb, struct cmos_rtc,
> > pm_nb);
> > +	struct rtc_device       *rtc = cmos->rtc;
> > +	unsigned long           now;
> > +	struct rtc_wkalrm       alm;
> > +
> > +	if (rtc_wake_override_sec == 0)
> > +		return NOTIFY_OK;
> > +
> > +	switch (mode) {
> > +	case PM_SUSPEND_PREPARE:
> > +		/*
> > +		 * Cancel the timer to make sure it won't fire
> > +		 * before rtc is rearmed later.
> > +		 */
> > +		rtc_timer_cancel(rtc, &rtc->aie_timer);
> > +		break;
> > +	case PM_SUSPEND_LATE:
> > +		if (rtc_read_time(rtc, &alm.time))
> > +			return NOTIFY_BAD;
> > +
> > +		now = rtc_tm_to_time64(&alm.time);
> > +		memset(&alm, 0, sizeof(alm));
> > +		rtc_time64_to_tm(now + rtc_wake_override_sec,
> > &alm.time);
> > +		alm.enabled = true;
> > +		if (rtc_set_alarm(rtc, &alm))
> > +			return NOTIFY_BAD;
> > +		if (cmos->wake_on)
> > +			cmos->wake_on(cmos->dev);
> > +		break;
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> >  #ifdef	CONFIG_PNP
> >  #define	INITSECTION
> >  
> > @@ -937,6 +978,9 @@ cmos_do_probe(struct device *dev, struct
> > resource *ports, int rtc_irq)
> >  		 nvmem_cfg.size,
> >  		 use_hpet_alarm() ? ", hpet irqs" : "");
> >  
> > +	cmos_rtc.pm_nb.notifier_call = cmos_pm_notify;
> > +	register_pm_notifier(&cmos_rtc.pm_nb);
> > +
> >  	return 0;
> >  
> >  cleanup2:
> > @@ -965,6 +1009,7 @@ static void cmos_do_remove(struct device *dev)
> >  	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
> >  	struct resource *ports;
> >  
> > +	unregister_pm_notifier(&cmos_rtc.pm_nb);
> >  	cmos_do_shutdown(cmos->irq);
> >  
> >  	if (is_valid_irq(cmos->irq)) {
> > -- 
> > 2.17.1
> > 
> 
> 

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Alexandre Belloni]

On 07/05/2022 10:00:40+0800, Zhang Rui wrote:
> Hi, Alexandre,
> 
> Thanks for reviewing the patch.
> 
> On Fri, 2022-05-06 at 23:46 +0200, Alexandre Belloni wrote:
> > Hello,
> > 
> > I assume I can ignore this patch as this seems to be only for testing
> 
> The main purpose of this patch is for automate testing.
> But this doesn't mean it cannot be part of upstream code, right?
> 
> > I'm not even sure why this is needed as this completely breaks
> > setting
> > the alarm time.
> 
> Or overrides the alarm time, :)
> 
> People rely on the rtc alarm in the automated suspend stress test,
> which suspend and resume the system for over 1000 iterations.
> As I mentioned in the cover letter of this patch series, if the system
> suspend time varies from under 1 second to over 60 seconds, how to
> alarm the RTC before suspend?
> This feature is critical in this scenario.
> 
> Plus, the current solution is transparent to people who don't known/use
> this "rtc_wake_override_sec" parameter. And for people who use this,
> they should know that the previous armed RTC alarm will be overrode
> whenever a system suspend is triggered. I can add a message when the
> parameter is set, if needed.
> 

It is not transparent, if I read your patch properly, this breaks wakeup
for everyone...

> > On 05/05/2022 09:58:14+0800, Zhang Rui wrote:
> > > +static int cmos_pm_notify(struct notifier_block *nb, unsigned long
> > > mode, void *_unused)
> > > +{
> > > +	struct cmos_rtc *cmos = container_of(nb, struct cmos_rtc,
> > > pm_nb);
> > > +	struct rtc_device       *rtc = cmos->rtc;
> > > +	unsigned long           now;
> > > +	struct rtc_wkalrm       alm;
> > > +
> > > +	if (rtc_wake_override_sec == 0)
> > > +		return NOTIFY_OK;
> > > +
> > > +	switch (mode) {
> > > +	case PM_SUSPEND_PREPARE:
> > > +		/*
> > > +		 * Cancel the timer to make sure it won't fire
> > > +		 * before rtc is rearmed later.
> > > +		 */
> > > +		rtc_timer_cancel(rtc, &rtc->aie_timer);
> > > +		break;
> > > +	case PM_SUSPEND_LATE:
> > > +		if (rtc_read_time(rtc, &alm.time))
> > > +			return NOTIFY_BAD;
> > > +
> > > +		now = rtc_tm_to_time64(&alm.time);
> > > +		memset(&alm, 0, sizeof(alm));
> > > +		rtc_time64_to_tm(now + rtc_wake_override_sec,
> > > &alm.time);
> > > +		alm.enabled = true;
> > > +		if (rtc_set_alarm(rtc, &alm))
> > > +			return NOTIFY_BAD;

... because if rtc_wake_override_sec is not set, this sets the alarm to
now which is the current RTC time, ensuring the alarm will never
trigger.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

On Sat, 2022-05-07 at 09:31 +0200, Alexandre Belloni wrote:
> On 07/05/2022 10:00:40+0800, Zhang Rui wrote:
> > Hi, Alexandre,
> > 
> > Thanks for reviewing the patch.
> > 
> > On Fri, 2022-05-06 at 23:46 +0200, Alexandre Belloni wrote:
> > > Hello,
> > > 
> > > I assume I can ignore this patch as this seems to be only for
> > > testing
> > 
> > The main purpose of this patch is for automate testing.
> > But this doesn't mean it cannot be part of upstream code, right?
> > 
> > > I'm not even sure why this is needed as this completely breaks
> > > setting
> > > the alarm time.
> > 
> > Or overrides the alarm time, :)
> > 
> > People rely on the rtc alarm in the automated suspend stress test,
> > which suspend and resume the system for over 1000 iterations.
> > As I mentioned in the cover letter of this patch series, if the
> > system
> > suspend time varies from under 1 second to over 60 seconds, how to
> > alarm the RTC before suspend?
> > This feature is critical in this scenario.
> > 
> > Plus, the current solution is transparent to people who don't
> > known/use
> > this "rtc_wake_override_sec" parameter. And for people who use
> > this,
> > they should know that the previous armed RTC alarm will be overrode
> > whenever a system suspend is triggered. I can add a message when
> > the
> > parameter is set, if needed.
> > 
> 
> It is not transparent, if I read your patch properly, this breaks
> wakeup
> for everyone...
> 
> > > On 05/05/2022 09:58:14+0800, Zhang Rui wrote:
> > > > +static int cmos_pm_notify(struct notifier_block *nb, unsigned
> > > > long
> > > > mode, void *_unused)
> > > > +{
> > > > +	struct cmos_rtc *cmos = container_of(nb, struct
> > > > cmos_rtc,
> > > > pm_nb);
> > > > +	struct rtc_device       *rtc = cmos->rtc;
> > > > +	unsigned long           now;
> > > > +	struct rtc_wkalrm       alm;
> > > > +
> > > > +	if (rtc_wake_override_sec == 0)
> > > > +		return NOTIFY_OK;
> > > > +
> > > > +	switch (mode) {
> > > > +	case PM_SUSPEND_PREPARE:
> > > > +		/*
> > > > +		 * Cancel the timer to make sure it won't fire
> > > > +		 * before rtc is rearmed later.
> > > > +		 */
> > > > +		rtc_timer_cancel(rtc, &rtc->aie_timer);
> > > > +		break;
> > > > +	case PM_SUSPEND_LATE:
> > > > +		if (rtc_read_time(rtc, &alm.time))
> > > > +			return NOTIFY_BAD;
> > > > +
> > > > +		now = rtc_tm_to_time64(&alm.time);
> > > > +		memset(&alm, 0, sizeof(alm));
> > > > +		rtc_time64_to_tm(now + rtc_wake_override_sec,
> > > > &alm.time);
> > > > +		alm.enabled = true;
> > > > +		if (rtc_set_alarm(rtc, &alm))
> > > > +			return NOTIFY_BAD;
> 
> ... because if rtc_wake_override_sec is not set, this sets the alarm
> to
> now which is the current RTC time, ensuring the alarm will never
> trigger.

No.
As the code below
> > > > 
> > > > 	if (rtc_wake_override_sec == 0)
> > > > +		return NOTIFY_OK;

We check for rtc_wake_override_sec at the beginning of the notifier
callback. So it takes effect only if a) rtc_wake_override_sec is set,
AND b) a suspend is triggered.

thanks,
rui

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

Hi, Alexandre,

May I know if this addresses your concern or not?

thanks,
rui

On Sat, 2022-05-07 at 15:41 +0800, Zhang Rui wrote:
> On Sat, 2022-05-07 at 09:31 +0200, Alexandre Belloni wrote:
> > On 07/05/2022 10:00:40+0800, Zhang Rui wrote:
> > > Hi, Alexandre,
> > > 
> > > Thanks for reviewing the patch.
> > > 
> > > On Fri, 2022-05-06 at 23:46 +0200, Alexandre Belloni wrote:
> > > > Hello,
> > > > 
> > > > I assume I can ignore this patch as this seems to be only for
> > > > testing
> > > 
> > > The main purpose of this patch is for automate testing.
> > > But this doesn't mean it cannot be part of upstream code, right?
> > > 
> > > > I'm not even sure why this is needed as this completely breaks
> > > > setting
> > > > the alarm time.
> > > 
> > > Or overrides the alarm time, :)
> > > 
> > > People rely on the rtc alarm in the automated suspend stress
> > > test,
> > > which suspend and resume the system for over 1000 iterations.
> > > As I mentioned in the cover letter of this patch series, if the
> > > system
> > > suspend time varies from under 1 second to over 60 seconds, how
> > > to
> > > alarm the RTC before suspend?
> > > This feature is critical in this scenario.
> > > 
> > > Plus, the current solution is transparent to people who don't
> > > known/use
> > > this "rtc_wake_override_sec" parameter. And for people who use
> > > this,
> > > they should know that the previous armed RTC alarm will be
> > > overrode
> > > whenever a system suspend is triggered. I can add a message when
> > > the
> > > parameter is set, if needed.
> > > 
> > 
> > It is not transparent, if I read your patch properly, this breaks
> > wakeup
> > for everyone...
> > 
> > > > On 05/05/2022 09:58:14+0800, Zhang Rui wrote:
> > > > > +static int cmos_pm_notify(struct notifier_block *nb,
> > > > > unsigned
> > > > > long
> > > > > mode, void *_unused)
> > > > > +{
> > > > > +	struct cmos_rtc *cmos = container_of(nb, struct
> > > > > cmos_rtc,
> > > > > pm_nb);
> > > > > +	struct rtc_device       *rtc = cmos->rtc;
> > > > > +	unsigned long           now;
> > > > > +	struct rtc_wkalrm       alm;
> > > > > +
> > > > > +	if (rtc_wake_override_sec == 0)
> > > > > +		return NOTIFY_OK;
> > > > > +
> > > > > +	switch (mode) {
> > > > > +	case PM_SUSPEND_PREPARE:
> > > > > +		/*
> > > > > +		 * Cancel the timer to make sure it won't fire
> > > > > +		 * before rtc is rearmed later.
> > > > > +		 */
> > > > > +		rtc_timer_cancel(rtc, &rtc->aie_timer);
> > > > > +		break;
> > > > > +	case PM_SUSPEND_LATE:
> > > > > +		if (rtc_read_time(rtc, &alm.time))
> > > > > +			return NOTIFY_BAD;
> > > > > +
> > > > > +		now = rtc_tm_to_time64(&alm.time);
> > > > > +		memset(&alm, 0, sizeof(alm));
> > > > > +		rtc_time64_to_tm(now + rtc_wake_override_sec,
> > > > > &alm.time);
> > > > > +		alm.enabled = true;
> > > > > +		if (rtc_set_alarm(rtc, &alm))
> > > > > +			return NOTIFY_BAD;
> > 
> > ... because if rtc_wake_override_sec is not set, this sets the
> > alarm
> > to
> > now which is the current RTC time, ensuring the alarm will never
> > trigger.
> 
> No.
> As the code below
> > > > > 
> > > > > 	if (rtc_wake_override_sec == 0)
> > > > > +		return NOTIFY_OK;
> 
> We check for rtc_wake_override_sec at the beginning of the notifier
> callback. So it takes effect only if a) rtc_wake_override_sec is set,
> AND b) a suspend is triggered.
> 
> thanks,
> rui
> 
> 

Re: [PATCH 2/7] thermal: intel: pch: enhance overheat handling

[Rafael J. Wysocki]

On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> temperature above threshold") introduces delay loop mechanism that allows
> PCH temperature to go down below threshold during suspend so it won't
> block S0ix. And the default overall delay timeout is 1 second.
>
> However, in practice, we found that the time it takes to cool the PCH down
> below threshold highly depends on the initial PCH temperature when the
> delay starts, as well as the ambient temperature.
> And in some cases, the 1 second delay is not sufficient. As a result, the
> system stays in a shallower power state like PCx instead of S0ix, and
> drains the battery power, without user' notice.
>
> To make sure S0ix is not blocked by the PCH overheating, we
> 1. expand the default overall timeout to 60 seconds.
> 2. make sure the temperature is below threshold rather than equal to it.
> 3. move the delay to .suspend_noirq phase instead, in order to
>    a) do cooling delay with a more quiescent system
>    b) be aware of wakeup events during the long delay, because some wakeup
>       events (ACPI Power button Press, USB mouse, etc) become valid only
>       in .suspend_noirq phase and later.
>
> This may introduce longer suspend time, but only in the cases when the
> system overheats and Linux used to enter a shallower S2idle state, say,
> PCx instead of S0ix.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> ---
>  drivers/thermal/intel/intel_pch_thermal.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
> index 527c91f5960b..b7b32e2f5ae2 100644
> --- a/drivers/thermal/intel/intel_pch_thermal.c
> +++ b/drivers/thermal/intel/intel_pch_thermal.c
> @@ -70,8 +70,8 @@ static unsigned int delay_timeout = 100;
>  module_param(delay_timeout, int, 0644);
>  MODULE_PARM_DESC(delay_timeout, "amount of time delay for each iteration.");
>
> -/* Number of iterations for cooling delay, 10 counts by default for now */
> -static unsigned int delay_cnt = 10;
> +/* Number of iterations for cooling delay, 600 counts by default for now */
> +static unsigned int delay_cnt = 600;
>  module_param(delay_cnt, int, 0644);
>  MODULE_PARM_DESC(delay_cnt, "total number of iterations for time delay.");
>
> @@ -193,10 +193,11 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp)
>         return 0;
>  }
>
> +/* Cool the PCH when it's overheat in .suspend_noirq phase */
>  static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>  {
>         u8 tsel;
> -       u8 pch_delay_cnt = 1;
> +       int pch_delay_cnt = 1;
>         u16 pch_thr_temp, pch_cur_temp;
>
>         /* Shutdown the thermal sensor if it is not enabled by BIOS */
> @@ -233,7 +234,10 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>          * which helps to indentify the reason why S0ix entry was rejected.
>          */
>         while (pch_delay_cnt <= delay_cnt) {
> -               if (pch_cur_temp <= pch_thr_temp)
> +               if (pch_cur_temp < pch_thr_temp)
> +                       break;
> +
> +               if (pm_wakeup_pending())
>                         break;
>
>                 dev_warn(&ptd->pdev->dev,
> @@ -245,7 +249,7 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>                 pch_delay_cnt++;
>         }
>
> -       if (pch_cur_temp > pch_thr_temp)
> +       if (pch_cur_temp >= pch_thr_temp)
>                 dev_warn(&ptd->pdev->dev,
>                         "CPU-PCH is hot [%dC] even after delay, continue to suspend. S0ix might fail\n",
>                         pch_cur_temp);
> @@ -455,7 +459,7 @@ static void intel_pch_thermal_remove(struct pci_dev *pdev)
>         pci_disable_device(pdev);
>  }
>
> -static int intel_pch_thermal_suspend(struct device *device)
> +static int intel_pch_thermal_suspend_noirq(struct device *device)
>  {
>         struct pch_thermal_device *ptd = dev_get_drvdata(device);
>
> @@ -495,7 +499,7 @@ static const struct pci_device_id intel_pch_thermal_id[] = {
>  MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
>
>  static const struct dev_pm_ops intel_pch_pm_ops = {
> -       .suspend = intel_pch_thermal_suspend,
> +       .suspend_noirq = intel_pch_thermal_suspend_noirq,

IMO it would be better to put this change into a separate patch and
reorder the other changes after this one.  It is valid by itself.

>         .resume = intel_pch_thermal_resume,
>  };
>
> --

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Rafael J. Wysocki]

On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> PCH thermal sensor that monitors the PCH temperature and blocks the system
> from entering S0ix in case it overheats.
>
> Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> temperature above threshold") introduces a delay loop to cool the
> temperature down for this purpose.
>
> However, in practice, we found that the time it takes to cool the PCH down
> below threshold highly depends on the initial PCH temperature when the
> delay starts, as well as the ambient temperature.
>
> For example, on a Dell XPS 9360 laptop, the problem can be triggered
> 1. when it is suspended with heavy workload running.
> or
> 2. when it is moved from New Hampshire to Florida.
>
> In these cases, the 1 second delay is not sufficient. As a result, the
> system stays in a shallower power state like PCx instead of S0ix, and
> drains the battery power, without user' notice.
>
> In this patch series, we first fix the problem in patch 1/7 ~ 3/7, by
> 1. expand the default overall cooling delay timeout to 60 seconds.
> 2. make sure the temperature is below threshold rather than equal to it.
> 3. move the delay to .suspend_noirq phase instead, in order to
>    a) do the cooling when the system is in a more quiescent state
>    b) be aware of wakeup events during the long delay, because some wakeup
>       events (ACPI Power button Press, USB mouse, etc) become valid only
>       in .suspend_noirq phase and later.
>
> However, this potential long delay introduces a problem to our suspend
> stress automation test, because the delay makes it hard to predict how
> much time it takes to suspend the system.
> As we want to do as much suspend iterations as possible in limited time,
> setting a 60+ seconds rtc alarm for suspend which usually takes shorter
> than 1 second is far beyond overkill.
>
> Thus, in patch 4/7 ~ 7/7, a rtc driver hook is introduced, which cancels
> the armed rtc alarm in the beginning of suspend and then rearm the rtc
> alarm with a short interval (say, 2 second) right before system suspended.
>
> By running
>  # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> before suspend, the system can be resumed by RTC alarm right after it is
> suspended, no matter how much time the suspend really takes.
>
> This patch series has been tested on the same Dell XPS 9360 laptop and
> S0ix is 100% achieved across 1000+ s2idle iterations.

Overall, the first three patches in the series can go in without the
rest, so let's put them into a separate series.

Patch [4/7] doesn't depend on the first three ones, so it can go in by itself.

Patch [5/7] is to be dropped anyway as per the earlier discussion.

Patch [6/7] is only needed to apply patch [7/7] which is controversial.

I think that we can drop or defer patches [6-7/7] for now.

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Rafael J. Wysocki]

On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Automated suspend/resume testing uses the RTC for wakeup.
> A short rtcwake period is desirable, so that more suspend/resume
> cycles can be completed, while the machine is available for testing.
>
> But if too short a wake interval is specified, the event can occur,
> while still suspending, and then no event wakes the suspended system
> until the user notices that testing has stalled, and manually intervenes.

If the wakeup event occurs while still suspending, it should abort the
suspend in progress, shouldn't it?  But the above implies that it
doesn't do that.

If this is fixed, wouldn't it address the issue at hand?

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Alexandre Belloni]

On 17/05/2022 17:11:05+0200, Rafael J. Wysocki wrote:
> On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> > PCH thermal sensor that monitors the PCH temperature and blocks the system
> > from entering S0ix in case it overheats.
> >
> > Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> > temperature above threshold") introduces a delay loop to cool the
> > temperature down for this purpose.
> >
> > However, in practice, we found that the time it takes to cool the PCH down
> > below threshold highly depends on the initial PCH temperature when the
> > delay starts, as well as the ambient temperature.
> >
> > For example, on a Dell XPS 9360 laptop, the problem can be triggered
> > 1. when it is suspended with heavy workload running.
> > or
> > 2. when it is moved from New Hampshire to Florida.
> >
> > In these cases, the 1 second delay is not sufficient. As a result, the
> > system stays in a shallower power state like PCx instead of S0ix, and
> > drains the battery power, without user' notice.
> >
> > In this patch series, we first fix the problem in patch 1/7 ~ 3/7, by
> > 1. expand the default overall cooling delay timeout to 60 seconds.
> > 2. make sure the temperature is below threshold rather than equal to it.
> > 3. move the delay to .suspend_noirq phase instead, in order to
> >    a) do the cooling when the system is in a more quiescent state
> >    b) be aware of wakeup events during the long delay, because some wakeup
> >       events (ACPI Power button Press, USB mouse, etc) become valid only
> >       in .suspend_noirq phase and later.
> >
> > However, this potential long delay introduces a problem to our suspend
> > stress automation test, because the delay makes it hard to predict how
> > much time it takes to suspend the system.
> > As we want to do as much suspend iterations as possible in limited time,
> > setting a 60+ seconds rtc alarm for suspend which usually takes shorter
> > than 1 second is far beyond overkill.
> >
> > Thus, in patch 4/7 ~ 7/7, a rtc driver hook is introduced, which cancels
> > the armed rtc alarm in the beginning of suspend and then rearm the rtc
> > alarm with a short interval (say, 2 second) right before system suspended.
> >
> > By running
> >  # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> > before suspend, the system can be resumed by RTC alarm right after it is
> > suspended, no matter how much time the suspend really takes.
> >
> > This patch series has been tested on the same Dell XPS 9360 laptop and
> > S0ix is 100% achieved across 1000+ s2idle iterations.
> 
> Overall, the first three patches in the series can go in without the
> rest, so let's put them into a separate series.
> 
> Patch [4/7] doesn't depend on the first three ones, so it can go in by itself.
> 
> Patch [5/7] is to be dropped anyway as per the earlier discussion.
> 
> Patch [6/7] is only needed to apply patch [7/7] which is controversial.
> 
> I think that we can drop or defer patches [6-7/7] for now.

I don't think 7/7 is really useful in the upstream kernel, I don't plan
to apply it

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] PM: Solution for S0ix failure caused by PCH overheating

[Zhang Rui]

Hi, Rafael,

On Tue, 2022-05-17 at 17:11 +0200, Rafael J. Wysocki wrote:
> On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > 
> > On some Intel client platforms like SKL/KBL/CNL/CML, there is a
> > PCH thermal sensor that monitors the PCH temperature and blocks the
> > system
> > from entering S0ix in case it overheats.
> > 
> > Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to
> > PCH
> > temperature above threshold") introduces a delay loop to cool the
> > temperature down for this purpose.
> > 
> > However, in practice, we found that the time it takes to cool the
> > PCH down
> > below threshold highly depends on the initial PCH temperature when
> > the
> > delay starts, as well as the ambient temperature.
> > 
> > For example, on a Dell XPS 9360 laptop, the problem can be
> > triggered
> > 1. when it is suspended with heavy workload running.
> > or
> > 2. when it is moved from New Hampshire to Florida.
> > 
> > In these cases, the 1 second delay is not sufficient. As a result,
> > the
> > system stays in a shallower power state like PCx instead of S0ix,
> > and
> > drains the battery power, without user' notice.
> > 
> > In this patch series, we first fix the problem in patch 1/7 ~ 3/7,
> > by
> > 1. expand the default overall cooling delay timeout to 60 seconds.
> > 2. make sure the temperature is below threshold rather than equal
> > to it.
> > 3. move the delay to .suspend_noirq phase instead, in order to
> >    a) do the cooling when the system is in a more quiescent state
> >    b) be aware of wakeup events during the long delay, because some
> > wakeup
> >       events (ACPI Power button Press, USB mouse, etc) become valid
> > only
> >       in .suspend_noirq phase and later.
> > 
> > However, this potential long delay introduces a problem to our
> > suspend
> > stress automation test, because the delay makes it hard to predict
> > how
> > much time it takes to suspend the system.
> > As we want to do as much suspend iterations as possible in limited
> > time,
> > setting a 60+ seconds rtc alarm for suspend which usually takes
> > shorter
> > than 1 second is far beyond overkill.
> > 
> > Thus, in patch 4/7 ~ 7/7, a rtc driver hook is introduced, which
> > cancels
> > the armed rtc alarm in the beginning of suspend and then rearm the
> > rtc
> > alarm with a short interval (say, 2 second) right before system
> > suspended.
> > 
> > By running
> >  # echo 2 > /sys/module/rtc_cmos/parameters/rtc_wake_override_sec
> > before suspend, the system can be resumed by RTC alarm right after
> > it is
> > suspended, no matter how much time the suspend really takes.
> > 
> > This patch series has been tested on the same Dell XPS 9360 laptop
> > and
> > S0ix is 100% achieved across 1000+ s2idle iterations.
> 
> Overall, the first three patches in the series can go in without the
> rest, so let's put them into a separate series.
> 
> Patch [4/7] doesn't depend on the first three ones, so it can go in
> by itself.
> 
> Patch [5/7] is to be dropped anyway as per the earlier discussion.
> 
> Patch [6/7] is only needed to apply patch [7/7] which is
> controversial.
> 
> I think that we can drop or defer patches [6-7/7] for now.

This all sounds reasonable to me.
I will resend them separately.

-rui

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

On Tue, 2022-05-17 at 17:14 +0200, Rafael J. Wysocki wrote:
> On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > 
> > Automated suspend/resume testing uses the RTC for wakeup.
> > A short rtcwake period is desirable, so that more suspend/resume
> > cycles can be completed, while the machine is available for
> > testing.
> > 
> > But if too short a wake interval is specified, the event can occur,
> > while still suspending, and then no event wakes the suspended
> > system
> > until the user notices that testing has stalled, and manually
> > intervenes.
> 
> If the wakeup event occurs while still suspending, it should abort
> the
> suspend in progress, shouldn't it?  But the above implies that it
> doesn't do that.
> 
> If this is fixed, wouldn't it address the issue at hand?

I think the rootcause of the original problem is that
1. on some systems, the ACPI RTC Fixed event is used during suspend
only, and the ACPI Fixed event is enabled in the rtc-cmos driver
.suspend() callback
and
2. if the RTC Alarm already expires before .suspend() invoked, we will
lose the ACPI RTC Fixed Event as well as the wakeup event, say 20
seconds delay in freeze processes.

But, even if that problem is fixed, the suspend aborts and "fails" as
expected, this is still a problem for the suspend-automation scenario,
because the system actually can suspend successfully if we don't set
the RTC alarm too aggressively. And in PCH overheating case, surely we
will get false alarms, because we will never use a 60s+ rtc alarm for
suspend-automation.

thanks,
rui

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Rafael J. Wysocki]

On Wed, May 18, 2022 at 4:45 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Tue, 2022-05-17 at 17:14 +0200, Rafael J. Wysocki wrote:
> > On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > >
> > > Automated suspend/resume testing uses the RTC for wakeup.
> > > A short rtcwake period is desirable, so that more suspend/resume
> > > cycles can be completed, while the machine is available for
> > > testing.
> > >
> > > But if too short a wake interval is specified, the event can occur,
> > > while still suspending, and then no event wakes the suspended
> > > system
> > > until the user notices that testing has stalled, and manually
> > > intervenes.
> >
> > If the wakeup event occurs while still suspending, it should abort
> > the
> > suspend in progress, shouldn't it?  But the above implies that it
> > doesn't do that.
> >
> > If this is fixed, wouldn't it address the issue at hand?
>
> I think the rootcause of the original problem is that
> 1. on some systems, the ACPI RTC Fixed event is used during suspend
> only, and the ACPI Fixed event is enabled in the rtc-cmos driver
> .suspend() callback
> and
> 2. if the RTC Alarm already expires before .suspend() invoked, we will
> lose the ACPI RTC Fixed Event as well as the wakeup event, say 20
> seconds delay in freeze processes.

Well, the RTC Fixed event can be armed in a PM/HIBERNATE notifier and
if it fires before .suspend() runs, system wakeup can be triggered
from there.

> But, even if that problem is fixed, the suspend aborts and "fails" as
> expected, this is still a problem for the suspend-automation scenario,
> because the system actually can suspend successfully if we don't set
> the RTC alarm too aggressively. And in PCH overheating case, surely we
> will get false alarms, because we will never use a 60s+ rtc alarm for
> suspend-automation.

I'm not sure why this is a problem.

It only means that occasionally the system will not reach the final
"suspended" state, but that can happen regardless.

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Zhang Rui]

On Wed, 2022-05-18 at 17:02 +0200, Rafael J. Wysocki wrote:
> On Wed, May 18, 2022 at 4:45 PM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > On Tue, 2022-05-17 at 17:14 +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > Automated suspend/resume testing uses the RTC for wakeup.
> > > > A short rtcwake period is desirable, so that more
> > > > suspend/resume
> > > > cycles can be completed, while the machine is available for
> > > > testing.
> > > > 
> > > > But if too short a wake interval is specified, the event can
> > > > occur,
> > > > while still suspending, and then no event wakes the suspended
> > > > system
> > > > until the user notices that testing has stalled, and manually
> > > > intervenes.
> > > 
> > > If the wakeup event occurs while still suspending, it should
> > > abort
> > > the
> > > suspend in progress, shouldn't it?  But the above implies that it
> > > doesn't do that.
> > > 
> > > If this is fixed, wouldn't it address the issue at hand?
> > 
> > I think the rootcause of the original problem is that
> > 1. on some systems, the ACPI RTC Fixed event is used during suspend
> > only, and the ACPI Fixed event is enabled in the rtc-cmos driver
> > .suspend() callback
> > and
> > 2. if the RTC Alarm already expires before .suspend() invoked, we
> > will
> > lose the ACPI RTC Fixed Event as well as the wakeup event, say 20
> > seconds delay in freeze processes.
> 
> Well, the RTC Fixed event can be armed in a PM/HIBERNATE notifier and
> if it fires before .suspend() runs, system wakeup can be triggered
> from there.

Agreed.

Len,
Do you recall any other case that we may miss the RTC wakeup event?

> 
> > But, even if that problem is fixed, the suspend aborts and "fails"
> > as
> > expected, this is still a problem for the suspend-automation
> > scenario,
> > because the system actually can suspend successfully if we don't
> > set
> > the RTC alarm too aggressively. And in PCH overheating case, surely
> > we
> > will get false alarms, because we will never use a 60s+ rtc alarm
> > for
> > suspend-automation.
> 
> I'm not sure why this is a problem.
> 
> It only means that occasionally the system will not reach the final
> "suspended" state, but that can happen regardless.

It is not a kernel problem.
It is a problem for suspend-automation. Because suspend-automation is
chasing for kernel suspend problems, and IMO, cases like suspend aborts
because of long suspend delay from PCH thermal driver are not kernel
problems.

It would be nice to leverage a kernel I/F to get rid of such issues, 
But if the patch is rejected, I agree we can live without it.

thanks,
rui

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Len Brown]

First let's agree on why this should not be ignored.

Our development team at Intel has lab with laptops, we run sleepgraph
on every RC, and we publish the tool in public:
https://www.intel.com/content/www/us/en/developer/topic-technology/open/pm-graph/overview.html

But even if we were funded to do it (which we are not), we can't
possibly test every kind of device.
We need the community to help testing Linux (suspend/resume,
specifically) on a broad range of devices, so together we can make it
better for all.

The community is made up mostly of users, rather than kernel hackers,
and so this effectively means that distro binary kernels need to be
able to support testing.

Enabling that broad community of users/contributors is the goal.

As Rui explained, this patch does nothing and breaks nothing if the
new hook remains unused.
If it is used, then overrides the wakeup duration for all subsequent
system suspends, until it is cleared.
If it does more than that, or does that in a clumsy way, then let's fix that.

Today it gives us two new capabilities:

1. Prevents a lost wake event.  Commonly we see this with kcompatd
taking 20 seconds when we had previously armed the RTC for 15 seconds.
The system will sleep forever, until the user intervenes -- which may
be a very long time later.

Rafael, If you have a better way to fix that, I'm all ears.  Aborted
suspend flows are ugly -- particularly when the user didn't want them,
but they are much less ugly then losing a wake event, which can result
in losing, say 10-hours of test time.

2. Allows more suspends/resume cycles per time.  Say the early wake is
fixed.  Then we have to decide how long to sleep before being
suspended.  If we set it for 1 second, and suspend takes longer than 1
second, then all of our tests will fail with early wakeups and we have
tested nothing.  If we set it to 60 seconds, and suspend takes 1
second, then 59/60 seconds are spent sleeping, when they could be
spent testing Linux.  With this patch, we can set it to the minimum of
2 seconds right before we sleep, guaranteeing that we spend at least 1
second, and under 2 seconds sleeping, and the rest of the time testing
-- which allows us to meet the goal.

thanks,
Len Brown, Intel

Re: [PATCH 7/7] rtc: cmos: Add suspend/resume endurance testing hook

[Rafael J. Wysocki]

On Thu, May 19, 2022 at 4:33 AM Len Brown <lenb@kernel.org> wrote:
>
> First let's agree on why this should not be ignored.
>
> Our development team at Intel has lab with laptops, we run sleepgraph
> on every RC, and we publish the tool in public:
> https://www.intel.com/content/www/us/en/developer/topic-technology/open/pm-graph/overview.html
>
> But even if we were funded to do it (which we are not), we can't
> possibly test every kind of device.
> We need the community to help testing Linux (suspend/resume,
> specifically) on a broad range of devices, so together we can make it
> better for all.
>
> The community is made up mostly of users, rather than kernel hackers,
> and so this effectively means that distro binary kernels need to be
> able to support testing.
>
> Enabling that broad community of users/contributors is the goal.
>
> As Rui explained, this patch does nothing and breaks nothing if the
> new hook remains unused.
> If it is used, then overrides the wakeup duration for all subsequent
> system suspends, until it is cleared.
> If it does more than that, or does that in a clumsy way, then let's fix that.
>
> Today it gives us two new capabilities:
>
> 1. Prevents a lost wake event.  Commonly we see this with kcompatd
> taking 20 seconds when we had previously armed the RTC for 15 seconds.
> The system will sleep forever, until the user intervenes -- which may
> be a very long time later.
>
> Rafael, If you have a better way to fix that, I'm all ears.  Aborted
> suspend flows are ugly -- particularly when the user didn't want them,
> but they are much less ugly then losing a wake event, which can result
> in losing, say 10-hours of test time.

The real problem here is the missed wakeup events and I've already
said in this thread how this can be fixed and Rui appears to agree
with me.

So I'd say why don't we just go and fix it?

And it is orthogonal to the first 3 patches in this series, because
they move the PCH thermal delay to the noirq phase which is later than
the arming of the RTC Fixed Event IIUC.

> 2. Allows more suspends/resume cycles per time.  Say the early wake is
> fixed.  Then we have to decide how long to sleep before being
> suspended.  If we set it for 1 second, and suspend takes longer than 1
> second, then all of our tests will fail with early wakeups and we have
> tested nothing.

We have tested "early" wakeups which is what the users would see on
the system in question if they set the RTC wake time to 1 second
before suspending.

This may not be what we want to test, though, but that is a different
problem, as discussed below.

> If we set it to 60 seconds, and suspend takes 1
> second, then 59/60 seconds are spent sleeping, when they could be
> spent testing Linux.  With this patch, we can set it to the minimum of
> 2 seconds right before we sleep, guaranteeing that we spend at least 1
> second, and under 2 seconds sleeping, and the rest of the time testing
> -- which allows us to meet the goal.

So the goal specifically is to test the last phase of system suspend
and in particular whether or not the platform has reached the specific
minimum-power state at the end of it.

In order to do that, we basically want to ignore all of the wakeup
events except for the special RTC wakeup 1 second after the platform
has been asked to get into the minimum-power state, so what we are
talking about here really is a special suspend test mode using the RTC
as a wakeup vehicle.