Discussion about fixing dead code / ACPI TRAP

[Patrick Rudolph]

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 ?

Regards,
Patrick

References:
[1]: https://review.coreboot.org/#/c/20328/

[Stefan Reinauer]

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/

[Peter Stuge]

Stefan Reinauer wrote:
> (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.

So it is a technology showcase and not required code.

Those two should never be allowed to mix, and Patrick has found one
instance where they can be separated. That's fantastic! Thanks Patrick.

I agree with Stefan that we should not remove the code from the *tree*,
and I also agree with Patrick that this can be cleaned up in many boards.

The question is how we could deal with such technology showcases? I
don't see them belonging in working mainboard code.

Maybe maintain all technologies in a coreboot/showcase or coreboot/reference
mainboard?

//Peter

[Nico Huber]

On 06.07.2017 17:37, Peter Stuge wrote:
> Stefan Reinauer wrote:
> > (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.
>
> So it is a technology showcase and not required code.

It might have started as a showcase but at least for the T60/X60
ThinkPads it looks like working code (Patrick also mentioned that).
They implement lots of docking handling and some brightness control
in SMM. It's not necessary to do it in SMM. But the C code is needed
anyway (to initialize the superio in the dock during boot, to have
serial), so nobody is going to rewrite it in ASL.

I suppose as long as nobody is going to remove it, we won't need
an extra showcase (I don't think it's a good example, using the
I/O trap into SMM, but that's a different story).

Nico

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