Re: [PATCH 2/2] MIPS: Remove custom implementations CPU hang implementations

[James Hogan <jhogan@kernel.org>]

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

On Wed, Aug 23, 2017 at 01:53:17PM -0700, Paul Burton wrote:
> Many platforms implement variations upon the same theme of hanging the
> CPU using an infinite loop in their _machine_halt, _machine_restart or
> pm_power_off callbacks. Our generic machine_halt(), machine_restart() &
> machine_power_off() functions will do this for us if the platform
> doesn't specify the appropriate callback or the callback function
> returns, so there's no need for each platform to implement the same
> thing.
> 
> This patch removes many platforms implementations of this functionality,
> leaving it to the generic code to handle.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org

This doesn't apply cleanly due to removal of lantiq/xway/reset.c.

Any reason not to remove ip27_machine_power_off() also? I guess not the
noreturn in ip27_machine_halt() due to the SMP management it does.

Should stop_this_cpu() do something more efficient too?

We do have to be careful about these callbacks disabling IRQs and
returning, on SMP platforms at least. smp_call_function() says not to
call it with IRQs disabled. Perhaps generic code should warn in the SMP
#ifdef if interrupts have been disabled by the platform callbacks before
doing the SMP call, though of course for half of them it might never
reach that point.


> diff --git a/arch/mips/alchemy/board-gpr.c b/arch/mips/alchemy/board-gpr.c
> index 6fb6b3faa158..218d1255a4cb 100644
> --- a/arch/mips/alchemy/board-gpr.c
> +++ b/arch/mips/alchemy/board-gpr.c
> @@ -80,18 +80,10 @@ static void gpr_reset(char *c)
>  		cpu_wait();
>  }
>  
> -static void gpr_power_off(void)
> -{
> -	while (1)
> -		cpu_wait();
> -}
> -

Presumably the loop in gpr_reset() could go too, so it falls back to
generic code? (I can't see any sign of SMP support for alchemy).

> diff --git a/arch/mips/ath79/setup.c b/arch/mips/ath79/setup.c
> index f206dafbb0a3..ce28428cf256 100644
> --- a/arch/mips/ath79/setup.c
> +++ b/arch/mips/ath79/setup.c
> @@ -46,12 +46,6 @@ static void ath79_restart(char *command)
>  			cpu_wait();
>  }
>  
> -static void ath79_halt(void)
> -{
> -	while (1)
> -		cpu_wait();
> -}

The ath79_restart could presumably fall back to generic too?

> diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
> index 6054d49e608e..f7ab6b4085ad 100644
> --- a/arch/mips/bcm47xx/setup.c
> +++ b/arch/mips/bcm47xx/setup.c
> @@ -77,8 +77,6 @@ static void bcm47xx_machine_restart(char *command)
>  		break;
>  #endif
>  	}
> -	while (1)
> -		cpu_relax();
>  }
>  
>  static void bcm47xx_machine_halt(void)
> @@ -97,8 +95,6 @@ static void bcm47xx_machine_halt(void)
>  		break;
>  #endif
>  	}
> -	while (1)
> -		cpu_relax();
>  }

These do disable interrupts, but no SMP that I can see so I suppose its
safe.

>  
>  #ifdef CONFIG_BCM47XX_SSB
> diff --git a/arch/mips/bcm63xx/setup.c b/arch/mips/bcm63xx/setup.c
> index 2be9caaa2085..43a9617a19af 100644
> --- a/arch/mips/bcm63xx/setup.c
> +++ b/arch/mips/bcm63xx/setup.c
> @@ -22,13 +22,6 @@
>  #include <bcm63xx_io.h>
>  #include <bcm63xx_gpio.h>
>  
> -void bcm63xx_machine_halt(void)

There's a declaration of this in
arch/mips/include/asm/mach-bcm63xx/bcm63xx_cpu.h that can be removed
now too.

> -{
> -	pr_info("System halted\n");
> -	while (1)
> -		;
> -}
> -
>  static void bcm6348_a1_reboot(void)
>  {
>  	u32 reg;
> @@ -148,9 +141,7 @@ void __init plat_mem_setup(void)
>  {
>  	add_memory_region(0, bcm63xx_get_memory_size(), BOOT_MEM_RAM);
>  
> -	_machine_halt = bcm63xx_machine_halt;
>  	_machine_restart = __bcm63xx_machine_reboot;

Worth bcm63xx_machine_reboot()'s fallback loop falling back to generic?
It seems to support SMP, but it doesn't disable IRQs.

> diff --git a/arch/mips/cobalt/reset.c b/arch/mips/cobalt/reset.c
> index 4eedd481dd00..727f139ed460 100644
> --- a/arch/mips/cobalt/reset.c
> +++ b/arch/mips/cobalt/reset.c
> @@ -37,10 +37,6 @@ void cobalt_machine_halt(void)
>  	led_trigger_event(power_off_led_trigger, LED_FULL);
>  
>  	local_irq_disable();
> -	while (1) {
> -		if (cpu_wait)
> -			cpu_wait();
> -	}

The local_irq_disable() could be dropped to.

> diff --git a/arch/mips/lasat/reset.c b/arch/mips/lasat/reset.c
> index e21f0b9a586e..d4a5d5b787a9 100644
> --- a/arch/mips/lasat/reset.c
> +++ b/arch/mips/lasat/reset.c
> @@ -49,7 +49,6 @@ static void lasat_machine_halt(void)
>  	local_irq_disable();
>  
>  	prom_monitor();
> -	for (;;) ;

same for lasat_machine_restart?

(no SMP here either AFAICT)

> diff --git a/arch/mips/loongson64/common/reset.c b/arch/mips/loongson64/common/reset.c
> index a60715e11306..ec290392c175 100644
> --- a/arch/mips/loongson64/common/reset.c
> +++ b/arch/mips/loongson64/common/reset.c
> @@ -48,10 +48,6 @@ static void loongson_restart(char *command)
>  	void (*fw_restart)(void) = (void *)loongson_sysconf.restart_addr;
>  
>  	fw_restart();
> -	while (1) {
> -		if (cpu_wait)
> -			cpu_wait();
> -	}

Loongson64 does support SMP. If fw_restart() disabled IRQs before
returning that could be a problem?

But maybe it'd be a problem for it to return at all...

> @@ -64,26 +60,12 @@ static void loongson_poweroff(void)
>  	void (*fw_poweroff)(void) = (void *)loongson_sysconf.poweroff_addr;
>  
>  	fw_poweroff();
> -	while (1) {
> -		if (cpu_wait)
> -			cpu_wait();
> -	}

same?

> diff --git a/arch/mips/sgi-ip22/ip22-reset.c b/arch/mips/sgi-ip22/ip22-reset.c
> index 03a39ac5ead9..ce1f435f31de 100644
> --- a/arch/mips/sgi-ip22/ip22-reset.c
> +++ b/arch/mips/sgi-ip22/ip22-reset.c
> @@ -71,7 +71,6 @@ static void __noreturn sgi_machine_restart(char *command)
>  	if (machine_state & MACHINE_SHUTTING_DOWN)
>  		sgi_machine_power_off();
>  	sgimc->cpuctrl0 |= SGIMC_CCTRL0_SYSINIT;
> -	while (1);

Don't forget to drop the __noreturn (I haven't checked this on other
paths).

> diff --git a/arch/mips/txx9/generic/setup.c b/arch/mips/txx9/generic/setup.c
> index 1791a44ee570..c58ab1bdd3e5 100644
> --- a/arch/mips/txx9/generic/setup.c
> +++ b/arch/mips/txx9/generic/setup.c
> @@ -370,25 +370,6 @@ const char *__init prom_getenv(const char *name)
>  	return NULL;
>  }
>  
> -static void __noreturn txx9_machine_halt(void)
> -{
> -	local_irq_disable();
> -	clear_c0_status(ST0_IM);
> -	while (1) {
> -		if (cpu_wait) {
> -			(*cpu_wait)();
> -			if (cpu_has_counter) {
> -				/*
> -				 * Clear counter interrupt while it
> -				 * breaks WAIT instruction even if
> -				 * masked.
> -				 */
> -				write_c0_compare(0);
> -			}
> -		}
> -	}
> -}

I think this was used indirectly (via _machine_halt) by
tx4927_machine_restart(), tx4938_machine_restart(),
tx4939_machine_restart(), jmr3927_machine_restart(),
toshiba_rbtx4927_restart(), and rbtx4938_machine_restart() as a fallback
if restart doesn't work.

I suppose those fallbacks should be dropped to avoid calling NULL and it
should just fall through to the generic halt code?

There's also a while loop in rbtx4939_machine_restart(). No SMP here
either apparently.

> diff --git a/arch/mips/vr41xx/common/pmu.c b/arch/mips/vr41xx/common/pmu.c
> index 39a0db3e2b34..65157b686b5c 100644
> --- a/arch/mips/vr41xx/common/pmu.c
> +++ b/arch/mips/vr41xx/common/pmu.c
> @@ -84,7 +84,6 @@ static void vr41xx_restart(char *command)
>  {
>  	local_irq_disable();
>  	software_reset();
> -	while (1) ;

No SMP, so I suppose its fine.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]