Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752486AbdGGIRt (ORCPT ); Fri, 7 Jul 2017 04:17:49 -0400 Received: from foss.arm.com ([217.140.101.70]:44998 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751833AbdGGIRr (ORCPT ); Fri, 7 Jul 2017 04:17:47 -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> <3310f2df-839b-5357-a928-5375e183d614@arm.com> <595EFF9F.4080304@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: Date: Fri, 7 Jul 2017 09:17:38 +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: <595EFF9F.4080304@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: 6228 Lines: 148 On 07/07/17 04:27, Hanjun Guo wrote: > On 2017/7/6 21:05, Marc Zyngier wrote: >> 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. > > Later one is better to me as allocate a FWNODE_IRQCHIP in the irqchip > driver will override the FWNODE_ACPI handler. > >> >> Does the following hack work for you? > > Yes, it works. shall we go this way with a proper fix? I already have something written, together with name generation and stuff. > >> >> 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... > > How about the weird way below (using dev_name() which shows ACPI HID + number) ? > > +#include > #include > +#include > #include > #include > #include > @@ -172,6 +174,15 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, > > domain->name = name; > domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED; > + } else if (is_acpi_device_node(fwnode)) { > + struct acpi_device *adev = to_acpi_device_node(fwnode); > + > + domain->name = kstrdup(dev_name(&adev->dev), GFP_KERNEL); > + if (!domain->name) > + return NULL; > + > + domain->fwnode = fwnode; > + domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED; > } > > But my hack code retrieving the ACPI data in irq domain core is really > weird :) Dunno. I'm using acpi_get_name() on the acpi handle, which seems to do the right thing on a D05: /sys/kernel/debug/irq/domains/ default irqchip@00000000c6000000-4 irqchip@000004006c000000-4 \_SB_.MBI1 irqchip@000000004c000000-2 irqchip@00000008c6000000-2 irqchip@00000400c6000000-2 \_SB_.MBI2 irqchip@000000004c000000-3 irqchip@00000008c6000000-3 irqchip@00000400c6000000-3 \_SB_.MBI3 irqchip@000000004c000000-4 irqchip@00000008c6000000-4 irqchip@00000400c6000000-4 \_SB_.MBI4 irqchip@000000006c000000-2 irqchip@000004004c000000-2 irqchip@00000408c6000000-2 \_SB_.MBI5 irqchip@000000006c000000-3 irqchip@000004004c000000-3 irqchip@00000408c6000000-3 \_SB_.MBI6 irqchip@000000006c000000-4 irqchip@000004004c000000-4 irqchip@00000408c6000000-4 \_SB_.MBI7 irqchip@00000000c6000000-2 irqchip@000004006c000000-2 irqchip@ffff000008000000 \_SB_.MBI8 irqchip@00000000c6000000-3 irqchip@000004006c000000-3 \_SB_.MBI0 \_SB_.MBI9 I'll post the patch later today. M. -- Jazz is not dead. It just smells funny...