Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq
From: Marc Zyngier
Date: Sun May 31 2020 - 07:09:32 EST
On 2020-05-30 17:49, Marc Zyngier wrote:
Hi Ali,
On Fri, 29 May 2020 12:36:42 +0000
"Saidi, Ali" <alisaidi@xxxxxxxxxx> wrote:
Hi Marc,
> On May 29, 2020, at 3:33 AM, Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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...