On 31/08/16 10:56, Bharat Kumar Gogada wrote: > > On 30/08/16 15:13, Bharat Kumar Gogada wrote: >>>> Hi Bharat, >>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct >>>>> nwl_pcie >>>> *pcie) >>>>> } >>>>> >>>>> pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node, >>>>> - INTX_NUM, >>>>> + INTX_NUM + 1, >>>>> &legacy_domain_ops, >>>>> pcie); >>>> >>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so the >>>> domain allocation should reflect this. On the other hand, the way the >>>> driver currently deals with mappings is quite broken (consistently adding 1 to >> the HW interrupt). >>>> >>> Hi Marc, >>> >>> Without above change I get following crash in kernel while booting. >>> >>> [ 2.441684] error: hwirq 0x4 is too large for dummy >>> >>> [ 2.441694] ------------[ cut here ]------------ >>> >>> [ 2.441698] WARNING: at kernel/irq/irqdomain.c:344 >>> >>> [ 2.441702] Modules linked in: >>> >>> [ 2.441706] >>> >>> [ 2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8 >>> >>> [ 2.441718] Hardware name: xlnx,zynqmp (DT) >>> >>> [ 2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti: >> ffffffc071888000 >>> >>> [ 2.441732] PC is at irq_domain_associate+0x138/0x1c0 >>> >>> [ 2.441738] LR is at irq_domain_associate+0x138/0x1c0 >>> >>> In kernel/irq/irqdomain.c function irq_domain_associate >>> >>> if (WARN(hwirq >= domain->hwirq_max, >>> "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain->name)) >>> return -EINVAL; >>> >>> Here the hwirq and hwirq_max are equal to 4 without the above condition >> (INTX_NUM + 1) due to which crash is coming. >>> This is happening as the legacy interrupts are starting from 1 (INTA). >> >> I understood that. I'm still persisting in saying that you have the wrong fix. >> >> Your domain should always allocate many interrupts as you have interrupt >> sources. These interrupts (hwirq) should be numbered from 0 to (n-1). > > Agreed, but here comes the problem the hwirq for legacy interrupts > will start at 0x1 to 0x4 (INTA to INTD) and these values are as per > PCIe specification for legacy interrupts. So these cannot be numbered > from 0. So when 0x4 (INTD) for a multi-function device comes the > crash occurs. So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to 4? >> >>> And I'm consistently adding 1 to the HW interrupt as in >>> nwl_pcie_leg_handler I get 0th bit set from MSGF_LEG_STATUS if INTA >>> interrupt is raised but my hwirq number being mapped for INTA is 0x1 >>> so that's I'm adding 1 to obtain correct virtual irq. Same case in >>> nwl_pcie_free_irq_domain since hwirq starts from one I'm adding 1 to >>> obtain virtual irq and free it. >> >> I can see that. Nonetheless, this is wrong. Can you please test the patch I >> provided in my reply and report what happens? > > Can you be more specific on what is the wrong, I'm adding one since > the hwirq starts from 0x1 as mentioned above. hwirq should always be the value that is reported by the HW. In your case, this ranges from 0 to 3, never 4. So if we can understand why you get called with 4 as a hwirq, we can fix this properly, for everyone. It is also worth noting that other drivers do not have to do this +1 dance. > I did try your suggestion with Ethernet card, but kernel hangs (it > does not show any crash also, just hangs) when I do interface up > (without bit + 1, using only bit position in handler). This is not > working because in the legacy domain virq mapping starts with hwirq > 0x1, there is no mapping for 0x0 in the domain, so EP interrupt is > not serviced since virq being returned is zero. Right. So let's go back to first principles and find out *who* decides about the hwirq starting at 1 instead of zero. Thanks, M. -- Jazz is not dead. It just smells funny... -- 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
