From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751994AbdGFRkr (ORCPT ); Thu, 6 Jul 2017 13:40:47 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:40636 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751813AbdGFRkq (ORCPT ); Thu, 6 Jul 2017 13:40:46 -0400 Date: Thu, 6 Jul 2017 18:39:49 +0100 From: Mark Rutland To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: Florian Fainelli , linux-arm-kernel@lists.infradead.org, support@lemaker.org, =?utf-8?B?5byg5aSp55uK?= , Jason Cooper , Arnd Bergmann , =?utf-8?B?5qKF5Yip?= , Neil Armstrong , linux-kernel@vger.kernel.org, Thomas Liau , Russell King , support@cubietech.com, lee@cubietech.com, Andrew Lunn , =?utf-8?B?5byg5Lic6aOO?= , =?utf-8?B?5YiY54Kc?= , Gregory Clement , Alexandre Belloni , Sebastian Hesselbarth Subject: Re: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() Message-ID: <20170706173949.GC2068@leverpostej> References: <20170704233219.18790-1-afaerber@suse.de> <3F3740FC-3873-4C01-A625-C8DC7F50A018@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" 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 > >> Signed-off-by: Andreas Färber > >> --- > > > >> - 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Thu, 6 Jul 2017 18:39:49 +0100 Subject: [PATCH] ARM: owl: smp: Drop owl_secondary_boot() In-Reply-To: References: <20170704233219.18790-1-afaerber@suse.de> <3F3740FC-3873-4C01-A625-C8DC7F50A018@gmail.com> Message-ID: <20170706173949.GC2068@leverpostej> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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" 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 > >> Signed-off-by: Andreas F?rber > >> --- > > > >> - 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.