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
