lkml.org 
[lkml]   [2022]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains
From
Date


On 2022/6/18 下午6:36, Marc Zyngier wrote:
> On Wed, 15 Jun 2022 10:28:47 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>>
>>
>> On 2022/6/15 下午3:14, Marc Zyngier wrote:
>>> On Wed, 15 Jun 2022 07:07:21 +0100,
>>> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>>>
>>>> From: Marc Zyngier <maz@kernel.org>
>>>>
>>>> In an unfortunate departure from the ACPI spec, the LoongArch
>>>> architecture split its GSI space across multiple interrupt
>>>> controllers.
>>>>
>>>> In order to be able to reuse sthe core code and prevent
>>>> architectures from reinventing an already square wheel, offer
>>>> the arch code the ability to register a dispatcher function
>>>> that will return the domain fwnode for a given GSI.
>>>>
>>>> The ARM GIC drivers are updated to support this (with a single
>>>> domain, as intended).
>>>>
>>>> Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
>>>
>>> I don't think this tag is appropriate here.
>>>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> Cc: Hanjun Guo <guohanjun@huawei.com>
>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>>>> ---
>>>> drivers/acpi/irq.c | 40 +++++++++++++++++++++++-----------------
>>>> drivers/irqchip/irq-gic-v3.c | 18 ++++++++++++------
>>>> drivers/irqchip/irq-gic.c | 18 ++++++++++++------
>>>> include/linux/acpi.h | 2 +-
>>>> 4 files changed, 48 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
>>>> index c68e694..b7460ab 100644
>>>> --- a/drivers/acpi/irq.c
>>>> +++ b/drivers/acpi/irq.c
>>>> @@ -12,7 +12,7 @@
>>>> enum acpi_irq_model_id acpi_irq_model;
>>>> -static struct fwnode_handle *acpi_gsi_domain_id;
>>>> +static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
>>>> /**
>>>> * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
>>>> @@ -26,10 +26,7 @@
>>>> */
>>>> int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>>>> {
>>>> - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
>>>> - DOMAIN_BUS_ANY);
>>>> -
>>>> - *irq = irq_find_mapping(d, gsi);
>>>> + *irq = acpi_register_gsi(NULL, gsi, -1, -1);
>>>
>>> What is this?
>>>
>>> - This wasn't part of my initial patch, and randomly changing patches
>>> without mentioning it isn't acceptable
>>>
>>> - you *cannot* trigger a registration here, as this isn't what the API
>>> advertises
>>>
>>> - what makes you think that passing random values (NULL, -1... )to
>>> acpi_register_gsi() is an acceptable thing to do?
>>>
>>> The original patch had:
>>>
>>> @@ -26,8 +26,10 @@ static struct fwnode_handle *acpi_gsi_domain_id;
>>> */
>>> int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>>> {
>>> - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
>>> - DOMAIN_BUS_ANY);
>>> + struct irq_domain *d;
>>> +
>>> + d = irq_find_matching_fwnode(acpi_get_gsi_domain_id(gsi),
>>> + DOMAIN_BUS_ANY);
>>> *irq = irq_find_mapping(d, gsi);
>>> /*
>>>
>>> and I don't think it needs anything else. If something breaks, let's
>>> discuss it, but don't abuse the API nor the fact that I usually don't
>>> review my own patches to sneak things in...
>>>
>>
>> Sorry, Marc, I don't know how to communicate with you for my change
>> here before submitting the patch, maybe I should mention it in the
>> patch commit or code.
>
> It should at least be discussed first, like you are doing it here.
>

Ok, thanks for your correcting.


>> When I use the patch, I found that acpi_gsi_to_irq in driver/acpi/irq.c
>> only handle existed mapping and will return -EINVAL if mapping not
>> found. When I test on my machine, a calling stack is as following:
>>
>>
>> acpi_bus_init
>> ->acpi_enable_subsystem
>> ->acpi_ev_install_xrupt_handlers
>> ->acpi_ev_install_sci_handler
>> ->acpi_os_install_interrupt_handler
>> ->acpi_gsi_to_irq
>>
>>
>> the acpi_gsi_to_irq returned -EINVAL because of no mapping found. I
>> looked into acpi_gsi_to_irq of x86, acpi_register_gsi is called in it
>> so that new mapping for gsi is created if no mapping is found.
>
> So it looks like we have a discrepancy between the x86 and ARM on that
> front.
>

Yes, I think so.

> Lorenzo, Hanjun, can you please have a look at this and shed some
> light on what the expected behaviour is? It looks like we never
> encountered an issue with this on arm64, which tends to indicate that
> we don't usually use the above path.
>
>> I looked into generic acpi_register_gsi, the existed mapping will be
>> checked first by calling irq_find_mapping, so I think calling
>> acpi_register_gsi in acpi_gsi_to_irq can address the problem.
>>
>> But you're right, I'm wrong that I passed random value of -1 to
>> acpi_register_gsi. I don't find a right way to address the problem
>> without changing acpi_gsi_to_irq. I'll continue to work for the
>> problem.
>
> At the very least, this should be indirected so that the existing
> behaviour isn't affected, no matter how badly broken arm64 may or may
> not be here. Please have a look at the patch below that should help
> you with this.
>

Ok, Marc, thanks very much for your way to address the problem, I'll try
it on my machine. And I'll put the patch in my patch series without
change( :) ) if that works well.

> Thanks,
>
> M.
>
> From 3e6b87ea49473d0eb384f42e76d584a1495a538c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Sat, 18 Jun 2022 11:29:33 +0100
> Subject: [PATCH] ACPI: irq: Allow acpi_gsi_to_irq() to have an arch-specific
> fallback
>
> It appears that the generic version of acpi_gsi_to_irq() doesn't
> fallback to establishing a mapping if there is no pre-existing
> one while the x86 version does.
>
> While arm64 seems unaffected by it, LoongArch is relying on the x86
> behaviour. In an effort to prevent new architectures from reinventing
> the proverbial wheel, provide an optional callback that the arch code
> can set to restore the x86 behaviour.
>
> Hopefully we can eventually get rid of this in the future once
> the expected behaviour has been clarified.
>
> Reported-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> drivers/acpi/irq.c | 8 ++++++--
> include/linux/acpi.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> index 6e1633ac1756..66c5f01995d0 100644
> --- a/drivers/acpi/irq.c
> +++ b/drivers/acpi/irq.c
> @@ -13,6 +13,7 @@
> enum acpi_irq_model_id acpi_irq_model;
>
> static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
> +static int (*acpi_gsi_to_irq_fallback)(u32 gsi);
>
> /**
> * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> @@ -33,9 +34,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>
> *irq = irq_find_mapping(d, gsi);
> /*
> - * *irq == 0 means no mapping, that should
> - * be reported as a failure
> + * *irq == 0 means no mapping, that should be reported as a
> + * failure, unless there is an arch-specific fallback handler.
> */
> + if (!*irq && acpi_gsi_to_irq_fallback)
> + *irq = acpi_gsi_to_irq_fallback(gsi);
> +
> return (*irq > 0) ? 0 : -EINVAL;
> }
> EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 957e23f727ea..71d3719e3ec4 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -357,6 +357,7 @@ int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);
>
> void acpi_set_irq_model(enum acpi_irq_model_id model,
> struct fwnode_handle *(*)(u32));
> +void acpi_set_gsi_to_irq_fallback(int (*)(u32));
>
> struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> unsigned int size,
>

\
 
 \ /
  Last update: 2022-06-20 04:47    [W:0.130 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site