Messages in this thread | | | Date | Wed, 25 Oct 2023 10:25:11 -0500 | Subject | Re: PIC probing code from e179f6914152 failing | From | Mario Limonciello <> |
| |
On 10/25/2023 09:41, Mario Limonciello wrote: > On 10/25/2023 04:23, Thomas Gleixner wrote: >> On Mon, Oct 23 2023 at 17:59, Thomas Gleixner wrote: >>> On Thu, Oct 19 2023 at 16:20, Mario Limonciello wrote: >>>> 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? >> >> So I sat down and did a thorough analysis of legacy PIC dependencies. >> >> Unfortunately this is an unholy mess and sprinkled all over the place, >> so there is no trivial way to resolve this quickly. This needs a proper >> overhaul to decouple the actual PIC driver selection from the fact that >> the kernel runs on a i8259 equipped hardware and therefore needs to >> honour the legacy PNP overrides etc. >> >> The probing itself is to stay in order to avoid sprinkling weird >> conditions and NULL PIC selections all over the place. >> >> It could be argued that the probe function should try to initialize the >> PIC, but that's overkill for scenarios where the PIC does not exist. >> >> Though it turns out that ACPI/MADT is helpful here because the MADT >> header has a flags field which denotes in bit 0, whether the system has >> a 8259 setup or not. >> >> This allows to override the probe for now until we actually resolved the >> dependency problems in a clean way. >> >> Untested patch below. > > +David from the bugzilla. > > I checked his acpidump and I do think this will work for him. > > [024h 0036 4] Local Apic Address : FEE00000 > [028h 0040 4] Flags (decoded below) : 00000001 > PC-AT Compatibility : 1 > > > David - can you see if the below helps your hardware?
FYI, David confirmed this works for fixing his hardware, thanks.
https://bugzilla.kernel.org/show_bug.cgi?id=218003#c84
> >> >> Thanks, >> >> tglx >> --- >> --- a/arch/x86/include/asm/i8259.h >> +++ b/arch/x86/include/asm/i8259.h >> @@ -69,6 +69,8 @@ struct legacy_pic { >> void (*make_irq)(unsigned int irq); >> }; >> +void legacy_pic_pcat_compat(void); >> + >> extern struct legacy_pic *legacy_pic; >> extern struct legacy_pic null_legacy_pic; >> --- a/arch/x86/kernel/acpi/boot.c >> +++ b/arch/x86/kernel/acpi/boot.c >> @@ -148,6 +148,9 @@ static int __init acpi_parse_madt(struct >> pr_debug("Local APIC address 0x%08x\n", madt->address); >> } >> + if (madt->flags & ACPI_MADT_PCAT_COMPAT) >> + legacy_pic_pcat_compat(); >> + >> /* ACPI 6.3 and newer support the online capable bit. */ >> if (acpi_gbl_FADT.header.revision > 6 || >> (acpi_gbl_FADT.header.revision == 6 && >> --- a/arch/x86/kernel/i8259.c >> +++ b/arch/x86/kernel/i8259.c >> @@ -32,6 +32,7 @@ >> */ >> static void init_8259A(int auto_eoi); >> +static bool pcat_compat __ro_after_init; >> static int i8259A_auto_eoi; >> DEFINE_RAW_SPINLOCK(i8259A_lock); >> @@ -299,15 +300,32 @@ static void unmask_8259A(void) >> static int probe_8259A(void) >> { >> + unsigned char new_val, probe_val = ~(1 << PIC_CASCADE_IR); >> unsigned long flags; >> - unsigned char probe_val = ~(1 << PIC_CASCADE_IR); >> - unsigned char new_val; >> + >> + /* >> + * If MADT has the PCAT_COMPAT flag set, then do not bother probing >> + * for the PIC. Some BIOSes leave the PIC uninitialized and probing >> + * fails. >> + * >> + * Right now this causes problems as quite some code depends on >> + * nr_legacy_irqs() > 0 or has_legacy_pic() == true. This is silly >> + * when the system has an IO/APIC because then PIC is not required >> + * at all, except for really old machines where the timer interrupt >> + * must be routed through the PIC. So just pretend that the PIC is >> + * there and let legacy_pic->init() initialize it for nothing. >> + * >> + * Alternatively this could just try to initialize the PIC and >> + * repeat the probe, but for cases where there is no PIC that's >> + * just pointless. >> + */ >> + if (pcat_compat) >> + return nr_legacy_irqs(); >> + >> /* >> - * Check to see if we have a PIC. >> - * Mask all except the cascade and read >> - * back the value we just wrote. If we don't >> - * have a PIC, we will read 0xff as opposed to the >> - * value we wrote. >> + * Check to see if we have a PIC. Mask all except the cascade and >> + * read back the value we just wrote. If we don't have a PIC, we >> + * will read 0xff as opposed to the value we wrote. >> */ >> raw_spin_lock_irqsave(&i8259A_lock, flags); >> @@ -429,5 +447,9 @@ static int __init i8259A_init_ops(void) >> return 0; >> } >> - >> device_initcall(i8259A_init_ops); >> + >> +void __init legacy_pic_pcat_compat(void) >> +{ >> + pcat_compat = true; >> +} >> >
| |