Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
I'm not clear on the motivation for the change, e.g., to get rid of PC_KEYBOARD_AT_TRANSLATED or fix some other issue. I've worked on enough other OSes that "because Linux does it" isn't usually a very compelling argument, though this does seem like one of the few cases I've seen where Linux goes by what the firmware set (usually Linux goes off and does its own thing regardless). I'm not arguing against the change, but given the potential for breakage the phrase "if it ain't broke don't fix it" comes to mind. I'm sure something's being fixed, but it's not obvious to me and I'd generally expect the translation mode to be based on what code set(s) the keyboard can support, rather than setting the keyboard mode based on the current translation mode.
Hi Matt,
your wariness is absolutely warranted. I didn't mention all that's broken in the commit message simply because that would bloat it too much, IMHO. I think this should be tested with as much hardware as possible (in reasonable time).
The current state is that depending on hardware defaults and core- boot settings, it might work only with one setting of PC_KEYBOARD_ AT_TRANSLATED. And for that, you can pretty much roll a dice, it's not easy to predict.
What's broken: * The !PC_KEYBOARD_AT_TRANSLATED path resets the keyboard back to it's defaults after selecting set #1, reverting the latter for all keyboards that default to set #2. * Both paths ignore the state of the other setting. The PC_KEYBOARD_AT_TRANSLATED path is broken for any keyboard that doesn't default to set #2. The other path is broken for all keyboard controllers that have the translation enabled by default (in the original state of the path) or for either controller setting depending on the default state of the keyboard (in the current broken state). * If upfront knowledge about the hardware is needed, PC_KEYBOARD_AT_TRANSLATED would carry information that belongs into coreboot's devicetree.
Now that was considering integrated keyboards. If we consider plug- gable keyboards, we can take the above times 2.
Payloads are no onetime concept anymore. Considering that we might run after a different payload that made its own settings, we can take it times x additionally (I've stopped counting).
So long story short, changing one setting without considering the other is easily broken by flipping a switch in coreboot or a prior payload.
The usual upstream goal is to have coreboot work with any payload. And Linux is the one that has the least issues with PS/2 keyboards. Hence the thought to follow it. Their idea seems to be to leave the controller configuration to the firmware and to configure the key- board accordingly.
All the code I've read so far on the topic seems to assume that all keyboards can switch between scancode sets #1 and #2 at least.