On 01.07.2017 15:20, Patrick Rudolph wrote: > Hello community, > I'll want to start a discussion about fixing dead code. > > How it all started: > I tried to run docking code on Lenovo T400 and found it's not working. > While investigation it turns out that the ACPI TRAP mechanism is being > used, and that it doesn't start the SMM handler. > The mechanism works fine on Lenovo X60 and T60 because they enable the > IO TRAP in romstage. > > Fixing it: > I found that every Intel southbridge has code to support ACPI TRAP, but > doesn't enable the trap mechanism by default. The idea is to enable ACPI > TRAP in southbridge depending on a Kconfig option. The fix is here [1] > > The problem: > Nico pointed out that while the fix might be technically correct, it > would touch a lot dead code. There are only a very few boards using the > mechanism.
There are some boards (probably all ThinkPads but the X60/T60) that have SMM handlers (some for docking, some for naught) which, looking at the code, were never called. As long as there is no tested / working handler that really does something useful, I'd rather not see code added around it. The older ThinkPad's docking code seems like a valid use case to me. I'm not convinced that newer docks need it too (do they still have complex configuration to do? is there a superio?). If newer docks can be handled easily in ASL and their configuration is not required pre-OS, I'd do it in ASL only. > > The following questions came up: > Should dead code be "fixed" at all ? Please investigate first if the code is needed at all. And in the case of SMM handlers, if it needs to be done in SMM. > Should the "dead" code be removed on platforms that do not use ACPI TRAP > ? Developers that want to use the mechanism in the future will have to > reimplement it. Please remove as much as you can, we'll have it all in the history. As long as there are boards that still use it, we have a _working_ example. That's better than some mix of dead code paths. > As it touches a lot "dead" code, it cannot be easily tested on some > platforms. Should patches be accepted for those platforms ? > Should we get rid of ACPI TRAP mechanism and reimplement everything in > AML only ? If the current code for X60/T60 works, I'd keep it as is. The dead code, if it's good for anything at all, I'd port to ASL. > Should we mark ACPI TRAP as bad and force future development to use AML > only ? There might always be a valid use case to trap from ASL to SMM. But if it doesn't have to be SMM, I encourage everyone to write it in ASL! Nico > > Regards, > Patrick > > References: > [1]: https://review.coreboot.org/#/c/20328/ > -- coreboot mailing list: coreboot@coreboot.org https://mail.coreboot.org/mailman/listinfo/coreboot
