[V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)

On 11/3/2014 4:51 PM, Thomas Gleixner wrote:

On Mon, 3 Nov 2014, suravee.suthikulpanit at amd.com wrote:

quoted

+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+	int pos;
+	struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+	spin_lock(&v2m->msi_cnt_lock);

Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?

If not, why don't we have the serialization there instead of forcing
every callback to implement its own?

 From the following call paths:
   |--> pci_enable_msi_range
    |--> msi_capability_init
     |--> arch_setup_msi_irqs
      |--> arch_setup_msi_irq
and
   |--> pci_enable_msix
    |--> msix_capability_init
     |--> arch_setup_msi_irqs
      |--> arch_setup_msi_irq

It serialize when a PCI device driver tries to allocate multiple 
interrupts. However, AFAICT, it would not serialize the allocation when 
multiple drivers trying to setup MSI irqs at the same time. I needed 
that to protect the bitmap structure. I also noticed the same in other 
drivers as well.

I can look into this more to see where would be a good point.

quoted

+	pos = irq - v2m->spi_start;

So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.

And if I'm missing something, then that msi_chip stuff is seriously
broken.

It works this way because of the direct mapping (as you noticed). But I 
am planning to change that. See below.

quoted

+	if (pos >= 0 && pos < v2m->nr_spis)

So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?

I'll provide appropriate warnings.

I would not be surprised if that is related to my question above.

Not quite sure which of the above questions.

quoted

+	spin_lock(&v2m->msi_cnt_lock);
+	offset = bitmap_find_free_region(v2m->bm, v2m->nr_spis, 0);
+	spin_unlock(&v2m->msi_cnt_lock);
+	if (offset < 0)
+		return offset;
+
+	hwirq = v2m->spi_start + offset;
+	virq = __irq_domain_alloc_irqs(v2m->domain, hwirq,
+				       1, NUMA_NO_NODE, v2m, true);
+	if (virq < 0) {
+		gicv2m_teardown_msi_irq(chip, hwirq);
+		return virq;
+	}
+
+	irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq,
+				&v2m_chip, v2m);
+
+	irq_set_msi_desc(hwirq, desc);
+	irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);

Sure both calls work perfectly fine as long as virq == hwirq, right?

I was running into an issue when calling the 
irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be 
passed in. However, this does not work for GICv2m since it does not have 
interrupt information in the device tree. So, I decided at first to use 
direct (virq == hwirq) mapping, which simplifies the code a bit, but 
might not be ideal solution, as you pointed out.

An alternative would be to create a temporary struct of_phandle_args, 
and populate it with the interrupt information for the requested MSI. 
Then pass it to:
   --> irq_domain_alloc_irq_parent
    |--> gic_irq_domain_alloc
      |--> gic_irq_domain_xlate
      |--> gic_irq_domain_map

However, this would still not be ideal if we want to support ACPI. 
Another alternative would be coming up with a dedicate structure to be 
used here. I noticed on X86, it uses struct irq_alloc_info. May be 
that's what we also need here.

[...]
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.

I don't intend to waste yours or anybody's precious time. Sorry if it 
takes a couple iterations to work out the issues. Also, I will try to 
put more comment in my code to make it more clear. Let me know what 
works best for you to work out the issues.

Thanks,

Suravee

Thanks,

	tglx