From: Brian Norris <computersforpeace@gmail.com> To: Russell King - ARM Linux <linux@arm.linux.org.uk> Cc: Rob Herring <robherring2@gmail.com>, Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, Florian Fainelli <f.fainelli@gmail.com>, Christian Daudt <bcm@fixthebug.org>, Linux Kernel <linux-kernel@vger.kernel.org>, Matt Porter <mporter@linaro.org>, Marc Carino <marc.ceeeee@gmail.com>, Gregory Fong <gregory.0xf0@gmail.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs Date: Wed, 13 Aug 2014 16:47:06 -0700 [thread overview] Message-ID: <20140813234706.GC18411@ld-irv-0074> (raw) In-Reply-To: <20140802092756.GZ30282@n2100.arm.linux.org.uk> Hi Russell, Picking up this thread again, as things are now set for dropping this patch and resubmitting SMP support for 3.18. On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote: > On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote: > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and > > smp_ops.cpu_kill() are not synchronized. > ... > > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() > > ensure all reads/writes reach at least the L2? > > What exactly are you trying to achieve here? Synchronization between v7_exit_coherency_flush() (on the dying CPU) and yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core completion-based synchronization is not sufficient, since it allows brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race. Am I somehow not achieving what I intend here? > > How does that ensure that the CPU is down by the time the work is > > scheduled? It seems like this would just defer the work long enough that > > it *most likely* has quiesced, but I don't see how this gives us a > > better guarantee. Or maybe I'm missing something. (If so, please do > > enlighten!) > > Note that I said a delayed work queue. The dying CPU runs a predictable > sequence once cpu_die() has been entered - interrupts at the GIC have > been programmed to be routed to other CPUs, interrupts at the CPU are > masked, so the CPU isn't going to be doing anything else except executing > that code path. So, it's going to be a predictable number of CPU cycles. > > That allows you to arrange a delayed workqueue (or a timer) to fire > after a period of time that you can guarantee that the dying CPU has > reached that wfi(). OK, that sounds workable for the active hotplug case. But what about for the suspend case? CPUs are hot-unplugged during disable_nonboot_cpus(), and I don't see that this would guarantee the workqueue will complete before we enter suspend. > Another point which raises itself in your patch is this: > > + /* Settle-time from Broadcom-internal DVT reference code */ > + udelay(7); > > 7us looks very precise, but udelay() may not be that precise. What is > the actual specification? Does it say "you must wait at least 7us"? > > udelay() _may_ return early, especially if it is using the CPU delay > loop to perform the delay - I've explained why this happens previously, > and why it isn't a bug. > > If you're using a timer-based delay for udelay() (which you should be > using if you support cpufreq) then the delay should be more accurate, > but it's still good practise to give a little leeway on the figure. I'm looking into this specific delay. I'd bet it's just "wait at least 7us." I could probably factor in some leeway to be safe. Thanks, Brian
WARNING: multiple messages have this Message-ID (diff)
From: computersforpeace@gmail.com (Brian Norris) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs Date: Wed, 13 Aug 2014 16:47:06 -0700 [thread overview] Message-ID: <20140813234706.GC18411@ld-irv-0074> (raw) In-Reply-To: <20140802092756.GZ30282@n2100.arm.linux.org.uk> Hi Russell, Picking up this thread again, as things are now set for dropping this patch and resubmitting SMP support for 3.18. On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote: > On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote: > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and > > smp_ops.cpu_kill() are not synchronized. > ... > > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() > > ensure all reads/writes reach at least the L2? > > What exactly are you trying to achieve here? Synchronization between v7_exit_coherency_flush() (on the dying CPU) and yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core completion-based synchronization is not sufficient, since it allows brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race. Am I somehow not achieving what I intend here? > > How does that ensure that the CPU is down by the time the work is > > scheduled? It seems like this would just defer the work long enough that > > it *most likely* has quiesced, but I don't see how this gives us a > > better guarantee. Or maybe I'm missing something. (If so, please do > > enlighten!) > > Note that I said a delayed work queue. The dying CPU runs a predictable > sequence once cpu_die() has been entered - interrupts at the GIC have > been programmed to be routed to other CPUs, interrupts at the CPU are > masked, so the CPU isn't going to be doing anything else except executing > that code path. So, it's going to be a predictable number of CPU cycles. > > That allows you to arrange a delayed workqueue (or a timer) to fire > after a period of time that you can guarantee that the dying CPU has > reached that wfi(). OK, that sounds workable for the active hotplug case. But what about for the suspend case? CPUs are hot-unplugged during disable_nonboot_cpus(), and I don't see that this would guarantee the workqueue will complete before we enter suspend. > Another point which raises itself in your patch is this: > > + /* Settle-time from Broadcom-internal DVT reference code */ > + udelay(7); > > 7us looks very precise, but udelay() may not be that precise. What is > the actual specification? Does it say "you must wait at least 7us"? > > udelay() _may_ return early, especially if it is using the CPU delay > loop to perform the delay - I've explained why this happens previously, > and why it isn't a bug. > > If you're using a timer-based delay for udelay() (which you should be > using if you support cpufreq) then the delay should be more accurate, > but it's still good practise to give a little leeway on the figure. I'm looking into this specific delay. I'd bet it's just "wait at least 7us." I could probably factor in some leeway to be safe. Thanks, Brian
next prev parent reply other threads:[~2014-08-13 23:47 UTC|newest] Thread overview: 121+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-21 21:07 [PATCH v8 00/11] ARM: brcmstb: Add Broadcom STB SoC support Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-30 9:26 ` Russell King - ARM Linux 2014-07-30 9:26 ` Russell King - ARM Linux 2014-07-31 2:36 ` Brian Norris 2014-07-31 2:36 ` Brian Norris 2014-07-31 2:36 ` Brian Norris 2014-07-30 17:09 ` Rob Herring 2014-07-30 17:09 ` Rob Herring 2014-07-30 17:09 ` Rob Herring 2014-07-31 2:23 ` Brian Norris 2014-07-31 2:23 ` Brian Norris 2014-07-31 2:23 ` Brian Norris 2014-07-31 8:43 ` Russell King - ARM Linux 2014-07-31 8:43 ` Russell King - ARM Linux 2014-07-31 8:43 ` Russell King - ARM Linux 2014-07-31 22:06 ` Brian Norris 2014-07-31 22:06 ` Brian Norris 2014-07-31 22:06 ` Brian Norris 2014-08-02 9:27 ` Russell King - ARM Linux 2014-08-02 9:27 ` Russell King - ARM Linux 2014-08-02 9:27 ` Russell King - ARM Linux 2014-08-13 23:47 ` Brian Norris [this message] 2014-08-13 23:47 ` Brian Norris 2014-08-13 23:47 ` Brian Norris 2014-08-19 0:02 ` Brian Norris 2014-08-19 0:02 ` Brian Norris 2014-08-19 0:02 ` Brian Norris 2014-08-01 14:33 ` Rob Herring 2014-08-01 14:33 ` Rob Herring 2014-08-01 14:33 ` Rob Herring 2014-08-01 19:29 ` Florian Fainelli 2014-08-01 19:29 ` Florian Fainelli 2014-08-01 19:29 ` Florian Fainelli 2014-08-01 19:46 ` Matt Porter 2014-08-01 19:46 ` Matt Porter 2014-08-01 19:46 ` Matt Porter 2014-08-02 8:21 ` Russell King - ARM Linux 2014-08-02 8:21 ` Russell King - ARM Linux 2014-08-02 8:21 ` Russell King - ARM Linux 2014-08-02 8:19 ` Russell King - ARM Linux 2014-08-02 8:19 ` Russell King - ARM Linux 2014-08-04 17:39 ` Brian Norris 2014-08-04 17:39 ` Brian Norris 2014-07-21 21:07 ` [PATCH v8 02/11] power: reset: Add reboot driver for brcmstb Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-22 7:28 ` Arnd Bergmann 2014-07-22 7:28 ` Arnd Bergmann 2014-07-22 7:28 ` Arnd Bergmann 2014-07-22 20:02 ` Brian Norris 2014-07-22 20:02 ` Brian Norris 2014-07-22 20:02 ` Brian Norris 2014-07-22 21:02 ` Arnd Bergmann 2014-07-22 21:02 ` Arnd Bergmann 2014-07-22 21:02 ` Arnd Bergmann 2014-07-22 22:51 ` Brian Norris 2014-07-22 22:51 ` Brian Norris 2014-07-21 21:07 ` [PATCH v8 03/11] ARM: brcmstb: add debug UART for earlyprintk support Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-08-02 8:30 ` Russell King - ARM Linux 2014-08-02 8:30 ` Russell King - ARM Linux 2014-08-04 16:56 ` Brian Norris 2014-08-04 16:56 ` Brian Norris 2014-08-13 22:11 ` Brian Norris 2014-08-13 22:11 ` Brian Norris 2014-08-13 22:11 ` Brian Norris 2014-08-13 22:16 ` Olof Johansson 2014-08-13 22:16 ` Olof Johansson 2014-08-13 22:16 ` Olof Johansson 2014-09-02 22:22 ` Florian Fainelli 2014-09-02 22:22 ` Florian Fainelli 2014-09-02 22:22 ` Florian Fainelli 2014-09-02 22:44 ` Brian Norris 2014-09-02 22:44 ` Brian Norris 2014-09-02 22:44 ` Brian Norris 2014-07-21 21:07 ` [PATCH v8 04/11] ARM: do CPU-specific init for Broadcom Brahma15 cores Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:07 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 05/11] ARM: Enable erratum 798181 for Broadcom Brahma-B15 Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 06/11] ARM: brcmstb: add CPU binding for Broadcom Brahma15 Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 07/11] ARM: brcmstb: add misc. DT bindings for brcmstb Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 08/11] ARM: brcmstb: gic: add compatible string for Broadcom Brahma15 Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 09/11] ARM: brcmstb: dts: add a reference DTS for Broadcom 7445 Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 10/11] ARM: brcmstb: select GISB arbiter and interrupt drivers Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` [PATCH v8 11/11] MAINTAINERS: add entry for Broadcom ARM STB architecture Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-21 21:08 ` Brian Norris 2014-07-22 7:35 ` [PATCH v8 00/11] ARM: brcmstb: Add Broadcom STB SoC support Arnd Bergmann 2014-07-22 7:35 ` Arnd Bergmann 2014-07-22 7:35 ` Arnd Bergmann 2014-07-22 20:44 ` Brian Norris 2014-07-22 20:44 ` Brian Norris 2014-07-22 20:57 ` Arnd Bergmann 2014-07-22 20:57 ` Arnd Bergmann 2014-07-22 20:57 ` Arnd Bergmann 2014-07-22 21:33 ` Matt Porter 2014-07-22 21:33 ` Matt Porter 2014-07-22 21:33 ` Matt Porter 2014-07-22 22:24 ` Arnd Bergmann 2014-07-22 22:24 ` Arnd Bergmann 2014-07-22 22:24 ` Arnd Bergmann 2014-07-22 22:30 ` Brian Norris 2014-07-22 22:30 ` Brian Norris
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140813234706.GC18411@ld-irv-0074 \ --to=computersforpeace@gmail.com \ --cc=arnd@arndb.de \ --cc=bcm@fixthebug.org \ --cc=devicetree@vger.kernel.org \ --cc=f.fainelli@gmail.com \ --cc=gregory.0xf0@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=marc.ceeeee@gmail.com \ --cc=mporter@linaro.org \ --cc=olof@lixom.net \ --cc=robherring2@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.