From e825d1d9ef0ca871ae87b731263cf9f1b424162f Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 15 Oct 2025 11:13:14 -0700 Subject: [PATCH] cpu_seq: fix `set_power_state` idempotency for substates PR #2064 was _supposed_ to change the Gimlet and Cosmo CPU sequencer API so that requesting an idempotent power state transition (i.e. A0 -> A0) returns `Ok(Transition::Unchanged)` rather than `Err(SeqError::IllegalTransition)`. Unfortunately, it doesn't always do that in some cases. This is because the match arm in which we detect no-op power state transitions tests that the current and requested `PowerState`s are _equal_. But, unfortunately, the `PowerState` enum has more variants than just "A2" and "A0" --- it also represents _substates_ of A0 and A2, such as `A0PlusHP` and `A2PlusFans`. In particular, when a compute sled is fully up and running, it's actually *not* in `PowerState::A0`, it's in `PowerState::A0PlusHP`, because the NIC hotplug controller is enabled. The `PowerState::A0` and `PowerState::A0PlusHP` enum variants are not equal, so when a sled that's actually in `A0PlusHP` is told to go to `A0`, it doesn't match and falls through to the error case incorrectly. I'm pretty sure the reason we didn't realize this earlier is because, while I did test both A2->A2 and A0->A0 transitions when I was testing PR #2064, I would send the A0->A0 request more or less as soon as the system reached A0. I didn't wait for the host OS to come up before testing it, so the system was still in `PowerState::A0` and not `PowerState::A0PlusHP`. Whoopsie. This commit makes this operation idempotent in those cases by treating `A0PlusHP->A0` and `A2PlusFans->A2` transitions as idempotent successes rather than `IllegalTransition`s. I did *not* change the behavior in A0 substates that indicate a CPU reset condition (`A0Reset` and `A0Thermtrip`), as in those cases, we require an explicit transition back to A2 before the system will return to A0. Fixes #2271 --- drv/cosmo-seq-server/src/main.rs | 10 ++++++++++ drv/gimlet-seq-server/src/main.rs | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/drv/cosmo-seq-server/src/main.rs b/drv/cosmo-seq-server/src/main.rs index 609e63ced..50374f2f1 100644 --- a/drv/cosmo-seq-server/src/main.rs +++ b/drv/cosmo-seq-server/src/main.rs @@ -599,6 +599,16 @@ impl ServerImpl { // This is purely an accounting change (PowerState::A0, PowerState::A0PlusHP) => (), + // A0PlusHP is a substate of A0; if we are in A0PlusHP and we are + // asked to go to A0, return `Unchanged`, because `A0PlusHP` means + // we are already in A0. + // Similarly, A2PlusFans "counts as" A2 for the purpose of + // externally-requested transitions. + (PowerState::A0PlusHP, PowerState::A0) + | (PowerState::A2PlusFans, PowerState::A2) => { + return Ok(Transition::Unchanged) + } + // If we are already in the requested state, return `Unchanged`. (current, requested) if current == requested => { return Ok(Transition::Unchanged) } diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs index cd92457fc..dacc38896 100644 --- a/drv/gimlet-seq-server/src/main.rs +++ b/drv/gimlet-seq-server/src/main.rs @@ -967,6 +967,20 @@ impl ServerImpl { Ok(Transition::Changed) } + // + // A0PlusHP is a substate of A0; if we are in A0PlusHP and we are + // asked to go to A0, return `Unchanged`, because `A0PlusHP` means + // we are already in A0. + // Similarly, A2PlusFans "counts as" A2 for the purpose of + // externally-requested transitions. + // + (PowerState::A0PlusHP, PowerState::A0) + | (PowerState::A2PlusFans, PowerState::A2) => { + Ok(Transition::Unchanged) + } + // + // If we are already in the requested state, return `Unchanged`. + // (current, requested) if current == requested => { Ok(Transition::Unchanged) }