From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Date: Fri, 18 Dec 2015 10:10:15 +0000 Message-ID: <5673DB87.6040106@nvidia.com> References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-2-git-send-email-jonathanh@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Linus Walleij Cc: Thomas Gleixner , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Soren Brinkmann , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-tegra@vger.kernel.org On 17/12/15 13:16, Linus Walleij wrote: > On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter wrote: > >> When mapping an IRQ, if a mapping already exists, then we simply return >> the virual IRQ number. However, we do not check that the type settings for > > ^virtual > > Just that it isn't virtual, it's a Linux IRQ number, we actually use > hwirq for the non-virtual IRQ number/offse in this function. > > But I know I may be fighting weathermills here. Ok, will re-word this. >> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> { >> + struct device_node *of_node; >> struct irq_domain *domain; >> irq_hw_number_t hwirq; >> + unsigned int cur_type = IRQ_TYPE_NONE; >> unsigned int type = IRQ_TYPE_NONE; >> int virq; >> >> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> if (irq_domain_translate(domain, fwspec, &hwirq, &type)) >> return 0; >> >> - if (irq_domain_is_hierarchy(domain)) { >> - /* >> - * If we've already configured this interrupt, >> - * don't do it again, or hell will break loose. >> - */ >> - virq = irq_find_mapping(domain, hwirq); >> - if (virq) >> - return virq; >> + of_node = irq_domain_get_of_node(domain); > > Marc's patches went to great lengths to do this fwspec-neutral, > i.e. it doesn't matter if it's done by DT or ACPI (or whatever). > > This just drives a truck through all of that by making > the whole function OF-specific again. Yes, was not sure if this would be popular. I was on the fence, but I saw the following ... if (!domain) { pr_warn("no irq domain found for %s !\n", of_node_full_name(to_of_node(fwspec->fwnode))); return 0; } ... and thought we are not completely agnostic. However, if you prefer I park my mini else where, I can definitely drop this, no big deal ;-) >> >> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec); >> - if (virq <= 0) >> - return 0; >> + /* >> + * If we've already configured this interrupt, >> + * don't do it again, or hell will break loose. >> + */ >> + virq = irq_find_mapping(domain, hwirq); >> + if (!virq) { >> + if (irq_domain_is_hierarchy(domain)) { >> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, >> + fwspec); >> + if (virq <= 0) >> + return 0; >> + } else { >> + virq = irq_domain_alloc_descs(-1, 1, hwirq, >> + of_node_to_nid(of_node)); > > What is this all of a sudden? Not even mentioned in the > commit. Plus I bet ACPI need something else than OF nid > passed here. Do you mean the else part of all of the above? So in the current code, the else part calls irq_create_mapping() and this function internally, calls irq_find_mapping(). Given that I am now calling irq_find_mapping() before the if, I don't really need to call irq_create_mapping() again, I just need to call the functions in irq_create_mapping() that allocate and setup the IRQ number. Sorry, I did not really explain this. However, if it is simpler, I can call irq_create_mapping() instead and may be this makes the change easier to read. Cheers Jon From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751926AbbLRKK2 (ORCPT ); Fri, 18 Dec 2015 05:10:28 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:18802 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750811AbbLRKKW (ORCPT ); Fri, 18 Dec 2015 05:10:22 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Fri, 18 Dec 2015 01:55:40 -0800 Subject: Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping To: Linus Walleij References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-2-git-send-email-jonathanh@nvidia.com> CC: Thomas Gleixner , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Soren Brinkmann , "linux-kernel@vger.kernel.org" , "linux-tegra@vger.kernel.org" From: Jon Hunter Message-ID: <5673DB87.6040106@nvidia.com> Date: Fri, 18 Dec 2015 10:10:15 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.21.132.159] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 17/12/15 13:16, Linus Walleij wrote: > On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter wrote: > >> When mapping an IRQ, if a mapping already exists, then we simply return >> the virual IRQ number. However, we do not check that the type settings for > > ^virtual > > Just that it isn't virtual, it's a Linux IRQ number, we actually use > hwirq for the non-virtual IRQ number/offse in this function. > > But I know I may be fighting weathermills here. Ok, will re-word this. >> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> { >> + struct device_node *of_node; >> struct irq_domain *domain; >> irq_hw_number_t hwirq; >> + unsigned int cur_type = IRQ_TYPE_NONE; >> unsigned int type = IRQ_TYPE_NONE; >> int virq; >> >> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> if (irq_domain_translate(domain, fwspec, &hwirq, &type)) >> return 0; >> >> - if (irq_domain_is_hierarchy(domain)) { >> - /* >> - * If we've already configured this interrupt, >> - * don't do it again, or hell will break loose. >> - */ >> - virq = irq_find_mapping(domain, hwirq); >> - if (virq) >> - return virq; >> + of_node = irq_domain_get_of_node(domain); > > Marc's patches went to great lengths to do this fwspec-neutral, > i.e. it doesn't matter if it's done by DT or ACPI (or whatever). > > This just drives a truck through all of that by making > the whole function OF-specific again. Yes, was not sure if this would be popular. I was on the fence, but I saw the following ... if (!domain) { pr_warn("no irq domain found for %s !\n", of_node_full_name(to_of_node(fwspec->fwnode))); return 0; } ... and thought we are not completely agnostic. However, if you prefer I park my mini else where, I can definitely drop this, no big deal ;-) >> >> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec); >> - if (virq <= 0) >> - return 0; >> + /* >> + * If we've already configured this interrupt, >> + * don't do it again, or hell will break loose. >> + */ >> + virq = irq_find_mapping(domain, hwirq); >> + if (!virq) { >> + if (irq_domain_is_hierarchy(domain)) { >> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, >> + fwspec); >> + if (virq <= 0) >> + return 0; >> + } else { >> + virq = irq_domain_alloc_descs(-1, 1, hwirq, >> + of_node_to_nid(of_node)); > > What is this all of a sudden? Not even mentioned in the > commit. Plus I bet ACPI need something else than OF nid > passed here. Do you mean the else part of all of the above? So in the current code, the else part calls irq_create_mapping() and this function internally, calls irq_find_mapping(). Given that I am now calling irq_find_mapping() before the if, I don't really need to call irq_create_mapping() again, I just need to call the functions in irq_create_mapping() that allocate and setup the IRQ number. Sorry, I did not really explain this. However, if it is simpler, I can call irq_create_mapping() instead and may be this makes the change easier to read. Cheers Jon