Messages in this thread Patch in this message | | | Date | Sun, 31 May 2020 12:09:25 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq |
| |
On 2020-05-30 17:49, Marc Zyngier wrote: > Hi Ali, > > On Fri, 29 May 2020 12:36:42 +0000 > "Saidi, Ali" <alisaidi@amazon.com> wrote: > >> Hi Marc, >> >> > On May 29, 2020, at 3:33 AM, Marc Zyngier <maz@kernel.org> wrote: >> > >> > Hi Ali, >> > >> >> On 2020-05-29 02:55, Ali Saidi wrote: >> >> If an interrupt is disabled the ITS driver has sent a discard removing >> >> the DeviceID and EventID from the ITT. After this occurs it can't be >> >> moved to another collection with a MOVI and a command error occurs if >> >> attempted. Before issuing the MOVI command make sure that the IRQ isn't >> >> disabled and change the activate code to try and use the previous >> >> affinity. >> >> >> >> Signed-off-by: Ali Saidi <alisaidi@amazon.com> >> >> --- >> >> drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++++++++--- >> >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c >> >> b/drivers/irqchip/irq-gic-v3-its.c >> >> index 124251b0ccba..1235dd9a2fb2 100644 >> >> --- a/drivers/irqchip/irq-gic-v3-its.c >> >> +++ b/drivers/irqchip/irq-gic-v3-its.c >> >> @@ -1540,7 +1540,11 @@ static int its_set_affinity(struct irq_data *d, >> >> const struct cpumask *mask_val, >> >> /* don't set the affinity when the target cpu is same as current one >> >> */ >> >> 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)) >> >> + its_send_movi(its_dev, target_col, id); >> >> + >> > >> > This looks wrong. What you are testing here is whether the interrupt >> > is masked, not that there isn't a valid translation. >> I’m not exactly sure the correct condition, but what I’m looking for >> is interrupts which are deactivated and we have thus sent a discard. > > That looks like IRQD_IRQ_STARTED not being set in this case. > >> > >> > In the commit message, you're saying that we've issued a discard. >> > This hints at doing a set_affinity on an interrupt that has been >> > deactivated (mapping removed). Is that actually the case? If so, >> > why was it deactivated >> > the first place? >> This is the case. If we down a NIC, that interface’s MSIs will be >> deactivated but remain allocated until the device is unbound from the >> driver or the NIC is brought up. >> >> While stressing down/up a device I’ve found that irqbalance can move >> interrupts and you end up with the situation described. The device is >> downed, the interrupts are deactivated but still present and then >> trying to move one results in sending a MOVI after the DISCARD which >> is an error per the GIC spec. > > Not great indeed. But this is not, as far as I can tell, a GIC > driver problem. > > 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.
How about this:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 453a8a0f4804..1a2ac1392c0f 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -147,7 +147,8 @@ cpumask_var_t irq_default_affinity; static bool __irq_can_set_affinity(struct irq_desc *desc) { if (!desc || !irqd_can_balance(&desc->irq_data) || - !desc->irq_data.chip || !desc->irq_data.chip->irq_set_affinity) + !desc->irq_data.chip || !desc->irq_data.chip->irq_set_affinity || + !irqd_is_started(&desc->irq_data)) return false; return true; } Thanks,
M. -- Jazz is not dead. It just smells funny...
| |