Date:	Wed, 13 Aug 2014 16:47:06 -0700
From:	Brian Norris <computersforpeace@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Rob Herring <robherring2@...il.com>, Arnd Bergmann <arnd@...db.de>,
	Olof Johansson <olof@...om.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Christian Daudt <bcm@...thebug.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Matt Porter <mporter@...aro.org>,
	Marc Carino <marc.ceeeee@...il.com>,
	Gregory Fong <gregory.0xf0@...il.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 01/11] ARM: brcmstb: add infrastructure for ARM-based
 Broadcom STB SoCs

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists