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

[Geert Uytterhoeven]

Hi Marc, Russell,

On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds