From patchwork Thu Apr 18 19:09:23 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Russell King - ARM Linux X-Patchwork-Id: 237721 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:770:15f::2]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 631AF2C01E6 for ; Fri, 19 Apr 2013 05:09:46 +1000 (EST) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1USuDG-0003QR-98; Thu, 18 Apr 2013 19:09:42 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1USuDD-0006NR-BF; Thu, 18 Apr 2013 19:09:39 +0000 Received: from caramon.arm.linux.org.uk ([78.32.30.218]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1USuD6-0006Mm-V2 for linux-arm-kernel@lists.infradead.org; Thu, 18 Apr 2013 19:09:36 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=arm.linux.org.uk; s=caramon; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=i6x9CotWjWyGebyTZc1M9/UAh1rlddZ55SEoGWrWNSc=; b=LSg9K1hubmSyxPwwlH/3b+ANGwnKWfh96ATaHlg0JA7Mpse2Ff3/DhTQhkXmbcwKau4KUBq+gE/Ym3fJG+CvqoscFOBiauBfKqOuCeXs3mwPW+mGjsdQgF5tHjmRt3pNSt/CnxAoFC0DNIGy/Pk+39NnCfUuVISBfFgC52nbh8w=; Received: from n2100.arm.linux.org.uk ([2002:4e20:1eda:1:214:fdff:fe10:4f86]:55924) by caramon.arm.linux.org.uk with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.76) (envelope-from ) id 1USuCz-0005h4-5Y; Thu, 18 Apr 2013 20:09:25 +0100 Received: from linux by n2100.arm.linux.org.uk with local (Exim 4.76) (envelope-from ) id 1USuCx-0006MJ-Vo; Thu, 18 Apr 2013 20:09:24 +0100 Date: Thu, 18 Apr 2013 20:09:23 +0100 From: Russell King - ARM Linux To: Kevin Bracey Subject: Re: [PATCH] ARM: smp: call platform's cpu_die in ipi_cpu_stop Message-ID: <20130418190923.GE14496@n2100.arm.linux.org.uk> References: <1365251824-4852-1-git-send-email-kevin@bracey.fi> <1365251824-4852-2-git-send-email-kevin@bracey.fi> <20130418171801.GC14496@n2100.arm.linux.org.uk> <20130418182534.GD14496@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130418182534.GD14496@n2100.arm.linux.org.uk> User-Agent: Mutt/1.5.19 (2009-01-05) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130418_150933_636666_EB4C5235 X-CRM114-Status: GOOD ( 49.58 ) X-Spam-Score: -5.0 (-----) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-5.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, medium trust [78.32.30.218 listed in list.dnswl.org] -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org On Thu, Apr 18, 2013 at 07:25:34PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 18, 2013 at 06:18:01PM +0100, Russell King - ARM Linux wrote: > > On Sat, Apr 06, 2013 at 03:37:04PM +0300, Kevin Bracey wrote: > > > When hotplugging out, both cpu_kill and cpu_die have always been called. > > > But when smp_send_stop was used in machine_shutdown or panic, only > > > cpu_kill was called. > > > > > > This causes problems for co-ordination between cpu_kill and cpu_die, as > > > attempted by shmobile. So add cpu_die call to ipi_cpu_stop, to ensure > > > that cpu_kill and cpu_die calls always occur together. > > > > Actually, I'd prefer to pull more code out of the platforms. Now > > that we have flush_cache_louis(), we can flush the L1 cache for the > > CPU going down in cpu_die() itself. We just need to be careful > > with that complete() call to ensure that becomes visible to other > > cores before power is cut to the L1 cache. > > > > That's fine though - because it must become visible for __cpu_die() > > to continue (otherwise it will time out). > > > > That should render shmobile's cpu_dead thing unnecessary, because the > > platform independent code will do the required synchronisation. > > So, something like the below (which is a combined patch of three). The > key points here being that: > > 1. We flush the L1 cache in cpu_die(), which pushes any dirty cache lines > out of the L1 cache and invalidates it. At the point this function > completes, any data in the L1 cache has been pushed out and all > cache lines are invalid. > 2. We complete(), which allows __cpu_die() to proceed and call > platform_cpu_kill(). This may create dirty cache lines which this > CPU exclusively owns, but the CPU in __cpu_die() will gain those > cache lines before wait_for_completion() can return - not only the > completion counter but also the spinlock, which this CPU will have > ended up reading and writing - and so can no longer be owned by the > dying CPU. > 3. The following mb() is belt and braces to ensure that the completion > is visible. It isn't strictly required because the spinlocks will > do the necessary stuff to ensure this. > 4. Any dirtying of the cache after this rellay doesn't matter _if_ only > the stack is touched, or other data is only read. > 5. I've left those platforms which disable the L1 cache, then flush > alone; that _should_ be removable with this patch as well, but as > a separate patch. > > Now, with this patch applied, we guarantee that we push out any data > that matters from the dying CPU before platform_cpu_kill() is called. > That should mean that shmobile can remove that whole cpu_dead thing. > > I've tested this on OMAP, and it appears to work from a simple test of > CPU hotplug. This patch(set) also kills the cpu_disable() that tegra > has which is just a copy of the generic version. Okay, last version for tonight, with additional comments too, and a hole plugged if the powering down is done by the platform via cpu_die(). arch/arm/kernel/smp.c | 42 +++++++++++++++++++++++++++++++--- arch/arm/mach-exynos/hotplug.c | 1 - arch/arm/mach-highbank/hotplug.c | 4 --- arch/arm/mach-imx/hotplug.c | 2 - arch/arm/mach-msm/hotplug.c | 4 --- arch/arm/mach-omap2/omap-hotplug.c | 3 -- arch/arm/mach-prima2/hotplug.c | 3 -- arch/arm/mach-realview/hotplug.c | 2 - arch/arm/mach-shmobile/smp-sh73a0.c | 8 ------ arch/arm/mach-spear13xx/hotplug.c | 2 - arch/arm/mach-tegra/common.h | 1 - arch/arm/mach-tegra/hotplug.c | 10 -------- arch/arm/mach-tegra/platsmp.c | 1 - arch/arm/mach-ux500/hotplug.c | 3 -- arch/arm/mach-vexpress/hotplug.c | 2 - 15 files changed, 38 insertions(+), 50 deletions(-) diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 1f2cccc..4231034 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -211,6 +211,13 @@ void __cpuinit __cpu_die(unsigned int cpu) } printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); + /* + * platform_cpu_kill() is generally expected to do the powering off + * and/or cutting of clocks to the dying CPU. Optionally, this may + * be done by the CPU which is dying in preference to supporting + * this call, but that means there is _no_ synchronisation between + * the requesting CPU and the dying CPU actually losing power. + */ if (!platform_cpu_kill(cpu)) printk("CPU%u: unable to kill\n", cpu); } @@ -230,14 +237,41 @@ void __ref cpu_die(void) idle_task_exit(); local_irq_disable(); - mb(); - /* Tell __cpu_die() that this CPU is now safe to dispose of */ + /* + * Flush the data out of the L1 cache for this CPU. This must be + * before the completion to ensure that data is safely written out + * before platform_cpu_kill() gets called - which may disable + * *this* CPU and power down its cache. + */ + flush_cache_louis(); + + /* + * Tell __cpu_die() that this CPU is now safe to dispose of. Once + * this returns, power and/or clocks can be removed at any point + * from this CPU and its cache by platform_cpu_kill(). + */ RCU_NONIDLE(complete(&cpu_died)); /* - * actual CPU shutdown procedure is at least platform (if not - * CPU) specific. + * Ensure that the cache lines associated with that completion are + * written out. This covers the case where _this_ CPU is doing the + * powering down, to ensure that the completion is visible to the + * CPU waiting for this one. + */ + flush_cache_louis(); + + /* + * The actual CPU shutdown procedure is at least platform (if not + * CPU) specific. This may remove power, or it may simply spin. + * + * Platforms are generally expected *NOT* to return from this call, + * although there are some which do because they have no way to + * power down the CPU. These platforms are the _only_ reason we + * have a return path which uses the fragment of assembly below. + * + * The return path should not be used for platforms which can + * power off the CPU. */ if (smp_ops.cpu_die) smp_ops.cpu_die(cpu); diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c index c3f825b..af90cfa 100644 --- a/arch/arm/mach-exynos/hotplug.c +++ b/arch/arm/mach-exynos/hotplug.c @@ -28,7 +28,6 @@ static inline void cpu_enter_lowpower_a9(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" diff --git a/arch/arm/mach-highbank/hotplug.c b/arch/arm/mach-highbank/hotplug.c index f30c528..35dd42e 100644 --- a/arch/arm/mach-highbank/hotplug.c +++ b/arch/arm/mach-highbank/hotplug.c @@ -15,8 +15,6 @@ */ #include -#include - #include "core.h" #include "sysregs.h" @@ -28,8 +26,6 @@ extern void secondary_startup(void); */ void __ref highbank_cpu_die(unsigned int cpu) { - flush_cache_all(); - highbank_set_cpu_jump(cpu, phys_to_virt(0)); highbank_set_core_pwr(); diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c index 361a253..5e91112 100644 --- a/arch/arm/mach-imx/hotplug.c +++ b/arch/arm/mach-imx/hotplug.c @@ -11,7 +11,6 @@ */ #include -#include #include #include "common.h" @@ -20,7 +19,6 @@ static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( "mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" diff --git a/arch/arm/mach-msm/hotplug.c b/arch/arm/mach-msm/hotplug.c index 750446f..326a872 100644 --- a/arch/arm/mach-msm/hotplug.c +++ b/arch/arm/mach-msm/hotplug.c @@ -10,16 +10,12 @@ #include #include -#include #include #include "common.h" static inline void cpu_enter_lowpower(void) { - /* Just flush the cache. Changing the coherency is not yet - * available on msm. */ - flush_cache_all(); } static inline void cpu_leave_lowpower(void) diff --git a/arch/arm/mach-omap2/omap-hotplug.c b/arch/arm/mach-omap2/omap-hotplug.c index e712d17..ceb30a5 100644 --- a/arch/arm/mach-omap2/omap-hotplug.c +++ b/arch/arm/mach-omap2/omap-hotplug.c @@ -35,9 +35,6 @@ void __ref omap4_cpu_die(unsigned int cpu) unsigned int boot_cpu = 0; void __iomem *base = omap_get_wakeupgen_base(); - flush_cache_all(); - dsb(); - /* * we're ready for shutdown now, so do it */ diff --git a/arch/arm/mach-prima2/hotplug.c b/arch/arm/mach-prima2/hotplug.c index f4b17cb..0ab2f8b 100644 --- a/arch/arm/mach-prima2/hotplug.c +++ b/arch/arm/mach-prima2/hotplug.c @@ -10,13 +10,10 @@ #include #include -#include #include static inline void platform_do_lowpower(unsigned int cpu) { - flush_cache_all(); - /* we put the platform to just WFI */ for (;;) { __asm__ __volatile__("dsb\n\t" "wfi\n\t" diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c index 53818e5..ac22dd4 100644 --- a/arch/arm/mach-realview/hotplug.c +++ b/arch/arm/mach-realview/hotplug.c @@ -12,7 +12,6 @@ #include #include -#include #include #include @@ -20,7 +19,6 @@ static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" diff --git a/arch/arm/mach-shmobile/smp-sh73a0.c b/arch/arm/mach-shmobile/smp-sh73a0.c index acb46a9..2f1ef1b 100644 --- a/arch/arm/mach-shmobile/smp-sh73a0.c +++ b/arch/arm/mach-shmobile/smp-sh73a0.c @@ -119,14 +119,6 @@ static int sh73a0_cpu_kill(unsigned int cpu) static void sh73a0_cpu_die(unsigned int cpu) { - /* - * The ARM MPcore does not issue a cache coherency request for the L1 - * cache when powering off single CPUs. We must take care of this and - * further caches. - */ - dsb(); - flush_cache_all(); - /* Set power off mode. This takes the CPU out of the MP cluster */ scu_power_mode(scu_base_addr(), SCU_PM_POWEROFF); diff --git a/arch/arm/mach-spear13xx/hotplug.c b/arch/arm/mach-spear13xx/hotplug.c index a7d2dd1..d97749c 100644 --- a/arch/arm/mach-spear13xx/hotplug.c +++ b/arch/arm/mach-spear13xx/hotplug.c @@ -13,7 +13,6 @@ #include #include #include -#include #include #include @@ -21,7 +20,6 @@ static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " dsb\n" diff --git a/arch/arm/mach-tegra/common.h b/arch/arm/mach-tegra/common.h index 32f8eb3..5900cc4 100644 --- a/arch/arm/mach-tegra/common.h +++ b/arch/arm/mach-tegra/common.h @@ -2,4 +2,3 @@ extern struct smp_operations tegra_smp_ops; extern int tegra_cpu_kill(unsigned int cpu); extern void tegra_cpu_die(unsigned int cpu); -extern int tegra_cpu_disable(unsigned int cpu); diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c index a599f6e..e8323bc 100644 --- a/arch/arm/mach-tegra/hotplug.c +++ b/arch/arm/mach-tegra/hotplug.c @@ -12,7 +12,6 @@ #include #include -#include #include #include "sleep.h" @@ -47,15 +46,6 @@ void __ref tegra_cpu_die(unsigned int cpu) BUG(); } -int tegra_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} - #ifdef CONFIG_ARCH_TEGRA_2x_SOC extern void tegra20_hotplug_shutdown(void); void __init tegra20_hotplug_init(void) diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c index 2c6b3d5..ec33ec8 100644 --- a/arch/arm/mach-tegra/platsmp.c +++ b/arch/arm/mach-tegra/platsmp.c @@ -192,6 +192,5 @@ struct smp_operations tegra_smp_ops __initdata = { #ifdef CONFIG_HOTPLUG_CPU .cpu_kill = tegra_cpu_kill, .cpu_die = tegra_cpu_die, - .cpu_disable = tegra_cpu_disable, #endif }; diff --git a/arch/arm/mach-ux500/hotplug.c b/arch/arm/mach-ux500/hotplug.c index 2f6af25..1c55a55 100644 --- a/arch/arm/mach-ux500/hotplug.c +++ b/arch/arm/mach-ux500/hotplug.c @@ -12,7 +12,6 @@ #include #include -#include #include #include @@ -24,8 +23,6 @@ */ void __ref ux500_cpu_die(unsigned int cpu) { - flush_cache_all(); - /* directly enter low power state, skipping secure registers */ for (;;) { __asm__ __volatile__("dsb\n\t" "wfi\n\t" diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c index a141b98..f0ce6b8 100644 --- a/arch/arm/mach-vexpress/hotplug.c +++ b/arch/arm/mach-vexpress/hotplug.c @@ -12,7 +12,6 @@ #include #include -#include #include #include @@ -20,7 +19,6 @@ static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( "mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n"