Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750905AbdGGJip (ORCPT ); Fri, 7 Jul 2017 05:38:45 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:9291 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbdGGJio (ORCPT ); Fri, 7 Jul 2017 05:38:44 -0400 Subject: Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace To: Marc Zyngier , 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: , Ma Jun , "Agustin Vega-Frias" , John Garry , Hanjun Guo From: Hanjun Guo Message-ID: <595F5683.7020905@huawei.com> Date: Fri, 7 Jul 2017 17:38:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.17.188] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020201.595F5699.00AA,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 8f981083103cecb1456134ab036e73d8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6460 Lines: 146 On 2017/7/7 16:17, Marc Zyngier wrote: > 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: Fine to me as we only need unique names for debugfs :) > > /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. Thank you, I will test it on D03 as well. Thanks Hanjun