On 25 November 2014 at 23:12, Nicolas Pitre [off-list ref] wrote:

quoted

+
+static void
+mb86s7x_set_wficolor(unsigned clstr, unsigned cpu, unsigned clr)
+{
+     u8 val;
+
+     if (clr & ~AT_WFI_COLOR_MASK)
+             return;

Shouldn't this be a BUG() rather than a return?

There are only 2 callers of this and they use explicit defines
AT_WFI_DO_xxx, so the check is actually lame and could be removed.

quoted

+
+     val = readb_relaxed(mb86s7x_shm_base
+                         + WFI_COLOR_OFFSET + clstr * 2 + cpu);
+     val &= ~AT_WFI_COLOR_MASK;
+     val |= clr;
+     writeb_relaxed(val, mb86s7x_shm_base
+                    + WFI_COLOR_OFFSET + clstr * 2 + cpu);
+}
+
+static int mb86s7x_pm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     int ret = 0;
+
+     if (cluster >= S7X_MAX_CLUSTER || cpu >= S7X_MAX_CPU)
+             return -EINVAL;
+
+     pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+
+     local_irq_disable();
+     arch_spin_lock(&mb86s7x_pm_lock);
+
+     mb86s7x_pm_use_count[cluster][cpu]++;
+
+     if (mb86s7x_pm_use_count[cluster][cpu] == 1) {
+             struct mb86s7x_cpu_gate cmd;
+
+             arch_spin_unlock(&mb86s7x_pm_lock);
+             local_irq_enable();

From that point onward, nothing prevents concurrent execution in
mb86s7x_pm_suspend() racing to call mb86s7x_set_wficolor() resulting in
an incoherent state depending on who wins the race.

yeah, the set_wficolor() better be before we release the arch_spin_unlock.

Thanks,
Jassi