Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Marc, Russell,

On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier maz@kernel.org wrote:

The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.

But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.

Drop the terminally broken code and replace it by a byte write.

Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature") Cc: stable@vger.kernel.org Signed-off-by: Marc Zyngier maz@kernel.org

Thanks for your patch, which is now commit 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.

Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.

Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch. Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?

Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?

Thanks for your comments!

--- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force) {

•   void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
  

•   unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
  

•   u32 val, mask, bit;
  

•   unsigned long flags;
  

•   void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
  

•   unsigned int cpu;
  
    if (!force)
            cpu = cpumask_any_and(mask_val, cpu_online_mask);
  

@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) return -EINVAL;

•   gic_lock_irqsave(flags);
  

•   mask = 0xff << shift;
  

•   bit = gic_cpu_map[cpu] << shift;
  

•   val = readl_relaxed(reg) & ~mask;
  

•   writel_relaxed(val | bit, reg);
  

•   gic_unlock_irqrestore(flags);
  

•   writeb_relaxed(gic_cpu_map[cpu], reg);
    irq_data_update_effective_affinity(d, cpumask_of(cpu));
  
    return IRQ_SET_MASK_OK_DONE;
  

Gr{oetje,eeting}s,

Geert