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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/3/2014 4:51 PM, Thomas Gleixner wrote:
On Mon, 3 Nov 2014, suravee.suthikulpanit@xxxxxxx wrote:
+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.

+	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.


+	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.

+	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



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux