Messages in this thread Patch in this message | | | Date | Wed, 25 Oct 2023 09:41:06 -0500 | Subject | Re: PIC probing code from e179f6914152 failing | From | Mario Limonciello <> |
| |
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?
> > 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; > +} >
| |