openbmc / phosphor-host-ipmid

dbus-based ipmid for host-endpoint IPMI commands
Apache License 2.0
37 stars 74 forks source link

Extra SDR Entries With Dynamic Sensors #186

Open iwoloschin opened 2 years ago

iwoloschin commented 2 years ago

I am using phosphor-host-ipmid at SRCREV = "a23af1206bc4c835516909c87c71be0e7428264c" (this is slightly out of date), when I run ipmitool commands, such as ipmitool sdr or ipmitool fru on the host or BMC I get something like this:

Get SDR 004b command failed: Invalid data field in request
Get SDR 004b command failed: Invalid data field in request
Get SDR 004b command failed: Invalid data field in request
Get SDR 004b command failed: Invalid data field in request
Get SDR 004b command failed: Invalid data field in request

I can recreate this in qemu, less sensors so the number is lower.

~# ipmitool sdr
CPU              | no reading        | ns
Memory           | no reading        | ns
Storage RW       | no reading        | ns
Get SDR 0005 command failed: Invalid data field in request
Get SDR 0005 command failed: Invalid data field in request
Get SDR 0005 command failed: Invalid data field in request
Get SDR 0005 command failed: Invalid data field in request
Get SDR 0005 command failed: Invalid data field in request

The error is printed to stderr so it is pretty easy to ignore, but it winds up causing ipmitool commands to retry and bunch and take a lot longer to exit than they should. It seems as if ipmitool commands are exiting cleanly, despite the error, but in this case that is actually good as all of the expected data is present.

I unfortunately cannot share source, but I believe this can be recreated by simply enabling dynamic sensors.

iwoloschin commented 2 years ago

This was resolved by @zevweiss by adding the following snippet to phosphor-ipmi-config.bbappend:

do_install:append() {
    # the default dummy data provided for this file causes breakage in
    # Get SDR commands.  Clearing out the provided entity-map entirely
    # avoids the issue.
    echo "[]" > ${D}${datadir}/ipmi-providers/entity-map.json
}

I'll leave this open for maintainers to decide whether or not that is a sufficient fix.

pointbazaar commented 1 year ago
diff --git a/dbus-sdr/sensorcommands.cpp b/dbus-sdr/sensorcommands.cpp
index e48cd91..5877e66 100644
--- a/dbus-sdr/sensorcommands.cpp
+++ b/dbus-sdr/sensorcommands.cpp
@@ -2413,14 +2413,8 @@ ipmi::RspType<uint16_t,            // next record ID
         return ipmi::response(ret);
     }

-    const auto& entityRecords =
-        ipmi::sensor::EntityInfoMapContainer::getContainer()
-            ->getIpmiEntityRecords();
-    int entityCount = entityRecords.size();
-
     auto& sensorTree = getSensorTree();
-    size_t lastRecord = getNumberOfSensors() + fruCount +
-                        ipmi::storage::type12Count + entityCount - 1;
+    size_t lastRecord = getNumberOfSensors() + fruCount - 1;
     uint16_t nextRecordId = lastRecord > recordID ? recordID + 1 : 0XFFFF;

     if (!getSensorSubtree(sensorTree) && sensorTree.empty())
-- 
2.41.0

what i have. probably breaks something but atleast sensors + fru devices are there. above fix is probably better.