i2cvr: restrict apply time to OnReset
Applying VR firmware updates with Immediate ApplyTime may trigger
power fluctuations in a running system and lead to unstable or
abnormal behavior. To ensure safe updates, the ApplyTime for VR
devices is restricted to OnReset. This guarantees updates occur
only during a system reset, when power rails are stable.
Change-Id: If7e4bc5d8fb5bb187b3baacd9e6f782f41738bcd
Signed-off-by: Kevin Tung <kevin.tung.openbmc@gmail.com>
diff --git a/i2c-vr/i2cvr_device.cpp b/i2c-vr/i2cvr_device.cpp
index 37c8cad..b9242cc 100644
--- a/i2c-vr/i2cvr_device.cpp
+++ b/i2c-vr/i2cvr_device.cpp
@@ -28,15 +28,6 @@
co_return false;
}
- setUpdateProgress(80);
-
- // NOLINTBEGIN(clang-analyzer-core.uninitialized.Branch)
- if (!(co_await vrInterface->reset()))
- // NOLINTEND(clang-analyzer-core.uninitialized.Branch)
- {
- co_return false;
- }
-
setUpdateProgress(100);
lg2::info("Successfully updated VR {NAME}", "NAME", config.configName);
diff --git a/i2c-vr/i2cvr_device.hpp b/i2c-vr/i2cvr_device.hpp
index 198d1c6..8160a1d 100644
--- a/i2c-vr/i2cvr_device.hpp
+++ b/i2c-vr/i2cvr_device.hpp
@@ -27,8 +27,7 @@
ManagerInf::SoftwareManager* parent) :
DeviceInf::Device(
ctx, config, parent,
- {SDBusPlusSoftware::ApplyTime::RequestedApplyTimes::Immediate,
- SDBusPlusSoftware::ApplyTime::RequestedApplyTimes::OnReset}),
+ {SDBusPlusSoftware::ApplyTime::RequestedApplyTimes::OnReset}),
vrInterface(VRInf::create(ctx, vrType, bus, address))
{}
diff --git a/i2c-vr/i2cvr_software_manager.cpp b/i2c-vr/i2cvr_software_manager.cpp
index 29e198f..691a20d 100644
--- a/i2c-vr/i2cvr_software_manager.cpp
+++ b/i2c-vr/i2cvr_software_manager.cpp
@@ -91,10 +91,7 @@
software->setVersion(std::format("{:X}", sum),
SoftwareInf::SoftwareVersion::VersionPurpose::Other);
- std::set<RequestedApplyTimes> allowedApplyTime = {
- RequestedApplyTimes::Immediate, RequestedApplyTimes::OnReset};
-
- software->enableUpdate(allowedApplyTime);
+ software->enableUpdate({RequestedApplyTimes::OnReset});
i2cDevice->softwareCurrent = std::move(software);
diff --git a/i2c-vr/isl69269/isl69269.cpp b/i2c-vr/isl69269/isl69269.cpp
index dc9cd9e..42fb925 100644
--- a/i2c-vr/isl69269/isl69269.cpp
+++ b/i2c-vr/isl69269/isl69269.cpp
@@ -98,6 +98,12 @@
sdbusplus::async::task<bool> ISL69269::dmaReadWrite(uint8_t* reg, uint8_t* resp)
{
+ if (reg == nullptr || resp == nullptr)
+ {
+ error("dmaReadWrite invalid input");
+ co_return false;
+ }
+
uint8_t tbuf[defaultBufferSize] = {0};
uint8_t tlen = threeByteLen;
uint8_t rbuf[defaultBufferSize] = {0};
@@ -166,6 +172,12 @@
sdbusplus::async::task<bool> ISL69269::getDeviceId(uint32_t* deviceId)
{
+ if (deviceId == nullptr)
+ {
+ error("getDeviceId invalid input");
+ co_return false;
+ }
+
uint8_t tbuf[defaultBufferSize] = {0};
uint8_t tLen = oneByteLen;
uint8_t rbuf[defaultBufferSize] = {0};
@@ -186,6 +198,12 @@
sdbusplus::async::task<bool> ISL69269::getDeviceRevision(uint32_t* revision)
{
+ if (revision == nullptr)
+ {
+ error("getDeviceRevision invalid input");
+ co_return false;
+ }
+
uint8_t tbuf[defaultBufferSize] = {0};
uint8_t tlen = oneByteLen;
uint8_t rbuf[defaultBufferSize] = {0};
@@ -383,6 +401,12 @@
}
}
+ if (!(co_await getProgStatus()))
+ {
+ error("program failed at getProgStatus");
+ co_return false;
+ }
+
co_return true;
}
@@ -576,46 +600,6 @@
co_return true;
}
-sdbusplus::async::task<bool> ISL69269::reset()
-{
- bool ret = true;
- // NOLINTBEGIN(clang-analyzer-core.uninitialized.Branch)
- ret = co_await getProgStatus();
- // NOLINTEND(clang-analyzer-core.uninitialized.Branch)
- if (!ret)
- {
- error("reset failed at getProgStatus");
- }
-
- ret = co_await restoreCfg();
- if (!ret)
- {
- error("reset failed at restoreCfg");
- }
-
- // Reset configuration for next update.
- configuration.addr = 0;
- configuration.mode = 0;
- configuration.cfgId = 0;
- configuration.devIdExp = 0;
- configuration.devRevExp = 0;
- configuration.crcExp = 0x00;
- for (int i = 0; i < configuration.wrCnt; i++)
- {
- configuration.pData[i].pec = 0x00;
- configuration.pData[i].addr = 0x00;
- configuration.pData[i].cmd = 0x00;
- for (int j = 0; j < configuration.pData[i].len; j++)
- {
- configuration.pData[i].data[j] = 0x00;
- }
- configuration.pData[i].len = 0x00;
- }
- configuration.wrCnt = 0;
-
- co_return ret;
-}
-
bool ISL69269::forcedUpdateAllowed()
{
return true;
@@ -624,7 +608,9 @@
sdbusplus::async::task<bool> ISL69269::updateFirmware(bool force)
{
(void)force;
+ // NOLINTBEGIN(clang-analyzer-core.uninitialized.Branch)
if (!(co_await program()))
+ // NOLINTEND(clang-analyzer-core.uninitialized.Branch)
{
error("programing ISL69269 failed");
co_return false;
diff --git a/i2c-vr/isl69269/isl69269.hpp b/i2c-vr/isl69269/isl69269.hpp
index 986c65d..789bc34 100644
--- a/i2c-vr/isl69269/isl69269.hpp
+++ b/i2c-vr/isl69269/isl69269.hpp
@@ -20,7 +20,6 @@
sdbusplus::async::task<bool> updateFirmware(bool force) final;
sdbusplus::async::task<bool> getCRC(uint32_t* checksum) final;
- sdbusplus::async::task<bool> reset() final;
bool forcedUpdateAllowed() final;
diff --git a/i2c-vr/vr.hpp b/i2c-vr/vr.hpp
index 688b37b..1212a39 100644
--- a/i2c-vr/vr.hpp
+++ b/i2c-vr/vr.hpp
@@ -36,10 +36,6 @@
// @return sdbusplus::async::task<bool> true indicates success.
virtual sdbusplus::async::task<bool> updateFirmware(bool force) = 0;
- // @brief resets the voltage regulator for the update to take effect.
- // @return sdbusplus::async::task<bool> true indicates success.
- virtual sdbusplus::async::task<bool> reset() = 0;
-
// @brief Requests the CRC value of the voltage regulator over I2C.
// @param pointer to write the result to.
// @returns < 0 on error
diff --git a/i2c-vr/xdpe1x2xx/xdpe1x2xx.cpp b/i2c-vr/xdpe1x2xx/xdpe1x2xx.cpp
index 51e0352..a2d919b 100644
--- a/i2c-vr/xdpe1x2xx/xdpe1x2xx.cpp
+++ b/i2c-vr/xdpe1x2xx/xdpe1x2xx.cpp
@@ -40,7 +40,6 @@
constexpr uint8_t IFXMFRRegWrite = 0xDE;
constexpr uint8_t IFXMFRFwCmdData = 0xFD;
constexpr uint8_t IFXMFRFwCmd = 0xFE;
-constexpr uint8_t MFRFwCmdReset = 0x0e;
constexpr uint8_t MFRFwCmdRmng = 0x10;
constexpr uint8_t MFRFwCmdGetHWAddress = 0x2E;
constexpr uint8_t MFRFwCmdOTPConfSTO = 0x11;
@@ -56,7 +55,6 @@
constexpr uint16_t MFRGetHWAddressWaitTime = 5;
constexpr uint16_t MFROTPFileInvalidationWaitTime = 100;
constexpr uint16_t MFRSectionInvalidationWaitTime = 4;
-constexpr uint16_t VRResetDelay = 500;
constexpr uint32_t CRC32Poly = 0xEDB88320;
@@ -721,17 +719,6 @@
co_return true;
}
-sdbusplus::async::task<bool> XDPE1X2XX::reset()
-{
- if (!(co_await mfrFWcmd(MFRFwCmdReset, VRResetDelay, NULL, NULL)))
- {
- error("Failed to reset the VR");
- co_return false;
- }
-
- co_return true;
-}
-
uint32_t XDPE1X2XX::calcCRC32(const uint32_t* data, int len)
{
if (data == NULL)
diff --git a/i2c-vr/xdpe1x2xx/xdpe1x2xx.hpp b/i2c-vr/xdpe1x2xx/xdpe1x2xx.hpp
index 56944f5..6c38667 100644
--- a/i2c-vr/xdpe1x2xx/xdpe1x2xx.hpp
+++ b/i2c-vr/xdpe1x2xx/xdpe1x2xx.hpp
@@ -19,7 +19,6 @@
size_t imageSize) final;
sdbusplus::async::task<bool> updateFirmware(bool force) final;
- sdbusplus::async::task<bool> reset() final;
sdbusplus::async::task<bool> getCRC(uint32_t* checksum) final;
bool forcedUpdateAllowed() final;