Attention is currently required from: Angel Pons, Michael Niewöhner, Sergii Dmytruk.
1 comment:
Patchset:
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.
To view, visit change 55714. To unsubscribe, or for help writing mail filters, visit settings.