lkml.org 
[lkml]   [2022]   [Jun]   [28]   [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/28 下午3:42, Hanjun Guo wrote:
    > On 2022/6/18 18:36, Marc Zyngier wrote:
    > [...]
    >>>>> --- 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.
    >>
    >>> 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.
    >>
    >> 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.
    >
    > Sorry for the late reply, I just noticed this tomorrow.
    >
    > As you said, we never encountered Jianmin's issue on ARM64 hardware,
    > for the call stack which Jianmin shows, acpi_ev_install_xrupt_handlers()
    > is only called for non-reduced ACPI hardware, but ARM64 is always
    > defined as reduced ACPI hardware in the ACPI spec, from the first
    > supported version of ACPI spec for ARM.
    >
    > Jianmin, is the LoongArch using the redunced hardware mode in ACPI?
    > if it's using SCI interrupt, I think not, correct me if I'm wrong.
    >

    Thanks for your reply, Hanjun, LoongArch uses non-reduced ACPI hardware,
    so SCI interrupt is used, which is different from ARM using reduced
    hardware.

    >>
    >>> 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.
    >
    > Looks good to me, I will review and test the v13 patch set.
    >
    > Thanks
    > Hanjun

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