From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753576AbdGGD3y (ORCPT ); Thu, 6 Jul 2017 23:29:54 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:9289 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753655AbdGGD3u (ORCPT ); Thu, 6 Jul 2017 23:29:50 -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> CC: , Ma Jun , "Agustin Vega-Frias" , John Garry , Hanjun Guo From: Hanjun Guo Message-ID: <595EFF9F.4080304@huawei.com> Date: Fri, 7 Jul 2017 11:27:27 +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: <3310f2df-839b-5357-a928-5375e183d614@arm.com> 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.0A020202.595F0027.003D,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 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? > > 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 :) Thanks Hanjun