arm64: psci: warn if psci_power_state variable is not initialised

PATCH New Amit Daniel Kachhap 3 participants 11 messages Oct 30, 2014 – Dec 15, 2014 arm64

lore

Share & tools

lore

Amit Daniel Kachhap

author

2014-10-30 03:55 UTC #0 lore raw

PATCH 1/3 arm64: psci: warn if psci_power_state variable is not initialised

Without this cpu_suspend may cause crash dump when psci cpuidle is not initialised and cpu_suspend is called.

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/kernel/psci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/arm64/kernel/psci.c +4 -2

index 866c1c8..2178d6e 100644
@@ -523,9 +523,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 	struct psci_power_state *state = __get_cpu_var(psci_power_state);
 	/*
 	 * idle state index 0 corresponds to wfi, should never be called
-	 * from the cpu_suspend operations
+	 * from the cpu_suspend operations.
+	 * Also psci_power_state variable should have been populated by
+	 * above init idle routine.
 	 */
-	if (WARN_ON_ONCE(!index))
+	if (WARN_ON_ONCE(!index || !state))
 		return -EINVAL;
 
 	if (state->type == PSCI_POWER_STATE_TYPE_STANDBY)

Signature · 1 line

1.9.1

Amit Daniel Kachhap

author

2014-10-30 03:55 UTC #1 lore raw

PATCH 2/3 arm64: psci: fix cpu_suspend to check idle state type for index

This fix rectifies the psci cpu_suspend to check the C-state type corresponding to the requested index.

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/kernel/psci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/psci.c +1 -1

index 2178d6e..4ebc146 100644
@@ -530,7 +530,7 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 	if (WARN_ON_ONCE(!index || !state))
 		return -EINVAL;
 
-	if (state->type == PSCI_POWER_STATE_TYPE_STANDBY)
+	if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY)
 		ret = psci_ops.cpu_suspend(state[index - 1], 0);
 	else
 		ret = __cpu_suspend(index, psci_suspend_finisher);

Signature · 1 line

1.9.1

Lorenzo Pieralisi

2014-10-30 10:19 UTC #2 lore raw

PATCH 2/3 arm64: psci: fix cpu_suspend to check idle state type for index

Hi Amit,

Quoted 2 lines · Amit Daniel Kachhap

This fix rectifies the psci cpu_suspend to check the C-state type corresponding to the requested index.

Can you reword the commit log please ?

"This fix rectifies the psci cpu_suspend implementation to check the PSCI power state parameter type field associated with the requested idle state index."

Quoted 20 lines

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/kernel/psci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/psci.c +1 -1

index 2178d6e..4ebc146 100644
@@ -530,7 +530,7 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 	if (WARN_ON_ONCE(!index || !state))
 		return -EINVAL;
 
-	if (state->type == PSCI_POWER_STATE_TYPE_STANDBY)
+	if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY)
 		ret = psci_ops.cpu_suspend(state[index - 1], 0);
 	else
 		ret = __cpu_suspend(index, psci_suspend_finisher);

Signature · 1 line

1.9.1

Can you resend it as a single patch (ie this patch does not apply if patch 1 is missing, and I am not sure we need patch 1) and send it to Catalin and Will to merge it asap ?

Please add my:

Acked-by Lorenzo Pieralisi <[email protected]>

Thanks,
Lorenzo

amit daniel kachhap

author

2014-10-30 11:31 UTC #3 lore raw

PATCH 2/3 arm64: psci: fix cpu_suspend to check idle state type for index

Quoted 25 lines · Lorenzo Pieralisi

Hi Amit,
Quoted 2 lines · Amit Daniel Kachhap
This fix rectifies the psci cpu_suspend to check the C-state type corresponding to the requested index.
Can you reword the commit log please ?
"This fix rectifies the psci cpu_suspend implementation to check the PSCI power state parameter type field associated with the requested idle state index."
Quoted 13 lines
Signed-off-by Amit Daniel Kachhap <[email protected]>
---
 arch/arm64/kernel/psci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/psci.c +0 -0
index 2178d6e..4ebc146 100644
@@ -530,7 +530,7 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
      if (WARN_ON_ONCE(!index || !state))
              return -EINVAL;
 

Quoted 11 lines

Quoted 7 lines
arch/arm64/kernel/psci.c +1 -2
-     if (state->type == PSCI_POWER_STATE_TYPE_STANDBY)
+     if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY)
              ret = psci_ops.cpu_suspend(state[index - 1], 0);
      else
              ret = __cpu_suspend(index, psci_suspend_finisher);
--
1.9.1
Can you resend it as a single patch (ie this patch does not apply if patch 1 is missing, and I am not sure we need patch 1) and send it to Catalin and Will to merge it asap ?

Ok sure.

Quoted 11 lines

Please add my:
Acked-by Lorenzo Pieralisi <[email protected]>
Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Amit Daniel Kachhap

author

2014-10-30 03:55 UTC #4 lore raw

PATCH 3/3 arm64: psci: Add support to call cpu_suspend with deepest C state.

In reply to Amit Daniel Kachhap

This feature will be useful for system suspend/resume which may intend to invoke cpu_suspend with deepest possible C state level.

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/include/asm/psci.h | 2 ++
 arch/arm64/kernel/psci.c      | 8 ++++++++
 2 files changed, 10 insertions(+)

arch/arm64/include/asm/psci.h +2 -0

index e5312ea..487e52b 100644
@@ -14,6 +14,8 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+#define CPU_PSCI_IDLE_INDEX_MAX		0xff
+
 int psci_init(void);
 
 #endif /* __ASM_PSCI_H */

arch/arm64/kernel/psci.c +8 -0

index 4ebc146..a761513 100644
@@ -70,6 +70,7 @@ enum psci_function {
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
 
 static u32 psci_function_id[PSCI_FN_MAX];
+static u32 cpu_psci_idle_state_count;
 
 static int psci_to_linux_errno(int errno)
 {

@@ -239,6 +240,8 @@ static int __maybe_unused cpu_psci_cpu_init_idle(struct device_node *cpu_node,
 	if (!count)
 		return -ENODEV;
 
+	cpu_psci_idle_state_count = count;
+
 	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;

@@ -521,6 +524,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
 	int ret;
 	struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+	/* Check and select the deepest idle state */
+	if (index == CPU_PSCI_IDLE_INDEX_MAX)
+		index = cpu_psci_idle_state_count;
+
 	/*
 	 * idle state index 0 corresponds to wfi, should never be called
 	 * from the cpu_suspend operations.

Signature · 1 line

1.9.1

Lorenzo Pieralisi

2014-10-30 10:46 UTC #5 lore raw

PATCH 3/3 arm64: psci: Add support to call cpu_suspend with deepest C state.

Quoted 2 lines · Amit Daniel Kachhap

This feature will be useful for system suspend/resume which may intend to invoke cpu_suspend with deepest possible C state level.

We are working on defining system states in the PSCI specification, but it is not set in stone yet (Cc'ed Charles).

I guess you are posting this code because you have already an implementation that supports system suspend/resume, so I am taking an immediate action to come up with an API suitable for that asap.

On a side note I do not necessarily like this magic idle index logic, I prefer adding an API for that, that translates the call into whatever is required by the back-end (ie PSCI) implementation.

Thanks,
Lorenzo

Quoted 56 lines

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/include/asm/psci.h | 2 ++
 arch/arm64/kernel/psci.c      | 8 ++++++++
 2 files changed, 10 insertions(+)

arch/arm64/include/asm/psci.h +2 -0

index e5312ea..487e52b 100644
@@ -14,6 +14,8 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+#define CPU_PSCI_IDLE_INDEX_MAX		0xff
+
 int psci_init(void);
 
 #endif /* __ASM_PSCI_H */

arch/arm64/kernel/psci.c +8 -0

index 4ebc146..a761513 100644
@@ -70,6 +70,7 @@ enum psci_function {
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
 
 static u32 psci_function_id[PSCI_FN_MAX];
+static u32 cpu_psci_idle_state_count;
 
 static int psci_to_linux_errno(int errno)
 {

@@ -239,6 +240,8 @@ static int __maybe_unused cpu_psci_cpu_init_idle(struct device_node *cpu_node,
 	if (!count)
 		return -ENODEV;
 
+	cpu_psci_idle_state_count = count;
+
 	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;

@@ -521,6 +524,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
 	int ret;
 	struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+	/* Check and select the deepest idle state */
+	if (index == CPU_PSCI_IDLE_INDEX_MAX)
+		index = cpu_psci_idle_state_count;
+
 	/*
 	 * idle state index 0 corresponds to wfi, should never be called
 	 * from the cpu_suspend operations.

Signature · 1 line

1.9.1

amit daniel kachhap

author

2014-10-30 11:22 UTC #6 lore raw

PATCH 3/3 arm64: psci: Add support to call cpu_suspend with deepest C state.

Quoted 11 lines · Lorenzo Pieralisi

Quoted 2 lines · Amit Daniel Kachhap
This feature will be useful for system suspend/resume which may intend to invoke cpu_suspend with deepest possible C state level.
We are working on defining system states in the PSCI specification, but it is not set in stone yet (Cc'ed Charles).
I guess you are posting this code because you have already an implementation that supports system suspend/resume, so I am taking an immediate action to come up with an API suitable for that asap.

Yes that would be useful. Thanks

Quoted 4 lines

On a side note I do not necessarily like this magic idle index logic, I prefer adding an API for that, that translates the call into whatever is required by the back-end (ie PSCI) implementation.

yeah this is a kind of workaround and API will be better.

Quoted 64 lines

Thanks,
Lorenzo

Quoted 53 lines

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/include/asm/psci.h | 2 ++
 arch/arm64/kernel/psci.c      | 8 ++++++++
 2 files changed, 10 insertions(+)

arch/arm64/include/asm/psci.h +2 -0

index e5312ea..487e52b 100644
@@ -14,6 +14,8 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+#define CPU_PSCI_IDLE_INDEX_MAX              0xff
+
 int psci_init(void);
 
 #endif /* __ASM_PSCI_H */

arch/arm64/kernel/psci.c +8 -1

index 4ebc146..a761513 100644
@@ -70,6 +70,7 @@ enum psci_function {
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
 
 static u32 psci_function_id[PSCI_FN_MAX];
+static u32 cpu_psci_idle_state_count;
 
 static int psci_to_linux_errno(int errno)
 {

@@ -239,6 +240,8 @@ static int __maybe_unused cpu_psci_cpu_init_idle(struct device_node *cpu_node,
      if (!count)
              return -ENODEV;
 
+     cpu_psci_idle_state_count = count;
+
      psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
      if (!psci_states)
              return -ENOMEM;

@@ -521,6 +524,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
      int ret;
      struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+     /* Check and select the deepest idle state */
+     if (index == CPU_PSCI_IDLE_INDEX_MAX)
+             index = cpu_psci_idle_state_count;
+
      /*
       * idle state index 0 corresponds to wfi, should never be called
       * from the cpu_suspend operations.
--

Quoted 3 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

amit daniel kachhap

author

2014-12-01 08:08 UTC #7 lore raw

PATCH 3/3 arm64: psci: Add support to call cpu_suspend with deepest C state.

In reply to Lorenzo Pieralisi

Quoted 11 lines · Lorenzo Pieralisi

Quoted 2 lines · Amit Daniel Kachhap
This feature will be useful for system suspend/resume which may intend to invoke cpu_suspend with deepest possible C state level.
We are working on defining system states in the PSCI specification, but it is not set in stone yet (Cc'ed Charles).
I guess you are posting this code because you have already an implementation that supports system suspend/resume, so I am taking an immediate action to come up with an API suitable for that asap.

Hi Lorenzo,

Any update on this API for system sleep.

Regards,
Amit

Quoted 68 lines

On a side note I do not necessarily like this magic idle index logic, I prefer adding an API for that, that translates the call into whatever is required by the back-end (ie PSCI) implementation.

Thanks,
Lorenzo

Quoted 53 lines

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/include/asm/psci.h | 2 ++
 arch/arm64/kernel/psci.c      | 8 ++++++++
 2 files changed, 10 insertions(+)

arch/arm64/include/asm/psci.h +2 -0

index e5312ea..487e52b 100644
@@ -14,6 +14,8 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+#define CPU_PSCI_IDLE_INDEX_MAX              0xff
+
 int psci_init(void);
 
 #endif /* __ASM_PSCI_H */

arch/arm64/kernel/psci.c +8 -1

index 4ebc146..a761513 100644
@@ -70,6 +70,7 @@ enum psci_function {
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
 
 static u32 psci_function_id[PSCI_FN_MAX];
+static u32 cpu_psci_idle_state_count;
 
 static int psci_to_linux_errno(int errno)
 {

@@ -239,6 +240,8 @@ static int __maybe_unused cpu_psci_cpu_init_idle(struct device_node *cpu_node,
      if (!count)
              return -ENODEV;
 
+     cpu_psci_idle_state_count = count;
+
      psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
      if (!psci_states)
              return -ENOMEM;

@@ -521,6 +524,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
      int ret;
      struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+     /* Check and select the deepest idle state */
+     if (index == CPU_PSCI_IDLE_INDEX_MAX)
+             index = cpu_psci_idle_state_count;
+
      /*
       * idle state index 0 corresponds to wfi, should never be called
       * from the cpu_suspend operations.
--

Quoted 3 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-12-15 18:34 UTC #8 lore raw

PATCH 3/3 arm64: psci: Add support to call cpu_suspend with deepest C state.

Quoted 9 lines · amit daniel kachhap

Quoted 7 lines · Lorenzo Pieralisi
Quoted 2 lines · Amit Daniel Kachhap
This feature will be useful for system suspend/resume which may intend to invoke cpu_suspend with deepest possible C state level.
We are working on defining system states in the PSCI specification, but it is not set in stone yet (Cc'ed Charles).

Quoted 7 lines

Quoted 4 lines
I guess you are posting this code because you have already an implementation that supports system suspend/resume, so I am taking an immediate action to come up with an API suitable for that asap.
Hi Lorenzo,
Any update on this API for system sleep.

Hi Amit,

Sorry for the delay in replying, I am working on it at the moment, we will post the API in the coming weeks.

Thank you,
Lorenzo

Quoted 73 lines

Regards,
Amit

Quoted 68 lines

On a side note I do not necessarily like this magic idle index logic, I prefer adding an API for that, that translates the call into whatever is required by the back-end (ie PSCI) implementation.

Thanks,
Lorenzo

Quoted 53 lines

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/include/asm/psci.h | 2 ++
 arch/arm64/kernel/psci.c      | 8 ++++++++
 2 files changed, 10 insertions(+)

arch/arm64/include/asm/psci.h +2 -0

index e5312ea..487e52b 100644
@@ -14,6 +14,8 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+#define CPU_PSCI_IDLE_INDEX_MAX              0xff
+
 int psci_init(void);
 
 #endif /* __ASM_PSCI_H */

arch/arm64/kernel/psci.c +8 -1

index 4ebc146..a761513 100644
@@ -70,6 +70,7 @@ enum psci_function {
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
 
 static u32 psci_function_id[PSCI_FN_MAX];
+static u32 cpu_psci_idle_state_count;
 
 static int psci_to_linux_errno(int errno)
 {

@@ -239,6 +240,8 @@ static int __maybe_unused cpu_psci_cpu_init_idle(struct device_node *cpu_node,
      if (!count)
              return -ENODEV;
 
+     cpu_psci_idle_state_count = count;
+
      psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
      if (!psci_states)
              return -ENOMEM;

@@ -521,6 +524,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
      int ret;
      struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+     /* Check and select the deepest idle state */
+     if (index == CPU_PSCI_IDLE_INDEX_MAX)
+             index = cpu_psci_idle_state_count;
+
      /*
       * idle state index 0 corresponds to wfi, should never be called
       * from the cpu_suspend operations.
--

Quoted 3 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-10-30 10:29 UTC #9 lore raw

PATCH 1/3 arm64: psci: warn if psci_power_state variable is not initialised

In reply to Amit Daniel Kachhap

Quoted 14 lines · Amit Daniel Kachhap

Without this cpu_suspend may cause crash dump when psci cpuidle is not initialised and cpu_suspend is called.
Signed-off-by Amit Daniel Kachhap <[email protected]>
---
 arch/arm64/kernel/psci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arm64/kernel/psci.c +0 -0
index 866c1c8..2178d6e 100644
@@ -523,9 +523,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 	struct psci_power_state *state = __get_cpu_var(psci_power_state);

arch/arm64/kernel/psci.c static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) · line 531

Quoted 10 lines

arch/arm64/kernel/psci.c +4 -2

 	/*
 	 * idle state index 0 corresponds to wfi, should never be called
-	 * from the cpu_suspend operations
+	 * from the cpu_suspend operations.
+	 * Also psci_power_state variable should have been populated by
+	 * above init idle routine.
 	 */
-	if (WARN_ON_ONCE(!index))
+	if (WARN_ON_ONCE(!index || !state))
 		return -EINVAL;

I thought about this when developing the code, at the moment this can't happen so I did not add any check for that. I would wait for the dust to settle on the API usage (and if we need it for something different than idle) before adding checks that might turn out useless, so we will keep this patch on the back-burner for now.

Thanks,
Lorenzo

Amit Kachhap

2014-10-30 11:35 UTC #10 lore raw

PATCH 1/3 arm64: psci: warn if psci_power_state variable is not initialised

Quoted 23 lines · Lorenzo Pieralisi

Quoted 22 lines · Amit Daniel Kachhap

Without this cpu_suspend may cause crash dump when psci cpuidle is not initialised and cpu_suspend is called.

Signed-off-by Amit Daniel Kachhap <[email protected]>

---
 arch/arm64/kernel/psci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/arm64/kernel/psci.c +3 -2

index 866c1c8..2178d6e 100644
@@ -523,9 +523,11 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
      struct psci_power_state *state = __get_cpu_var(psci_power_state);
      /*
       * idle state index 0 corresponds to wfi, should never be called
-      * from the cpu_suspend operations
+      * from the cpu_suspend operations.
+      * Also psci_power_state variable should have been populated by
+      * above init idle routine.
       */
-     if (WARN_ON_ONCE(!index))

Quoted 8 lines

arch/arm64/kernel/psci.c static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) · line 531
Quoted 2 lines
arch/arm64/kernel/psci.c +1 -0
+     if (WARN_ON_ONCE(!index || !state))
              return -EINVAL;
I thought about this when developing the code, at the moment this can't happen so I did not add any check for that. I would wait for the dust to settle on the API usage (and if we need it for something different than idle) before adding checks that might turn out useless, so we will keep this patch on the back-burner for now.

Ok right this fix is not mandatory.

Quoted 7 lines

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html