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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>>> ---
>>>  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...