diff mbox series

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

Message ID 1499315732-63950-1-git-send-email-guohanjun@huawei.com
State New
Headers show
Series [1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace | expand

Commit Message

Hanjun Guo July 6, 2017, 4:35 a.m. UTC
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(-)

-- 
1.7.12.4

Comments

Marc Zyngier July 6, 2017, 7:43 a.m. UTC | #1
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...
Marc Zyngier July 6, 2017, 1:05 p.m. UTC | #2
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?

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

Hanjun Guo July 7, 2017, 3:27 a.m. UTC | #3
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
Marc Zyngier July 7, 2017, 8:17 a.m. UTC | #4
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...
Hanjun Guo July 7, 2017, 9:38 a.m. UTC | #5
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
diff mbox series

Patch

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;