cpu_seq: standardize ereport naming#2231
Conversation
This commit tweaks the field and class naming in the CPU sequencer ereports for VRM PMbus alerts (added in #2184) based on a discussion with @rmustacc on standardizing ereport naming and class hierarchy. In particular, we namespace all "hardware" events under "hw", and emit a different class name string when the POWER_GOOD state has changed. Additionally, we've rename the "status" field for PMBus status registers to "pmbus_status" for clarity, and the "dev_id" to "refdes" --- the idea is that "dev_id" will be used when we want to indicate that the `control-plane-agent` device ID is _different_ from the refdes, but all `hw.` ereports will include a "refdes".
| k: if !power_good { | ||
| "hw.pwr_good.bad" | ||
| } else { | ||
| "hw.pmbus.alert" | ||
| }, |
There was a problem hiding this comment.
Do we want to change the class for pmbus alerts? If we find power isn't good, is that something we should generate a separate ereport for?
There was a problem hiding this comment.
Yeah, I suppose we could do one ereport for the PMBus alert and then immediately add another one for the POWER_GOOD state changing. That seems like the theoretically most proper thing to do, but on the other hand it also feels a little silly to do two ereports immediately from the same place. It's also less space efficient than adding a field/changing the class string, as it will require packrat to encode a second ENA, task name, task restart generation, and timestamp for the subsequent ereport. But, on the other hand, if there's only a little space in the buffer, we might be able to fit a "power good state changed" ereport but we might not have been able to fit a "here's a dump of all the status registers", so then we at least recorded something. Hm.
There was a problem hiding this comment.
I guess the distinction here is that we're getting this by polling PMBus alert and noticing it. If we were actually paying attention to the PG outputs and acting on that, then I think the separate power good class would feel like it made more sense to me. In this case maybe we should be asking what would actually notice this and tell us and the fact that if we lost power on one rail we're losing it on several, but the PMBus alert as to why may be what's distinctively useful here.
I guess to summarize, since we're getting this by polling alert, probably a consistent class makes the most sense?
There was a problem hiding this comment.
Yeah, I think that makes sense; I'll put it back.
| let ereport = Ereport { | ||
| k: "pmbus.alert", | ||
| k: if !power_good { | ||
| "hw.pwr_good.bad" |
There was a problem hiding this comment.
Do we want to put this in a hw.power.* class? Though saying that it leads me to also wonder if pmbus should be a subclass of power or not.
There was a problem hiding this comment.
Having a power (or disemvoweled to pwr to save uh, 2B) seems reasonable, although I suspect it's more useful for a human trying to debug a problem by searching ereports than for automated diagnosis. I would probably expect most/all PMBus alerts to fall under that, yeah.
There was a problem hiding this comment.
I think the part of it for me are what are the properties that would be consistent across the class. For example, I expect every hw.pwr event to include a rail property and refdes say.
as per @rmustacc's comment in #2231 (comment), we should probably include a `rail` field on all `hw.pwr.*` ereports.
|
This build failure looks unrelated, probably transient. Restarting it. |
|
Okay, I think I've addressed all @rmustacc's feedback; the ereports on Gimlet now look like this: eliza@castle ~ humility --ip 'fe80::aa40:25ff:fe04:147%madrid_sw0tp0' pmbus -r V12_SYS_A2 -w VOUT_COMMAND=8
humility: connecting to fe80::aa40:25ff:fe04:147%21
humility: I2C4, port F, dev 0x67: successfully wrote VOUT_COMMAND
eliza@castle ~ $ humility --ip 'fe80::aa40:25ff:fe04:147%madrid_sw0tp0' pmbus -r V12_SYS_A2 -w VOUT_COMMAND=12
humility: connecting to fe80::aa40:25ff:fe04:147%21
humility: I2C4, port F, dev 0x67: successfully wrote VOUT_COMMAND
eliza@castle ~ $ pilot sp exec -e "ereports" BRM42220004
Sep 17 17:00:27.595 INFO creating SP handle on interface madrid_sw0tp0, component: faux-mgs
restart ID: 3704242c-dd22-23cd-db1e-29f308cf15fb
restart IDs did not match (requested 00000000-0000-0000-0000-000000000000)
count: 2
ereports:
0x1: {
"baseboard_part_number": String("913-0000019"),
"baseboard_rev": Number(6),
"baseboard_serial_number": String("BRM42220004"),
"ereport_message_version": Number(0),
"hubris_archive_id": String("lYT99bGsUTY"),
"hubris_task_gen": Number(0),
"hubris_task_name": String("packrat"),
"hubris_uptime_ms": Number(0),
"lost": Null,
}
0x2: {
"baseboard_part_number": String("913-0000019"),
"baseboard_rev": Number(6),
"baseboard_serial_number": String("BRM42220004"),
"ereport_message_version": Number(0),
"hubris_archive_id": String("lYT99bGsUTY"),
"hubris_task_gen": Number(0),
"hubris_task_name": String("gimlet_seq"),
"hubris_uptime_ms": Number(13436),
"k": String("hw.pwr.pmbus.alert"),
"pmbus_status": Object {
"cml": Number(0),
"input": Number(32),
"iout": Number(0),
"mfr": Number(8),
"temp": Number(0),
"vout": Number(0),
"word": Number(12289),
},
"pwr_good": Bool(true),
"rail": String("VDD_VCORE"),
"refdes": String("U350"),
"time": Number(13430),
"v": Number(0),
} |
This commit tweaks the field and class naming in the CPU sequencer ereports for VRM PMbus alerts (added in oxidecomputer#2184) based on a discussion with @rmustacc on standardizing ereport naming and class hierarchy. In particular, we namespace all "hardware" events under "hw", and emit a different class name string when the POWER_GOOD state has changed. Additionally, we've rename the "status" field for PMBus status registers to "pmbus_status" for clarity, and the "dev_id" to "refdes" --- the idea is that "dev_id" will be used when we want to indicate that the `control-plane-agent` device ID is _different_ from the refdes, but all `hw.` ereports will include a "refdes".
This commit tweaks the field and class naming in the CPU sequencer ereports for VRM PMbus alerts (added in #2184) based on a discussion with @rmustacc on standardizing ereport naming and class hierarchy. In particular, we namespace all "hardware" events under "hw", and emit a different class name string when the POWER_GOOD state has changed. Additionally, we've rename the "status" field for PMBus status registers to "pmbus_status" for clarity, and the "dev_id" to "refdes" --- the idea is that "dev_id" will be used when we want to indicate that the
control-plane-agentdevice ID is different from the refdes, but allhw.ereports will include a "refdes".