Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752634AbdGFNFx (ORCPT ); Thu, 6 Jul 2017 09:05:53 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:38044 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752068AbdGFNFw (ORCPT ); Thu, 6 Jul 2017 09:05:52 -0400 Subject: Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace To: Hanjun Guo , Thomas Gleixner References: <1499315732-63950-1-git-send-email-guohanjun@huawei.com> <595DFC87.9060800@huawei.com> Cc: linux-kernel@vger.kernel.org, Ma Jun , Agustin Vega-Frias , John Garry , Hanjun Guo From: Marc Zyngier Organization: ARM Ltd Message-ID: <3310f2df-839b-5357-a928-5375e183d614@arm.com> Date: Thu, 6 Jul 2017 14:05:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <595DFC87.9060800@huawei.com> 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 Content-Length: 3752 Lines: 96 On 06/07/17 10:01, Hanjun Guo wrote: > Hi Marc, > > On 2017/7/6 15:43, Marc Zyngier wrote: >> On 06/07/17 05:35, Hanjun Guo wrote: >>> From: Hanjun Guo >>> >>> commit d59f6617eef0 (genirq: Allow fwnode to carry name information only) >>> forgot to do "domain->fwnode = fwnode;" for irqchips being probed via >>> ACPI namesapce (DSDT/SSDT), that will break platforms with irqchip such >>> as mbigen or qcom irq combiner, set the fwnode back to fix the issue. >>> >>> Reported-by: John Garry >>> Signed-off-by: Hanjun Guo >>> --- >>> kernel/irq/irqdomain.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 14fe862..1bc38fa 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -151,7 +151,6 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, >>> domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED; >>> break; >>> default: >>> - domain->fwnode = fwnode; >>> domain->name = fwid->name; >>> break; >>> } >>> @@ -172,7 +171,6 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, >>> strreplace(name, '/', ':'); >>> >>> domain->name = name; >>> - domain->fwnode = fwnode; >>> domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED; >>> } >>> >>> @@ -196,6 +194,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, >>> INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL); >>> domain->ops = ops; >>> domain->host_data = host_data; >>> + domain->fwnode = fwnode; >>> domain->hwirq_max = hwirq_max; >>> domain->revmap_size = size; >>> domain->revmap_direct_max_irq = direct_max; >>> >> This doesn't seem right. >> >> Why is is_fwnode_irqchip() returning false when presented with an >> irqchip probed via the ACPI namespace? That's what you should consider >> fixing instead of moving that code around. > > irqchips probed via the ACPI namespace will have a fwnode handler > with the fwnode type FWNODE_ACPI, similar to irqchips probed > via DT having a fwnode handler with type FWNODE_OF, in this > function with DT case, fwnode is set for irqdomain's fwnode, > ACPI just reuse that code because they are similar. > > is_fwnode_irqchip() just checks if it's FWNODE_IRQCHIP, which is only > available for irqchips probing via ACPI static tables such as ITS, GIC > on ARM, or via DMAR on x86, those cases don't have a fwnode handler then > need to create one via __irq_domain_alloc_fwnode(), which is different > from DT/ACPI namesapce scan mechanism. So the patch above it's the best > solution I got so far which just resume the code as before, correct me > if you have something new :) This violates the irqdomain API that takes two kind of fwnodes: FWNODE_OF or FWNODE_IRQCHIP, and no others. You got away with it so far, but it is over. You have two choices here: either you allocate a FWNODE_IRQCHIP in your irqchip driver, and use this as a handle for your IRQ domain, or you make the irqdomain code able to deal with any FWNODE_ACPI fwnode. Does the following hack work for you? diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 14fe862aa2e3..37f4aa3985b3 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -155,6 +155,8 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, domain->name = fwid->name; break; } + } else if (is_acpi_device_node(fwnode)) { + domain->fwnode = fwnode; } else if (of_node) { char *name; If it does, we can then find weird and wonderful ways to give the domain a shiny name in debugfs... M. -- Jazz is not dead. It just smells funny...