fw-update: make percentage reporting more granular
This makes percentage completion reporting more granular.
Previously issues were seen around devices that took
longer than five minutes to update, since xfering the firmware
wasn't counted as progress. This adds a new class
UpdateProgress, which tracks the stages of update,
including how far into a transfer we are
Tested: loaded onto nvl32-obmc and performed a full fwpkg update
The percentage complete increases monotonically and doesn't
time out as it did previously for updates that take longer
than 5 min
```
{
"@odata.id": "/redfish/v1/TaskService/Tasks/0",
"@odata.type": "#Task.v1_4_3.Task",
"EndTime": "2026-01-06T21:49:01+00:00",
"HidePayload": false,
"Id": "0",
"Messages": [
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has started.",
"MessageArgs": [
"0"
],
"MessageId": "TaskEvent.1.0.TaskStarted",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 1 percent complete.",
"MessageArgs": [
"0",
"1"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 2 percent complete.",
"MessageArgs": [
"0",
"2"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 3 percent complete.",
"MessageArgs": [
"0",
"3"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 4 percent complete.",
"MessageArgs": [
"0",
"4"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 5 percent complete.",
"MessageArgs": [
"0",
"5"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 6 percent complete.",
"MessageArgs": [
"0",
"6"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 7 percent complete.",
"MessageArgs": [
"0",
"7"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
...
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has changed to progress 100 percent complete.",
"MessageArgs": [
"0",
"100"
],
"MessageId": "TaskEvent.1.0.TaskProgressChanged",
"MessageSeverity": "OK",
"Resolution": "None."
},
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The task with Id '0' has completed.",
"MessageArgs": [
"0"
],
"MessageId": "TaskEvent.1.0.TaskCompletedOK",
"MessageSeverity": "OK",
"Resolution": "None."
}
],
```
Change-Id: Ib2d5b6b9e2dbf025fd1391a5ba63c78937186122
Signed-off-by: Marc Olberding <molberding@nvidia.com>
diff --git a/fw-update/device_updater.cpp b/fw-update/device_updater.cpp
index 0033a13..0cf58e2 100644
--- a/fw-update/device_updater.cpp
+++ b/fw-update/device_updater.cpp
@@ -17,6 +17,101 @@
namespace fw_update
{
+// The highest percentage that component transfer can give
+static constexpr uint8_t componentXferProgressPercent = 97;
+static constexpr uint8_t componentVerifyProgressPercent = 98;
+static constexpr uint8_t componentApplyProgressPercent = 99;
+static constexpr uint8_t firmwareActivationProgressPercent = 100;
+
+UpdateProgress::UpdateProgress(uint32_t totalSize, mctp_eid_t eid) :
+ progress{}, eid{eid}, totalSize{totalSize}, totalUpdated{},
+ currentState{state::Update}
+{}
+
+uint32_t UpdateProgress::getTotalSize() const
+{
+ return totalSize;
+}
+uint8_t UpdateProgress::getProgress() const
+{
+ return progress;
+}
+
+void UpdateProgress::updateState(state newState)
+{
+ switch (newState)
+ {
+ case state::Update:
+ break;
+ case state::Verify:
+ progress = componentVerifyProgressPercent;
+ break;
+ case state::Apply:
+ progress = componentApplyProgressPercent;
+ break;
+ default:
+ warning("Invalid state {STATE} provided", "STATE", newState);
+ return;
+ };
+ currentState = newState;
+}
+
+void UpdateProgress::reportFwUpdate(uint32_t amountUpdated)
+{
+ if (currentState != state::Update)
+ {
+ error("invalid state when updating progress: eid {EID}", "EID", eid);
+ return;
+ }
+
+ if (amountUpdated > totalSize)
+ {
+ error("invalid size when updating progress: eid {EID}", "EID", eid);
+ return;
+ }
+
+ if (amountUpdated + totalUpdated > totalSize)
+ {
+ error("invalid total size when updating progress: eid {EID}", "EID",
+ eid);
+ return;
+ }
+
+ if (totalSize == 0)
+ {
+ error("invalid package size when updating progress: eid {EID}", "EID",
+ eid);
+ return;
+ }
+
+ totalUpdated += amountUpdated;
+ progress = static_cast<uint8_t>(std::floor(
+ 1.0 * componentXferProgressPercent * totalUpdated / totalSize));
+ return;
+}
+
+DeviceUpdater::DeviceUpdater(
+ mctp_eid_t eid, std::istream& package,
+ const FirmwareDeviceIDRecord& fwDeviceIDRecord,
+ const ComponentImageInfos& compImageInfos, const ComponentInfo& compInfo,
+ uint32_t maxTransferSize, UpdateManager* updateManager) :
+ eid(eid), package(package), fwDeviceIDRecord(fwDeviceIDRecord),
+ compImageInfos(compImageInfos), compInfo(compInfo),
+ maxTransferSize(maxTransferSize), updateManager(updateManager),
+ activationComplete{false}
+{
+ const auto& applicableComponents =
+ std::get<ApplicableComponents>(fwDeviceIDRecord);
+ // create as many progress objects as there are components
+ // and initialize them with the size of each component
+ for (const auto& applicableComponent : applicableComponents)
+ {
+ const auto& componentSize =
+ std::get<6>(compImageInfos[applicableComponent]);
+ progress.emplace_back(componentSize, eid);
+ }
+}
+
void DeviceUpdater::startFwUpdateFlow()
{
auto instanceIdResult = updateManager->instanceIdDb.next(eid);
@@ -468,6 +563,17 @@
padBytes = offset + length - compSize;
}
+ if (componentIndex < progress.size())
+ {
+ // technically this should never happen, as its an invariant
+ // but check to prefer a failed fw update over a pldm crash
+ progress[componentIndex].reportFwUpdate(length);
+ if (updateManager != nullptr)
+ {
+ updateManager->updateActivationProgress();
+ }
+ }
+
response.resize(sizeof(pldm_msg_hdr) + sizeof(completionCode) + length);
responseMsg = new (response.data()) pldm_msg;
package.seekg(compOffset + offset);
@@ -608,7 +714,14 @@
std::get<ApplicableComponents>(fwDeviceIDRecord);
const auto& comp = compImageInfos[applicableComponents[componentIndex]];
const auto& compVersion = std::get<7>(comp);
-
+ if (componentIndex < progress.size())
+ {
+ progress[componentIndex].updateState(UpdateProgress::state::Verify);
+ if (updateManager != nullptr)
+ {
+ updateManager->updateActivationProgress();
+ }
+ }
if (verifyResult == PLDM_FWUP_VERIFY_SUCCESS)
{
info(
@@ -678,24 +791,38 @@
info(
"Component endpoint ID '{EID}' with '{COMPONENT_VERSION}' apply complete.",
"EID", eid, "COMPONENT_VERSION", compVersion);
- updateManager->updateActivationProgress();
+ if (componentIndex < progress.size())
+ {
+ progress[componentIndex].updateState(UpdateProgress::state::Apply);
+ if (updateManager != nullptr)
+ {
+ updateManager->updateActivationProgress();
+ }
+ }
if (componentIndex == applicableComponents.size() - 1)
{
componentIndex = 0;
componentUpdateStatus.clear();
componentUpdateStatus[componentIndex] = true;
- pldmRequest = std::make_unique<sdeventplus::source::Defer>(
- updateManager->event,
- std::bind(&DeviceUpdater::sendActivateFirmwareRequest, this));
+ if (updateManager != nullptr)
+ {
+ pldmRequest = std::make_unique<sdeventplus::source::Defer>(
+ updateManager->event,
+ std::bind(&DeviceUpdater::sendActivateFirmwareRequest,
+ this));
+ }
}
else
{
componentIndex++;
componentUpdateStatus[componentIndex] = true;
- pldmRequest = std::make_unique<sdeventplus::source::Defer>(
- updateManager->event,
- std::bind(&DeviceUpdater::sendUpdateComponentRequest, this,
- componentIndex));
+ if (updateManager != nullptr)
+ {
+ pldmRequest = std::make_unique<sdeventplus::source::Defer>(
+ updateManager->event,
+ std::bind(&DeviceUpdater::sendUpdateComponentRequest, this,
+ componentIndex));
+ }
}
}
else
@@ -703,7 +830,10 @@
error(
"Failed to apply component endpoint ID '{EID}' and version '{COMPONENT_VERSION}', error - {ERROR}",
"EID", eid, "COMPONENT_VERSION", compVersion, "ERROR", applyResult);
- updateManager->updateDeviceCompletion(eid, false);
+ if (updateManager != nullptr)
+ {
+ updateManager->updateDeviceCompletion(eid, false);
+ }
componentUpdateStatus[componentIndex] = false;
sendCancelUpdateComponentRequest();
}
@@ -794,6 +924,13 @@
return;
}
+ activationComplete = true;
+ if (updateManager == nullptr)
+ {
+ return;
+ }
+
+ updateManager->updateActivationProgress();
updateManager->updateDeviceCompletion(eid, true);
}
@@ -901,6 +1038,39 @@
return;
}
+uint8_t DeviceUpdater::getProgress() const
+{
+ if (progress.empty())
+ {
+ return 0;
+ }
+
+ if (activationComplete)
+ {
+ return firmwareActivationProgressPercent;
+ }
+
+ uint64_t totalSize = 0;
+ uint64_t weightedProgress = 0;
+ for (const auto& el : progress)
+ {
+ totalSize += el.getTotalSize();
+ weightedProgress +=
+ el.getTotalSize() * static_cast<uint64_t>(el.getProgress());
+ }
+
+ if (totalSize == 0)
+ {
+ error("total component size is 0 on eid {EID}", "EID", eid);
+ return 0;
+ }
+
+ const double percentage =
+ static_cast<double>(weightedProgress) / static_cast<double>(totalSize);
+
+ return static_cast<uint8_t>(std::floor(percentage));
+}
+
} // namespace fw_update
} // namespace pldm
diff --git a/fw-update/device_updater.hpp b/fw-update/device_updater.hpp
index 790db76..9738791 100644
--- a/fw-update/device_updater.hpp
+++ b/fw-update/device_updater.hpp
@@ -9,6 +9,7 @@
#include <sdeventplus/source/event.hpp>
#include <fstream>
+#include <numeric>
namespace pldm
{
@@ -24,6 +25,89 @@
class UpdateManager;
+/** @class UpdateProgress
+ *
+ * Attempts to provide accurate reporting of firmware update progress
+ * Called at the various phases of firmware update.
+ * A single UpdateProgress object represents a component of a pldm package
+ */
+class UpdateProgress
+{
+ public:
+ /** @brief an enum to define the different states of progress
+ *
+ */
+ enum class state
+ {
+ Update,
+ Verify,
+ Apply,
+ };
+
+ /** @brief Construct an UpdateProgress object given to the total component
+ * size
+ *
+ * @param[in] totalSize the size in bytes of a component for a given device
+ * @param[in] eid the eid of the device this class tracks progress for
+ */
+ UpdateProgress(uint32_t totalSize, mctp_eid_t eid);
+ ~UpdateProgress() = default;
+ UpdateProgress& operator=(UpdateProgress&&) = default;
+ UpdateProgress& operator=(const UpdateProgress&) = delete;
+ UpdateProgress(UpdateProgress&&) = default;
+ UpdateProgress(const UpdateProgress&) = delete;
+
+ /** @brief Called when switching between phases of firmware update
+ *
+ * @param[in] newState the state that we are entering. See
+ * UpdateProgress::state
+ * @return true on success, false on failure
+ */
+ void updateState(state newState);
+
+ /** @brief called when servicing RequestFirmwareData commands
+ *
+ * @param[in] amountUpdate the length of firmware transmitted, in bytes
+ * @return true on success, false on failure
+ */
+ void reportFwUpdate(uint32_t amountUpdated);
+
+ /** @brief get the progress as a percentage of this component
+ *
+ * @return the progress of this component as an int in [0-100]
+ */
+ uint8_t getProgress() const;
+
+ /** @brief get the total size of the firmware component in bytes
+ *
+ * @return the total number of bytes for the component
+ */
+ uint32_t getTotalSize() const;
+
+ private:
+ /**
+ * @brief progress of firmware update in percentage [0-100]
+ */
+ uint8_t progress;
+
+ /**
+ * @brief the eid of the device this object tracks
+ */
+ mctp_eid_t eid;
+ /**
+ * @brief the total size in bytes of this component
+ */
+ uint32_t totalSize;
+ /**
+ * @brief the total number of bytes sent
+ */
+ uint32_t totalUpdated;
+ /**
+ * @brief the current state of update for this component
+ */
+ state currentState;
+};
+
/** @class DeviceUpdater
*
* DeviceUpdater orchestrates the firmware update of the firmware device and
@@ -59,11 +143,16 @@
const ComponentImageInfos& compImageInfos,
const ComponentInfo& compInfo,
uint32_t maxTransferSize,
- UpdateManager* updateManager) :
- eid(eid), package(package), fwDeviceIDRecord(fwDeviceIDRecord),
- compImageInfos(compImageInfos), compInfo(compInfo),
- maxTransferSize(maxTransferSize), updateManager(updateManager)
- {}
+ UpdateManager* updateManager);
+
+ /** @brief Get the progress of updating this device as percentage
+ *
+ * Goes through each component for this device and calculates
+ * the percentage we are through the update process
+ *
+ * @return The percentage as an int [0-100], 0 is no progress, 100 is done.
+ */
+ uint8_t getProgress() const;
/** @brief Start the firmware update flow for the FD
*
@@ -252,6 +341,17 @@
*
*/
std::unique_ptr<sdbusplus::Timer> reqFwDataTimer;
+ /**
+ * @brief a list of UpdateProgress objects, one for each firmware component
+ * applicable to this device
+ */
+ std::vector<UpdateProgress> progress;
+ /**
+ * @brief Whether this device has gone through application. Needed because
+ * UpdateProgress handles each component but application happens at
+ * the device level
+ */
+ bool activationComplete;
};
} // namespace fw_update
diff --git a/fw-update/test/device_updater_test.cpp b/fw-update/test/device_updater_test.cpp
index 6190953..8937b9f 100644
--- a/fw-update/test/device_updater_test.cpp
+++ b/fw-update/test/device_updater_test.cpp
@@ -134,4 +134,54 @@
0xC4, 0x12, 0xD7, 0x3C, 0x65, 0x6C, 0xE1, 0x5A, 0x32, 0xAA, 0x0B, 0xA3,
0xA2, 0x72, 0x33, 0x00, 0x3C, 0x7E, 0x28, 0x36, 0x10, 0x90, 0x38, 0xFB};
EXPECT_EQ(response, compFirst512B);
+ EXPECT_EQ(deviceUpdater.getProgress(), 48);
+}
+
+TEST_F(DeviceUpdaterTest, FullUpdateProgress)
+{
+ DeviceUpdater deviceUpdater(0, package, fwDeviceIDRecord, compImageInfos,
+ compInfo, 512, nullptr);
+
+ constexpr std::array<uint8_t, sizeof(pldm_msg_hdr) +
+ sizeof(pldm_request_firmware_data_req)>
+ reqFwDataReq{0x8A, 0x05, 0x15, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x02, 0x00, 0x00};
+ auto requestMsg = reinterpret_cast<const pldm_msg*>(reqFwDataReq.data());
+ auto response = deviceUpdater.requestFwData(
+ requestMsg, sizeof(pldm_request_firmware_data_req));
+ ASSERT_EQ(response[sizeof(pldm_msg_hdr)], PLDM_SUCCESS);
+ EXPECT_EQ(deviceUpdater.getProgress(), 48);
+
+ constexpr std::array<uint8_t, sizeof(pldm_msg_hdr) +
+ sizeof(pldm_request_firmware_data_req)>
+ reqFwDataReq2{0x8B, 0x05, 0x15, 0x00, 0x02, 0x00,
+ 0x00, 0x00, 0x02, 0x00, 0x00};
+ requestMsg = reinterpret_cast<const pldm_msg*>(reqFwDataReq2.data());
+ response = deviceUpdater.requestFwData(
+ requestMsg, sizeof(pldm_request_firmware_data_req));
+ ASSERT_EQ(response[sizeof(pldm_msg_hdr)], PLDM_SUCCESS);
+ EXPECT_EQ(deviceUpdater.getProgress(), 97);
+
+ constexpr std::array<uint8_t, sizeof(pldm_msg_hdr) + 1> verifyComplete = {
+ 0x8C, 0x05, 0x17, 0x0};
+ requestMsg = reinterpret_cast<const pldm_msg*>(verifyComplete.data());
+ response = deviceUpdater.verifyComplete(requestMsg, 1);
+
+ ASSERT_EQ(response[sizeof(pldm_msg_hdr)], PLDM_SUCCESS);
+ EXPECT_EQ(deviceUpdater.getProgress(), 98);
+
+ constexpr std::array<uint8_t, sizeof(pldm_msg_hdr) + 3> applyComplete = {
+ 0x8D, 0x05, 0x18, 0x0, 0x0, 0x0};
+ requestMsg = reinterpret_cast<const pldm_msg*>(applyComplete.data());
+ response = deviceUpdater.applyComplete(requestMsg, 3);
+ ASSERT_EQ(response[sizeof(pldm_msg_hdr)], PLDM_SUCCESS);
+ EXPECT_EQ(deviceUpdater.getProgress(), 99);
+
+ // ActivateFirmware response: header + completion_code + estimated_time
+ constexpr std::array<uint8_t, sizeof(pldm_msg_hdr) + 3>
+ activateFirmwareResp = {0x0E, 0x05, 0x1A, 0x00, 0x00, 0x00};
+ auto activateMsg =
+ reinterpret_cast<const pldm_msg*>(activateFirmwareResp.data());
+ deviceUpdater.activateFirmware(0, activateMsg, 3);
+ EXPECT_EQ(deviceUpdater.getProgress(), 100);
}
diff --git a/fw-update/update_manager.cpp b/fw-update/update_manager.cpp
index d545645..9250962 100644
--- a/fw-update/update_manager.cpp
+++ b/fw-update/update_manager.cpp
@@ -241,6 +241,7 @@
{
if (!status)
{
+ info("Firmware update failed on eid {EID}", "EID", eid);
activation->activation(
software::Activation::Activations::Failed);
return;
@@ -325,15 +326,27 @@
parser.reset();
std::filesystem::remove(fwPackageFilePath);
totalNumComponentUpdates = 0;
- compUpdateCompletedCount = 0;
}
void UpdateManager::updateActivationProgress()
{
- compUpdateCompletedCount++;
- auto progressPercent = static_cast<uint8_t>(std::floor(
- (100 * compUpdateCompletedCount) / totalNumComponentUpdates));
- activationProgress->progress(progressPercent);
+ using mapEl = std::pair<const mctp_eid_t, std::unique_ptr<DeviceUpdater>>;
+ auto min = std::ranges::min_element(
+ deviceUpdaterMap, [](const mapEl& lhs, const mapEl& rhs) {
+ return lhs.second->getProgress() < rhs.second->getProgress();
+ });
+
+ if (min == deviceUpdaterMap.end())
+ {
+ return;
+ }
+
+ uint8_t progress = min->second->getProgress();
+ if (progress != lastProgress)
+ {
+ activationProgress->progress(progress);
+ lastProgress = progress;
+ }
}
} // namespace fw_update
diff --git a/fw-update/update_manager.hpp b/fw-update/update_manager.hpp
index 4320de1..8ee98d3 100644
--- a/fw-update/update_manager.hpp
+++ b/fw-update/update_manager.hpp
@@ -1,10 +1,10 @@
#pragma once
-
#include "common/instance_id.hpp"
#include "common/types.hpp"
#include "device_updater.hpp"
#include "fw-update/activation.hpp"
#include "fw-update/update.hpp"
+
#ifdef FW_UPDATE_INOTIFY_ENABLED
#include "fw-update/watch.hpp"
#endif
@@ -67,7 +67,7 @@
"/xyz/openbmc_project/software/pldm",
this)),
#endif
- totalNumComponentUpdates(0), compUpdateCompletedCount(0)
+ totalNumComponentUpdates(0)
{}
/** @brief Handle PLDM request for the commands in the FW update
@@ -165,14 +165,14 @@
*/
size_t totalNumComponentUpdates;
- /** @brief FW update package can contain updates for multiple firmware
- * devices and each device can have multiple components. Once
- * each component is updated (Transfer completed, Verified and
- * Applied) ActivationProgress is updated.
- */
- size_t compUpdateCompletedCount;
decltype(std::chrono::steady_clock::now()) startTime;
std::unique_ptr<sdeventplus::source::Defer> updateDeferHandler;
+
+ /** @brief The last progress that was calculated. Used to avoid spamming
+ * dbus
+ *
+ */
+ uint8_t lastProgress;
};
} // namespace fw_update
