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

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