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/