[PATCH v2 0/4] platform/x86: intel_telemetry: Fix logs and formatting

[PATCH v2 0/4] platform/x86: intel_telemetry: Fix logs and formatting

[Souvik Kumar Chakravarty]

This patchset fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833, and
other issues related to telemetry counters. It also cleans up formatting
and removes redundant code.

It is rebased on top of the TESTING branch.

Code-Review comments have been incorporated.

Souvik Kumar Chakravarty (4):
  platform/x86: intel_pmc_ipc: Add readq API for GCR
  platform/x86: intel_telemetry: Fix suspend stats
  platform/x86: intel_telemetry: Improve S0ix logs
  platform/x86: intel_telemetry: Remove redundancies

 arch/x86/include/asm/intel_pmc_ipc.h           | 10 ++-
 drivers/platform/x86/intel_pmc_ipc.c           | 37 +++++++++--
 drivers/platform/x86/intel_telemetry_debugfs.c | 87 +++++++++++---------------
 3 files changed, 78 insertions(+), 56 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] platform/x86: intel_pmc_ipc: Add readq API for GCR

[Souvik Kumar Chakravarty]

Add intel_pmc_gcr_readq API for reading from 64-bit GCR registers.
This API will be called from intel_telemetry. Rename intel_pmc_gcr_read
to more appropriate intel_pmc_gcr_readl.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h | 10 ++++++++--
 drivers/platform/x86/intel_pmc_ipc.c | 37 ++++++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index fac89eb..0aa5be9 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -36,7 +36,8 @@ int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
 int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
 		u32 *out, u32 outlen);
 int intel_pmc_s0ix_counter_read(u64 *data);
-int intel_pmc_gcr_read(u32 offset, u32 *data);
+int intel_pmc_gcr_readl(u32 offset, u32 *data);
+int intel_pmc_gcr_readq(u32 offset, u64 *data);
 int intel_pmc_gcr_write(u32 offset, u32 data);
 int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
@@ -64,7 +65,12 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
-static inline int intel_pmc_gcr_read(u32 offset, u32 *data)
+static inline int intel_pmc_gcr_readl(u32 offset, u32 *data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_readq(u32 offset, u64 *data)
 {
 	return -EINVAL;
 }
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index e03fa314..bef9c57 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -215,15 +215,15 @@ static inline int is_gcr_valid(u32 offset)
 }
 
 /**
- * intel_pmc_gcr_read() - Read PMC GCR register
+ * intel_pmc_gcr_readl() - Read a 32-bit PMC GCR register
  * @offset:	offset of GCR register from GCR address base
  * @data:	data pointer for storing the register output
  *
- * Reads the PMC GCR register of given offset.
+ * Reads the 32-bit PMC GCR register at given offset.
  *
  * Return:	negative value on error or 0 on success.
  */
-int intel_pmc_gcr_read(u32 offset, u32 *data)
+int intel_pmc_gcr_readl(u32 offset, u32 *data)
 {
 	int ret;
 
@@ -241,7 +241,36 @@ int intel_pmc_gcr_read(u32 offset, u32 *data)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_readl);
+
+/**
+ * intel_pmc_gcr_readq() - Read a 64-bit PMC GCR register
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	data pointer for storing the register output
+ *
+ * Reads the 64-bit PMC GCR register at given offset.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_readq(u32 offset, u64 *data)
+{
+	int ret;
+
+	spin_lock(&ipcdev.gcr_lock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		spin_unlock(&ipcdev.gcr_lock);
+		return ret;
+	}
+
+	*data = readq(ipcdev.gcr_mem_base + offset);
+
+	spin_unlock(&ipcdev.gcr_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_readq);
 
 /**
  * intel_pmc_gcr_write() - Write PMC GCR register
-- 
2.7.4

[PATCH v2 2/4] platform/x86: intel_telemetry: Fix suspend stats

[Souvik Kumar Chakravarty]

Suspend stats are not reported consistently due to a limitation in the PMC
firmware. This limitation causes a delay in updating the s0ix counters and
residencies in the telemetry log upon s0ix exit. As a consequence, reading
these counters from the suspend-exit notifier may result in zero read.

This patch fixes this issue by cross-verifying the s0ix residencies from
the GCR TELEM registers in case the counters are not incremented in the
telemetry log after suspend.

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833

We also remove unnecessary 'static' qualifiers from local variables.

Reported-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 29 ++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Changes since v1:
 * Use pmc_ipc_gcr_readq API to read 64-bits at a time

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index 4249e826..d7e90fd 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -855,9 +855,9 @@ static int pm_suspend_prep_cb(void)
 static int pm_suspend_exit_cb(void)
 {
 	struct telemetry_evtlog evtlog[TELEM_MAX_OS_ALLOCATED_EVENTS];
-	static u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
-	static u64 suspend_shlw_res_exit, suspend_deep_res_exit;
 	struct telemetry_debugfs_conf *conf = debugfs_conf;
+	u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
+	u64 suspend_shlw_res_exit, suspend_deep_res_exit;
 	int ret, index;
 
 	if (!suspend_prep_ok)
@@ -890,6 +890,31 @@ static int pm_suspend_exit_cb(void)
 		goto out;
 	}
 
+	/*
+	 * Due to some design limitations in the firmware, sometimes the
+	 * counters do not get updated by the time we reach here. As a
+	 * workaround, we try to see if this was a genuine case of sleep
+	 * failure or not by cross-checking from PMC GCR registers directly.
+	 */
+	if (suspend_shlw_ctr_exit == suspend_shlw_ctr_temp &&
+	    suspend_deep_ctr_exit == suspend_deep_ctr_temp) {
+		ret = intel_pmc_gcr_readq(PMC_GCR_TELEM_SHLW_S0IX_REG,
+					  &suspend_shlw_res_exit);
+		if (ret < 0)
+			goto out;
+
+		ret = intel_pmc_gcr_readq(PMC_GCR_TELEM_DEEP_S0IX_REG,
+					  &suspend_deep_res_exit);
+		if (ret < 0)
+			goto out;
+
+		if (suspend_shlw_res_exit > suspend_shlw_res_temp)
+			suspend_shlw_ctr_exit++;
+
+		if (suspend_deep_res_exit > suspend_deep_res_temp)
+			suspend_deep_ctr_exit++;
+	}
+
 	suspend_shlw_ctr_exit -= suspend_shlw_ctr_temp;
 	suspend_deep_ctr_exit -= suspend_deep_ctr_temp;
 	suspend_shlw_res_exit -= suspend_shlw_res_temp;
-- 
2.7.4

[PATCH v2 3/4] platform/x86: intel_telemetry: Improve S0ix logs

[Souvik Kumar Chakravarty]

Suspend with shallow wakes is not a useful parameter since the phenomena
does not exist on deployed devices and is only a parameter of use during
device power-on phase. The field always reads zero. Additionally there
are other easier methods to detect it, e.g., if the S0ix counter
increments by more than one during suspend. Hence the field is superfluous
and can be removed.

This patch also slightly renames the S0ix total field for better
viewability.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 45 ++++----------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

Changes since v1:
 * Remove alignment changes from this patch

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index d7e90fd..ab4a20f 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -98,10 +98,6 @@ static u32 suspend_shlw_ctr_temp, suspend_deep_ctr_temp;
 static u64 suspend_shlw_res_temp, suspend_deep_res_temp;
 
 struct telemetry_susp_stats {
-	u32 shlw_swake_ctr;
-	u32 deep_swake_ctr;
-	u64 shlw_swake_res;
-	u64 deep_swake_res;
 	u32 shlw_ctr;
 	u32 deep_ctr;
 	u64 shlw_res;
@@ -598,19 +594,15 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 
 	seq_printf(s, "S0IX Shallow\t\t\t %10u\t %10llu\n",
 		   s0ix_shlw_ctr -
-		   conf->suspend_stats.shlw_ctr -
-		   conf->suspend_stats.shlw_swake_ctr,
+		   conf->suspend_stats.shlw_ctr,
 		   (u64)((s0ix_shlw_res -
-		   conf->suspend_stats.shlw_res -
-		   conf->suspend_stats.shlw_swake_res)*10/192));
+		   conf->suspend_stats.shlw_res)*10/192));
 
 	seq_printf(s, "S0IX Deep\t\t\t %10u\t %10llu\n",
 		   s0ix_deep_ctr -
-		   conf->suspend_stats.deep_ctr -
-		   conf->suspend_stats.deep_swake_ctr,
+		   conf->suspend_stats.deep_ctr,
 		   (u64)((s0ix_deep_res -
-		   conf->suspend_stats.deep_res -
-		   conf->suspend_stats.deep_swake_res)*10/192));
+		   conf->suspend_stats.deep_res)*10/192));
 
 	seq_printf(s, "Suspend(With S0ixShallow)\t %10u\t %10llu\n",
 		   conf->suspend_stats.shlw_ctr,
@@ -620,13 +612,7 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 		   conf->suspend_stats.deep_ctr,
 		   (u64)(conf->suspend_stats.deep_res*10)/192);
 
-	seq_printf(s, "Suspend(With Shallow-Wakes)\t %10u\t %10llu\n",
-		   conf->suspend_stats.shlw_swake_ctr +
-		   conf->suspend_stats.deep_swake_ctr,
-		   (u64)((conf->suspend_stats.shlw_swake_res +
-		   conf->suspend_stats.deep_swake_res)*10/192));
-
-	seq_printf(s, "S0IX+Suspend Total\t\t %10u\t %10llu\n", s0ix_total_ctr,
+	seq_printf(s, "TOTAL S0IX\t\t\t %10u\t %10llu\n", s0ix_total_ctr,
 				(u64)(s0ix_total_res*10/192));
 	seq_puts(s, "\n-------------------------------------------------\n");
 	seq_puts(s, "\t\tDEVICE STATES\n");
@@ -920,23 +906,15 @@ static int pm_suspend_exit_cb(void)
 	suspend_shlw_res_exit -= suspend_shlw_res_temp;
 	suspend_deep_res_exit -= suspend_deep_res_temp;
 
-	if (suspend_shlw_ctr_exit == 1) {
+	if (suspend_shlw_ctr_exit != 0) {
 		conf->suspend_stats.shlw_ctr +=
 		suspend_shlw_ctr_exit;
 
 		conf->suspend_stats.shlw_res +=
 		suspend_shlw_res_exit;
 	}
-	/* Shallow Wakes Case */
-	else if (suspend_shlw_ctr_exit > 1) {
-		conf->suspend_stats.shlw_swake_ctr +=
-		suspend_shlw_ctr_exit;
-
-		conf->suspend_stats.shlw_swake_res +=
-		suspend_shlw_res_exit;
-	}
 
-	if (suspend_deep_ctr_exit == 1) {
+	if (suspend_deep_ctr_exit != 0) {
 		conf->suspend_stats.deep_ctr +=
 		suspend_deep_ctr_exit;
 
@@ -944,15 +922,6 @@ static int pm_suspend_exit_cb(void)
 		suspend_deep_res_exit;
 	}
 
-	/* Shallow Wakes Case */
-	else if (suspend_deep_ctr_exit > 1) {
-		conf->suspend_stats.deep_swake_ctr +=
-		suspend_deep_ctr_exit;
-
-		conf->suspend_stats.deep_swake_res +=
-		suspend_deep_res_exit;
-	}
-
 out:
 	suspend_prep_ok = 0;
 	return NOTIFY_OK;
-- 
2.7.4

[PATCH v2 4/4] platform/x86: intel_telemetry: Remove redundancies

[Souvik Kumar Chakravarty]

This patch removes unnecessary header files and newlines.
It also fixes some alignment issues.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Changes since v1:
 * Consolidated alignment changes into this patch

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index ab4a20f..6e0a9dd 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -23,7 +23,6 @@
  */
 #include <linux/debugfs.h>
 #include <linux/device.h>
-#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/seq_file.h>
@@ -32,11 +31,10 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/intel_pmc_ipc.h>
-#include <asm/intel_punit_ipc.h>
 #include <asm/intel_telemetry.h>
 
-#define DRIVER_NAME	"telemetry_soc_debugfs"
-#define DRIVER_VERSION	"1.0.0"
+#define DRIVER_NAME			"telemetry_soc_debugfs"
+#define DRIVER_VERSION			"1.0.0"
 
 /* ApolloLake SoC Event-IDs */
 #define TELEM_APL_PSS_PSTATES_ID	0x2802
@@ -246,7 +244,6 @@ static struct telem_ioss_pg_info telem_apl_ioss_pg_data[] = {
 	{"PRTC",	25},
 };
 
-
 struct telemetry_debugfs_conf {
 	struct telemetry_susp_stats suspend_stats;
 	struct dentry *telemetry_dbg_dir;
@@ -381,7 +378,6 @@ static int telem_pss_states_show(struct seq_file *s, void *unused)
 			TELEM_APL_MASK_PCS_STATE;
 		}
 
-
 		TELEM_CHECK_AND_PARSE_EVTS(conf->pss_idle_id,
 					   conf->pss_idle_evts - 1,
 					   pss_idle, evtlog[index].telem_evtlog,
@@ -401,7 +397,6 @@ static int telem_pss_states_show(struct seq_file *s, void *unused)
 					   conf->pcs_s0ix_blkd_data,
 					   TELEM_MASK_BYTE);
 
-
 		TELEM_CHECK_AND_PARSE_EVTS(conf->pss_wakeup_id,
 					   conf->pss_wakeup_evts,
 					   pss_s0ix_wakeup,
@@ -494,7 +489,6 @@ static const struct file_operations telem_pss_ops = {
 	.release	= single_release,
 };
 
-
 static int telem_ioss_states_show(struct seq_file *s, void *unused)
 {
 	struct telemetry_evtlog evtlog[TELEM_MAX_OS_ALLOCATED_EVENTS];
@@ -613,7 +607,7 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 		   (u64)(conf->suspend_stats.deep_res*10)/192);
 
 	seq_printf(s, "TOTAL S0IX\t\t\t %10u\t %10llu\n", s0ix_total_ctr,
-				(u64)(s0ix_total_res*10/192));
+		   (u64)(s0ix_total_res*10/192));
 	seq_puts(s, "\n-------------------------------------------------\n");
 	seq_puts(s, "\t\tDEVICE STATES\n");
 	seq_puts(s, "-------------------------------------------------\n");
@@ -758,7 +752,6 @@ static const struct file_operations telem_pss_trc_verb_ops = {
 	.release	= single_release,
 };
 
-
 static int telem_ioss_trc_verb_show(struct seq_file *s, void *unused)
 {
 	u32 verbosity;
-- 
2.7.4

Re: [PATCH v2 2/4] platform/x86: intel_telemetry: Fix suspend stats

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5236 bytes --]

Hi Souvik,

I love your patch! Perhaps something to improve:

[auto build test WARNING on platform-drivers-x86/for-next]
[also build test WARNING on v4.14 next-20171122]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Souvik-Kumar-Chakravarty/platform-x86-intel_telemetry-Fix-logs-and-formatting/20171123-052155
base:   git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/platform/x86/intel_telemetry_debugfs.c: In function 'pm_suspend_exit_cb':
>> drivers/platform/x86/intel_telemetry_debugfs.c:915:25: warning: 'suspend_deep_ctr_exit' may be used uninitialized in this function [-Wmaybe-uninitialized]
       suspend_deep_ctr_exit++;
       ~~~~~~~~~~~~~~~~~~~~~^~
>> drivers/platform/x86/intel_telemetry_debugfs.c:912:25: warning: 'suspend_shlw_ctr_exit' may be used uninitialized in this function [-Wmaybe-uninitialized]
       suspend_shlw_ctr_exit++;
       ~~~~~~~~~~~~~~~~~~~~~^~

vim +/suspend_deep_ctr_exit +915 drivers/platform/x86/intel_telemetry_debugfs.c

   854	
   855	static int pm_suspend_exit_cb(void)
   856	{
   857		struct telemetry_evtlog evtlog[TELEM_MAX_OS_ALLOCATED_EVENTS];
   858		struct telemetry_debugfs_conf *conf = debugfs_conf;
   859		u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
   860		u64 suspend_shlw_res_exit, suspend_deep_res_exit;
   861		int ret, index;
   862	
   863		if (!suspend_prep_ok)
   864			goto out;
   865	
   866		ret = telemetry_raw_read_eventlog(TELEM_IOSS, evtlog,
   867						  TELEM_MAX_OS_ALLOCATED_EVENTS);
   868		if (ret < 0)
   869			goto out;
   870	
   871		for (index = 0; index < ret; index++) {
   872			TELEM_CHECK_AND_PARSE_CTRS(conf->s0ix_shlw_occ_id,
   873						   suspend_shlw_ctr_exit);
   874	
   875			TELEM_CHECK_AND_PARSE_CTRS(conf->s0ix_deep_occ_id,
   876						   suspend_deep_ctr_exit);
   877	
   878			TELEM_CHECK_AND_PARSE_CTRS(conf->s0ix_shlw_res_id,
   879						   suspend_shlw_res_exit);
   880	
   881			TELEM_CHECK_AND_PARSE_CTRS(conf->s0ix_deep_res_id,
   882						   suspend_deep_res_exit);
   883		}
   884	
   885		if ((suspend_shlw_ctr_exit < suspend_shlw_ctr_temp) ||
   886		    (suspend_deep_ctr_exit < suspend_deep_ctr_temp) ||
   887		    (suspend_shlw_res_exit < suspend_shlw_res_temp) ||
   888		    (suspend_deep_res_exit < suspend_deep_res_temp)) {
   889			pr_err("Wrong s0ix counters detected\n");
   890			goto out;
   891		}
   892	
   893		/*
   894		 * Due to some design limitations in the firmware, sometimes the
   895		 * counters do not get updated by the time we reach here. As a
   896		 * workaround, we try to see if this was a genuine case of sleep
   897		 * failure or not by cross-checking from PMC GCR registers directly.
   898		 */
   899		if (suspend_shlw_ctr_exit == suspend_shlw_ctr_temp &&
   900		    suspend_deep_ctr_exit == suspend_deep_ctr_temp) {
   901			ret = intel_pmc_gcr_readq(PMC_GCR_TELEM_SHLW_S0IX_REG,
   902						  &suspend_shlw_res_exit);
   903			if (ret < 0)
   904				goto out;
   905	
   906			ret = intel_pmc_gcr_readq(PMC_GCR_TELEM_DEEP_S0IX_REG,
   907						  &suspend_deep_res_exit);
   908			if (ret < 0)
   909				goto out;
   910	
   911			if (suspend_shlw_res_exit > suspend_shlw_res_temp)
 > 912				suspend_shlw_ctr_exit++;
   913	
   914			if (suspend_deep_res_exit > suspend_deep_res_temp)
 > 915				suspend_deep_ctr_exit++;
   916		}
   917	
   918		suspend_shlw_ctr_exit -= suspend_shlw_ctr_temp;
   919		suspend_deep_ctr_exit -= suspend_deep_ctr_temp;
   920		suspend_shlw_res_exit -= suspend_shlw_res_temp;
   921		suspend_deep_res_exit -= suspend_deep_res_temp;
   922	
   923		if (suspend_shlw_ctr_exit == 1) {
   924			conf->suspend_stats.shlw_ctr +=
   925			suspend_shlw_ctr_exit;
   926	
   927			conf->suspend_stats.shlw_res +=
   928			suspend_shlw_res_exit;
   929		}
   930		/* Shallow Wakes Case */
   931		else if (suspend_shlw_ctr_exit > 1) {
   932			conf->suspend_stats.shlw_swake_ctr +=
   933			suspend_shlw_ctr_exit;
   934	
   935			conf->suspend_stats.shlw_swake_res +=
   936			suspend_shlw_res_exit;
   937		}
   938	
   939		if (suspend_deep_ctr_exit == 1) {
   940			conf->suspend_stats.deep_ctr +=
   941			suspend_deep_ctr_exit;
   942	
   943			conf->suspend_stats.deep_res +=
   944			suspend_deep_res_exit;
   945		}
   946	
   947		/* Shallow Wakes Case */
   948		else if (suspend_deep_ctr_exit > 1) {
   949			conf->suspend_stats.deep_swake_ctr +=
   950			suspend_deep_ctr_exit;
   951	
   952			conf->suspend_stats.deep_swake_res +=
   953			suspend_deep_res_exit;
   954		}
   955	
   956	out:
   957		suspend_prep_ok = 0;
   958		return NOTIFY_OK;
   959	}
   960	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61051 bytes --]

Re: [PATCH v1 1/4] platform/x86: intel_pmc_ipc: Add readq API for GCR

[Andy Shevchenko]

On Tue, Nov 21, 2017 at 4:36 PM, Souvik Kumar Chakravarty
<souvik.k.chakravarty@intel.com> wrote:
> Add intel_pmc_gcr_readq API for reading from 64-bit GCR registers.
> This API will be called from intel_telemetry. Rename intel_pmc_gcr_read
> to more appropriate intel_pmc_gcr_readl.

>  int intel_pmc_s0ix_counter_read(u64 *data);
> -int intel_pmc_gcr_read(u32 offset, u32 *data);
> +int intel_pmc_gcr_readl(u32 offset, u32 *data);
> +int intel_pmc_gcr_readq(u32 offset, u64 *data);
>  int intel_pmc_gcr_write(u32 offset, u32 data);
>  int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);

Ah, I didn't notice that we have a bunch of related functions.

>From above it looks like we better to leave read as is and introduce
_read64() instead of _readq(). This makes names more clear in case we
would need _update64 in the future.

Something like below at the end:

int intel_pmc_gcr_read(u32 offset, u32 *data);
int intel_pmc_gcr_write(u32 offset, u32 data);
int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);

+int intel_pmc_gcr_read64(u32 offset, u64 *data);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] platform/x86: intel_telemetry: Fix suspend stats

[Andy Shevchenko]

On Tue, Nov 21, 2017 at 4:36 PM, Souvik Kumar Chakravarty
<souvik.k.chakravarty@intel.com> wrote:
> Suspend stats are not reported consistently due to a limitation in the PMC
> firmware. This limitation causes a delay in updating the s0ix counters and
> residencies in the telemetry log upon s0ix exit. As a consequence, reading
> these counters from the suspend-exit notifier may result in zero read.
>
> This patch fixes this issue by cross-verifying the s0ix residencies from
> the GCR TELEM registers in case the counters are not incremented in the
> telemetry log after suspend.
>
> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833
>
> We also remove unnecessary 'static' qualifiers from local variables.
>
> Reported-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>

> -       static u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
> -       static u64 suspend_shlw_res_exit, suspend_deep_res_exit;
>         struct telemetry_debugfs_conf *conf = debugfs_conf;
> +       u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
> +       u64 suspend_shlw_res_exit, suspend_deep_res_exit;
>         int ret, index;

> +       if (suspend_shlw_ctr_exit == suspend_shlw_ctr_temp &&
> +           suspend_deep_ctr_exit == suspend_deep_ctr_temp) {

kbuildbot is absolutely right. How this code is supposed to work? It's flaky.

Please, redesign this approach.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/4] platform/x86: intel_telemetry: Fix suspend stats

[Chakravarty, Souvik K]

On Fri, November 24, 2017 at 2:55 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 21, 2017 at 4:36 PM, Souvik Kumar Chakravarty
> <souvik.k.chakravarty@intel.com> wrote:
> > Suspend stats are not reported consistently due to a limitation in the
> > PMC firmware. This limitation causes a delay in updating the s0ix
> > counters and residencies in the telemetry log upon s0ix exit. As a
> > consequence, reading these counters from the suspend-exit notifier may result
> in zero read.
> >
> > This patch fixes this issue by cross-verifying the s0ix residencies
> > from the GCR TELEM registers in case the counters are not incremented
> > in the telemetry log after suspend.
> >
> > This fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833
> >
> > We also remove unnecessary 'static' qualifiers from local variables.
> >
> > Reported-and-tested-by: Rajneesh Bhardwaj
> > <rajneesh.bhardwaj@intel.com>
> > Signed-off-by: Souvik Kumar Chakravarty
> > <souvik.k.chakravarty@intel.com>
> 
> > -       static u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
> > -       static u64 suspend_shlw_res_exit, suspend_deep_res_exit;
> >         struct telemetry_debugfs_conf *conf = debugfs_conf;
> > +       u32 suspend_shlw_ctr_exit, suspend_deep_ctr_exit;
> > +       u64 suspend_shlw_res_exit, suspend_deep_res_exit;
> >         int ret, index;
> 
> > +       if (suspend_shlw_ctr_exit == suspend_shlw_ctr_temp &&
> > +           suspend_deep_ctr_exit == suspend_deep_ctr_temp) {
> 
> kbuildbot is absolutely right. How this code is supposed to work? It's flaky.

suspend_shlw_ctr_exit & suspend_deep_ctr_exit have already been initialized before this comparison (comparing the counters before and after sleep).
I will explicitly initialize them so that kbuildbot does not complain.
> 
> Please, redesign this approach.
> 
> --
> With Best Regards,
> Andy Shevchenko