(In reply to Mario Limonciello (AMD) from comment #61) > #54 > > 1. I had a similar concern while adjusting v2. HOWEVER I think it's > mitigated by the fact the DMI check ONLY runs in the failure path now. So > even though it technically applies to all laptops from "82" family for > Lenovo only ones with a problem will run it. So what I'm wondering is if the magic: outb(0x68, PIC_MASTER_IMR); outb(0x1, PIC_MASTER_IMR); sequence is something AMD specific or some generic PIC enable sequence ? If this is AMD specific then adding an boot_cpu_has(X86_FEATURE_ZEN) check seems to make sense. If this is not AMD specific then I'm thinking to maybe make the fix much more generic (continued below) > I would much rather we don't use a Zen heuristic because this has nothing to > do with it being an AMD bug. It happens to be a BIOS bug on some Lenovo AMD > systems. See above, this may not be an AMD bug, but if the magic enable sequence is AMD specific then IMHO it should still be guarded by some AMD check. > 2. So you mean like the PIC malfunctions specifically on the quirked > systems? Seems unlikely to me. It just generally seems like a good idea to re-check things to ensure that the enable sequence actually worked. Also since the probe path is already poking the PIC_MASTER_IMR register anyway doing the enable-sequence magic should not do anything on systems where there really is no PIC. I quick duckduckgo shows that the "Using NULL legacy PIC" message has only ever been seen in the wild (before now) on virtual machines either oracle vms (virtualbox?) or WSL2. Which presumably really don't have anything at the PIC io addresses in the specific VM config which triggers this. Also note that such configs really should set the acpi-reduced-hw flag at which point probe_8259A() will not be called at all, because the legacy_pic pointer is already set to the NULL PIC early on then. So what I'm thinking is that since the Lenovo BIOS setup does work under Windows we may see more of this and a more generic non DMI quirked fix would be better (with the retry). Specifically I'm thinking about doing something like this: bool retried = false; /* ... */ raw_spin_lock_irqsave(&i8259A_lock, flags); retry: outb(0xff, PIC_SLAVE_IMR); /* mask all of 8259A-2 */ outb(probe_val, PIC_MASTER_IMR); new_val = inb(PIC_MASTER_IMR); if (new_val != probe_val) { if (!retried) { printk(KERN_INFO "No PIC trying to force enable PIC\n"); outb(0x68, PIC_MASTER_IMR); outb(0x1, PIC_MASTER_IMR); retried = true; goto retry; } printk(KERN_INFO "Using NULL legacy PIC\n"); ... This is non AMD specific and should be harmless in the case where there really is no PIC: If there really is nothing at the PIC_MASTER_IMR IO address this just does 4 extra writes (2 enable sequence + 2 for retry probe) + 1 extra read and then continues as before. And if there actually is a PIC there it should now be enabled and things should work the same as when the BIOS itself had actually bothered to enable the PIC.