oxidecomputer / oxidecomputer/hubris
Inelegant solution to UpdateError::AlreadyFinished in finish_image_update should be improved
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 239
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 23
Description
finish_image_update can take more than TIMEOUT_QUICK to complete and the operation is treated as retryable.
However, It does not take as long as TIMEOUT_QUICK times DEFAULT_ATTEMPTS. Since the first request is honored
by the RoT but the RoT response is discarded by the SP, some subsequent request is reported as redundant by the RoT
(UpdateError::AlreadyFinished) and the SP reports the error up the stack.
The simplest fix is to increase the timeout but we could do better.
- the ultimate client could check if the update finished was the one it is concerned about. But the SP is holding the UUID used to identify the update session and there are multiple components that can be updated on the RoT (stage0next and Hubris).
- The current FWID of the contents of a flash bank could be requested at any time (not just the boot time record).
Timeout hack to sidestep the problem:
diff --git a/drv/stm32h7-sprot-server/src/main.rs b/drv/stm32h7-sprot-server/src/main.rs
index 3c9cd229..8e8e1371 100644
--- a/drv/stm32h7-sprot-server/src/main.rs
+++ b/drv/stm32h7-sprot-server/src/main.rs
@@ -77,6 +77,8 @@ const TIMEOUT_QUICK: u32 = 5;
const DEFAULT_ATTEMPTS: u16 = 3;
/// Slightly longer timeout
const TIMEOUT_MEDIUM: u32 = 50;
+/// Long timeout
+const TIMEOUT_LONG: u32 = 200;
// Delay between asserting CSn and sending the portion of a message
// that fits entirely in the RoT's FIFO.
@@ -736,9 +738,12 @@ impl<S: SpiServer> idl::InOrderSpRotImpl for ServerImpl<S> {
) -> Result<(), idol_runtime::RequestError<SprotError>> {
let body = ReqBody::Update(UpdateReq::Finish);
let tx_size = Request::pack(&body, self.tx_buf);
+ // For stage0next updates, erase and flash doesn't happen
+ // until the finish operations. Use a long timeout.
let rsp = self.do_send_recv_retries(
tx_size,
- TIMEOUT_QUICK,
+ // TODO: Tune TIMEOUT_LONG and deal with retried finish_image_update.
+ TIMEOUT_LONG,
DEFAULT_ATTEMPTS,
)?;
if let RspBody::Ok = rsp.body? {
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in drv/stm32h7-sprot-server/src/main.rs at finish_image_update and follow do_send_recv_retries, including the TIMEOUT_QUICK/TIMEOUT_LONG handling. Compare the retry behavior with the RoT's UpdateError::AlreadyFinished response and the update-session UUID/FWID context described here. Done means a legitimate completed update is not surfaced as a failure or handled only by the timeout hack.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100