Re: [PATCH 1/2] genirq: Get the fwnode back for irqchips being probed via ACPI namespace
From: Hanjun Guo
Date: Fri Jul 07 2017 - 05:38:49 EST
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@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?
> 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