[PATCH v4 0/3] powerpc/pseries: fix issues in suspend/resume code

[PATCH v4 0/3] powerpc/pseries: fix issues in suspend/resume code

[Tyrel Datwyler]

This patchset fixes a couple of issues encountered in the suspend/resume code
base. First when using the kernel device tree update code update-nodes is
unnecessarily called more than once. Second the cpu cache lists are not
updated after a suspend/resume which under certain conditions may cause a
panic. Finally, since the cache list fix utilzes in kernel device tree update
code a means for telling drmgr that updating the device tree from userspace
is unnecessary.

Changes from v3:
- Updated patch descriptions to better reflect the behavior/interface changes
  and why they are acceptable per Ben's concerns.

Changes from v2:
- Moved dynamic configuration update code into pseries specific routine
  per Nathan's suggestion.

Changes from v1:
- Fixed several commit message typos
- Fixed authorship of first two patches

Haren Myneni (2):
  powerpc/pseries: Device tree should only be updated once after
    suspend/migrate
  powerpc/pseries: Update dynamic cache nodes for suspend/resume
    operation

Tyrel Datwyler (1):
  powerpc/pseries: Report in kernel device tree update to drmgr

 arch/powerpc/include/asm/rtas.h           |  1 +
 arch/powerpc/platforms/pseries/mobility.c | 26 +++++++-----------
 arch/powerpc/platforms/pseries/suspend.c  | 44 ++++++++++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 17 deletions(-)

-- 
1.7.12.2

[PATCH v4 1/3] powerpc/pseries: Device tree should only be updated once after suspend/migrate

[Tyrel Datwyler]

From: Haren Myneni <hbabu@us.ibm.com>

The current code makes rtas calls for update-nodes, activate-firmware and then
update-nodes again. The FW provides the same data for both update-nodes calls.
As a result a proc entry exists error is reported for the second update while
adding device nodes.

This patch makes a single rtas call for update-nodes after activating the FW.
It also add rtas_busy delay for the activate-firmware rtas call.

Signed-off-by: Haren Myneni <hbabu@us.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index cde4e0a..bde7eba 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -290,13 +290,6 @@ void post_mobility_fixup(void)
 	int rc;
 	int activate_fw_token;
 
-	rc = pseries_devicetree_update(MIGRATION_SCOPE);
-	if (rc) {
-		printk(KERN_ERR "Initial post-mobility device tree update "
-		       "failed: %d\n", rc);
-		return;
-	}
-
 	activate_fw_token = rtas_token("ibm,activate-firmware");
 	if (activate_fw_token == RTAS_UNKNOWN_SERVICE) {
 		printk(KERN_ERR "Could not make post-mobility "
@@ -304,16 +297,17 @@ void post_mobility_fixup(void)
 		return;
 	}
 
-	rc = rtas_call(activate_fw_token, 0, 1, NULL);
-	if (!rc) {
-		rc = pseries_devicetree_update(MIGRATION_SCOPE);
-		if (rc)
-			printk(KERN_ERR "Secondary post-mobility device tree "
-			       "update failed: %d\n", rc);
-	} else {
+	do {
+		rc = rtas_call(activate_fw_token, 0, 1, NULL);
+	} while (rtas_busy_delay(rc));
+
+	if (rc)
 		printk(KERN_ERR "Post-mobility activate-fw failed: %d\n", rc);
-		return;
-	}
+
+	rc = pseries_devicetree_update(MIGRATION_SCOPE);
+	if (rc)
+		printk(KERN_ERR "Post-mobility device tree update "
+			"failed: %d\n", rc);
 
 	return;
 }
-- 
1.7.12.2

[PATCH v4 2/3] powerpc/pseries: Update dynamic cache nodes for suspend/resume operation

[Tyrel Datwyler]

From: Haren Myneni <hbabu@us.ibm.com>

pHyp can change cache nodes for suspend/resume operation. Currently the
device tree is updated by drmgr in userspace after all non boot CPUs are
enabled. Hence, we do not modify the cache list based on the latest cache
nodes. Also we do not remove cache entries for the primary CPU.

This patch removes the cache list for the boot CPU, updates the device tree
before enabling nonboot CPUs and adds cache list for the boot cpu.

This patch also has the side effect that older versions of drmgr will
perform a second device tree update from userspace. While this is a
redundant waste of a couple cycles it is harmless since firmware returns the
same data for the subsequent update-nodes/properties rtas calls.

Signed-off-by: Haren Myneni <hbabu@us.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/rtas.h          |  1 +
 arch/powerpc/platforms/pseries/suspend.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bd52c6..a0e1add 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -283,6 +283,7 @@ extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
 
 #ifdef CONFIG_PPC_PSERIES
 extern int pseries_devicetree_update(s32 scope);
+extern void post_mobility_fixup(void);
 #endif
 
 #ifdef CONFIG_PPC_RTAS_DAEMON
diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 16a2552..1d9c580 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -26,6 +26,7 @@
 #include <asm/mmu.h>
 #include <asm/rtas.h>
 #include <asm/topology.h>
+#include "../../kernel/cacheinfo.h"
 
 static u64 stream_id;
 static struct device suspend_dev;
@@ -79,6 +80,23 @@ static int pseries_suspend_cpu(void)
 }
 
 /**
+ * pseries_suspend_enable_irqs
+ *
+ * Post suspend configuration updates
+ *
+ **/
+static void pseries_suspend_enable_irqs(void)
+{
+	/*
+	 * Update configuration which can be modified based on device tree
+	 * changes during resume.
+	 */
+	cacheinfo_cpu_offline(smp_processor_id());
+	post_mobility_fixup();
+	cacheinfo_cpu_online(smp_processor_id());
+}
+
+/**
  * pseries_suspend_enter - Final phase of hibernation
  *
  * Return value:
@@ -235,6 +253,7 @@ static int __init pseries_suspend_init(void)
 		return rc;
 
 	ppc_md.suspend_disable_cpu = pseries_suspend_cpu;
+	ppc_md.suspend_enable_irqs = pseries_suspend_enable_irqs;
 	suspend_set_ops(&pseries_suspend_ops);
 	return 0;
 }
-- 
1.7.12.2

[PATCH v4 3/3] powerpc/pseries: Expose in kernel device tree update to drmgr

[Tyrel Datwyler]

Traditionally it has been drmgr's responsibilty to update the device tree
through the /proc/ppc64/ofdt interface after a suspend/resume operation.
This patchset however has modified suspend/resume ops to preform an update
entirely in the kernel during the resume. Therefore, a mechanism is required
to expose that information to drmgr.

This patch adds a show function to the "hibernate" attribute that returns 1
if the kernel performs a device tree update after the resume and 0 otherwise.
This allows newer versions of drmgr to avoid doing a second unnecessary
device tree update.

Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 1d9c580..b87b978 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -192,7 +192,30 @@ out:
 	return rc;
 }
 
-static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
+#define USER_DT_UPDATE	0
+#define KERN_DT_UPDATE	1
+
+/**
+ * show_hibernate - Report device tree update responsibilty
+ * @dev:		subsys root device
+ * @attr:		device attribute struct
+ * @buf:		buffer
+ *
+ * Report whether a device tree update is performed by the kernel after a
+ * resume, or if drmgr must coordinate the update from user space.
+ *
+ * Return value:
+ *	0 if drmgr is to initiate update, and 1 otherwise
+ **/
+static ssize_t show_hibernate(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	return sprintf(buf, "%d\n", KERN_DT_UPDATE);
+}
+
+static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
+		   show_hibernate, store_hibernate);
 
 static struct bus_type suspend_subsys = {
 	.name = "power",
-- 
1.7.12.2

[PATCH v3 3/3] powerpc/pseries: Report in kernel device tree update to drmgr

[Tyrel Datwyler]

Traditionally it has been drmgr's responsibilty to update the device tree
through the /proc/ppc64/ofdt interface after a suspend/resume operation.
This patchset however has modified suspend/resume ops to preform that update
entirely in the kernel during the resume. Therefore, a mechanism is required
for drmgr to determine who is responsible for the update. This patch adds a
show function to the "hibernate" attribute that returns 1 if the kernel
updates the device tree after the resume and 0 if drmgr is responsible.

Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 1d9c580..b87b978 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -192,7 +192,30 @@ out:
 	return rc;
 }
 
-static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
+#define USER_DT_UPDATE	0
+#define KERN_DT_UPDATE	1
+
+/**
+ * show_hibernate - Report device tree update responsibilty
+ * @dev:		subsys root device
+ * @attr:		device attribute struct
+ * @buf:		buffer
+ *
+ * Report whether a device tree update is performed by the kernel after a
+ * resume, or if drmgr must coordinate the update from user space.
+ *
+ * Return value:
+ *	0 if drmgr is to initiate update, and 1 otherwise
+ **/
+static ssize_t show_hibernate(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	return sprintf(buf, "%d\n", KERN_DT_UPDATE);
+}
+
+static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
+		   show_hibernate, store_hibernate);
 
 static struct bus_type suspend_subsys = {
 	.name = "power",
-- 
1.7.12.2

Re: [PATCH v3 3/3] powerpc/pseries: Report in kernel device tree update to drmgr

[Tyrel Datwyler]

On 02/19/2014 12:56 PM, Tyrel Datwyler wrote:
> Traditionally it has been drmgr's responsibilty to update the device tree
> through the /proc/ppc64/ofdt interface after a suspend/resume operation.
> This patchset however has modified suspend/resume ops to preform that update
> entirely in the kernel during the resume. Therefore, a mechanism is required
> for drmgr to determine who is responsible for the update. This patch adds a
> show function to the "hibernate" attribute that returns 1 if the kernel
> updates the device tree after the resume and 0 if drmgr is responsible.

Ignore this one. Apparently, I forgot to clear it out of my patch
directory before I resent.

> 
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
> index 1d9c580..b87b978 100644
> --- a/arch/powerpc/platforms/pseries/suspend.c
> +++ b/arch/powerpc/platforms/pseries/suspend.c
> @@ -192,7 +192,30 @@ out:
>  	return rc;
>  }
> 
> -static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
> +#define USER_DT_UPDATE	0
> +#define KERN_DT_UPDATE	1
> +
> +/**
> + * show_hibernate - Report device tree update responsibilty
> + * @dev:		subsys root device
> + * @attr:		device attribute struct
> + * @buf:		buffer
> + *
> + * Report whether a device tree update is performed by the kernel after a
> + * resume, or if drmgr must coordinate the update from user space.
> + *
> + * Return value:
> + *	0 if drmgr is to initiate update, and 1 otherwise
> + **/
> +static ssize_t show_hibernate(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	return sprintf(buf, "%d\n", KERN_DT_UPDATE);
> +}
> +
> +static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
> +		   show_hibernate, store_hibernate);
> 
>  static struct bus_type suspend_subsys = {
>  	.name = "power",
> 

Re: [PATCH v4 0/3] powerpc/pseries: fix issues in suspend/resume code

[Benjamin Herrenschmidt]

On Wed, 2014-02-19 at 12:56 -0800, Tyrel Datwyler wrote:
> This patchset fixes a couple of issues encountered in the suspend/resume code
> base. First when using the kernel device tree update code update-nodes is
> unnecessarily called more than once. Second the cpu cache lists are not
> updated after a suspend/resume which under certain conditions may cause a
> panic. Finally, since the cache list fix utilzes in kernel device tree update
> code a means for telling drmgr that updating the device tree from userspace
> is unnecessary.

This series breaks the !SMP build.

Cheers,
Ben.

> Changes from v3:
> - Updated patch descriptions to better reflect the behavior/interface changes
>   and why they are acceptable per Ben's concerns.
> 
> Changes from v2:
> - Moved dynamic configuration update code into pseries specific routine
>   per Nathan's suggestion.
> 
> Changes from v1:
> - Fixed several commit message typos
> - Fixed authorship of first two patches
> 
> Haren Myneni (2):
>   powerpc/pseries: Device tree should only be updated once after
>     suspend/migrate
>   powerpc/pseries: Update dynamic cache nodes for suspend/resume
>     operation
> 
> Tyrel Datwyler (1):
>   powerpc/pseries: Report in kernel device tree update to drmgr
> 
>  arch/powerpc/include/asm/rtas.h           |  1 +
>  arch/powerpc/platforms/pseries/mobility.c | 26 +++++++-----------
>  arch/powerpc/platforms/pseries/suspend.c  | 44 ++++++++++++++++++++++++++++++-
>  3 files changed, 54 insertions(+), 17 deletions(-)
> 

[prefix=PATCH v5 2/3] powerpc/pseries: Update dynamic cache nodes for suspend/resume operation

[Tyrel Datwyler]

From: Haren Myneni <hbabu@us.ibm.com>

pHyp can change cache nodes for suspend/resume operation. Currently the
device tree is updated by drmgr in userspace after all non boot CPUs are
enabled. Hence, we do not modify the cache list based on the latest cache
nodes. Also we do not remove cache entries for the primary CPU.

This patch removes the cache list for the boot CPU, updates the device tree
before enabling nonboot CPUs and adds cache list for the boot cpu.

This patch also has the side effect that older versions of drmgr will
perform a second device tree update from userspace. While this is a
redundant waste of a couple cycles it is harmless since firmware returns the
same data for the subsequent update-nodes/properties rtas calls.

Signed-off-by: Haren Myneni <hbabu@us.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---

Changes from v4:
- fixes build break for !SMP configs.
 

 arch/powerpc/include/asm/rtas.h          |  1 +
 arch/powerpc/kernel/cacheinfo.c          |  7 +++++--
 arch/powerpc/platforms/pseries/suspend.c | 19 +++++++++++++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bd52c6..a0e1add 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -283,6 +283,7 @@ extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
 
 #ifdef CONFIG_PPC_PSERIES
 extern int pseries_devicetree_update(s32 scope);
+extern void post_mobility_fixup(void);
 #endif
 
 #ifdef CONFIG_PPC_RTAS_DAEMON
diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index 2912b87..40198d5 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c
@@ -756,7 +756,10 @@ void cacheinfo_cpu_online(unsigned int cpu_id)
 	cacheinfo_sysfs_populate(cpu_id, cache);
 }
 
-#ifdef CONFIG_HOTPLUG_CPU /* functions needed for cpu offline */
+/* functions needed to remove cache entry for cpu offline or suspend/resume */
+
+#if (defined(CONFIG_PPC_PSERIES) && defined(CONFIG_SUSPEND)) || \
+    defined(CONFIG_HOTPLUG_CPU)
 
 static struct cache *cache_lookup_by_cpu(unsigned int cpu_id)
 {
@@ -843,4 +846,4 @@ void cacheinfo_cpu_offline(unsigned int cpu_id)
 	if (cache)
 		cache_cpu_clear(cache, cpu_id);
 }
-#endif /* CONFIG_HOTPLUG_CPU */
+#endif /* (CONFIG_PPC_PSERIES && CONFIG_SUSPEND) || CONFIG_HOTPLUG_CPU */
diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 16a2552..1d9c580 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -26,6 +26,7 @@
 #include <asm/mmu.h>
 #include <asm/rtas.h>
 #include <asm/topology.h>
+#include "../../kernel/cacheinfo.h"
 
 static u64 stream_id;
 static struct device suspend_dev;
@@ -79,6 +80,23 @@ static int pseries_suspend_cpu(void)
 }
 
 /**
+ * pseries_suspend_enable_irqs
+ *
+ * Post suspend configuration updates
+ *
+ **/
+static void pseries_suspend_enable_irqs(void)
+{
+	/*
+	 * Update configuration which can be modified based on device tree
+	 * changes during resume.
+	 */
+	cacheinfo_cpu_offline(smp_processor_id());
+	post_mobility_fixup();
+	cacheinfo_cpu_online(smp_processor_id());
+}
+
+/**
  * pseries_suspend_enter - Final phase of hibernation
  *
  * Return value:
@@ -235,6 +253,7 @@ static int __init pseries_suspend_init(void)
 		return rc;
 
 	ppc_md.suspend_disable_cpu = pseries_suspend_cpu;
+	ppc_md.suspend_enable_irqs = pseries_suspend_enable_irqs;
 	suspend_set_ops(&pseries_suspend_ops);
 	return 0;
 }
-- 
1.7.12.2

Re: [PATCH v4 0/3] powerpc/pseries: fix issues in suspend/resume code

[Tyrel Datwyler]

On 02/23/2014 05:53 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2014-02-19 at 12:56 -0800, Tyrel Datwyler wrote:
>> This patchset fixes a couple of issues encountered in the suspend/resume code
>> base. First when using the kernel device tree update code update-nodes is
>> unnecessarily called more than once. Second the cpu cache lists are not
>> updated after a suspend/resume which under certain conditions may cause a
>> panic. Finally, since the cache list fix utilzes in kernel device tree update
>> code a means for telling drmgr that updating the device tree from userspace
>> is unnecessary.
> 
> This series breaks the !SMP build.
> 
> Cheers,
> Ben.

The second patch in this series created the build breakage. I've replied
to that patch with a new version that fixes the break.

-Tyrel

> 
>> Changes from v3:
>> - Updated patch descriptions to better reflect the behavior/interface changes
>>   and why they are acceptable per Ben's concerns.
>>
>> Changes from v2:
>> - Moved dynamic configuration update code into pseries specific routine
>>   per Nathan's suggestion.
>>
>> Changes from v1:
>> - Fixed several commit message typos
>> - Fixed authorship of first two patches
>>
>> Haren Myneni (2):
>>   powerpc/pseries: Device tree should only be updated once after
>>     suspend/migrate
>>   powerpc/pseries: Update dynamic cache nodes for suspend/resume
>>     operation
>>
>> Tyrel Datwyler (1):
>>   powerpc/pseries: Report in kernel device tree update to drmgr
>>
>>  arch/powerpc/include/asm/rtas.h           |  1 +
>>  arch/powerpc/platforms/pseries/mobility.c | 26 +++++++-----------
>>  arch/powerpc/platforms/pseries/suspend.c  | 44 ++++++++++++++++++++++++++++++-
>>  3 files changed, 54 insertions(+), 17 deletions(-)
>>
> 
>