Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640516 > unrolled thread

Re: [PATCH] socfpga_a10: reset CPU1 in socfpga_cpu_kill()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] socfpga_a10: reset CPU1 in socfpga_cpu_kill() Mark Rutland <mark.rutland@arm.com> - 2017-05-12 16:40 +0200
    Re: [PATCH] socfpga_a10: reset CPU1 in socfpga_cpu_kill() yjin <yanjiang.jin@windriver.com> - 2017-05-15 11:10 +0200
    Re: [PATCH] socfpga_a10: reset CPU1 in socfpga_cpu_kill() Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-15 12:10 +0200

#1640516 — Re: [PATCH] socfpga_a10: reset CPU1 in socfpga_cpu_kill()

On Wed, May 10, 2017 at 01:13:04AM -0400, yanjiang.jin@windriver.com wrote:
> From: Yanjiang Jin <yanjiang.jin@windriver.com>
> 
> Kexec's second kernel would hang if CPU1 isn't reset.
> 
> Signed-off-by: Yanjiang Jin <yanjiang.jin@windriver.com>
> ---
>  arch/arm/mach-socfpga/platsmp.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
> index 0ee7677..db3940e 100644
> --- a/arch/arm/mach-socfpga/platsmp.c
> +++ b/arch/arm/mach-socfpga/platsmp.c
> @@ -117,6 +117,16 @@ static int socfpga_cpu_kill(unsigned int cpu)
>  {
>  	return 1;
>  }
> +
> +static int socfpga_a10_cpu_kill(unsigned int cpu)
> +{
> +	/* This will put CPU #1 into reset. */
> +	if (socfpga_cpu1start_addr)
> +		writel(RSTMGR_MPUMODRST_CPU1, rst_manager_base_addr +
> +			SOCFPGA_A10_RSTMGR_MODMPURST);
> +
> +	return 1;
> +}
>  #endif

I agree that currently, socfpga_cpu_die is completely bogus, as the CPU is just
sat in WFI with the MMU, caches, etc enabled, and not actually off:

static void socfpga_cpu_die(unsigned int cpu)
{
        /* Do WFI. If we wake up early, go back into WFI */
        while (1)
                cpu_do_idle();
}

... so that goes wrong as soon as the kerenl text gets ovewritten.

However, AFAICT, this patch forcibly resets is without any teardown
having happened. That will surely result in data being lost from the
caches, for example.

So I think this is incomplete.

Thanks,
Mark.

>  
>  static const struct smp_operations socfpga_smp_ops __initconst = {
> @@ -133,7 +143,7 @@ static int socfpga_cpu_kill(unsigned int cpu)
>  	.smp_boot_secondary	= socfpga_a10_boot_secondary,
>  #ifdef CONFIG_HOTPLUG_CPU
>  	.cpu_die		= socfpga_cpu_die,
> -	.cpu_kill		= socfpga_cpu_kill,
> +	.cpu_kill		= socfpga_a10_cpu_kill,
>  #endif
>  };
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [next] | [standalone]

#1641446

On 2017年05月12日 22:36, Mark Rutland wrote:
> On Wed, May 10, 2017 at 01:13:04AM -0400, yanjiang.jin@windriver.com wrote:
>> From: Yanjiang Jin <yanjiang.jin@windriver.com>
>>
>> Kexec's second kernel would hang if CPU1 isn't reset.
>>
>> Signed-off-by: Yanjiang Jin <yanjiang.jin@windriver.com>
>> ---
>>   arch/arm/mach-socfpga/platsmp.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
>> index 0ee7677..db3940e 100644
>> --- a/arch/arm/mach-socfpga/platsmp.c
>> +++ b/arch/arm/mach-socfpga/platsmp.c
>> @@ -117,6 +117,16 @@ static int socfpga_cpu_kill(unsigned int cpu)
>>   {
>>   	return 1;
>>   }
>> +
>> +static int socfpga_a10_cpu_kill(unsigned int cpu)
>> +{
>> +	/* This will put CPU #1 into reset. */
>> +	if (socfpga_cpu1start_addr)
>> +		writel(RSTMGR_MPUMODRST_CPU1, rst_manager_base_addr +
>> +			SOCFPGA_A10_RSTMGR_MODMPURST);
>> +
>> +	return 1;
>> +}
>>   #endif
> I agree that currently, socfpga_cpu_die is completely bogus, as the CPU is just
> sat in WFI with the MMU, caches, etc enabled, and not actually off:
>
> static void socfpga_cpu_die(unsigned int cpu)
> {
>          /* Do WFI. If we wake up early, go back into WFI */
>          while (1)
>                  cpu_do_idle();
> }
>
> ... so that goes wrong as soon as the kerenl text gets ovewritten.
>
> However, AFAICT, this patch forcibly resets is without any teardown
> having happened. That will surely result in data being lost from the
> caches, for example.
>
> So I think this is incomplete.
Hi Mark,

I think I got what you mean. So far, flush_cache is the only thing that 
I can think of.
Can we just add flush_cache_all in cpu_die() now, and add other missing 
parts(if have) in the future.

Thanks!
Yanjiang

> Thanks,
> Mark.
>
>>   
>>   static const struct smp_operations socfpga_smp_ops __initconst = {
>> @@ -133,7 +143,7 @@ static int socfpga_cpu_kill(unsigned int cpu)
>>   	.smp_boot_secondary	= socfpga_a10_boot_secondary,
>>   #ifdef CONFIG_HOTPLUG_CPU
>>   	.cpu_die		= socfpga_cpu_die,
>> -	.cpu_kill		= socfpga_cpu_kill,
>> +	.cpu_kill		= socfpga_a10_cpu_kill,
>>   #endif
>>   };
>>   
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [next] | [standalone]

#1641507

On Fri, May 12, 2017 at 03:36:01PM +0100, Mark Rutland wrote:
> However, AFAICT, this patch forcibly resets is without any teardown
> having happened. That will surely result in data being lost from the
> caches, for example.

You're wrong on that point.  Having each bloody platform implement
the same friggin teardown is utter madness, and leads to all sorts
of synchronisation issues.

The generic code already deals with the cache issues, and has
synchronisation to ensure that the dying CPU completes the cache
handling before the requesting CPU continues with the killing.
The only thing that platform code need concern itself with is doing
is the "make the CPU die" thing.

It's not perfect, but it's good enough for the majority of cases.

In any case, encouraging people to add flush_cache_all() into their
cpu_die() function is NOT the way forward if there is a problem -
that introduces a new race between flush_cache_all() walking all the
cache lines and cpu_kill() actually turning the power off to the CPU,
which could very well happen either before or during the
flush_cache_all() execution.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web