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

[PATCH v2 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 not to perform a device tree update from
userspace is required.

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           |  4 ++++
 arch/powerpc/kernel/rtas.c                | 17 +++++++++++++++++
 arch/powerpc/kernel/time.c                |  6 ++++++
 arch/powerpc/platforms/pseries/mobility.c | 26 ++++++++++----------------
 arch/powerpc/platforms/pseries/suspend.c  | 25 ++++++++++++++++++++++++-
 5 files changed, 61 insertions(+), 17 deletions(-)

-- 
1.7.12.4

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

[Tyrel Datwyler]

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

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.4

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

[Nathan Fontenot]

On 01/22/2014 01:58 PM, Tyrel Datwyler wrote:
> From: Haren Myneni <hbabu@us.ibm.com>
> 
> 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>

Acked-by: Nathan fontenot <nfont@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;
>  }
> 

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

[Tyrel Datwyler]

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

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

pHyp can change cache nodes for suspend/resume operation. The current code
updates the device tree 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.

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 |  4 ++++
 arch/powerpc/kernel/rtas.c      | 17 +++++++++++++++++
 arch/powerpc/kernel/time.c      |  6 ++++++
 3 files changed, 27 insertions(+)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bd52c6..da9d733 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -283,6 +283,10 @@ 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);
+extern void update_dynamic_configuration(void);
+#else /* !CONFIG_PPC_PSERIES */
+void update_dynamic_configuration(void) { }
 #endif
 
 #ifdef CONFIG_PPC_RTAS_DAEMON
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 4cf674d..8249eb2 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -43,6 +43,7 @@
 #include <asm/time.h>
 #include <asm/mmu.h>
 #include <asm/topology.h>
+#include "cacheinfo.h"
 
 struct rtas_t rtas = {
 	.lock = __ARCH_SPIN_LOCK_UNLOCKED
@@ -972,6 +973,22 @@ out:
 	free_cpumask_var(offline_mask);
 	return atomic_read(&data.error);
 }
+
+/*
+ * The device tree cache nodes can be modified during suspend/ resume.
+ * So delete all cache entries and recreate them again after the device tree
+ * update.
+ * We already deleted cache entries for notboot CPUs before suspend. So delete
+ * entries for the primary CPU, recreate entries after the device tree update.
+ * We can create entries for nonboot CPU when enable them later.
+ */
+
+void update_dynamic_configuration(void)
+{
+	cacheinfo_cpu_offline(smp_processor_id());
+	post_mobility_fixup();
+	cacheinfo_cpu_online(smp_processor_id());
+}
 #else /* CONFIG_PPC_PSERIES */
 int rtas_ibm_suspend_me(struct rtas_args *args)
 {
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b3b1441..5f1ca28 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -69,6 +69,7 @@
 #include <asm/vdso_datapage.h>
 #include <asm/firmware.h>
 #include <asm/cputime.h>
+#include <asm/rtas.h>
 
 /* powerpc clocksource/clockevent code */
 
@@ -592,6 +593,11 @@ void arch_suspend_enable_irqs(void)
 	generic_suspend_enable_irqs();
 	if (ppc_md.suspend_enable_irqs)
 		ppc_md.suspend_enable_irqs();
+	/*
+	 * Update configuration which can be modified based on devicetree
+	 * changes during resume.
+	 */
+	update_dynamic_configuration();
 }
 #endif
 
-- 
1.7.12.4

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

[Nathan Fontenot]

On 01/22/2014 01:58 PM, Tyrel Datwyler wrote:
> From: Haren Myneni <hbabu@us.ibm.com>
> 
> From: Haren Myneni <hbabu@us.ibm.com>
> 
> pHyp can change cache nodes for suspend/resume operation. The current code
> updates the device tree 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.
> 
> 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 |  4 ++++
>  arch/powerpc/kernel/rtas.c      | 17 +++++++++++++++++
>  arch/powerpc/kernel/time.c      |  6 ++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 9bd52c6..da9d733 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -283,6 +283,10 @@ 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);
> +extern void update_dynamic_configuration(void);
> +#else /* !CONFIG_PPC_PSERIES */
> +void update_dynamic_configuration(void) { }
>  #endif
> 
>  #ifdef CONFIG_PPC_RTAS_DAEMON
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 4cf674d..8249eb2 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -43,6 +43,7 @@
>  #include <asm/time.h>
>  #include <asm/mmu.h>
>  #include <asm/topology.h>
> +#include "cacheinfo.h"
> 
>  struct rtas_t rtas = {
>  	.lock = __ARCH_SPIN_LOCK_UNLOCKED
> @@ -972,6 +973,22 @@ out:
>  	free_cpumask_var(offline_mask);
>  	return atomic_read(&data.error);
>  }
> +
> +/*
> + * The device tree cache nodes can be modified during suspend/ resume.
> + * So delete all cache entries and recreate them again after the device tree
> + * update.
> + * We already deleted cache entries for notboot CPUs before suspend. So delete
> + * entries for the primary CPU, recreate entries after the device tree update.
> + * We can create entries for nonboot CPU when enable them later.
> + */
> +
> +void update_dynamic_configuration(void)
> +{
> +	cacheinfo_cpu_offline(smp_processor_id());
> +	post_mobility_fixup();
> +	cacheinfo_cpu_online(smp_processor_id());
> +}
>  #else /* CONFIG_PPC_PSERIES */
>  int rtas_ibm_suspend_me(struct rtas_args *args)
>  {
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index b3b1441..5f1ca28 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -69,6 +69,7 @@
>  #include <asm/vdso_datapage.h>
>  #include <asm/firmware.h>
>  #include <asm/cputime.h>
> +#include <asm/rtas.h>
> 
>  /* powerpc clocksource/clockevent code */
> 
> @@ -592,6 +593,11 @@ void arch_suspend_enable_irqs(void)
>  	generic_suspend_enable_irqs();
>  	if (ppc_md.suspend_enable_irqs)
>  		ppc_md.suspend_enable_irqs();
> +	/*
> +	 * Update configuration which can be modified based on devicetree
> +	 * changes during resume.
> +	 */
> +	update_dynamic_configuration();

Instead of creating this new routine update_dynamic_reconfiguration() for
pseries systems only, shouldn't we be defining ppc_md.suspend_enable_irqs
for pseries. I think this could be done in platforms/pseries/suspend.c

-Nathan

>  }
>  #endif
> 

[PATCH v2 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 16a2552..723115d 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -174,7 +174,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.4

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

[Nathan Fontenot]

On 01/22/2014 01:58 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.
> 
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Acked-by: Nathan Fontenot <nfont@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 16a2552..723115d 100644
> --- a/arch/powerpc/platforms/pseries/suspend.c
> +++ b/arch/powerpc/platforms/pseries/suspend.c
> @@ -174,7 +174,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",
>