Attention is currently required from: Michał Żygowski, Angel Pons, Michael Niewöhner, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 )
Change subject: acpi_ec: Implement basic ACPI embedded controller API ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
Patchset:
PS9: Look good enough to me now.
However, some remarks:
I'm really not a fan of avoidable timeouts. I don't mean the code handling them, but the fact that we risk to run into a timeout. When, for instance, for one command waiting 100ms is enough but for another we'd want to wait 300ms, what harm would be done if we always wait 500ms? Of course there are limits; we don't want a user to become impatient.
This is why I came up with two classes of timeouts: 1. those we just check to ensure that a program terminates gracefully; 2. those that we actually handle and know how to recover and see a chance to continue. In the former case we usually want to avoid the timeout at all costs but bail out if we hit it just once. Thus, the timeout can and should be very long. 1, 2, 3 full seconds, it wouldn't hurt. For the second class of timeouts, we might hit it more than once, we should find a trade-off between functionality and responsiveness.
Looking at other implementations I've notice that even coreboot's are slightly more complex. Those wait for 10us between reads of the status register. This brings the timeout numbers closer to realtime. But it might also matter for the EC. Imagine, for instance, an implementation where every status read triggers an interrupt in the controller. In a polling loop like the one implemented here, the controller might get stalled answering those interrupts, leaving less processing time to do the work we are waiting for.
-
Nico Huber (Code Review)