lkml.org 
[lkml]   [2023]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: PIC probing code from e179f6914152 failing
From
On 10/23/2023 10:59, Thomas Gleixner wrote:
> On Thu, Oct 19 2023 at 16:20, Mario Limonciello wrote:
>> On 10/18/2023 17:50, Thomas Gleixner wrote:
>>> The only interrupt which does not work is interrupt 0 because nothing
>>> allocates interrupt 0 due to nr_legacy_irqs == 0, but that's a trivially
>>> solvable problem.
>>
>> From David's logs I can see that the timer interrupt gets wired up to
>> IRQ2 instead of IRQ0.
>
> Sure, but that's not really a problem. Nothing needs the timer
> interrupt in principle.
>
>> IOAPIC[0]: Preconfigured routing entry (33-2 -> IRQ 2 Level:0 ActiveLow:0)
>>
>> In my hacked up forcing NULL pic case this fixes that:
>>
>> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
>> index 43c1c24e934b..885687e64e4e 100644
>> --- a/arch/x86/kernel/i8259.c
>> +++ b/arch/x86/kernel/i8259.c
>> @@ -424,7 +424,7 @@ static int legacy_pic_probe(void)
>> }
>>
>> struct legacy_pic null_legacy_pic = {
>> - .nr_legacy_irqs = 0,
>> + .nr_legacy_irqs = 1,
>> .chip = &dummy_irq_chip,
>> .mask = legacy_pic_uint_noop,
>> .unmask = legacy_pic_uint_noop,
>>
>> I think it's cleaner than changing all the places that use
>> nr_legacy_irqs().
>
> No. It's not cleaner. It's a hack and you still need to audit all places
> which depend on nr_legacy_irqs(). Also why '1'? You could as well use
> '16', no? >
>> On my side this makes:
>>
>> IOAPIC[0]: Preconfigured routing entry (33-2 -> IRQ 0 Level:0
>> ActiveLow:0)
>
> Sure, but that can be achieved by other means in a clean way as
> well. Can we please focus on analyzing the underlying problems instead
> of trying random hacks? The timer part is well understood already.
>
>>> That machine does not even need the timer interrupt because it has a
>>> usable APIC and TSC deadline timer, so no APIC timer calibration
>>> required. The same is true for CPUs which do not have a TSC deadline
>>> timer, but enumerate the APIC frequency via CPUID or MSRs.
>>
>> Don't you need it for things like rtcwake to be able to work?
>
> Timer != RTC.
>
> The RTC interrupt is separate (IRQ 8), but in the case of this system it
> is using the HPET-RTC emulation which fails to initialize because
> interrupt 0 is not available.

That's exactly why I allocated 1 IRQ for IRQ 0.

>
>>> 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)
>
> So why is acpi_irq_get() reached when the PIC is disabled, but not when
> the PIC is enabled? Because of the below:
>
>> but the resource that is returned from the next hunk ?
>
> next hunk? The resource is returned by platform_get_resource() above, no?
>
>> 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
>
> So the real question is WHY are the DISABLED/UNSET flags not set in the
> PIC case?
>
>> NULL case:
>> handler: handle_edge_irq
>> dstate: 0x3740c208
>> IRQ_TYPE_LEVEL_LOW
>>
>> PIC case:
>> handler: handle_fasteoi_irq
>> dstate: 0x3740e208
>> IRQ_TYPE_LEVEL_LOW
>> IRQD_LEVEL
>>
>> I guess something related to the callpath for mp_register_handler().
>
> Guessing is not helpful.
>
> There is a difference in how the allocation info is set up when legacy
> PIC is enabled, but that does not explain the above resource flag
> difference.

I did a pile of printks and that's how I realized it's because of the
missing call to mp_register_handler() which is dependent upon what
appeared to me to be a superfluous number of legacy IRQs check (patch 1
in my solution).

>
> As there is no override for IRQ7:
>
> [ 0.011415] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
> [ 0.011417] Int: type 0, pol 0, trig 0, bus 00, IRQ 00, APIC ID 20, APIC INT 02
> [ 0.011418] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 low level)
> [ 0.011419] Int: type 0, pol 3, trig 3, bus 00, IRQ 09, APIC ID 20, APIC INT 09
> ...
> [ 0.011425] Int: type 0, pol 0, trig 0, bus 00, IRQ 07, APIC ID 20, APIC INT 07
>
> the initial setup of the IOAPIC interrupt is edge, while the initial
> setup of the legacy PIC is level. But that gets changed later to edge
> when the IOAPIC is initialized.
>
> I'm not seeing the magic which make the above different yet, though I'm
> 100% sure by now that this "works" definitely not by design. It just
> works by pure luck.
>
> Now when platform_get_irq_optional() sets the trigger type via
> irqd_set_trigger_type() it just sets LEVEL_LOW, but does not change the
> handler and does not set IRQD_LEVEL. It does neither change the IO/APIC
> pin setup. This happens because the IOAPIC interrupt chip does not
> implement an irq_set_type() callback.
>
> IOW the whole machinery depends on magic setup ordering vs. PIC and pure
> luck. Adding the callback is not rocket science, but while it should
> make the interrupt work it still does not explain the magic "working"
> when the legacy PIC is enabled.
>
> Let me sit down and add a pile of debug printks to all the relevant
> places as we really need to understand the underlying magic effects of
> legacy PIC first.
>

OK let's see if you come up with different conclusions.

\
 
 \ /
  Last update: 2023-10-23 18:18    [W:0.113 / U:1.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site