From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X) Date: Mon, 3 Nov 2014 21:22:41 -0600 Message-ID: <54584681.2010103@amd.com> References: <1415052977-26036-1-git-send-email-suravee.suthikulpanit@amd.com> <1415052977-26036-3-git-send-email-suravee.suthikulpanit@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Thomas Gleixner Cc: Mark Rutland , devicetree@vger.kernel.org, jason@lakedaemon.net, linux-doc@vger.kernel.org, Marc Zyngier , Will.Deacon@arm.com, liviu.dudau@arm.com, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, Harish.Kasiviswanathan@amd.com, Catalin.Marinas@arm.com, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org On 11/3/2014 4:51 PM, Thomas Gleixner wrote: > On Mon, 3 Nov 2014, suravee.suthikulpanit@amd.com 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 >