[PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[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

[PATCH 2/2] genirq: Use is_fwnode_irqchip() directly

[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

Re: [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly

[Ethan Zhao]

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
>

Re: [PATCH 2/2] genirq: Use is_fwnode_irqchip() directly

[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

Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[Marc Zyngier]

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...

Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[Marc Zyngier]

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...

Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[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

Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[Marc Zyngier]

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...

Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace

[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