* [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace @ 2017-07-06 4:35 Hanjun Guo 2017-07-06 4:35 ` [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly Hanjun Guo 2017-07-06 7:43 ` [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Marc Zyngier 0 siblings, 2 replies; 9+ messages in thread From: Hanjun Guo @ 2017-07-06 4:35 UTC (permalink / raw) To: Thomas Gleixner, Marc Zyngier Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo From: Hanjun Guo <hanjun.guo@linaro.org> 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 <john.garry@huawei.com> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- 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; -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly 2017-07-06 4:35 [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Hanjun Guo @ 2017-07-06 4:35 ` Hanjun Guo 2017-07-06 5:46 ` Ethan Zhao 2017-07-06 7:43 ` [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Marc Zyngier 1 sibling, 1 reply; 9+ messages in thread From: Hanjun Guo @ 2017-07-06 4:35 UTC (permalink / raw) To: Thomas Gleixner, Marc Zyngier Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo From: Hanjun Guo <hanjun.guo@linaro.org> is_fwnode_irqchip() already check fwnode is NULL or not, just use is_fwnode_irqchip() directly. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- kernel/irq/irqdomain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 1bc38fa..14a21a3 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -137,7 +137,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, if (WARN_ON(!domain)) return NULL; - if (fwnode && is_fwnode_irqchip(fwnode)) { + if (is_fwnode_irqchip(fwnode)) { fwid = container_of(fwnode, struct irqchip_fwid, fwnode); switch (fwid->type) { -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly 2017-07-06 4:35 ` [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly Hanjun Guo @ 2017-07-06 5:46 ` Ethan Zhao 2017-07-06 6:17 ` Hanjun Guo 0 siblings, 1 reply; 9+ messages in thread From: Ethan Zhao @ 2017-07-06 5:46 UTC (permalink / raw) To: Hanjun Guo Cc: Thomas Gleixner, Marc Zyngier, LKML, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo Hanjun， What branch is this patch for ? Check v4.12, failed to apply. Thanks, Ethan On Thu, Jul 6, 2017 at 12:35 PM, Hanjun Guo <guohanjun@huawei.com> wrote: > From: Hanjun Guo <hanjun.guo@linaro.org> > > is_fwnode_irqchip() already check fwnode is NULL or not, > just use is_fwnode_irqchip() directly. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > kernel/irq/irqdomain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 1bc38fa..14a21a3 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -137,7 +137,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size, > if (WARN_ON(!domain)) > return NULL; > > - if (fwnode && is_fwnode_irqchip(fwnode)) { > + if (is_fwnode_irqchip(fwnode)) { > fwid = container_of(fwnode, struct irqchip_fwid, fwnode); > > switch (fwid->type) { > -- > 1.7.12.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly 2017-07-06 5:46 ` Ethan Zhao @ 2017-07-06 6:17 ` Hanjun Guo 0 siblings, 0 replies; 9+ messages in thread From: Hanjun Guo @ 2017-07-06 6:17 UTC (permalink / raw) To: Ethan Zhao Cc: Thomas Gleixner, Marc Zyngier, LKML, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo Hi Ethan, On 2017/7/6 13:46, Ethan Zhao wrote: > Hanjun， > > What branch is this patch for ? Check v4.12, failed to apply. I prepared patches based on master branch of linux-next git tree, but they can be applied cleanly on top of latest Linus tree. This patch set is for latest merged 4.13 material, not for 4.12 :) Thanks Hanjun ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace 2017-07-06 4:35 [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Hanjun Guo 2017-07-06 4:35 ` [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly Hanjun Guo @ 2017-07-06 7:43 ` Marc Zyngier [not found] ` <595DFC87.9060800@huawei.com> 1 sibling, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2017-07-06 7:43 UTC (permalink / raw) To: Hanjun Guo, Thomas Gleixner Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo On 06/07/17 05:35, Hanjun Guo wrote: > From: Hanjun Guo <hanjun.guo@linaro.org> > > 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 <john.garry@huawei.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > 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. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <595DFC87.9060800@huawei.com>]
* Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace [not found] ` <595DFC87.9060800@huawei.com> @ 2017-07-06 13:05 ` Marc Zyngier 2017-07-07 3:27 ` Hanjun Guo 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2017-07-06 13:05 UTC (permalink / raw) To: Hanjun Guo, Thomas Gleixner Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo 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 <hanjun.guo@linaro.org> >>> >>> 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 <john.garry@huawei.com> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>> --- >>> 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... ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace 2017-07-06 13:05 ` Marc Zyngier @ 2017-07-07 3:27 ` Hanjun Guo 2017-07-07 8:17 ` Marc Zyngier 0 siblings, 1 reply; 9+ messages in thread From: Hanjun Guo @ 2017-07-07 3:27 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo 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 <hanjun.guo@linaro.org> >>>> >>>> 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 <john.garry@huawei.com> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> 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 <linux/acpi.h> #include <linux/debugfs.h> +#include <linux/device.h> #include <linux/hardirq.h> #include <linux/interrupt.h> #include <linux/irq.h> @@ -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace 2017-07-07 3:27 ` Hanjun Guo @ 2017-07-07 8:17 ` Marc Zyngier 2017-07-07 9:38 ` Hanjun Guo 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2017-07-07 8:17 UTC (permalink / raw) To: Hanjun Guo, Thomas Gleixner Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo 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 <hanjun.guo@linaro.org> >>>>> >>>>> 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 <john.garry@huawei.com> >>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>> --- >>>>> 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 <linux/acpi.h> > #include <linux/debugfs.h> > +#include <linux/device.h> > #include <linux/hardirq.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > @@ -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... ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace 2017-07-07 8:17 ` Marc Zyngier @ 2017-07-07 9:38 ` Hanjun Guo 0 siblings, 0 replies; 9+ messages in thread From: Hanjun Guo @ 2017-07-07 9:38 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner Cc: linux-kernel, Ma Jun, Agustin Vega-Frias, John Garry, Hanjun Guo 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 <hanjun.guo@linaro.org> >>>>>> >>>>>> 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 <john.garry@huawei.com> >>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>>> --- >>>>>> 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 <linux/acpi.h> >> #include <linux/debugfs.h> >> +#include <linux/device.h> >> #include <linux/hardirq.h> >> #include <linux/interrupt.h> >> #include <linux/irq.h> >> @@ -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-07 9:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-06 4:35 [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Hanjun Guo
2017-07-06 4:35 ` [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly Hanjun Guo
2017-07-06 5:46 ` Ethan Zhao
2017-07-06 6:17 ` Hanjun Guo
2017-07-06 7:43 ` [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace Marc Zyngier
[not found] ` <595DFC87.9060800@huawei.com>
2017-07-06 13:05 ` Marc Zyngier
2017-07-07 3:27 ` Hanjun Guo
2017-07-07 8:17 ` Marc Zyngier
2017-07-07 9:38 ` Hanjun Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox