Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq

[Saidi, Ali]

ïOn 6/8/20, 8:49 AM, "Thomas Gleixner" <tglx@xxxxxxxxxxxxx> wrote:
    
    
    "Herrenschmidt, Benjamin" <benh@xxxxxxxxxx> writes:
    > On Wed, 2020-06-03 at 16:16 +0100, Marc Zyngier wrote:
    >> > My original patch should certain check activated and not disabled.
    >> > With that do you still have reservations Marc?
    >>
    >> I'd still prefer it if we could do something in core code, rather
    >> than spreading these checks in the individual drivers. If we can't,
    >> fair enough. But it feels like the core set_affinity function could
    >> just do the same thing in a single place (although the started vs
    >> activated is yet another piece of the puzzle I didn't consider,
    >> and the ITS doesn't need the "can_reserve" thing).
    >
    > For the sake of fixing the problem in a timely and backportable way I
    > would suggest first merging the fix, *then* fixing the core core.
    
    The "fix" is just wrong
    
    >       if (cpu != its_dev->event_map.col_map[id]) {
    >               target_col = &its_dev->its->collections[cpu];
    > -             its_send_movi(its_dev, target_col, id);
    > +
    > +             /* If the IRQ is disabled a discard was sent so don't move */
    > +             if (!irqd_irq_disabled(d))
    
    That check needs to be !irqd_is_activated() because enable_irq() does
    not touch anything affinity related.
    
    > +                     its_send_movi(its_dev, target_col, id);
    > +
    >               its_dev->event_map.col_map[id] = cpu;
    >               irq_data_update_effective_affinity(d, cpumask_of(cpu));
    
    And then these associtations are disconnected from reality in any case.
    
    Something like the completely untested patch below should work.

I've been unable to reproduce the problem with your patch on an Arm system.

Thanks,

Ali