Messages in this thread | | | Date | Wed, 03 Jun 2020 13:44:58 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq |
| |
On 2020-06-02 21:54, Thomas Gleixner wrote: > "Herrenschmidt, Benjamin" <benh@amazon.com> writes: >> On Sun, 2020-05-31 at 12:09 +0100, Marc Zyngier wrote: >>> > The semantic of activate/deactivate (which maps to started/shutdown >>> > in the IRQ code) is that the HW resources for a given interrupt are >>> > only committed when the interrupt is activated. Trying to perform >>> > actions involving the HW on an interrupt that isn't active cannot be >>> > guaranteed to take effect. >>> > >>> > I'd rather address it in the core code, by preventing set_affinity (and >>> > potentially others) to take place when the interrupt is not in the >>> > STARTED state. Userspace would get an error, which is perfectly >>> > legitimate, and which it already has to deal with it for plenty of >>> > other >>> > reasons. >> >> So I finally found time to dig a bit in there :) Code has changed a >> bit >> since last I looked. But I have memories of the startup code messing >> around with the affinity, and here it is. In irq_startup() : >> >> >> switch (__irq_startup_managed(desc, aff, force)) { >> case IRQ_STARTUP_NORMAL: >> ret = __irq_startup(desc); >> irq_setup_affinity(desc); >> break; >> case IRQ_STARTUP_MANAGED: >> irq_do_set_affinity(d, aff, false); >> ret = __irq_startup(desc);
Grump. Nice catch. In hindsight, this is obvious, as managed interrupts may have been allocated to target CPUs that have been hot-plugged off.
>> break; >> case IRQ_STARTUP_ABORT: >> irqd_set_managed_shutdown(d); >> return 0; >> >> So we have two cases here. Normal and managed. >> >> In the managed case, we set the affinity before startup. I feel like >> your >> patch might break that or am I missing something ? > > It will break stuff because the affinity is not stored in case that the > interrupt is not started. > > I think we can fix this in the core code but that needs more thought. > __irq_can_set_affinity() is definitely the wrong place.
Indeed. I completely missed the above. Back to square one.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |