Skip to contentSkip to conversation

drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

PATCH Superseded Sudeep Holla 4 participants 8 messages Apr 22, 2016 – Apr 26, 2016 Device tree bindings

lore
Share & tools
lore

Sudeep Holla

author
#0 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 -
 drivers/firmware/psci.c                        | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Regards,
Sudeep

Documentation/devicetree/bindings/arm/cpus.txt +0 -1
index ccc62f145306..3f0cbbb8395f 100644
@@ -192,7 +192,6 @@ nodes to be present and contain the properties described below.
 			  can be one of:
 			    "allwinner,sun6i-a31"
 			    "allwinner,sun8i-a23"
-			    "arm,psci"
 			    "arm,realview-smp"
 			    "brcm,bcm-nsp-smp"
 			    "brcm,brahma-b15"
drivers/firmware/psci.c +1 -1
index 11bfee8b79a9..b5d05807e6ec 100644
@@ -360,7 +360,7 @@ static struct cpuidle_ops psci_cpuidle_ops __initdata = {
 	.init = psci_dt_cpu_init_idle,
 };
 
-CPUIDLE_METHOD_OF_DECLARE(psci, "arm,psci", &psci_cpuidle_ops);
+CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
 #endif
 #endif
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

Quoted 26 lines · Sudeep Holla

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
  Documentation/devicetree/bindings/arm/cpus.txt | 1 -
  drivers/firmware/psci.c                        | 2 +-
  2 files changed, 1 insertion(+), 2 deletions(-)
Quoted 9 lines

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Signature · 2 lines
Regards,
Sudeep
Applied
#2 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

Quoted 32 lines · On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
Quoted 31 lines · Sudeep Holla

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
  Documentation/devicetree/bindings/arm/cpus.txt | 1 -
  drivers/firmware/psci.c                        | 2 +-
  2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Quoted 9 lines
Quoted 4 lines

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

Arnd

#3 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

Quoted 38 lines · Arnd Bergmann
Quoted 37 lines · On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
Quoted 35 lines · Sudeep Holla

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
  Documentation/devicetree/bindings/arm/cpus.txt | 1 -
  drivers/firmware/psci.c                        | 2 +-
  2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Quoted 8 lines
Quoted 4 lines

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

The buggy code was merged for v4.6-rc1, so this hasn't been in a release, and should not break an existing user.

No compatible string is used here, which is precisely the confusion this addresses. The PSCI enable-method != the compatible string for a PSCI node, but the two got confused in a documentation patch, and then that confusion spread to code.

Thanks,
Mark.

#4 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

Quoted 13 lines · On Tuesday 26 April 2016 12:01:48 Mark Rutland wrote:
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

Quoted 8 lines

The buggy code was merged for v4.6-rc1, so this hasn't been in a release, and should not break an existing user.

No compatible string is used here, which is precisely the confusion this addresses. The PSCI enable-method != the compatible string for a PSCI node, but the two got confused in a documentation patch, and then that confusion spread to code.

Ok, thanks for the explanation.

ARnd

author
#5 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

In reply to Arnd Bergmann
Quoted 38 lines · Arnd Bergmann
Quoted 37 lines · On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
Quoted 35 lines · Sudeep Holla

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
   Documentation/devicetree/bindings/arm/cpus.txt | 1 -
   drivers/firmware/psci.c                        | 2 +-
   2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Quoted 9 lines
Quoted 4 lines

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

No it's not compatible string, it's the enable-method string value. So far, I don't see any DT in the mainline using "arm,psci" as enable method. It is indeed the compatible value for PSCI v0.1. Also, the kernel didn't honor it even if it was present earlier until

Commit 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware")

which was introduced in v4.6 and obeyed ARM32 DT bindings but Soby reported issue when using 64-bit machine to run 32-bit kernel. So it was good catch at right time. Hence we need to unify the binding though it was never added intentionally and I assume it a mistake(git blame showed it was not even reviewed). Luckily it was not used until v4.6, so we are safe.

Signature · 2 lines
Regards,
Sudeep
#6 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

In reply to Arnd Bergmann

Dear Arnd, Sudeep,

Quoted 38 lines · On Tue, 26 Apr 2016 12:49:00 +0200 Arnd Bergmann wrote:
Quoted 37 lines · On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
Quoted 35 lines · Sudeep Holla

Currently ARM CPUs DT bindings allows different enable-method value for PSCI based systems. On ARM 64-bit this property is required and must be "psci" while on ARM 32-bit systems this property is optional and must be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI node, and was never intended to be the enable-method. So this is a bug in the binding and not a deliberate attempt at specifying 32-bit differently.

This is problematic if 32-bit OS is run on 64-bit system which has "psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci" before it finds any users.

Reported-by Soby Mathew <[email protected]>
Cc Rob Herring <[email protected]>
Acked-by Mark Rutland <[email protected]>
Acked-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
  Documentation/devicetree/bindings/arm/cpus.txt | 1 -
  drivers/firmware/psci.c                        | 2 +-
  2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
- Added Mark and Lorenzo's ACKs
- Updated commit message to specify that it also conflicts with PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Quoted 8 lines
Quoted 4 lines

Gentle ping to include this in next round of arm-soc fixes. It would be good to get this fixed before v4.6 as we don't want to have user of the binding which we want to remove in the release.

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

To be honest, this breaks some Marvell DEBU's 32 bit SoCs' kernel, but these SoCs' support are not mainlined yet, so it's fine to fix it as early as possible.

Thanks,
Jisheng

author
#7 lore raw

PATCH v2 1/1 drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

Hi Jisheng,

Quoted 4 lines · Jisheng Zhang

Dear Arnd, Sudeep,

On Tue, 26 Apr 2016 12:49:00 +0200 Arnd Bergmann wrote:

[...]

Quoted 6 lines
Quoted 4 lines

Applied to the fixes branch. I have to admit that I don't understand how the compatible string is really used here, so I have to trust that this cannot break any existing machine.

To be honest, this breaks some Marvell DEBU's 32 bit SoCs' kernel, but these

Sorry for that, but it's better to fix it in the firmware/DT. As I mentioned earlier it was never honored in the kernel before v4.6, so technically nothing is broken yet :)

Quoted 3 lines

SoCs' support are not mainlined yet, so it's fine to fix it as early as possible.

Good to hear that. Thanks indeed.

Signature · 2 lines
Regards,
Sudeep