[coreboot] Discussion about fixing dead code / ACPI TRAP

[Nico Huber]

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/
>