On 01-Jul-17 06: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.
The following questions came up:
Should dead code be "fixed" at all ?
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.
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 ?
Should we mark ACPI TRAP as bad and force future development to use AML
only ?
I added the ACPI TRAP code as an example on how to add ACPI traps in
case a platform would ever need that feature.
There can be a number of scenarios where that is needed, I believe at
the time Windows was disallowing access
of the smbus through ACPI. On platforms where smbus attached devices
were used to control the platform, an ACPI
trap would be the only way to achieve this sort of thing.
I don't know if turning on ACPI traps can be called "fixing the code".
It's there as an example on how to use it, but it is
disabled, because no platform seems to actually use that feature (it was
just copy-catted around from my original i945 implementation)
But I don't think that we should remove the knowledge from the code base.
Stefan
Regards,
Patrick
References:
[1]: https://review.coreboot.org/#/c/20328/
--
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot
