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

Groups > linux.kernel > #1681212 > unrolled thread

[PATCH] ARM: owl: smp: Drop owl_secondary_boot()

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

  [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Andreas Färber <afaerber@suse.de> - 2017-07-05 01:40 +0200
    Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Florian Fainelli <f.fainelli@gmail.com> - 2017-07-05 04:40 +0200
      Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Andreas Färber <afaerber@suse.de> - 2017-07-06 19:20 +0200
        Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-07-06 19:40 +0200
          Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Florian Fainelli <f.fainelli@gmail.com> - 2017-07-06 21:50 +0200
            Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-07-07 09:40 +0200
              Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Florian Fainelli <f.fainelli@gmail.com> - 2017-07-07 19:40 +0200
        Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Mark Rutland <mark.rutland@arm.com> - 2017-07-06 19:50 +0200
          Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Florian Fainelli <f.fainelli@gmail.com> - 2017-07-06 23:20 +0200
        Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Andreas Färber <afaerber@suse.de> - 2017-07-10 00:00 +0200
          Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Florian Fainelli <f.fainelli@gmail.com> - 2017-07-10 06:30 +0200

#1681212 — [PATCH] ARM: owl: smp: Drop owl_secondary_boot()

Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
bogus holding pen") simplified the S500 SMP code by removing a loop for
pen_release in owl_secondary_boot(). Since then it is only calling
owl_v7_invalidate_l1() before branching to secondary_startup().

The owl_v7_invalidate_l1() assembler function is superfluous, too.
Therefore drop owl_secondary_boot() and use secondary_boot() directly.

Cc: David Liu <liuwei@actions-semi.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 arch/arm/mach-actions/Makefile  |  4 +---
 arch/arm/mach-actions/headsmp.S | 52 -----------------------------------------
 arch/arm/mach-actions/platsmp.c |  2 +-
 3 files changed, 2 insertions(+), 56 deletions(-)
 delete mode 100644 arch/arm/mach-actions/headsmp.S

diff --git a/arch/arm/mach-actions/Makefile b/arch/arm/mach-actions/Makefile
index c0f116241da7..13831037d8cd 100644
--- a/arch/arm/mach-actions/Makefile
+++ b/arch/arm/mach-actions/Makefile
@@ -1,3 +1 @@
-obj-${CONFIG_SMP} += platsmp.o headsmp.o
-
-AFLAGS_headsmp.o := -Wa,-march=armv7-a
+obj-${CONFIG_SMP} += platsmp.o
diff --git a/arch/arm/mach-actions/headsmp.S b/arch/arm/mach-actions/headsmp.S
deleted file mode 100644
index 65f53bdb69e7..000000000000
--- a/arch/arm/mach-actions/headsmp.S
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * Copyright 2012 Actions Semi Inc.
- * Author: Actions Semi, Inc.
- *
- * Copyright (c) 2017 Andreas Färber
- *
- * This program is free software; you can redistribute  it and/or modify it
- * under  the terms of  the GNU General  Public License as published by the
- * Free Software Foundation;  either version 2 of the  License, or (at your
- * option) any later version.
- */
-
-#include <linux/linkage.h>
-#include <linux/init.h>
-
-ENTRY(owl_v7_invalidate_l1)
-	mov	r0, #0
-	mcr	p15, 0, r0, c7, c5, 0	@ invalidate I cache
-	mcr	p15, 2, r0, c0, c0, 0
-	mrc	p15, 1, r0, c0, c0, 0
-
-	ldr	r1, =0x7fff
-	and	r2, r1, r0, lsr #13
-
-	ldr	r1, =0x3ff
-
-	and	r3, r1, r0, lsr #3	@ NumWays - 1
-	add	r2, r2, #1		@ NumSets
-
-	and	r0, r0, #0x7
-	add	r0, r0, #4	@ SetShift
-
-	clz	r1, r3		@ WayShift
-	add	r4, r3, #1	@ NumWays
-1:	sub	r2, r2, #1	@ NumSets--
-	mov	r3, r4		@ Temp = NumWays
-2:	subs	r3, r3, #1	@ Temp--
-	mov	r5, r3, lsl r1
-	mov	r6, r2, lsl r0
-	orr	r5, r5, r6	@ Reg = (Temp<<WayShift)|(NumSets<<SetShift)
-	mcr	p15, 0, r5, c7, c6, 2
-	bgt	2b
-	cmp	r2, #0
-	bgt	1b
-	dsb
-	isb
-	mov	pc, lr
-ENDPROC(owl_v7_invalidate_l1)
-
-ENTRY(owl_secondary_startup)
-	bl	owl_v7_invalidate_l1
-	b	secondary_startup
diff --git a/arch/arm/mach-actions/platsmp.c b/arch/arm/mach-actions/platsmp.c
index 12a9e331b432..506ba32f235d 100644
--- a/arch/arm/mach-actions/platsmp.c
+++ b/arch/arm/mach-actions/platsmp.c
@@ -71,7 +71,7 @@ static int s500_wakeup_secondary(unsigned int cpu)
 	/* wait for CPUx to run to WFE instruction */
 	udelay(200);
 
-	writel(virt_to_phys(owl_secondary_startup),
+	writel(virt_to_phys(secondary_startup),
 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
 	writel(OWL_CPUx_FLAG_BOOT,
 	       timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
-- 
2.12.3

[toc] | [next] | [standalone]

#1681245

On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>bogus holding pen") simplified the S500 SMP code by removing a loop for
>pen_release in owl_secondary_boot(). Since then it is only calling
>owl_v7_invalidate_l1() before branching to secondary_startup().
>
>The owl_v7_invalidate_l1() assembler function is superfluous, too.
>Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>
>Cc: David Liu <liuwei@actions-semi.com>
>Signed-off-by: Andreas Färber <afaerber@suse.de>
>---

>-	writel(virt_to_phys(owl_secondary_startup),
>+	writel(virt_to_phys(secondary_startup),
> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);

This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)

-- 
Florian

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

#1682603

Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>> bogus holding pen") simplified the S500 SMP code by removing a loop for
>> pen_release in owl_secondary_boot(). Since then it is only calling
>> owl_v7_invalidate_l1() before branching to secondary_startup().
>>
>> The owl_v7_invalidate_l1() assembler function is superfluous, too.
>> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>>
>> Cc: David Liu <liuwei@actions-semi.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
> 
>> -	writel(virt_to_phys(owl_secondary_startup),
>> +	writel(virt_to_phys(secondary_startup),
>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
> 
> This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)

Thanks for the report. There are no other such uses in mach-actions, but
git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
other such usage in mach-oxnas:

arch/arm/mach-oxnas/platsmp.c:
writel(virt_to_phys(ox820_secondary_startup),

as well as this in mach-mvebu:

arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
MV98DX3236_CPU_RESUME_ADDR_REG);

and these in mach-at91:

arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);

What exactly is the difference between the two macros that makes it
wrong despite apparently working? In particular I am wondering whether
the SoC/board vendors in CC need to fix it in their 3.10 trees, too.

In any case if this is a bug, I would rather fix it in a separate patch
for 4.13 and leave the name swap (this patch) for 4.14.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

#1682608

On 06/07/2017 at 19:17:28 +0200, Andreas Färber wrote:
> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
> > On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
> >> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
> >> bogus holding pen") simplified the S500 SMP code by removing a loop for
> >> pen_release in owl_secondary_boot(). Since then it is only calling
> >> owl_v7_invalidate_l1() before branching to secondary_startup().
> >>
> >> The owl_v7_invalidate_l1() assembler function is superfluous, too.
> >> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
> >>
> >> Cc: David Liu <liuwei@actions-semi.com>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> > 
> >> -	writel(virt_to_phys(owl_secondary_startup),
> >> +	writel(virt_to_phys(secondary_startup),
> >> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
> > 
> > This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)
> 
> Thanks for the report. There are no other such uses in mach-actions, but
> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
> other such usage in mach-oxnas:
> 
> arch/arm/mach-oxnas/platsmp.c:
> writel(virt_to_phys(ox820_secondary_startup),
> 
> as well as this in mach-mvebu:
> 
> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
> MV98DX3236_CPU_RESUME_ADDR_REG);
> 
> and these in mach-at91:
> 
> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
> 

They can probably use __pa_symbol() (I must admit I didn't know about
it before this email)


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

#1682674

On 07/06/2017 10:38 AM, Alexandre Belloni wrote:
> On 06/07/2017 at 19:17:28 +0200, Andreas Färber wrote:
>> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>>> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>>>> bogus holding pen") simplified the S500 SMP code by removing a loop for
>>>> pen_release in owl_secondary_boot(). Since then it is only calling
>>>> owl_v7_invalidate_l1() before branching to secondary_startup().
>>>>
>>>> The owl_v7_invalidate_l1() assembler function is superfluous, too.
>>>> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>>>>
>>>> Cc: David Liu <liuwei@actions-semi.com>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>
>>>> -	writel(virt_to_phys(owl_secondary_startup),
>>>> +	writel(virt_to_phys(secondary_startup),
>>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>>
>>> This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)
>>
>> Thanks for the report. There are no other such uses in mach-actions, but
>> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
>> other such usage in mach-oxnas:
>>
>> arch/arm/mach-oxnas/platsmp.c:
>> writel(virt_to_phys(ox820_secondary_startup),
>>
>> as well as this in mach-mvebu:
>>
>> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
>> MV98DX3236_CPU_RESUME_ADDR_REG);
>>
>> and these in mach-at91:
>>
>> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
>> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
>>
> 
> They can probably use __pa_symbol() (I must admit I didn't know about
> it before this email)

This is fine, do you mind sending fixes for the at91 parts? Gregory, can
you also fix the mv98dx3236 platform SMP code?

Thanks!
-- 
Florian

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

#1682992

Hi Florian,
 
 On jeu., juil. 06 2017, Florian Fainelli <f.fainelli@gmail.com> wrote:

> On 07/06/2017 10:38 AM, Alexandre Belloni wrote:
>> On 06/07/2017 at 19:17:28 +0200, Andreas Färber wrote:
>>> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>>>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>>>> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>>>>> bogus holding pen") simplified the S500 SMP code by removing a loop for
>>>>> pen_release in owl_secondary_boot(). Since then it is only calling
>>>>> owl_v7_invalidate_l1() before branching to secondary_startup().
>>>>>
>>>>> The owl_v7_invalidate_l1() assembler function is superfluous, too.
>>>>> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>>>>>
>>>>> Cc: David Liu <liuwei@actions-semi.com>
>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>> ---
>>>>
>>>>> -	writel(virt_to_phys(owl_secondary_startup),
>>>>> +	writel(virt_to_phys(secondary_startup),
>>>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>>>
>>>> This is a kernel symbol so please use __pa_symbol() here, also you
>>>> might want to build with CONFIG_DEBUG_VIRTUAL and see if you get
>>>> other warnings about using virt_to_phys() in the owl platform code
>>>> (I did not check if there are other uses)
>>>
>>> Thanks for the report. There are no other such uses in mach-actions, but
>>> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
>>> other such usage in mach-oxnas:
>>>
>>> arch/arm/mach-oxnas/platsmp.c:
>>> writel(virt_to_phys(ox820_secondary_startup),
>>>
>>> as well as this in mach-mvebu:
>>>
>>> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
>>> MV98DX3236_CPU_RESUME_ADDR_REG);
>>>
>>> and these in mach-at91:
>>>
>>> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
>>> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
>>>
>> 
>> They can probably use __pa_symbol() (I must admit I didn't know about
>> it before this email)
>
> This is fine, do you mind sending fixes for the at91 parts? Gregory, can
> you also fix the mv98dx3236 platform SMP code?

Yes sure, actually, for Armada XP we already use  __pa_symbol() but from
an other file that why I didn't pay attention on it.

Gregory

>
> Thanks!
> -- 
> Florian

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

#1683339

On 07/07/2017 12:34 AM, Gregory CLEMENT wrote:
> Hi Florian,
>  
>  On jeu., juil. 06 2017, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> On 07/06/2017 10:38 AM, Alexandre Belloni wrote:
>>> On 06/07/2017 at 19:17:28 +0200, Andreas Färber wrote:
>>>> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>>>>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>>>>> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>>>>>> bogus holding pen") simplified the S500 SMP code by removing a loop for
>>>>>> pen_release in owl_secondary_boot(). Since then it is only calling
>>>>>> owl_v7_invalidate_l1() before branching to secondary_startup().
>>>>>>
>>>>>> The owl_v7_invalidate_l1() assembler function is superfluous, too.
>>>>>> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>>>>>>
>>>>>> Cc: David Liu <liuwei@actions-semi.com>
>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>>> ---
>>>>>
>>>>>> -	writel(virt_to_phys(owl_secondary_startup),
>>>>>> +	writel(virt_to_phys(secondary_startup),
>>>>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>>>>
>>>>> This is a kernel symbol so please use __pa_symbol() here, also you
>>>>> might want to build with CONFIG_DEBUG_VIRTUAL and see if you get
>>>>> other warnings about using virt_to_phys() in the owl platform code
>>>>> (I did not check if there are other uses)
>>>>
>>>> Thanks for the report. There are no other such uses in mach-actions, but
>>>> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
>>>> other such usage in mach-oxnas:
>>>>
>>>> arch/arm/mach-oxnas/platsmp.c:
>>>> writel(virt_to_phys(ox820_secondary_startup),
>>>>
>>>> as well as this in mach-mvebu:
>>>>
>>>> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
>>>> MV98DX3236_CPU_RESUME_ADDR_REG);
>>>>
>>>> and these in mach-at91:
>>>>
>>>> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
>>>> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
>>>>
>>>
>>> They can probably use __pa_symbol() (I must admit I didn't know about
>>> it before this email)
>>
>> This is fine, do you mind sending fixes for the at91 parts? Gregory, can
>> you also fix the mv98dx3236 platform SMP code?
> 
> Yes sure, actually, for Armada XP we already use  __pa_symbol() but from
> an other file that why I didn't pay attention on it.

No worries, the conversion sort of happened in one go, so it's always a
game of catching up patches merged after that one went in.
-- 
Florian

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

#1682610

On Thu, Jul 06, 2017 at 07:17:28PM +0200, Andreas Färber wrote:
> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
> > On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
> >> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
> >> bogus holding pen") simplified the S500 SMP code by removing a loop for
> >> pen_release in owl_secondary_boot(). Since then it is only calling
> >> owl_v7_invalidate_l1() before branching to secondary_startup().
> >>
> >> The owl_v7_invalidate_l1() assembler function is superfluous, too.
> >> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
> >>
> >> Cc: David Liu <liuwei@actions-semi.com>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> > 
> >> -	writel(virt_to_phys(owl_secondary_startup),
> >> +	writel(virt_to_phys(secondary_startup),
> >> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
> > 
> > This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)
> 
> Thanks for the report. There are no other such uses in mach-actions, but
> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
> other such usage in mach-oxnas:
> 
> arch/arm/mach-oxnas/platsmp.c:
> writel(virt_to_phys(ox820_secondary_startup),
> 
> as well as this in mach-mvebu:
> 
> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
> MV98DX3236_CPU_RESUME_ADDR_REG);
> 
> and these in mach-at91:
> 
> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
> 
> What exactly is the difference between the two macros that makes it
> wrong despite apparently working? 

virt_to_phys() is intended to operate on the linear/direct mapping of
RAM.

__pa_symbol() is intended to operate on the kernel mapping, which may
not be in the linear/direct mapping on all architectures. e.g. arm64 and
x86_64 map the kernel image and RAM separately.

On 32-bit ARM the kernel image mapping is tied to the linear/direct
mapping, so that works, but as it's semantically wrong (and broken for
generic code), the DEBUG_VIRTUAL checks complain.

> In particular I am wondering whether
> the SoC/board vendors in CC need to fix it in their 3.10 trees, too.
> 
> In any case if this is a bug, I would rather fix it in a separate patch
> for 4.13 and leave the name swap (this patch) for 4.14.

To the best of my knowledge there's no functional problem in this
particular case, though it should be cleaned up so as to keep
DEBUG_VRITUAL useful.

Thanks,
Mark.

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

#1682716

On 07/06/2017 10:39 AM, Mark Rutland wrote:
> On Thu, Jul 06, 2017 at 07:17:28PM +0200, Andreas Färber wrote:
>> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>>> Commit 18cfd9429d8a82c49add8f3ca9d366599bfcac45 ("ARM: owl: smp: Drop
>>>> bogus holding pen") simplified the S500 SMP code by removing a loop for
>>>> pen_release in owl_secondary_boot(). Since then it is only calling
>>>> owl_v7_invalidate_l1() before branching to secondary_startup().
>>>>
>>>> The owl_v7_invalidate_l1() assembler function is superfluous, too.
>>>> Therefore drop owl_secondary_boot() and use secondary_boot() directly.
>>>>
>>>> Cc: David Liu <liuwei@actions-semi.com>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>
>>>> -	writel(virt_to_phys(owl_secondary_startup),
>>>> +	writel(virt_to_phys(secondary_startup),
>>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>>
>>> This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)
>>
>> Thanks for the report. There are no other such uses in mach-actions, but
>> git-grep'ing for virt_to_phys in arch/arm/mach-* I spot at least one
>> other such usage in mach-oxnas:
>>
>> arch/arm/mach-oxnas/platsmp.c:
>> writel(virt_to_phys(ox820_secondary_startup),
>>
>> as well as this in mach-mvebu:
>>
>> arch/arm/mach-mvebu/platsmp.c:  writel(virt_to_phys(boot_addr), base +
>> MV98DX3236_CPU_RESUME_ADDR_REG);
>>
>> and these in mach-at91:
>>
>> arch/arm/mach-at91/pm.c:        pm_bu->canary = virt_to_phys(&canary);
>> arch/arm/mach-at91/pm.c:        pm_bu->resume = virt_to_phys(cpu_resume);
>>
>> What exactly is the difference between the two macros that makes it
>> wrong despite apparently working? 
> 
> virt_to_phys() is intended to operate on the linear/direct mapping of
> RAM.
> 
> __pa_symbol() is intended to operate on the kernel mapping, which may
> not be in the linear/direct mapping on all architectures. e.g. arm64 and
> x86_64 map the kernel image and RAM separately.
> 
> On 32-bit ARM the kernel image mapping is tied to the linear/direct
> mapping, so that works, but as it's semantically wrong (and broken for
> generic code), the DEBUG_VIRTUAL checks complain.
> 
>> In particular I am wondering whether
>> the SoC/board vendors in CC need to fix it in their 3.10 trees, too.
>>
>> In any case if this is a bug, I would rather fix it in a separate patch
>> for 4.13 and leave the name swap (this patch) for 4.14.
> 
> To the best of my knowledge there's no functional problem in this
> particular case, though it should be cleaned up so as to keep
> DEBUG_VRITUAL useful.

Exactly, it just happened to catch my eye while looking at this patch
series. I am wondering if there is a way we could check whether it makes
sense to use virt_to_phys() vs. __pa_symbol() at compile time and
produce an appropriate warning would that be the case. checkpatch.pl
would not be able to do this unless there is a prior build of the kernel
image, and a GCC plugin would not necessarily have global visibility
about other translation units either... hmmm.
-- 
Florian

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

#1683852

Am 06.07.2017 um 19:17 schrieb Andreas Färber:
> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>> -	writel(virt_to_phys(owl_secondary_startup),
>>> +	writel(virt_to_phys(secondary_startup),
>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>
>> This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)

Florian, I don't spot any build or runtime warning for this
virt_to_phys() with CONFIG_DEBUG_VIRTUAL=y on Guitar/S500:

[    0.062765] CPU: Testing write buffer coherency: ok
[    0.063468] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.100856] Setting up static identity map for 0x100000 - 0x100060
[    0.120864] Hierarchical SRCU implementation.
[    0.161092] smp: Bringing up secondary CPUs ...
[    0.291654] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.422226] CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
[    0.552798] CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
[    0.553074] smp: Brought up 1 node, 4 CPUs
[    0.553388] SMP: Total of 4 processors activated (1629.38 BogoMIPS).
[    0.553477] CPU: All CPU(s) started in SVC mode.

I've tested that __pa_symbol() works as well, but I'd like to understand
this for commit message and future testing. Am I missing other options?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

#1683948

On 07/09/2017 02:55 PM, Andreas Färber wrote:
> Am 06.07.2017 um 19:17 schrieb Andreas Färber:
>> Am 05.07.2017 um 04:36 schrieb Florian Fainelli:
>>> On July 4, 2017 4:32:18 PM PDT, "Andreas Färber" <afaerber@suse.de> wrote:
>>>> -	writel(virt_to_phys(owl_secondary_startup),
>>>> +	writel(virt_to_phys(secondary_startup),
>>>> 	       timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>>
>>> This is a kernel symbol so please use __pa_symbol() here, also you might want to build with CONFIG_DEBUG_VIRTUAL and see if you get other warnings about using virt_to_phys() in the owl platform code (I did not check if there are other uses)
> 
> Florian, I don't spot any build or runtime warning for this
> virt_to_phys() with CONFIG_DEBUG_VIRTUAL=y on Guitar/S500:

You would only see run time warnings, not build time warnings for this,
but in fact, no, see below.

> 
> [    0.062765] CPU: Testing write buffer coherency: ok
> [    0.063468] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.100856] Setting up static identity map for 0x100000 - 0x100060
> [    0.120864] Hierarchical SRCU implementation.
> [    0.161092] smp: Bringing up secondary CPUs ...
> [    0.291654] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.422226] CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.552798] CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.553074] smp: Brought up 1 node, 4 CPUs
> [    0.553388] SMP: Total of 4 processors activated (1629.38 BogoMIPS).
> [    0.553477] CPU: All CPU(s) started in SVC mode.
> 
> I've tested that __pa_symbol() works as well, but I'd like to understand
> this for commit message and future testing. Am I missing other options?

After re-checking the implementation you would get a warning only if you
were using virt_to_phys() against a part of the kernel that is not in
the linear map, similarly you would get a warning if __pa_symbol() was
used against symbols outside of the kernel image, this is obviously not
the case here. You should use __pa_symbol() just for correctness, no
warning would be produced, sorry for misleading you with that.
-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web