On Sat, Aug 02, 2014 at 09:19:24AM +0100, Russell King wrote:

Here's some more comments on this.

On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote:

quoted

+static void brcmstb_cpu_die(u32 cpu)
+{
+	v7_exit_coherency_flush(all);

This is ultimately what causes my builds to break:

/tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb '
/tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb '
make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1

It seems that v7_exit_coherency_flush() can only be used with code which
is ARMv7 only.

Yes, I noticed this already, and I proposed a solution:

  http://article.gmane.org/gmane.linux.drivers.devicetree/84517

quoted

+	/* Prevent all interrupts from reaching this CPU. */
+	arch_local_irq_disable();

Why do you think it is necessary to disable interrupts here?  Where
have they been re-enabled since this bit of generic code:

void __ref cpu_die(void)
{
        unsigned int cpu = smp_processor_id();

        idle_task_exit();

        local_irq_disable();

and why arch_local_irq_disable() at that?  Even if interrupts were
enabled prior to your call to arch_local_irq_disable(), what do you
think would be the effect of receiving an interrupt after you've
exited coherency?

This mistake was already noted. No need for the extra IRQ disable.

(http://article.gmane.org/gmane.linux.drivers.devicetree/84516)

quoted

+
+	/*
+	 * Final full barrier to ensure everything before this instruction has
+	 * quiesced.
+	 */
+	isb();
+	dsb();

If the call to arch_local_irq_disable() is removed, and
v7_exit_coherency_flush() is fixed, then this is not required, because
v7_exit_coherency_flush() already does this at the very end.

Right. Will drop.

quoted

+
+	per_cpu_sw_state_wr(cpu, 0);
+
+	/* Sit and wait to die */
+	wfi();
+
+	/* We should never get here... */
+	panic("Spurious interrupt on CPU %d received!\n", cpu);

You really should /not/ be calling panic here, because that uses data
shared with the CPUs which are still coherent.  This is akin to doing
DMA into bits of the kernel space without dealing with the cache
coherency issues.

OK.

Moreover, if you read the comments on
v7_exit_coherency_flush() about ldrex/strex, which are two instructions
spinlocks use, you'll see that ldrex/strex must not be executed, which
means you can't call any function which uses spinlocks.  That rules
out printk() et.al.  printascii is fine, but that's only available when
the low level debug stuff is enabled.

OK, so I'll drop the panic(). printascii doesn't look extremely useful,
but I suppose we could use it for debugging. Seems like a while (1) loop
might be a suitable replacement. If we get this far, we'll likely get
locked up trying to power this CPU off anyway, so it'll be apparent that
there was power-down failure.

Thanks,
Brian