OS-6743 need topo maps for the SMCI,SYS-2028U-E1CNRT+ OS-6748 extend disk topo plugin to enumerate nvme devices

[joyent-automation]

OS-6743 need topo maps for the SMCI,SYS-2028U-E1CNRT+
OS-6748 extend disk topo plugin to enumerate nvme devices

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6921/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@KodyKantor commented at 2019-10-04T15:24:00

Patch Set 1:

(11 comments)

I think this looks good. I just have a few questions due to my unfamiliarity and nit picks.

Patch Set 1 code comments

usr/src/lib/fm/topo/libtopo/common/topo_xml.c#1355 @KodyKantor

What problem occurs when this block is in its previous place?

usr/src/lib/fm/topo/libtopo/common/topo_xml.c#1355 @rejohnst

The disk enumerator was being executed before the "binding" property group was added to the parent "bay" node. As a result the disk enumerator didn't find the properties it was expecting because they hadn't been created, yet.

I actually hit this issue a few years ago and made this same change to Oracle Solaris. Deja vu.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#23 @KodyKantor

This falls under the umbrella of "Kody doesn't know how these maps work," but do we need to specify this if the 'node' that this is part of has the fac-enum's provider set to 'fac_prov_ipmi'?

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#23 @rejohnst

The XML at line 19 causes a set of methods to be registered to the "chassis=0" node so that it can use them. The XML at line 23 causes a set of methods to be registered to the "locate" facility node.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#25 @KodyKantor

I looked at a bunch of these XML mapping files and they each used spaces instead of tabs.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#25 @rejohnst

Yep, should be spaces. I fixed this line and also line 35.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#29 @KodyKantor

Should we drop this and instead use <propmethod ... /> on the previous line?

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-chassis-hc-topology.xml#29 @rejohnst

Yes, that would be better. Done.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#30 @KodyKantor

I take it the list of properties here is what IPMI will give us when we ask for sensor data? Is there a document that SMCI publishes that we can use to verify this list?

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#30 @rejohnst

Right. Basically we could discover all these sensors by iterating through the IPMI sensor data repository (SDR), but the SDR on this platform doesn't give enough additional metadata to associate the senor with the correct hardware resource. So we hardcode the association here - binding them all to the motherboard. We've had to do this on most of the SMCI platforms. SMCI doesn't provide a document for this, as far as I know and the names of the sensors aren't even consistent between SMCI platforms, which is annoying.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#47 @KodyKantor

Switch tabs to spaces here and later in this file?

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#47 @rejohnst

Yes - fixed.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#67 @KodyKantor

Why would this value be up to 254 instead of 256?

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#67 @rejohnst

Should actually be 255 (there are up to 256 PCI busses). Fixed.

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#104 @KodyKantor

Am I understanding this correctly in thinking that we're manually defining the last four bays to be NVMe bays? How do we calculate the parent-device 'value' field? What if we stick disks in those bays (the SMCI landing page for this chassis seems to indicate we could do that if we wanted to).

usr/src/lib/fm/topo/maps/SMCI,SYS-2028U-E1CNRT+/SYS-2028U-E1CNRT+-hc-topology.xml#104 @rejohnst

Yes. We calculate the parent-device field by looking at an actual system and seeing what the parent bus device actually looks like. Obviously the assumption here is that these paths will hold true for every instance of this platform model - but that should be the cases.

I wasn't aware that you could plug a non-NVME device into these four slots. I can try it on the box we have when I'm in the office tomorrow. If that does indeed work then I think this code will still be ok, because the ses-enumerator module (invoked at line 100) will find the disk and enumerate it. Later when we run the disk enumerator again for that bay it should bail out - but I will definitely test this case to be sure.

usr/src/lib/fm/topo/modules/common/disk/disk.c#92 @KodyKantor

Shouldn't topo_mod_dprintf be provided a newline? I see it's removed here, but present later in this file. The new disk_nvme.c file doesn't seem to have any newlines in topo_mod_dprintf.

usr/src/lib/fm/topo/modules/common/disk/disk.c#92 @rejohnst

topo_mod_dprintf() calls topo_vdprintf() which will automatically append a newline if the string doesn't already end in one.

usr/src/lib/fm/topo/modules/common/disk/disk_nvme.c#546 @KodyKantor

snapshop/snapshot

usr/src/lib/fm/topo/modules/common/disk/disk_nvme.c#546 @rejohnst

Done

usr/src/lib/fm/topo/modules/common/disk/disk_nvme.c#555 @KodyKantor

Should we have a 'goto out;' here as well?

usr/src/lib/fm/topo/modules/common/disk/disk_nvme.c#555 @rejohnst

Yes - good catch! Fixed.

@rejohnst commented at 2019-10-14T16:50:31

Patch Set 1:

(11 comments)

Thanks again for looking at this!