lkml.org 
[lkml]   [2023]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: PIC probing code from e179f6914152 failing
From
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;
> +}
>

\
 
 \ /
  Last update: 2023-10-25 16:42    [W:0.110 / U:1.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site