[PATCH 0/6] ARM/ARM64: Drop arm_pm_restart

[PATCH 0/6] ARM/ARM64: Drop arm_pm_restart

[Guenter Roeck]

This is the final push to replace arm_pm_restart with the kernel restart
handler. Finally drop arm_pm_restart after it is no longer used.
The following changes since commit 541d8f4d59d79f5d37c8c726f723d42ff307db57:

----------------------------------------------------------------
Guenter Roeck (6):
      ARM: prima2: Register with kernel restart handler
      ARM: xen: Register with kernel restart handler
      ARM: PSCI: Register with kernel restart handler
      ARM: Register with kernel restart handler
      ARM64: Remove arm_pm_restart
      ARM: Remove arm_pm_restart

 arch/arm/include/asm/system_misc.h   |  1 -
 arch/arm/kernel/reboot.c             |  6 +-----
 arch/arm/kernel/setup.c              | 20 ++++++++++++++++++--
 arch/arm/mach-prima2/rstc.c          | 11 +++++++++--
 arch/arm/xen/enlighten.c             | 13 +++++++++++--
 arch/arm64/include/asm/system_misc.h |  2 --
 arch/arm64/kernel/process.c          |  7 +------
 drivers/firmware/psci.c              | 11 +++++++++--
 8 files changed, 49 insertions(+), 22 deletions(-)

[PATCH 1/6] ARM: prima2: Register with kernel restart handler

[Guenter Roeck]

Register with kernel restart handler instead of setting arm_pm_restart
directly. By doing this, the prima2 reset handler can be prioritized
among other restart methods available on a particular board.

Select a high priority of 192 since the original code overwrites the
default arm restart handler.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/mach-prima2/rstc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-prima2/rstc.c b/arch/arm/mach-prima2/rstc.c
index 7c251eb11d01..1639997c5b49 100644
--- a/arch/arm/mach-prima2/rstc.c
+++ b/arch/arm/mach-prima2/rstc.c
@@ -65,11 +65,18 @@ static struct reset_controller_dev sirfsoc_reset_controller = {
 
 #define SIRFSOC_SYS_RST_BIT  BIT(31)
 
-static void sirfsoc_restart(enum reboot_mode mode, const char *cmd)
+static int sirfsoc_restart(struct notifier_block *nb, unsigned long action,
+			   void *data)
 {
 	writel(SIRFSOC_SYS_RST_BIT, sirfsoc_rstc_base);
+	return NOTIFY_DONE;
 }
 
+static struct notifier_block sirfsoc_restart_nb = {
+	.notifier_call  = sirfsoc_restart,
+	.priority       = 192,
+};
+
 static int sirfsoc_rstc_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -80,7 +87,7 @@ static int sirfsoc_rstc_probe(struct platform_device *pdev)
 	}
 
 	sirfsoc_reset_controller.of_node = np;
-	arm_pm_restart = sirfsoc_restart;
+	register_restart_handler(&sirfsoc_restart_nb);
 
 	if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
 		reset_controller_register(&sirfsoc_reset_controller);
-- 
2.5.0

[PATCH 2/6] ARM: xen: Register with kernel restart handler

[Guenter Roeck]

Register with kernel restart handler instead of setting arm_pm_restart
directly.

Select a high priority of 192 to ensure that default restart handlers
are replaced if Xen is running.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/xen/enlighten.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 75cd7345c654..76a1d12fd27e 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -27,6 +27,7 @@
 #include <linux/cpu.h>
 #include <linux/console.h>
 #include <linux/pvclock_gtod.h>
+#include <linux/reboot.h>
 #include <linux/time64.h>
 #include <linux/timekeeping.h>
 #include <linux/timekeeper_internal.h>
@@ -193,14 +194,22 @@ after_register_vcpu_info:
 	put_cpu();
 }
 
-static void xen_restart(enum reboot_mode reboot_mode, const char *cmd)
+static int xen_restart(struct notifier_block *nb, unsigned long action,
+		       void *data)
 {
 	struct sched_shutdown r = { .reason = SHUTDOWN_reboot };
 	int rc;
 	rc = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
 	BUG_ON(rc);
+
+	return NOTIFY_DONE;
 }
 
+static struct notifier_block xen_restart_nb = {
+	.notifier_call = xen_restart,
+	.priority = 192,
+};
+
 static void xen_power_off(void)
 {
 	struct sched_shutdown r = { .reason = SHUTDOWN_poweroff };
@@ -370,7 +379,7 @@ static int __init xen_pm_init(void)
 		return -ENODEV;
 
 	pm_power_off = xen_power_off;
-	arm_pm_restart = xen_restart;
+	register_restart_handler(&xen_restart_nb);
 	if (!xen_initial_domain()) {
 		struct timespec64 ts;
 		xen_read_wallclock(&ts);
-- 
2.5.0

Re: [Xen-devel] [PATCH 2/6] ARM: xen: Register with kernel restart handler

[Guenter Roeck]

On Fri, Apr 08, 2016 at 11:22:57AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 08, 2016 at 05:53:55AM -0700, Guenter Roeck wrote:
> > Register with kernel restart handler instead of setting arm_pm_restart
> > directly.
> > 
> > Select a high priority of 192 to ensure that default restart handlers
> 
> Is there some macro for that magic value?
> 
No, only guidelines in kernel/reboot.c.

Guenter

> > are replaced if Xen is running.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  arch/arm/xen/enlighten.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 75cd7345c654..76a1d12fd27e 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/cpu.h>
> >  #include <linux/console.h>
> >  #include <linux/pvclock_gtod.h>
> > +#include <linux/reboot.h>
> >  #include <linux/time64.h>
> >  #include <linux/timekeeping.h>
> >  #include <linux/timekeeper_internal.h>
> > @@ -193,14 +194,22 @@ after_register_vcpu_info:
> >  	put_cpu();
> >  }
> >  
> > -static void xen_restart(enum reboot_mode reboot_mode, const char *cmd)
> > +static int xen_restart(struct notifier_block *nb, unsigned long action,
> > +		       void *data)
> >  {
> >  	struct sched_shutdown r = { .reason = SHUTDOWN_reboot };
> >  	int rc;
> >  	rc = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
> >  	BUG_ON(rc);
> > +
> > +	return NOTIFY_DONE;
> >  }
> >  
> > +static struct notifier_block xen_restart_nb = {
> > +	.notifier_call = xen_restart,
> > +	.priority = 192,
> > +};
> > +
> >  static void xen_power_off(void)
> >  {
> >  	struct sched_shutdown r = { .reason = SHUTDOWN_poweroff };
> > @@ -370,7 +379,7 @@ static int __init xen_pm_init(void)
> >  		return -ENODEV;
> >  
> >  	pm_power_off = xen_power_off;
> > -	arm_pm_restart = xen_restart;
> > +	register_restart_handler(&xen_restart_nb);
> >  	if (!xen_initial_domain()) {
> >  		struct timespec64 ts;
> >  		xen_read_wallclock(&ts);
> > -- 
> > 2.5.0
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH 2/6] ARM: xen: Register with kernel restart handler

[Stefano Stabellini]

On Fri, 8 Apr 2016, Guenter Roeck wrote:
> Register with kernel restart handler instead of setting arm_pm_restart
> directly.
> 
> Select a high priority of 192 to ensure that default restart handlers
> are replaced if Xen is running.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  arch/arm/xen/enlighten.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 75cd7345c654..76a1d12fd27e 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -27,6 +27,7 @@
>  #include <linux/cpu.h>
>  #include <linux/console.h>
>  #include <linux/pvclock_gtod.h>
> +#include <linux/reboot.h>
>  #include <linux/time64.h>
>  #include <linux/timekeeping.h>
>  #include <linux/timekeeper_internal.h>
> @@ -193,14 +194,22 @@ after_register_vcpu_info:
>  	put_cpu();
>  }
>  
> -static void xen_restart(enum reboot_mode reboot_mode, const char *cmd)
> +static int xen_restart(struct notifier_block *nb, unsigned long action,
> +		       void *data)
>  {
>  	struct sched_shutdown r = { .reason = SHUTDOWN_reboot };
>  	int rc;
>  	rc = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
>  	BUG_ON(rc);
> +
> +	return NOTIFY_DONE;
>  }
>  
> +static struct notifier_block xen_restart_nb = {
> +	.notifier_call = xen_restart,
> +	.priority = 192,
> +};
> +
>  static void xen_power_off(void)
>  {
>  	struct sched_shutdown r = { .reason = SHUTDOWN_poweroff };
> @@ -370,7 +379,7 @@ static int __init xen_pm_init(void)
>  		return -ENODEV;
>  
>  	pm_power_off = xen_power_off;
> -	arm_pm_restart = xen_restart;
> +	register_restart_handler(&xen_restart_nb);
>  	if (!xen_initial_domain()) {
>  		struct timespec64 ts;
>  		xen_read_wallclock(&ts);
> -- 
> 2.5.0
> 

Re: [PATCH 2/6] ARM: xen: Register with kernel restart handler

[Stefano Stabellini]

On Sat, 9 Apr 2016, Stefano Stabellini wrote:
> On Fri, 8 Apr 2016, Guenter Roeck wrote:
> > Register with kernel restart handler instead of setting arm_pm_restart
> > directly.
> > 
> > Select a high priority of 192 to ensure that default restart handlers
> > are replaced if Xen is running.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

and queued for 4.7


> 
> >  arch/arm/xen/enlighten.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 75cd7345c654..76a1d12fd27e 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/cpu.h>
> >  #include <linux/console.h>
> >  #include <linux/pvclock_gtod.h>
> > +#include <linux/reboot.h>
> >  #include <linux/time64.h>
> >  #include <linux/timekeeping.h>
> >  #include <linux/timekeeper_internal.h>
> > @@ -193,14 +194,22 @@ after_register_vcpu_info:
> >  	put_cpu();
> >  }
> >  
> > -static void xen_restart(enum reboot_mode reboot_mode, const char *cmd)
> > +static int xen_restart(struct notifier_block *nb, unsigned long action,
> > +		       void *data)
> >  {
> >  	struct sched_shutdown r = { .reason = SHUTDOWN_reboot };
> >  	int rc;
> >  	rc = HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
> >  	BUG_ON(rc);
> > +
> > +	return NOTIFY_DONE;
> >  }
> >  
> > +static struct notifier_block xen_restart_nb = {
> > +	.notifier_call = xen_restart,
> > +	.priority = 192,
> > +};
> > +
> >  static void xen_power_off(void)
> >  {
> >  	struct sched_shutdown r = { .reason = SHUTDOWN_poweroff };
> > @@ -370,7 +379,7 @@ static int __init xen_pm_init(void)
> >  		return -ENODEV;
> >  
> >  	pm_power_off = xen_power_off;
> > -	arm_pm_restart = xen_restart;
> > +	register_restart_handler(&xen_restart_nb);
> >  	if (!xen_initial_domain()) {
> >  		struct timespec64 ts;
> >  		xen_read_wallclock(&ts);
> > -- 
> > 2.5.0
> > 
> 

[PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Guenter Roeck]

Register with kernel restart handler instead of setting arm_pm_restart
directly. This enables support for replacing the PSCI restart handler
with a different handler if necessary for a specific board.

Select a priority of 129 to indicate a higher than default priority, but
keep it as low as possible since PSCI reset is known to fail on some
boards.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
It might make sense to introduce a restart-priority property for devicetree
based configurations, but I am not sure if this would be acceptable.

 drivers/firmware/psci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 11bfee8b79a9..99fab3ac3fd5 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np)
 	return 0;
 }
 
-static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
+static int psci_sys_reset(struct notifier_block *np, unsigned long action,
+			  void *data)
 {
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+	return NOTIFY_DONE;
 }
 
+static struct notifier_block psci_sys_reset_nb = {
+	.notifier_call = psci_sys_reset,
+	.priority = 129,
+};
+
 static void psci_sys_poweroff(void)
 {
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
@@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void)
 
 	psci_ops.migrate_info_type = psci_migrate_info_type;
 
-	arm_pm_restart = psci_sys_reset;
+	register_restart_handler(&psci_sys_reset_nb);
 
 	pm_power_off = psci_sys_poweroff;
 }
-- 
2.5.0

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Wolfram Sang]

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


> +static int psci_sys_reset(struct notifier_block *np, unsigned long action,

Minor: notifier_block *nb instead of *np


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Mark Rutland]

On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote:
> Register with kernel restart handler instead of setting arm_pm_restart
> directly. This enables support for replacing the PSCI restart handler
> with a different handler if necessary for a specific board.
> 
> Select a priority of 129 to indicate a higher than default priority, but
> keep it as low as possible since PSCI reset is known to fail on some
> boards.

For reference, which boards?

It's unfortunate that that a PSCI 0.2+ implementation would be lacking a
working SYSTEM_RESET implementation, and it's certainly a mistake to
discourage.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> It might make sense to introduce a restart-priority property for devicetree
> based configurations, but I am not sure if this would be acceptable.

>From the DT side, I'm not keen on properties for priorities. They're
incredibly fragile and don't really encode a HW property.

A better option would be to have a property to describe how the PSCI
implementation is broken (e.g. broken-system-reset), and not register
the handler at all in that case.

Thanks,
Mark.

>  drivers/firmware/psci.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 11bfee8b79a9..99fab3ac3fd5 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np)
>  	return 0;
>  }
>  
> -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
> +static int psci_sys_reset(struct notifier_block *np, unsigned long action,
> +			  void *data)
>  {
>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +	return NOTIFY_DONE;
>  }
>  
> +static struct notifier_block psci_sys_reset_nb = {
> +	.notifier_call = psci_sys_reset,
> +	.priority = 129,
> +};
> +
>  static void psci_sys_poweroff(void)
>  {
>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void)
>  
>  	psci_ops.migrate_info_type = psci_migrate_info_type;
>  
> -	arm_pm_restart = psci_sys_reset;
> +	register_restart_handler(&psci_sys_reset_nb);
>  
>  	pm_power_off = psci_sys_poweroff;
>  }
> -- 
> 2.5.0
> 

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Jisheng Zhang]

Dear Mark,

On Wed, 13 Apr 2016 12:05:19 +0100 Mark Rutland wrote:

> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote:
> > Register with kernel restart handler instead of setting arm_pm_restart
> > directly. This enables support for replacing the PSCI restart handler
> > with a different handler if necessary for a specific board.
> > 
> > Select a priority of 129 to indicate a higher than default priority, but
> > keep it as low as possible since PSCI reset is known to fail on some
> > boards.  
> 
> For reference, which boards?
> 
> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a
> working SYSTEM_RESET implementation, and it's certainly a mistake to
> discourage.

I may understand the case: on some platforms, the only reset way is
to trigger the wdt, for various reason the underly firmware isn't
convenient to touch the wdt.

But I'd like 127 or lower for the default priority for the above case, because
various wdt reset_handler priority is 128.

Thanks,
Jisheng

> 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > It might make sense to introduce a restart-priority property for devicetree
> > based configurations, but I am not sure if this would be acceptable.  
> 
> From the DT side, I'm not keen on properties for priorities. They're
> incredibly fragile and don't really encode a HW property.
> 
> A better option would be to have a property to describe how the PSCI
> implementation is broken (e.g. broken-system-reset), and not register
> the handler at all in that case.
> 
> Thanks,
> Mark.
> 
> >  drivers/firmware/psci.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> > index 11bfee8b79a9..99fab3ac3fd5 100644
> > --- a/drivers/firmware/psci.c
> > +++ b/drivers/firmware/psci.c
> > @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np)
> >  	return 0;
> >  }
> >  
> > -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
> > +static int psci_sys_reset(struct notifier_block *np, unsigned long action,
> > +			  void *data)
> >  {
> >  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> > +	return NOTIFY_DONE;
> >  }
> >  
> > +static struct notifier_block psci_sys_reset_nb = {
> > +	.notifier_call = psci_sys_reset,
> > +	.priority = 129,
> > +};
> > +
> >  static void psci_sys_poweroff(void)
> >  {
> >  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> > @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void)
> >  
> >  	psci_ops.migrate_info_type = psci_migrate_info_type;
> >  
> > -	arm_pm_restart = psci_sys_reset;
> > +	register_restart_handler(&psci_sys_reset_nb);
> >  
> >  	pm_power_off = psci_sys_poweroff;
> >  }
> > -- 
> > 2.5.0
> >   
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Guenter Roeck]

On 04/13/2016 04:05 AM, Mark Rutland wrote:
> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote:
>> Register with kernel restart handler instead of setting arm_pm_restart
>> directly. This enables support for replacing the PSCI restart handler
>> with a different handler if necessary for a specific board.
>>
>> Select a priority of 129 to indicate a higher than default priority, but
>> keep it as low as possible since PSCI reset is known to fail on some
>> boards.
>
> For reference, which boards?
>
Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported
that it is broken on a board he is using, but I don't recall if it is
the same board.

> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a
> working SYSTEM_RESET implementation, and it's certainly a mistake to
> discourage.
>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> It might make sense to introduce a restart-priority property for devicetree
>> based configurations, but I am not sure if this would be acceptable.
>
>>From the DT side, I'm not keen on properties for priorities. They're
> incredibly fragile and don't really encode a HW property.
>
Depends. It is a convenient means to say "primary restart method" or
"may be broken".

> A better option would be to have a property to describe how the PSCI
> implementation is broken (e.g. broken-system-reset), and not register
> the handler at all in that case.
>
... just like this. I'll look into it.

Thanks,
Guenter

> Thanks,
> Mark.
>
>>   drivers/firmware/psci.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 11bfee8b79a9..99fab3ac3fd5 100644
>> --- a/drivers/firmware/psci.c
>> +++ b/drivers/firmware/psci.c
>> @@ -231,11 +231,18 @@ static int get_set_conduit_method(struct device_node *np)
>>   	return 0;
>>   }
>>
>> -static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
>> +static int psci_sys_reset(struct notifier_block *np, unsigned long action,
>> +			  void *data)
>>   {
>>   	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> +	return NOTIFY_DONE;
>>   }
>>
>> +static struct notifier_block psci_sys_reset_nb = {
>> +	.notifier_call = psci_sys_reset,
>> +	.priority = 129,
>> +};
>> +
>>   static void psci_sys_poweroff(void)
>>   {
>>   	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>> @@ -461,7 +468,7 @@ static void __init psci_0_2_set_functions(void)
>>
>>   	psci_ops.migrate_info_type = psci_migrate_info_type;
>>
>> -	arm_pm_restart = psci_sys_reset;
>> +	register_restart_handler(&psci_sys_reset_nb);
>>
>>   	pm_power_off = psci_sys_poweroff;
>>   }
>> --
>> 2.5.0
>>
>

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Geert Uytterhoeven]

On Wed, Apr 13, 2016 at 3:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 04/13/2016 04:05 AM, Mark Rutland wrote:
>> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote:
>>>
>>> Register with kernel restart handler instead of setting arm_pm_restart
>>> directly. This enables support for replacing the PSCI restart handler
>>> with a different handler if necessary for a specific board.
>>>
>>> Select a priority of 129 to indicate a higher than default priority, but
>>> keep it as low as possible since PSCI reset is known to fail on some
>>> boards.
>>
>> For reference, which boards?
>>
> Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported
> that it is broken on a board he is using, but I don't recall if it is
> the same board.

Yes it is.

>> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a
>> working SYSTEM_RESET implementation, and it's certainly a mistake to
>> discourage.
>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> It might make sense to introduce a restart-priority property for
>>> devicetree
>>> based configurations, but I am not sure if this would be acceptable.
>>
>>> From the DT side, I'm not keen on properties for priorities. They're
>> incredibly fragile and don't really encode a HW property.
>>
> Depends. It is a convenient means to say "primary restart method" or
> "may be broken".

The issue is supposed to be fixed in a more recent firmware, which I still have
to try.

DT indeed isn't the right place to work around this. What we need is a
blacklist of bad firmware versions...
Or Perfect Firmware from Day One on (like Perfect DT from Day One ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Guenter Roeck]

On Wed, Apr 13, 2016 at 03:22:44PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 13, 2016 at 3:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 04/13/2016 04:05 AM, Mark Rutland wrote:
> >> On Fri, Apr 08, 2016 at 05:53:56AM -0700, Guenter Roeck wrote:
> >>>
> >>> Register with kernel restart handler instead of setting arm_pm_restart
> >>> directly. This enables support for replacing the PSCI restart handler
> >>> with a different handler if necessary for a specific board.
> >>>
> >>> Select a priority of 129 to indicate a higher than default priority, but
> >>> keep it as low as possible since PSCI reset is known to fail on some
> >>> boards.
> >>
> >> For reference, which boards?
> >>
> > Salvator-X, reported by Geert Uytterhoeven. Wolfram Sang also reported
> > that it is broken on a board he is using, but I don't recall if it is
> > the same board.
> 
> Yes it is.
> 
> >> It's unfortunate that that a PSCI 0.2+ implementation would be lacking a
> >> working SYSTEM_RESET implementation, and it's certainly a mistake to
> >> discourage.
> >>
> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>> ---
> >>> It might make sense to introduce a restart-priority property for
> >>> devicetree
> >>> based configurations, but I am not sure if this would be acceptable.
> >>
> >>> From the DT side, I'm not keen on properties for priorities. They're
> >> incredibly fragile and don't really encode a HW property.
> >>
> > Depends. It is a convenient means to say "primary restart method" or
> > "may be broken".
> 
> The issue is supposed to be fixed in a more recent firmware, which I still have
> to try.
> 
> DT indeed isn't the right place to work around this. What we need is a
> blacklist of bad firmware versions...
> Or Perfect Firmware from Day One on (like Perfect DT from Day One ;-)
> 
That makes things quite tricky. Best I can think of is a series of boolean
devicetree properties, such as

	broken-reset-handler
	last-resort-restart-handler
	secondary-restart-handler
	default-restart-handler
	primary-restart-handler

which ends up being quite similar to the 'restart-priority' property. I'll
do this as follow-up patch, though - I do not see the point holding up the
series for this, and it is really a separate problem. I'll send rev2 with
the various Acked-by: and Reviewed-by: tags as well as the variable rename
suggested by Wolfram.

Thanks,
Guenter

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Wolfram Sang]

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


> That makes things quite tricky. Best I can think of is a series of boolean
> devicetree properties, such as
> 
> 	broken-reset-handler
> 	last-resort-restart-handler
> 	secondary-restart-handler
> 	default-restart-handler
> 	primary-restart-handler
> 
> which ends up being quite similar to the 'restart-priority' property. I'll
> do this as follow-up patch, though

Please CC me on this. I wanted to tackle this problem as well today. My
findings/conclusions so far:

* There is one driver bringing 'priority' directly to DT already: gpio-restart

* Watchdog priorities are board dependant

* Having the priorities clear at boot-time is safer than configuring them
  at run-time

* The linux scheme (0-255) shouldn't be enforced in DT

So, I wondered about a "priority" binding which just states "the higher,
the more important". Then any OS can decide what to do with it. In the
Linux case, this could be: sort them and give them priority 256 -
position_in_sorted_list.

Opinions?

> - I do not see the point holding up the series for this, and it is
> really a separate problem.

Ack.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Guenter Roeck]

On 04/14/2016 01:52 AM, Wolfram Sang wrote:
>
>> That makes things quite tricky. Best I can think of is a series of boolean
>> devicetree properties, such as
>>
>> 	broken-reset-handler
>> 	last-resort-restart-handler
>> 	secondary-restart-handler
>> 	default-restart-handler
>> 	primary-restart-handler
>>
>> which ends up being quite similar to the 'restart-priority' property. I'll
>> do this as follow-up patch, though
>
> Please CC me on this. I wanted to tackle this problem as well today. My

Sure.

> findings/conclusions so far:
>
> * There is one driver bringing 'priority' directly to DT already: gpio-restart
>
Correct.

> * Watchdog priorities are board dependant
>
> * Having the priorities clear at boot-time is safer than configuring them
>    at run-time
>
Correct.

> * The linux scheme (0-255) shouldn't be enforced in DT
>
> So, I wondered about a "priority" binding which just states "the higher,
> the more important". Then any OS can decide what to do with it. In the
> Linux case, this could be: sort them and give them priority 256 -
> position_in_sorted_list.
>

"the higher, the more important" makes sense to me. We don't have to
enforce the linux scheme, though that happens to be the same (the priority
argument in the notifier block takes an int, so it would not even be
necessary to adjust it unless someone specifies 0xffffffff).

> Opinions?
>
I am fine either way - boolean properties or numbers, with a personal
preference for numbers as more flexible. Whatever is acceptable for
the community is fine with me.

Guenter

>> - I do not see the point holding up the series for this, and it is
>> really a separate problem.
>
> Ack.
>

Re: [PATCH 3/6] ARM: PSCI: Register with kernel restart handler

[Wolfram Sang]

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


> "the higher, the more important" makes sense to me. We don't have to
> enforce the linux scheme, though that happens to be the same (the priority
> argument in the notifier block takes an int, so it would not even be
> necessary to adjust it unless someone specifies 0xffffffff).

I think we should enforce the scheme internally (but not in DT, of
course):

a) it is documented to be in the range 0-255
b) it should be valid to prioritize the watchdogs with 1,2,3 in DT.
   If we don't apply the '255 - pos_in_sorted_list' value, then the
   priority could be below some machine default of 128, or?

> I am fine either way - boolean properties or numbers, with a personal
> preference for numbers as more flexible.

Same here. Time to go to the DT list probably.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/6] ARM: Register with kernel restart handler

[Guenter Roeck]

By making use of the kernel restart handler, board specific restart
handlers can be prioritized amongst available mechanisms for a particular
board or system.

Select the default priority of 128 to indicate that the restart callback
in the machine description is the default restart mechanism.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/kernel/setup.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 139791ed473d..232dba199702 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -1002,6 +1002,20 @@ void __init hyp_mode_check(void)
 #endif
 }
 
+static void (*__arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
+
+static int arm_restart(struct notifier_block *nb, unsigned long action,
+		       void *data)
+{
+	__arm_pm_restart(action, data);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block arm_restart_nb = {
+	.notifier_call = arm_restart,
+	.priority = 128,
+};
+
 void __init setup_arch(char **cmdline_p)
 {
 	const struct machine_desc *mdesc;
@@ -1044,8 +1058,10 @@ void __init setup_arch(char **cmdline_p)
 	paging_init(mdesc);
 	request_standard_resources(mdesc);
 
-	if (mdesc->restart)
-		arm_pm_restart = mdesc->restart;
+	if (mdesc->restart) {
+		__arm_pm_restart = mdesc->restart;
+		register_restart_handler(&arm_restart_nb);
+	}
 
 	unflatten_device_tree();
 
-- 
2.5.0

[PATCH 5/6] ARM64: Remove arm_pm_restart

[Guenter Roeck]

All users of arm_pm_restart have been converted to use the kernel restart
handler.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm64/include/asm/system_misc.h | 2 --
 arch/arm64/kernel/process.c          | 7 +------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index 57f110bea6a8..f1d865b7d38d 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -43,8 +43,6 @@ struct mm_struct;
 extern void show_pte(struct mm_struct *mm, unsigned long addr);
 extern void __show_regs(struct pt_regs *);
 
-extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
-
 #define show_unhandled_signals_ratelimited()				\
 ({									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 80624829db61..29c29984eca0 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -66,8 +66,6 @@ EXPORT_SYMBOL(__stack_chk_guard);
 void (*pm_power_off)(void);
 EXPORT_SYMBOL_GPL(pm_power_off);
 
-void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
-
 /*
  * This is our default idle handler.
  */
@@ -153,10 +151,7 @@ void machine_restart(char *cmd)
 		efi_reboot(reboot_mode, NULL);
 
 	/* Now call the architecture specific reboot code. */
-	if (arm_pm_restart)
-		arm_pm_restart(reboot_mode, cmd);
-	else
-		do_kernel_restart(cmd);
+	do_kernel_restart(cmd);
 
 	/*
 	 * Whoops - the architecture was unable to reboot.
-- 
2.5.0

Re: [PATCH 5/6] ARM64: Remove arm_pm_restart

[Catalin Marinas]

On Fri, Apr 08, 2016 at 05:53:58AM -0700, Guenter Roeck wrote:
> All users of arm_pm_restart have been converted to use the kernel restart
> handler.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  arch/arm64/include/asm/system_misc.h | 2 --
>  arch/arm64/kernel/process.c          | 7 +------
>  2 files changed, 1 insertion(+), 8 deletions(-)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 6/6] ARM: Remove arm_pm_restart

[Guenter Roeck]

All users of arm_pm_restart have been converted to use the kernel restart
handler.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/include/asm/system_misc.h | 1 -
 arch/arm/kernel/reboot.c           | 6 +-----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad984af..6c952538f1e8 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -11,7 +11,6 @@
 extern void cpu_init(void);
 
 void soft_restart(unsigned long);
-extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 extern void (*arm_pm_idle)(void);
 
 #define UDBG_UNDEFINED	(1 << 0)
diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
index 71a2ff9ec490..4785c39ee3eb 100644
--- a/arch/arm/kernel/reboot.c
+++ b/arch/arm/kernel/reboot.c
@@ -20,7 +20,6 @@ typedef void (*phys_reset_t)(unsigned long);
 /*
  * Function pointers to optional machine specific functions
  */
-void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
@@ -140,10 +139,7 @@ void machine_restart(char *cmd)
 	local_irq_disable();
 	smp_send_stop();
 
-	if (arm_pm_restart)
-		arm_pm_restart(reboot_mode, cmd);
-	else
-		do_kernel_restart(cmd);
+	do_kernel_restart(cmd);
 
 	/* Give a grace period for failure to restart of 1s */
 	mdelay(1000);
-- 
2.5.0

Re: [PATCH 0/6] ARM/ARM64: Drop arm_pm_restart

[Wolfram Sang]

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

On Fri, Apr 08, 2016 at 05:53:53AM -0700, Guenter Roeck wrote:
> This is the final push to replace arm_pm_restart with the kernel restart
> handler. Finally drop arm_pm_restart after it is no longer used.

Nice! Will check it next week and add the reset_handler to my watchdog.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] ARM/ARM64: Drop arm_pm_restart

[Arnd Bergmann]

On Friday 08 April 2016, Guenter Roeck wrote:
> This is the final push to replace arm_pm_restart with the kernel restart
> handler. Finally drop arm_pm_restart after it is no longer used.
> The following changes since commit 541d8f4d59d79f5d37c8c726f723d42ff307db57:

Looks good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 0/6] ARM/ARM64: Drop arm_pm_restart

[Wolfram Sang]

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

On Fri, Apr 08, 2016 at 05:53:53AM -0700, Guenter Roeck wrote:
> This is the final push to replace arm_pm_restart with the kernel restart
> handler. Finally drop arm_pm_restart after it is no longer used.
> The following changes since commit 541d8f4d59d79f5d37c8c726f723d42ff307db57:
> 
> ----------------------------------------------------------------
> Guenter Roeck (6):
>       ARM: prima2: Register with kernel restart handler
>       ARM: xen: Register with kernel restart handler
>       ARM: PSCI: Register with kernel restart handler
>       ARM: Register with kernel restart handler
>       ARM64: Remove arm_pm_restart
>       ARM: Remove arm_pm_restart

I double checked that all arm_pm_restart were converted, did a visual
review of the patches (one nit found), and came to the same conclusions
about the priority settings. Thus:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I also added a watchdog restart handler with higher priority than the
PSCI handler. That now works nicely \o/

So, for patches 3+5 you could also add:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]