Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <789ff693-a4f0-eef7-7991-59c031fefe49@redhat.com>
Date:   Fri, 20 Oct 2023 17:16:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: PIC probing code from e179f6914152 failing
Content-Language: en-US, nl
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Thomas Gleixner <tglx@linutronix.de>, kys@microsoft.com,
        hpa@linux.intel.com
Cc:     x86@kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Borislav Petkov <bp@alien8.de>
References: <c8d43894-7e66-4a01-88fc-10708dc53b6b@amd.com>
 <878r7z4kb4.ffs@tglx> <e79dea49-0c07-4ca2-b359-97dd1bc579c8@amd.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <e79dea49-0c07-4ca2-b359-97dd1bc579c8@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Mario,

On 10/19/23 23:20, Mario Limonciello wrote:
> On 10/18/2023 17:50, Thomas Gleixner wrote:

<snip>

>> But that brings up an interesting question. How are those affected
>> machines even reaching a state where the user notices that just the
>> keyboard and the GPIO are not working? Why?
> 
> So the GPIO controller driver (pinctrl-amd) uses platform_get_irq() to try to discover the IRQ to use.
> 
> This calls acpi_irq_get() which isn't implemented on x86 (hardcodes -EINVAL).
> 
> I can "work around it" by:
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 76bfcba25003..2b4b436c65d8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -187,7 +187,8 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
>         }
> 
>         r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> -       if (has_acpi_companion(&dev->dev)) {
> +       if (IS_ENABLED(CONFIG_ACPI_GENERIC_GSI) &&
> +            has_acpi_companion(&dev->dev)) {
>                 if (r && r->flags & IORESOURCE_DISABLED) {
>                         ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
>                         if (ret)
> 
> but the resource that is returned from the next hunk has the resource flags set wrong in the NULL pic case:
> 
> NULL case:
> r: AMDI0030:00 flags: 0x30000418
> PIC case:
> r: AMDI0030:00 flags: 0x418
> 
> IOW NULL pic case has IORESOURCE_DISABLED / IORESOURCE_UNSET
> 
> This then later the GPIO controller interrupts are not actually working.
> For example the attn pin for my I2C touchpad doesn't work.

Right the issue is that with the legacy-pic path disabled /
with nr_legacy_irqs() returning 0 them there is no mapping
added for the Legacy ISA IRQs which causes this problem.

My hack to set nr_legacy_irqs to 16 also for the NULL PIC from:
https://bugzilla.kernel.org/show_bug.cgi?id=218003

Does cause the Legacy ISA IRQ mappings to get added and makes
the GPIO controller actually work, as can be seen from:

https://bugzilla.kernel.org/attachment.cgi?id=305241&action=edit

Which is a dmesg with that hack and it does NOT have this error:

[    0.276113] amd_gpio AMDI0030:00: error -EINVAL: IRQ index 0 not found
[    0.278464] amd_gpio: probe of AMDI0030:00 failed with error -22

and the reporter also reports the touchpad works with this patch.

As Thomas already said the legayc PIC really is not necessary,
but what is still necessary on these laptops with the legacy PIC
not initialized is to have the Legacy ISA IRQ mappings added
by the kernel itself since these are missing from the MADT
(if I have my ACPI/IOAPIC terminology correct).

This quick hack (which is the one from the working dmesg)
does this:

--- a/arch/x86/kernel/i8259.c	
+++ a/arch/x86/kernel/i8259.c	
@@ -394,7 +394,7 @@ static int legacy_pic_probe(void)
 }
 
 struct legacy_pic null_legacy_pic = {
-	.nr_legacy_irqs = 0,
+	.nr_legacy_irqs = NR_IRQS_LEGACY,
 	.chip = &dummy_irq_chip,
 	.mask = legacy_pic_uint_noop,
 	.unmask = legacy_pic_uint_noop,

But I believe this will break things when there are actually
non legacy ISA IRQs / GSI-s using GSI numbers < NR_IRQS_LEGACY

Thomas, I'm not at all familiar with this area of the kernel,
but would checking if the MADT defines any non ISA GSIs under
16 and if NOT use nr_legacy_irqs = NR_IRQS_LEGACY for the
NULL PIC be an option?

Or maybe some sort of DMI (sys_vendor == Lenovo) quirk to
set nr_legacy_irqs = NR_IRQS_LEGACY for the NULL PIC ?

Regards,

Hans