Attention is currently required from: Angel Pons, Michael Niewöhner, Sergii Dmytruk. Michał Żygowski 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:
(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.
True, there is no harm. We may poll in 10us intervals and do 100k checks or so (which gives 1 second). In a good scenario, the EC should be responsive and we shouldn't hit any timeout.
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.
Here we are talking about timeout type 1. If anything times out, then there is probably no reason to continue because there is high chance everything what is going next will fail.
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.
Totally understandable. I might add the 10us intervals.
-
Michał Żygowski (Code Review)