Re: PIC probing code from e179f6914152 failing

[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;
> > +}