Re: PCI MSI issue with reinserting a driver

[Marc Zyngier <maz@kernel.org>]

On 2021-02-02 15:46, John Garry wrote:
> On 02/02/2021 14:48, Marc Zyngier wrote:
>>>> 
>>>> Not sure. I also now notice an error for the SAS PCI driver on D06 
>>>> when nr_cpus < 16, which means number of MSI vectors allocated < 32, 
>>>> so looks the same problem. There we try to allocate 16 + max(nr 
>>>> cpus, 16) MSI.
>>>> 
>>>> Anyway, let me have a look today to see what is going wrong.
>>>> 
>>> Could this be the problem:
>>> 
>>> nr_cpus=11
>>> 
>>> In alloc path, we have:
>>>     its_alloc_device_irq(nvecs=27 = 16+11)
>>>       bitmap_find_free_region(order = 5);
>>> In free path, we have:
>>>     its_irq_domain_free(nvecs = 1) and free each 27 vecs
>>>       bitmap_release_region(order = 0)
>>> 
>>> So we allocate 32 bits, but only free 27. And 2nd alloc for 32 fails.
> 
> [ ... ]
> 
>>> 
>>> 
>>> But I'm not sure that we have any requirement for those map bits to 
>>> be
>>> consecutive.
>> 
>> We can't really do that. All the events must be contiguous,
>> and there is also a lot of assumptions in the ITS driver that
>> LPI allocations is also contiguous.
>> 
>> But there is also the fact that for Multi-MSI, we *must*
>> allocate 32 vectors. Any driver could assume that if we have
>> allocated 17 vectors, then there is another 15 available.
>> 
>> My question still stand: how was this working with the previous
>> behaviour?
> 
> Because previously in this scenario we would allocate 32 bits and free
> 32 bits in the map; but now we allocate 32 bits, yet only free 27 - so
> leak 5 bits. And this comes from how irq_domain_free_irqs_hierarchy()
> now frees per-interrupt, instead of all irqs per domain.
> 
> Before:
>  In free path, we have:
>      its_irq_domain_free(nvecs = 27)
>        bitmap_release_region(count order = 5 == 32bits)
> 
> Current:
>  In free path, we have:
>      its_irq_domain_free(nvecs = 1) for free each 27 vecs
>        bitmap_release_region(count order = 0 == 1bit)

Right. I was focusing on the patch and blindly ignored the explanation
at the top of the email. Apologies for that.

I'm not overly keen on handling this in the ITS though, and I'd rather
we try and do it in the generic code. How about this (compile tested
only)?

Thanks,

         M.

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6aacd342cd14..cfccad83c2df 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1399,8 +1399,19 @@ static void irq_domain_free_irqs_hierarchy(struct 
irq_domain *domain,
  		return;

  	for (i = 0; i < nr_irqs; i++) {
-		if (irq_domain_get_irq_data(domain, irq_base + i))
-			domain->ops->free(domain, irq_base + i, 1);
+		int n ;
+
+		/* Find the largest possible span of IRQs to free in one go */
+		for (n = 0;
+		     ((i + n) < nr_irqs &&
+		      irq_domain_get_irq_data(domain, irq_base + i + n));
+		     n++);
+
+		if (!n)
+			continue;
+
+		domain->ops->free(domain, irq_base + i, n);
+		i += n;
  	}
  }


-- 
Jazz is not dead. It just smells funny...