Fix Get and Set SM PWM Signal
On systems with multiple fan controllers, Index value is only unique
within the same controller. This means that Index alone can not be used
to correctly identify a fan sensor. This commit fixes this defect by
specifically searching for the "Fan_#" Entity Manager object where # is
the fan being accessed.
The commit which introduced this issue can be found here:
https://gerrit.openbmc.org/c/openbmc/intel-ipmi-oem/+/56909
This commit also replaces phosphor-logging log with lg2.
Testing:
1. System with multiple fan controllers that have duplicated index
property:
busctl introspect xyz.openbmc_project.EntityManager
/xyz/openbmc_project/inventory/system/board/XXXXXXXX/Fan_7
xyz.openbmc_project.Configuration.I2CFan interface
.Address property t
47
.BindConnector property s
"System Fan connector 7"
.Bus property t
6
.Index property t
0
busctl introspect xyz.openbmc_project.EntityManager
/xyz/openbmc_project/inventory/system/board/XXXXXXXX/Fan_1
xyz.openbmc_project.Configuration.I2CFan interface
.Address property t
44
.BindConnector property s
System Fan connector 1"
.Bus property t
6
.Index property t
0
Without fix:
ipmitool raw 0x30 0x14 0x0d 0 0x00
3c
ipmitool raw 0x30 0x14 0x0d 6 0x00
Unable to send RAW command (channel=0x0 netfn=0x30 lun=0x0 cmd=0x14
rsp=0xcc): Invalid data field in request.
With fix:
ipmitool raw 0x30 0x14 0x0d 0 0x00
3c
ipmitool raw 0x30 0x14 0x0d 6 0x00
3c
ipmitool raw 0x30 0x15 0x05 0 0x1 0x40
ipmitool raw 0x30 0x15 0x05 6 0x1 0x46
ipmitool raw 0x30 0x14 0x0d 0 0x00
40
ipmitool raw 0x30 0x14 0x0d 6 0x00
46
2. System with one pwm port controlling two fans:
busctl introspect xyz.openbmc_project.EntityManager
/xyz/openbmc_project/inventory/system/board/XXXXXXXX/Fan_1
xyz.openbmc_project.Configuration.I2CFan.Connector interface
.Name property s
"System Fan connector 1" emits-change
.Pwm property t
0 emits-change
.PwmName property s
"Pwm_1_2" emits-change
busctl introspect xyz.openbmc_project.EntityManager
/xyz/openbmc_project/inventory/system/board/XXXXXXXX/Fan_2
xyz.openbmc_project.Configuration.I2CFan.Connector interface
.Name property s
"System Fan connector 2" emits-change
.Pwm property t
0 emits-change
.PwmName property s
"Pwm_1_2" emits-change
ipmitool raw 0x30 0x14 0x0d 0 0x00
3c
ipmitool raw 0x31 0x14 0x0d 1 0x00
3c
ipmitool raw 0x30 0x15 0x05 0 0x1 0x40
ipmitool raw 0x30 0x14 0x0d 0 0x00
40
ipmitool raw 0x31 0x14 0x0d 1 0x00
40
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
Change-Id: I8e2323c4572ed21c4e2f58d282574e65270bf60c
diff --git a/src/manufacturingcommands.cpp b/src/manufacturingcommands.cpp
index 4563662..6376eee 100644
--- a/src/manufacturingcommands.cpp
+++ b/src/manufacturingcommands.cpp
@@ -21,6 +21,7 @@
#include <ipmid/api.hpp>
#include <manufacturingcommands.hpp>
#include <oemcommands.hpp>
+#include <phosphor-logging/lg2.hpp>
#include <types.hpp>
#include <filesystem>
@@ -67,8 +68,7 @@
"ResetTimer");
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Failed to reset the manufacturing mode timer");
+ lg2::error("Failed to reset the manufacturing mode timer");
return ccUnspecifiedError;
}
return ccSuccess;
@@ -236,8 +236,7 @@
}
catch (const sdbusplus::exception_t& e)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "ERROR: getProperty");
+ lg2::info("ERROR: getProperty");
return -1;
}
@@ -257,8 +256,7 @@
}
catch (const sdbusplus::exception_t& e)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "ERROR: setProperty");
+ lg2::info("ERROR: setProperty");
return -1;
}
@@ -278,8 +276,7 @@
}
catch (const sdbusplus::exception_t& e)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "ERROR: phosphor-pid-control service start or stop failed");
+ lg2::info("ERROR: phosphor-pid-control service start or stop failed");
return -1;
}
return 0;
@@ -296,9 +293,7 @@
"/xyz/openbmc_project/inventory", obj);
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "GetMangagedObjects failed",
- phosphor::logging::entry("ERROR=%s", ec.message().c_str()));
+ lg2::error("GetMangagedObjects failed", "ERROR", ec.message().c_str());
return false;
}
for (const auto& [path, objData] : obj)
@@ -310,14 +305,12 @@
intf == "xyz.openbmc_project.Configuration.I2CFan" ||
intf == "xyz.openbmc_project.Configuration.NuvotonFan")
{
- auto findIndex = propMap.find("Index");
- if (findIndex == propMap.end())
- {
- continue;
- }
+ std::string fanPath = "/Fan_";
- auto fanIndex = std::get_if<uint64_t>(&findIndex->second);
- if (!fanIndex || *fanIndex != instance)
+ fanPath += std::to_string(instance);
+ std::string objPath = path.str;
+ objPath = objPath.substr(objPath.find_last_of("/"));
+ if (objPath != fanPath)
{
continue;
}
@@ -333,8 +326,7 @@
std::get_if<std::string>(&findPwmName->second);
if (!fanPwmName)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "PwmName parse ERROR.");
+ lg2::error("PwmName parse ERROR.");
return false;
}
pwmName = *fanPwmName;
@@ -410,7 +402,7 @@
{
ipmi::Value reply;
std::string pwmName, fullPath;
- if (!findPwmName(ctx, instance, pwmName))
+ if (!findPwmName(ctx, instance + 1, pwmName))
{
// The default PWM name is Pwm_#
pwmName = "Pwm_" + std::to_string(instance + 1);
@@ -445,8 +437,7 @@
fanTachBasePath, 0, std::array<const char*, 1>{fanIntf});
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Failed to query fan tach sub tree objects");
+ lg2::error("Failed to query fan tach sub tree objects");
return ipmi::responseUnspecifiedError();
}
if (instance >= flatMap.size())
@@ -497,13 +488,11 @@
switch (action)
{
case SmActionGet::sample:
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionGet::sample");
+ lg2::info("case SmActionGet::sample");
break;
case SmActionGet::ignore:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionGet::ignore");
+ lg2::info("case SmActionGet::ignore");
if (mtm.setProperty(buttonService, path, buttonIntf,
"ButtonMasked", true) < 0)
{
@@ -513,8 +502,7 @@
break;
case SmActionGet::revert:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionGet::revert");
+ lg2::info("case SmActionGet::revert");
if (mtm.setProperty(buttonService, path, buttonIntf,
"ButtonMasked", false) < 0)
{
@@ -588,8 +576,7 @@
{
case SmActionSet::forceDeasserted:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionSet::forceDeasserted");
+ lg2::info("case SmActionSet::forceDeasserted");
retCode = ledStoreAndSet(signalType, std::string("Off"));
if (retCode != ccSuccess)
@@ -601,8 +588,7 @@
break;
case SmActionSet::forceAsserted:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionSet::forceAsserted");
+ lg2::info("case SmActionSet::forceAsserted");
retCode = ledStoreAndSet(signalType, std::string("On"));
if (retCode != ccSuccess)
@@ -626,8 +612,7 @@
break;
case SmActionSet::revert:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "case SmActionSet::revert");
+ lg2::info("case SmActionSet::revert");
retCode = ledRevert(signalType);
}
break;
@@ -682,7 +667,7 @@
}
mtm.revertTimer.start(revertTimeOut);
std::string pwmName, fanPwmInstancePath;
- if (!findPwmName(ctx, instance, pwmName))
+ if (!findPwmName(ctx, instance + 1, pwmName))
{
pwmName = "Pwm_" + std::to_string(instance + 1);
}
@@ -705,10 +690,8 @@
break;
case SmSignalSet::smSpeaker:
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "Performing Speaker SmActionSet",
- phosphor::logging::entry("ACTION=%d",
- static_cast<uint8_t>(action)));
+ lg2::info("Performing Speaker SmActionSet", "ACTION", lg2::dec,
+ static_cast<uint8_t>(action));
switch (action)
{
case SmActionSet::forceAsserted:
@@ -716,8 +699,7 @@
char beepDevName[] = "/dev/input/event0";
if (mtm.mtmTestBeepFd != -1)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "mtm beep device is opened already!");
+ lg2::info("mtm beep device is opened already!");
// returning success as already beep is in progress
return ipmi::response(retCode);
}
@@ -725,8 +707,7 @@
if ((mtm.mtmTestBeepFd =
::open(beepDevName, O_RDWR | O_CLOEXEC)) < 0)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Failed to open input device");
+ lg2::error("Failed to open input device");
return ipmi::responseUnspecifiedError();
}
@@ -739,8 +720,7 @@
sizeof(struct input_event)) !=
sizeof(struct input_event))
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Failed to write a tone sound event");
+ lg2::error("Failed to write a tone sound event");
::close(mtm.mtmTestBeepFd);
mtm.mtmTestBeepFd = -1;
return ipmi::responseUnspecifiedError();
@@ -783,8 +763,7 @@
driveBasePath, 0, std::array<const char*, 1>{driveLedIntf});
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Failed to query HSBP drive sub tree objects");
+ lg2::error("Failed to query HSBP drive sub tree objects");
return ipmi::responseUnspecifiedError();
}
std::string driveObjPath =
@@ -919,9 +898,7 @@
"/xyz/openbmc_project/inventory", obj);
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "GetMangagedObjects failed",
- phosphor::logging::entry("ERROR=%s", ec.message().c_str()));
+ lg2::error("GetManagedObjects failed", "ERROR", ec.message().c_str());
return false;
}
@@ -946,9 +923,8 @@
std::get_if<uint64_t>(&findMacOffset->second);
if (!fruBus || !fruAddress || !macFruOffset)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "ERROR: MotherBoard FRU config data type invalid, not "
- "used");
+ lg2::info("ERROR: MotherBoard FRU config data type "
+ "invalid, not used");
return false;
}
busNum = *fruBus;
@@ -987,11 +963,9 @@
if (findFruDevice(ctx, macOffset, fruBus, fruAddress))
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "Found mac fru",
- phosphor::logging::entry("BUS=%d", static_cast<uint8_t>(fruBus)),
- phosphor::logging::entry("ADDRESS=%d",
- static_cast<uint8_t>(fruAddress)));
+ lg2::info("Found mac fru", "BUS", lg2::dec,
+ static_cast<uint8_t>(fruBus), "ADDRESS", lg2::dec,
+ static_cast<uint8_t>(fruAddress));
if (macOffset % fruPageSize)
{
@@ -1000,8 +974,7 @@
macOffset += macIndex * fruPageSize;
if ((macOffset + macRecordSize) > fruEnd)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "ERROR: read fru mac failed, offset invalid");
+ lg2::error("ERROR: read fru mac failed, offset invalid");
return false;
}
std::vector<uint8_t> writeData;
@@ -1032,11 +1005,9 @@
if (findFruDevice(ctx, macOffset, fruBus, fruAddress))
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "Found mac fru",
- phosphor::logging::entry("BUS=%d", static_cast<uint8_t>(fruBus)),
- phosphor::logging::entry("ADDRESS=%d",
- static_cast<uint8_t>(fruAddress)));
+ lg2::info("Found mac fru", "BUS", lg2::dec,
+ static_cast<uint8_t>(fruBus), "ADDRESS", lg2::dec,
+ static_cast<uint8_t>(fruAddress));
if (macOffset % fruPageSize)
{
@@ -1045,8 +1016,7 @@
macOffset += macIndex * fruPageSize;
if ((macOffset + macRecordSize) > fruEnd)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "ERROR: write mac fru failed, offset invalid.");
+ lg2::error("ERROR: write mac fru failed, offset invalid.");
return ipmi::ccParmOutOfRange;
}
std::vector<uint8_t> writeData;
@@ -1090,25 +1060,22 @@
{
return ipmi::ccSuccess;
}
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "INFO: write mac fru verify failed, fru may be write "
- "protected.");
+ lg2::info("INFO: write mac fru verify failed, fru may be "
+ "write protected.");
}
return ipmi::ccCommandNotAvailable;
default:
if (ipmi::i2cWriteRead(i2cBus, fruAddress, writeData,
readBuf) == ipmi::ccSuccess)
{
- phosphor::logging::log<phosphor::logging::level::INFO>(
- "INFO: write mac fru failed, but successfully read "
- "from fru, fru may be write protected.");
+ lg2::info("INFO: write mac fru failed, but successfully "
+ "read from fru, fru may be write protected.");
return ipmi::ccCommandNotAvailable;
}
else // assume failure is due to no eeprom on board
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "ERROR: write mac fru failed, assume no eeprom is "
- "available.");
+ lg2::error("ERROR: write mac fru failed, assume no eeprom "
+ "is available.");
}
break;
}
@@ -1259,8 +1226,7 @@
}
else
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Master write read command: Cannot get BusID");
+ lg2::error("Master write read command: Cannot get BusID");
return ipmi::responseInvalidFieldRequest();
}
}
@@ -1272,8 +1238,7 @@
}
else
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Master write read command: invalid request");
+ lg2::error("Master write read command: invalid request");
return ipmi::responseInvalidFieldRequest();
}
@@ -1289,15 +1254,13 @@
if (readCount > slotI2CMaxReadSize)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Master write read command: Read count exceeds limit");
+ lg2::error("Master write read command: Read count exceeds limit");
return ipmi::responseParmOutOfRange();
}
if (!readCount && !writeCount)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(
- "Master write read command: Read & write count are 0");
+ lg2::error("Master write read command: Read & write count are 0");
return ipmi::responseInvalidFieldRequest();
}
@@ -1423,16 +1386,14 @@
}
break;
default:
- phosphor::logging::log<phosphor::logging::level::WARNING>(
- "ERROR: Invalid feature action selected",
- phosphor::logging::entry("ACTION=%d", enable));
+ lg2::warning("ERROR: Invalid feature action selected", "ACTION",
+ lg2::dec, enable);
return ipmi::responseInvalidFieldRequest();
}
if (ec)
{
- phosphor::logging::log<phosphor::logging::level::WARNING>(
- "ERROR: Service start or stop failed",
- phosphor::logging::entry("SERVICE=%s", serviceName.c_str()));
+ lg2::warning("ERROR: Service start or stop failed", "SERVICE",
+ serviceName.c_str());
return ipmi::responseUnspecifiedError();
}
return ipmi::responseSuccess();
@@ -1460,7 +1421,7 @@
}
catch (const std::exception& e)
{
- phosphor::logging::log<phosphor::logging::level::ERR>(e.what());
+ lg2::error(e.what());
return ipmi::responseUnspecifiedError();
}