Disable transceivers which reply with too many NACKs #1441

mkeeter · 2023-06-26T21:18:21Z

We track NACKs on a per-port basis. If we see too many (3), then we disable the port at the Hubris level.

When disabled:

The port is powered off, reset is asserted, and lpmode is not asserted (to prevent leakage)
UDP messages to change power, reset, and lpmode are rejected with a new HwError::DisabledBySp error type
Ports are skipped in the thermal loop
The new StatusV2 request/response includes StatusV2::DISABLED_BY_SP in the bitfield

Ports must be explicitly re-enabled with the HostRequest::ClearDisableLatch.

Open questions:

Bikeshedding about names for HwError::DisabledBySp and HostRequest::ClearDisableLatch, since I don't love either of them
Should we also reject the Idol APIs to change transceiver state when disabled? Probably!

Cargo.toml

Aaron-Hartwig

This broadly looks good, but I've left a couple thoughts on where I think we should make changes.

Aaron-Hartwig · 2023-06-27T12:12:55Z

drv/transceivers-server/src/main.rs

+        }
+        for (step, f) in [
+            Transceivers::assert_reset,
+            Transceivers::assert_lpmode,


While asserting LPMode makes sense, in reality driving LPMode to some modules while they're unpowered causes other 3V3 issues, so we want this step to be deassert_lpmode. Our investigations of this on the board: https://github.com/oxidecomputer/hardware-qsfp-x32/issues/47#issuecomment-1329846157

thanks, I remembered the leakage issue but got the sign wrong here!

Aaron-Hartwig · 2023-06-27T12:17:37Z

drv/transceivers-server/src/udp.rs

+    /// This function reads a `ModuleResultNoFailure` and populates error
+    /// information at the end of the trailing data buffer. This means it should
+    /// be called as the last operation before sending the response. For results
+    /// where a `ModuleResult` is returned, use sandle_errors_and_failures


Suggested change

/// where a `ModuleResult` is returned, use sandle_errors_and_failures

/// where a `ModuleResult` is returned, use handle_errors_and_failures

Fixed, thanks!

drv/transceivers-server/src/udp.rs

Aaron-Hartwig · 2023-06-27T12:31:08Z

drv/transceivers-server/src/udp.rs

@@ -742,6 +845,63 @@ impl ServerImpl {
        (count, desired_result)
    }

+    fn get_status_v2(


Just putting a reminder here to rename this to align with w/e we land on.

Yes, renamed to get_extended_status

mkeeter · 2023-06-27T21:25:00Z

I think this and oxidecomputer/transceiver-control#113 are read to go.

I tested on the niles Sidecar by adding a Chaos Monkey that gives a 50% change of failing every transceiver temperature read operation. Sure enough, this caused it to disable transceivers after some amount of time:

 NDX LINE      GEN    COUNT PAYLOAD
   4  336        1        1 GetInterfaceError(0x0, NotInitialized)
   5  336        1        1 GetInterfaceError(0x1, NotInitialized)
   6  336        1        1 GetInterfaceError(0x0, NotInitialized)
   7  336        1        1 GetInterfaceError(0x1, NotInitialized)
   8  336        1        1 GetInterfaceError(0x0, NotInitialized)
   9  336        1        1 GetInterfaceError(0x1, NotInitialized)
  10  336        1        1 GetInterfaceError(0x0, NotInitialized)
  11  336        1        1 GetInterfaceError(0x1, NotInitialized)
  12  295        1        1 GotInterface(0x0, Sff8636)
  13  295        1        1 GotInterface(0x1, Sff8636)
  14  408        1        2 TemperatureReadError(0x0, I2cAddressNack)
  15  408        1        1 TemperatureReadError(0x1, I2cAddressNack)
   0  408        2        1 TemperatureReadError(0x0, I2cAddressNack)
   1  461        2        1 DisablingPorts(LogicalPortMask(0x1))
   2  408        2       33 TemperatureReadError(0x1, I2cAddressNack)
   3  461        2        1 DisablingPorts(LogicalPortMask(0x2))

(it's probabilistic, since we have to have 3 failures in a row)

Once ports are disabled, we see it in xcvradm:

matt@niles ~ () $ ./xcvradm -iaxf4 -pfe80::aa40:25ff:fe05:ff00 -t0,1 status
Port Status
   0 PRESENT | RESET | INTERRUPT | DISABLED_BY_SP
   1 PRESENT | RESET | INTERRUPT | DISABLED_BY_SP

We get reasonable errors from xcvradm when performing Forbidden Operations:

matt@niles ~ () $ ./xcvradm -iaxf4 -pfe80::aa40:25ff:fe05:ff00 -t0,1 read-lower --sff 0 4
Port Data
   1 [0x11,0x08,0x00,0xf0]
Some operations failed, errors below
Port Error
   0 Hardware error accessing module 0: Port is disabled by the SP

(time passes, port 1 also gets disabled by Hubris)

matt@niles ~ () $ ./xcvradm -iaxf4 -pfe80::aa40:25ff:fe05:ff00 -t0,1 enable-power
Some operations failed, errors below
Port Error
   0 Hardware error accessing module 0: Port is disabled by the SP
   1 Hardware error accessing module 1: Port is disabled by the SP

Resetting the disabled flag with xcvradm clear-disable-latch allows us to power the transceivers back on and read from them, although they power off stochastically due to the Chaos Monkey.

Aaron-Hartwig

Thanks for tackling this!

bnaecker

A few style nits, but all functionality LGTM!

drv/transceivers-server/src/main.rs

bnaecker · 2023-06-27T22:09:24Z

drv/transceivers-server/src/main.rs

                        }
                    }
                }
                Err(e) => {
                    ringbuf_entry!(Trace::TemperatureReadUnexpectedError(i, e));
                }
            }
+
+            self.consecutive_nacks[i] = if got_nack {
+                self.consecutive_nacks[i].wrapping_add(1)


It seems like we'd want to saturate instead of wrap, though I hope that's pretty unlikely in any case.

Good catch, I failed my coin flip. Fixed in 5100704

drv/transceivers-server/src/udp.rs

bnaecker · 2023-06-27T22:16:47Z

drv/transceivers-server/src/udp.rs

@@ -600,9 +649,20 @@ impl ServerImpl {
        // any modules at index 32->63 are not currently supported.
        let invalid_modules = ModuleId(0xffffffff00000000);


There's an all_sidecar() helper function to return the lower 32 bits, so you could use that and negate it.

I don't see a function with this name anywhere in Hubris, can you point me to it?

drv/transceivers-server/src/udp.rs

bnaecker · 2023-06-28T15:12:38Z

Ah, sorry for the confusion. It’s a public constructor on the ModuleId type.

mkeeter · 2023-06-28T15:18:28Z

Hmmm, I see ModuleId::all(), but that actually sets every bit, including the invalid ones. Anyways, the 0xffffffff00000000 pattern was pre-existing, so I'm fine leaving it as-is.

This PR adds support for ports disabled by Hubris (see oxidecomputer/hubris#1441) - Adds a wider `ExtendedStatus` type and message, which includes a bit for `DISABLED_BY_SP` (and 23 spare bits, just in case) - Uses the `ExtendedStatus` message in `xcvradm` - Adds `HwError::DisabledBySp` and `HostRequest::ClearDisableLatch`

bnaecker · 2023-06-28T16:24:06Z

@mkeeter Yeah, that's fine it's a nit. Here's the method I was referring to.

Building on the module disabling feature introduced in #1441, this PR lets that disable get cleared when a disabled module is removed. Now the SP will enable power at the port again, so when a module is inserted it will get powered without intervention from the host. Fixes #1480

mkeeter requested review from Aaron-Hartwig and bnaecker June 26, 2023 21:22

mkeeter marked this pull request as draft June 26, 2023 21:22

mkeeter commented Jun 26, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

mkeeter mentioned this pull request Jun 26, 2023

Add support for ports disabled by Hubris oxidecomputer/transceiver-control#113

Merged

Aaron-Hartwig requested changes Jun 27, 2023

View reviewed changes

mkeeter force-pushed the transceiver-disable branch from 538eddf to 7f8c8c4 Compare June 27, 2023 20:03

Aaron-Hartwig approved these changes Jun 27, 2023

View reviewed changes

mkeeter marked this pull request as ready for review June 27, 2023 22:05

bnaecker approved these changes Jun 27, 2023

View reviewed changes

mkeeter added 16 commits June 28, 2023 12:19

Bump transceiver-messages

d90c204

Add support for disabled ports

20b2681

Switch back to Git repo

fa41a06

Minimizing diff

7d538fa

Include SP disabling in StatusV2

b16c3c4

s/StatusV2/ExtendedStatus/g

680c006

Fix naming and reply type

15d4d4c

Cleaning

aec9d65

Add logs, fix polarity of lpmode

caa5235

Block read and write to disabled ports

401e95b

Include requested_disabled_modules in failure mask

a1646c9

More logging and fixing self.thermal_models

5e40731

Make a const for MAX_CONSECUTIVE_NACKS

d814991

Saturate instead of wrapping

307dc4d

Remove superfluous into()

d4ebd9c

Use bitwise ops where possible

ed33a65

Revert to transceiver-messages:main

78c735e

mkeeter force-pushed the transceiver-disable branch from 56f4c85 to 78c735e Compare June 28, 2023 16:20

Minimizing diff

5abfdd2

mkeeter merged commit b752985 into master Jun 28, 2023
65 checks passed

mkeeter deleted the transceiver-disable branch June 28, 2023 18:14

mkeeter mentioned this pull request Jun 28, 2023

Sidecar requires a power cycle or RPC call to get out of A2 #1448

Closed

Aaron-Hartwig mentioned this pull request Aug 9, 2023

transceivers: clear module disable when a module is removed #1483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable transceivers which reply with too many NACKs #1441

Disable transceivers which reply with too many NACKs #1441

mkeeter commented Jun 26, 2023 •

edited

Loading

Aaron-Hartwig left a comment

Aaron-Hartwig Jun 27, 2023

mkeeter Jun 27, 2023

Aaron-Hartwig Jun 27, 2023

mkeeter Jun 27, 2023

Aaron-Hartwig Jun 27, 2023

mkeeter Jun 27, 2023

mkeeter commented Jun 27, 2023

Aaron-Hartwig left a comment

bnaecker left a comment

bnaecker Jun 27, 2023

mkeeter Jun 28, 2023

bnaecker Jun 27, 2023

mkeeter Jun 28, 2023

bnaecker commented Jun 28, 2023 via email

mkeeter commented Jun 28, 2023

bnaecker commented Jun 28, 2023

	/// where a `ModuleResult` is returned, use sandle_errors_and_failures
	/// where a `ModuleResult` is returned, use handle_errors_and_failures

		@@ -600,9 +649,20 @@ impl ServerImpl {
		// any modules at index 32->63 are not currently supported.
		let invalid_modules = ModuleId(0xffffffff00000000);

Disable transceivers which reply with too many NACKs #1441

Disable transceivers which reply with too many NACKs #1441

Conversation

mkeeter commented Jun 26, 2023 • edited Loading

Aaron-Hartwig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented Jun 27, 2023

Aaron-Hartwig left a comment

Choose a reason for hiding this comment

bnaecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnaecker commented Jun 28, 2023 via email

mkeeter commented Jun 28, 2023

bnaecker commented Jun 28, 2023

mkeeter commented Jun 26, 2023 •

edited

Loading