restart cleanups

setup_mm_for_reboot() doesn't make use of its argument, so remove it.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/kernel/machine_kexec.c |    4 ++--
 arch/arm/kernel/process.c       |    4 ++--
 arch/arm/mm/idmap.c             |    2 +-
 arch/arm/mm/nommu.c             |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

arch/arm/kernel/machine_kexec.c +1 -1

index c1b4463..6fcdfc4 100644
@@ -16,7 +16,7 @@
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned int relocate_new_kernel_size;
 
-extern void setup_mm_for_reboot(char mode);
+extern void setup_mm_for_reboot(void);
 
 extern unsigned long kexec_start_address;
 extern unsigned long kexec_indirection_page;

The comment should get removed now also.

IRQs are already disabled by the time arch_reset() is called, so these calls to local_irq_disable() instead arch_reset() are redundant. Remove them.

The following spatch was used:
@ rule1 @ identifier fn; @@
arch_reset = fn;

@@ identifier rule1.fn, m, c; @@
void fn(char m, const char *c)
{
<... when != local_irq_enable
-local_irq_disable();
...>
}

@@ identifier m, c; @@
arch_reset(char m, const char *c)
{
<... when != local_irq_enable
-local_irq_disable();
...>
}

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mach-ep93xx/include/mach/system.h  |    2 --

arch/arm/mach-ep93xx/include/mach/system.h static inline void arch_idle(void) · line 16

Quoted 13 lines

arch/arm/mach-ep93xx/include/mach/system.h +0 -2

index 6d661fe..bdf6c4f 100644
@@ -11,8 +11,6 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	local_irq_disable();
-
 	/*
 	 * Set then clear the SWRST bit to initiate a software reset
 	 */

For ep93xx.

Acked-by H Hartley Sweeten <[email protected]>

We only need to set the system up for a soft-restart if we're going to be doing a soft-restart. Provide a new function (soft_restart()) which does the setup and final call for this, and make platforms use it. Eliminate the call to setup_restart() from the default handler.

This means that platforms arch_reset() function is no longer called with the page tables prepared for a soft-restart, and caches will still be enabled.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/include/asm/system.h                     |    1 +
 arch/arm/kernel/process.c                         |   10 +++++++---
 arch/arm/mach-clps711x/include/mach/system.h      |    2 +-
 arch/arm/mach-ebsa110/include/mach/system.h       |    2 +-
 arch/arm/mach-footbridge/include/mach/system.h    |    2 +-
 arch/arm/mach-iop32x/include/mach/system.h        |    2 +-
 arch/arm/mach-iop33x/include/mach/system.h        |    2 +-
 arch/arm/mach-ixp4xx/include/mach/system.h        |    2 +-
 arch/arm/mach-ks8695/include/mach/system.h        |    2 +-
 arch/arm/mach-mmp/include/mach/system.h           |    4 ++--
 arch/arm/mach-mxs/system.c                        |    2 +-
 arch/arm/mach-nuc93x/include/mach/system.h        |    2 +-
 arch/arm/mach-pnx4008/include/mach/system.h       |    2 +-
 arch/arm/mach-pxa/reset.c                         |    2 +-
 arch/arm/mach-rpc/include/mach/system.h           |    2 +-

 arch/arm/mach-s3c2410/include/mach/system-reset.h |    4 ++--
 arch/arm/mach-s3c64xx/include/mach/system.h       |    2 +-
 arch/arm/mach-sa1100/include/mach/system.h        |    2 +-
 arch/arm/mach-shmobile/include/mach/system.h      |    2 +-
 arch/arm/mach-w90x900/include/mach/system.h       |    2 +-
 arch/arm/plat-mxc/system.c                        |    2 +-
 arch/arm/plat-spear/include/plat/system.h         |    2 +-
 22 files changed, 30 insertions(+), 25 deletions(-)

Hi Russell,

Quoted 26 lines · Russell King - ARM Linux

We only need to set the system up for a soft-restart if we're going to be doing a soft-restart. Provide a new function (soft_restart()) which does the setup and final call for this, and make platforms use it. Eliminate the call to setup_restart() from the default handler.

This means that platforms arch_reset() function is no longer called with the page tables prepared for a soft-restart, and caches will still be enabled.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/include/asm/system.h                     |    1 +
 arch/arm/kernel/process.c                         |   10 +++++++---
 arch/arm/mach-clps711x/include/mach/system.h      |    2 +-
 arch/arm/mach-ebsa110/include/mach/system.h       |    2 +-
 arch/arm/mach-footbridge/include/mach/system.h    |    2 +-
 arch/arm/mach-iop32x/include/mach/system.h        |    2 +-
 arch/arm/mach-iop33x/include/mach/system.h        |    2 +-
 arch/arm/mach-ixp4xx/include/mach/system.h        |    2 +-
 arch/arm/mach-ks8695/include/mach/system.h        |    2 +-
 arch/arm/mach-mmp/include/mach/system.h           |    4 ++--
 arch/arm/mach-mxs/system.c                        |    2 +-
 arch/arm/mach-nuc93x/include/mach/system.h        |    2 +-
 arch/arm/mach-pnx4008/include/mach/system.h       |    2 +-
 arch/arm/mach-pxa/reset.c                         |    2 +-
 arch/arm/mach-rpc/include/mach/system.h           |    2 +-

Quoted 8 lines

 arch/arm/mach-s3c2410/include/mach/system-reset.h |    4 ++--
 arch/arm/mach-s3c64xx/include/mach/system.h       |    2 +-
 arch/arm/mach-sa1100/include/mach/system.h        |    2 +-
 arch/arm/mach-shmobile/include/mach/system.h      |    2 +-
 arch/arm/mach-w90x900/include/mach/system.h       |    2 +-
 arch/arm/plat-mxc/system.c                        |    2 +-
 arch/arm/plat-spear/include/plat/system.h         |    2 +-
 22 files changed, 30 insertions(+), 25 deletions(-)

Which branch is this patch taken against? I'm getting conflicts if I try to apply it against mainline, next, 3.1 or your devel branch.

It's probably going to be a good idea to wait until after the merge window closes before tackling this stuff seriously, because things are still changing quite quickly - eg, we have new platforms merged last night which wouldn't have been updated with these patches.

I'd suggest not rushing this; we'll have a full 2-3 months before they can be merged into mainline.

Can someone please check whether this code:

/* Hook for arm_pm_restart to ensure we execute the reset code
* with the caches enabled. It seems at least the S3C2440 has a problem
* resetting if there is bus activity interrupted by the reset. */
static void s3c24xx_pm_restart(char mode, const char *cmd)

{
        if (mode != 's') {
                unsigned long flags;

                local_irq_save(flags);
                __cpuc_flush_kern_all();
                __cpuc_flush_user_all();

                arch_reset(mode, cmd);
                local_irq_restore(flags);
        }

        /* fallback, or unhandled */
        arm_machine_restart(mode, cmd);
}

will be broken by any of these patches - and more importantly whether this code can simply be deleted with these five patches applied.

Quoted 23 lines · Russell King - ARM Linux

Can someone please check whether this code:

/* Hook for arm_pm_restart to ensure we execute the reset code
* with the caches enabled. It seems at least the S3C2440 has a problem
* resetting if there is bus activity interrupted by the reset. */
static void s3c24xx_pm_restart(char mode, const char *cmd)

{
        if (mode != 's') {
                unsigned long flags;

                local_irq_save(flags);
                __cpuc_flush_kern_all();
                __cpuc_flush_user_all();

                arch_reset(mode, cmd);
                local_irq_restore(flags);
        }

        /* fallback, or unhandled */
        arm_machine_restart(mode, cmd);
}

Quoted 2 lines

will be broken by any of these patches - and more importantly whether this code can simply be deleted with these five patches applied.

Okay, I'm going to delete the above and hope for the best; I'm taking the lack of reply as meaning that no one in the Samsung community is that interested in this code.

Quoted 25 lines · Russell King - ARM Linux

Quoted 23 lines · Russell King - ARM Linux

Can someone please check whether this code:

/* Hook for arm_pm_restart to ensure we execute the reset code
* with the caches enabled. It seems at least the S3C2440 has a problem
* resetting if there is bus activity interrupted by the reset. */
static void s3c24xx_pm_restart(char mode, const char *cmd)

{
        if (mode != 's') {
                unsigned long flags;

                local_irq_save(flags);
                __cpuc_flush_kern_all();
                __cpuc_flush_user_all();

                arch_reset(mode, cmd);
                local_irq_restore(flags);
        }

        /* fallback, or unhandled */
        arm_machine_restart(mode, cmd);
}

Quoted 6 lines

Quoted 2 lines

will be broken by any of these patches - and more importantly whether this code can simply be deleted with these five patches applied.

Okay, I'm going to delete the above and hope for the best; I'm taking the lack of reply as meaning that no one in the Samsung community is that interested in this code.

Russell,
Sorry for late response.

According to its commit message, it is needed...

[ARM] 4987/1: S3C24XX: Ensure watchdog reset initiated from cached code.

There seems to be some problem with at-least the S3C2440 and bus traffic during an reset. It is unlikely, but still possible that the system will hang in such a way that the watchdog cannot get the system out of the state it is in.

Change to making the code that calls the watchdog reset run from cached memory so that instruction fetches have quiesced before the watchdog fires.

But frankly, I'm not sure now and there is no S3C2440 on my desk :( Let me check again, if no response on this, please keep going on.

(Poodle doesn't have any MAINTAINERS entry, I think Richard Purdie last touched Poodle in a 'maintainery' way back in 2006.)

poodle does this:

static void poodle_restart(char mode, const char *cmd)

{
        arm_machine_restart('h', cmd);
}

Given that the default value of 'mode' (in the absense of any reboot= command line argument to the kernel) is 'h', this seems to be a no-op. Below is the history; I suspect that the reason this first appeared was because of the need to fiddle with RCSR - that's now gone so I suspect the entire restart handler can be deleted.

What I don't understand in that case is why 'mode' wasn't simply passed through in the first patch.

Quoted 1 line

From what I remember that hardware either always reboots or always

halts. I think the option was therefore left hardcoded to make it clear it wasn't expected to work. Later Zaurii models could do either but required some manual poking of registers to make it happen iirc.

Regardless, you can probably clean this up as you suggest now.

Quoted 6 lines · Richard Purdie

Quoted 1 line

From what I remember that hardware either always reboots or always

halts. I think the option was therefore left hardcoded to make it clear it wasn't expected to work. Later Zaurii models could do either but required some manual poking of registers to make it happen iirc.

Regardless, you can probably clean this up as you suggest now.

Thanks, I've now committed a change which removes the indirection entirely from poodle.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 12 lines · Russell King - ARM Linux

Quoted 6 lines · Richard Purdie

Quoted 1 line

From what I remember that hardware either always reboots or always

halts. I think the option was therefore left hardcoded to make it clear it wasn't expected to work. Later Zaurii models could do either but required some manual poking of registers to make it happen iirc.

Regardless, you can probably clean this up as you suggest now.

Thanks, I've now committed a change which removes the indirection entirely from poodle.

Hi, sorry for the late reply

I'm playing with poodle and I can confirm that poweroff does indeed not work with 2.6.3x kernels.
See this thread:
http://lists.linuxtogo.org/pipermail/zaurus-devel/2011-May/000501.html

Quoted 14 lines · Andrea Adami

Quoted 12 lines · Russell King - ARM Linux

Quoted 6 lines · Richard Purdie

Quoted 1 line

From what I remember that hardware either always reboots or always

halts. I think the option was therefore left hardcoded to make it clear it wasn't expected to work. Later Zaurii models could do either but required some manual poking of registers to make it happen iirc.

Regardless, you can probably clean this up as you suggest now.

Thanks, I've now committed a change which removes the indirection entirely from poodle.

Quoted 6 lines

Hi, sorry for the late reply

I'm playing with poodle and I can confirm that poweroff does indeed not work with 2.6.3x kernels.
See this thread:
http://lists.linuxtogo.org/pipermail/zaurus-devel/2011-May/000501.html

Okay, at least we know that restart works with that.

I'm not offering to try and change the poweroff behaviour for the better, as I know absolutely nothing about the Zaurus platforms.

This series cleans up some of the restart code for ARM. It doesn't go as far as Will's patch set (or even anywhere close to it) mainly because of the poor state of the various implementations in various platforms.

Effectively, the various platforms themselves stand in the way of optimal cleanup of this code.

Other platforms use arm_pm_restart() to intercept the restart, and force the mode to 'g', 'h' or 's'. As the mode already defaults to 'h' I don't understand why people felt forcing it in that case was necessary. As for the 's' case, this can be done by merely specifying ".soft_reboot = 1" in the machine record. We can solve the 'g' case by changing .soft_reboot to be a char "restart_mode" argument which sets the default restart mode.

So, this patch set takes us a little way there by cleaning up some of the restart code:

1. remove the useless argument to setup_mm_for_reboot()

5. provide (and use) a function for soft rebooting which includes the setup of the MMU tables and call cpu_reset() - and remove the MMU setup from the code paths before arch_reset() is called.

Hi Russell,

Quoted 7 lines · Russell King - ARM Linux

Other platforms use arm_pm_restart() to intercept the restart, and force the mode to 'g', 'h' or 's'. As the mode already defaults to 'h' I don't understand why people felt forcing it in that case was necessary. As for the 's' case, this can be done by merely specifying ".soft_reboot = 1" in the machine record. We can solve the 'g' case by changing .soft_reboot to be a char "restart_mode" argument which sets the default restart mode.

Actually, I think we can go as far as deleteing the 'g' mode and using the GPIO reset as a fallback if we fail in the usual 'h' case. This mode is only used by mach-pxa, so it would be good to get rid of it.

We can always do this in a later patch series if it gets in the way (I have a series to clean up the reboot modes already).

Quoted 4 lines

So, this patch set takes us a little way there by cleaning up some of the restart code:

1. remove the useless argument to setup_mm_for_reboot()

I'm definitely in favour of this change, but be aware that I plan to modify setup_mm_for_reboot in the future to:

(a) Use a static identity mapping so we don't have to pgd_alloc explicitly [I can't see this affecting anything]

(b) Extend the identity mapping into kernel space, leaving some room for the kernel image. If this is all called from soft_reboot, which just does the cpu_reset(addr) afterwards, then we're fine. I have code to take care of changing stack and switching to the new mapping.

Quoted 3 lines

5. provide (and use) a function for soft rebooting which includes the setup of the MMU tables and call cpu_reset() - and remove the MMU setup from the code paths before arch_reset() is called.

This function will probably grow to contain the arm_machine_reset function body that I have for kexec.

The other thing is that the generic code shouldn't care what value 'mode' actually is - we just pass it through to the platform handler (via arm_pm_restart()) and if the platform handler wants to ignore it and just do the software reboot thing, it uses soft_restart(addr) which does all the necessary setup for that.

The nice thing about that is we no longer have to worry about calling arm_pm_restart() with 'h' and a platform doing a soft-reboot instead.

Quoted 10 lines

I'm definitely in favour of this change, but be aware that I plan to modify setup_mm_for_reboot in the future to:

(a) Use a static identity mapping so we don't have to pgd_alloc explicitly [I can't see this affecting anything]

(b) Extend the identity mapping into kernel space, leaving some room for the kernel image. If this is all called from soft_reboot, which just does the cpu_reset(addr) afterwards, then we're fine. I have code to take care of changing stack and switching to the new mapping.

Well, bear in mind that (1) + (5) produce something really self-contained. We no longer fiddle with MMU tables for anything but the 'soft reboot' path. That means things like hardware based reset (eg, using watchdogs, asserting GPIOs, etc) are all dealt with without fiddling with the MMU and caches.

arm_machine_restart() has become almost empty - I want to move the IRQ disables out into machine_restart() and kill off arm_machine_restart() entirely (at the moment its the 'default' action, which is to call the old arch_reset() thing.)

Eventually, when we have everyone hooking into arm_pm_restart() instead, arm_machine_restart() will be unused and meaningless to keep around.

Ah yes, you're right. I think it might be worth defining 'h' as REBOOT_MODE_HARD and 's' as REBOOT_MODE_SOFT, but there's no reason to disallow other combinations giving that the core code no longer has to do anything with these.

Quoted 2 lines

The nice thing about that is we no longer have to worry about calling arm_pm_restart() with 'h' and a platform doing a soft-reboot instead.

Yes, that's truly horrific. It's a miracle soft reboot worked at all from v6 onwards.

Quoted 7 lines

arm_machine_restart() has become almost empty - I want to move the IRQ disables out into machine_restart() and kill off arm_machine_restart() entirely (at the moment its the 'default' action, which is to call the old arch_reset() thing.)

Eventually, when we have everyone hooking into arm_pm_restart() instead, arm_machine_restart() will be unused and meaningless to keep around.

That's where our friend Coccinelle comes in at the end of my patch series.

However, we don't know whether removing the code will then prevent it rebooting even for a userspace-invoked reboot. We need feedback from the platform maintainer for that (added Philipp and Robert in the hope one of them can answer this - and maybe send a patch to fix it.)

Quoted 4 lines · Russell King - ARM Linux

However, we don't know whether removing the code will then prevent it rebooting even for a userspace-invoked reboot. We need feedback from the platform maintainer for that (added Philipp and Robert in the hope one of them can answer this - and maybe send a patch to fix it.)

OK, I tested the serie on mioa701 board, and everything works as before. I tested it with mioa701.c amended so that no more halt/reboot hooks are used anymore. The resources freeing is gone as well.

I will submit the following patch [1] to get rid of the hooks to Eric, unless you want it in your tree.

This series cleans up some of the restart code for ARM. It doesn't go as far as Will's patch set (or even anywhere close to it) mainly because of the poor state of the various implementations in various platforms.