fru-device: Fix issue with i2c scan race condition Add various changes to help fix the i2c scan race condition that may result in some i2c bus not being scan properly resulting in missing device in production. - Reuse old dbus name if possible to avoid making dbus call to nonexistent object. - Manage i2c fd better to avoid misleading error messages - Handle stall i2c bus better and continue the scan without blocking - Retry on stalled buses Tested: ``` /sys/kernel/debug/i2c-fault-injector/i2c-gpio-bus-2# echo 10 > lose_arbitration // # Wait until the error happen due to timeout ^C ``` ``` $ /tmp/fru-device Error reading bus 2 device at bus 3 address 85 busy device at bus 44 address 80 busy device at bus 46 address 80 busy device at bus 47 address 80 busy device at bus 48 address 80 busy device at bus 49 address 80 busy device at bus 50 address 80 busy device at bus 51 address 80 busy device at bus 52 address 85 busy device at bus 53 address 85 busy device at bus 54 address 85 busy device at bus 55 address 85 busy device at bus 56 address 85 busy device at bus 57 address 85 busy device at bus 58 address 85 busy device at bus 59 address 85 busy Retrying /dev/i2c-2 in 5 seconds... File descriptor at bus 2 is closed due to timeout All I2C devices discovered successfully. ``` Fusion-Link: - platform11: http://fusion2/231966c3-f095-3c48-af26-e059e31031a1 - platform11: http://fusion2/f4c6b8a2-9880-31a9-81d9-eefdc5c688be - platform5: http://fusion2/92294a57-ee71-3bba-90b3-4641d3d77397 - platform15: http://fusion2/aa309fa8-6643-372d-ad6b-b039f2b1c56c - platform17: http://fusion2/245ae348-8202-38ef-936c-82a3511035ce Google-Bug-Id: 471844579 Platforms-Affected: All Change-Id: I867877475151e14abdd788561693d3c8b47e8e48 Signed-off-by: Willy Tu <wltu@google.com> (cherry picked from commit 8715f0d7ff45a21cf7178a6bb1558a36331996f9)
diff --git a/dynamic-layers/nuvoton-layer/recipes-phosphor/configuration/entity-manager/DOWNSTREAM_0001-fru-device-Read-only-16-bytes-at-a-time.patch b/dynamic-layers/nuvoton-layer/recipes-phosphor/configuration/entity-manager/DOWNSTREAM_0001-fru-device-Read-only-16-bytes-at-a-time.patch index 88a87ca..aabd018 100644 --- a/dynamic-layers/nuvoton-layer/recipes-phosphor/configuration/entity-manager/DOWNSTREAM_0001-fru-device-Read-only-16-bytes-at-a-time.patch +++ b/dynamic-layers/nuvoton-layer/recipes-phosphor/configuration/entity-manager/DOWNSTREAM_0001-fru-device-Read-only-16-bytes-at-a-time.patch
@@ -14,11 +14,11 @@ 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fru_reader.hpp b/src/fru_reader.hpp -index f54d900..f38a521 100644 +index 59bd859..0778e0b 100644 --- a/src/fru_reader.hpp +++ b/src/fru_reader.hpp -@@ -47,7 +47,7 @@ class FRUReader - ssize_t read(off_t start, size_t len, uint8_t* outbuf); +@@ -50,7 +50,7 @@ class FRUReader + ssize_t read(off_t start, std::span<uint8_t> outbuf); private: - static constexpr size_t cacheBlockSize = 32; @@ -27,5 +27,5 @@ using CacheBlock = std::array<uint8_t, cacheBlockSize>; -- -2.40.0.348.gf938b09366-goog +2.53.0.rc2.204.g2597b5adb4-goog
diff --git a/recipes-phosphor/configuration/entity-manager/0001-fru-device-Use-stdplus-ManagedFd-for-fd-open.patch b/recipes-phosphor/configuration/entity-manager/0001-fru-device-Use-stdplus-ManagedFd-for-fd-open.patch new file mode 100644 index 0000000..4eb6ab7 --- /dev/null +++ b/recipes-phosphor/configuration/entity-manager/0001-fru-device-Use-stdplus-ManagedFd-for-fd-open.patch
@@ -0,0 +1,1307 @@ +From 93408e6ef0dcbb956cc15ce2d80f60272fc09d07 Mon Sep 17 00:00:00 2001 +From: Willy Tu <wltu@google.com> +Date: Thu, 29 Jan 2026 23:27:29 +0000 +Subject: [PATCH 1/5] fru-device: Use stdplus::ManagedFd for fd open + +Update all file descriptor open to use stdplus::ManagedFd to makge +make sure we always cleanup properly. This allows us to be notify +when the file descriptor is closed and not to continue using it. + +We have also updated the read/write + related functions to use +std::span<uint8_t> so that it works with the stdplus::ManagedFd +read/write. + +Patch Tracking Bug: b/483087027 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/87121 +Upstream-Status: Submitted +Justification: Need cherry-pick since we are not rebasing to latest entity-manager. +Tested: +Compared the list of FruDevice detected before/after this change +and found no difference in the data read. +Change-Id: Ic1d2f2f460ababc43e3050317d7b343891ca06bd +Signed-off-by: Willy Tu <wltu@google.com> +--- + meson.build | 4 +- + src/fru_device.cpp | 236 +++++++++++++++++++++++---------------- + src/fru_reader.cpp | 20 ++-- + src/fru_reader.hpp | 7 +- + src/fru_utils.cpp | 71 ++++++------ + src/fru_utils.hpp | 16 +-- + src/meson.build | 1 + + subprojects/stdplus.wrap | 6 + + test/test_fru-utils.cpp | 131 ++++++++++------------ + 9 files changed, 271 insertions(+), 221 deletions(-) + create mode 100644 subprojects/stdplus.wrap + +diff --git a/meson.build b/meson.build +index ab8c4b6..ab92dc9 100644 +--- a/meson.build ++++ b/meson.build +@@ -4,7 +4,7 @@ project( + default_options: [ + 'warning_level=3', + 'werror=true', +- 'cpp_std=c++20' ++ 'cpp_std=c++23' + ], + license: 'Apache-2.0', + version: '0.1', +@@ -35,7 +35,7 @@ if get_option('fru-device') + endif + + nlohmann_json_dep = dependency('nlohmann_json', include_type: 'system') +- ++stdplus = dependency('stdplus') + sdbusplus = dependency('sdbusplus', required: false) + if not sdbusplus.found() + sdbusplus_proj = subproject('sdbusplus', required: true) +diff --git a/src/fru_device.cpp b/src/fru_device.cpp +index 4ba8166..3cc1718 100644 +--- a/src/fru_device.cpp ++++ b/src/fru_device.cpp +@@ -29,11 +29,15 @@ + #include <nlohmann/json.hpp> + #include <sdbusplus/asio/connection.hpp> + #include <sdbusplus/asio/object_server.hpp> ++#include <stdplus/fd/create.hpp> ++#include <stdplus/fd/managed.hpp> ++#include <stdplus/raw.hpp> + + #include <array> + #include <cerrno> + #include <charconv> + #include <chrono> ++#include <cstdint> + #include <ctime> + #include <filesystem> + #include <fstream> +@@ -43,6 +47,7 @@ + #include <iostream> + #include <limits> + #include <map> ++#include <memory> + #include <optional> + #include <regex> + #include <set> +@@ -118,16 +123,27 @@ static bool hasEepromFile(size_t bus, size_t address) + } + } + +-static int64_t readFromEeprom(int fd, off_t offset, size_t len, uint8_t* buf) ++static int64_t readFromEeprom(const std::shared_ptr<stdplus::ManagedFd>& fd, ++ off_t offset, std::span<uint8_t> outbuf) + { +- auto result = lseek(fd, offset, SEEK_SET); +- if (result < 0) ++ try ++ { ++ fd->lseek(offset, stdplus::fd::Whence::Set); ++ } ++ catch (const std::system_error& e) + { +- std::cerr << "failed to seek\n"; ++ std::cerr << "failed to seek: " << e.what() << "\n"; + return -1; + } + +- return read(fd, buf, len); ++ try ++ { ++ return fd->read(stdplus::raw::asSpan<std::byte>(outbuf)).size(); ++ } ++ catch (const std::system_error& e) ++ { ++ return -1; ++ } + } + + static int busStrToInt(const std::string_view busName) +@@ -196,18 +212,19 @@ static void makeProbeInterface(size_t bus, size_t address, + it->second->initialize(); + } + +-static std::optional<bool> isDevice16Bit(int file) ++static std::optional<bool> ++ isDevice16Bit(const std::shared_ptr<stdplus::ManagedFd>& fd) + { + // Set the higher data word address bits to 0. It's safe on 8-bit addressing + // EEPROMs because it doesn't write any actual data. +- int ret = i2c_smbus_write_byte(file, 0); ++ int ret = i2c_smbus_write_byte(fd->get(), 0); + if (ret < 0) + { + return std::nullopt; + } + + /* Get first byte */ +- int byte1 = i2c_smbus_read_byte_data(file, 0); ++ int byte1 = i2c_smbus_read_byte_data(fd->get(), 0); + if (byte1 < 0) + { + return std::nullopt; +@@ -217,7 +234,7 @@ static std::optional<bool> isDevice16Bit(int file) + */ + for (int i = 0; i < 7; i++) + { +- int byte2 = i2c_smbus_read_byte_data(file, 0); ++ int byte2 = i2c_smbus_read_byte_data(fd->get(), 0); + if (byte2 < 0) + { + return std::nullopt; +@@ -232,69 +249,74 @@ static std::optional<bool> isDevice16Bit(int file) + + // Issue an I2C transaction to first write to_target_buf_len bytes,then read + // from_target_buf_len bytes. +-static int i2cSmbusWriteThenRead(int file, uint16_t address, +- uint8_t* toTargetBuf, uint8_t toTargetBufLen, +- uint8_t* fromTargetBuf, +- uint8_t fromTargetBufLen) ++static int i2cSmbusWriteThenRead(const std::shared_ptr<stdplus::ManagedFd>& fd, ++ uint16_t address, ++ std::span<uint8_t> toTargetBuf, ++ std::span<uint8_t> fromTargetBuf) + { +- if (toTargetBuf == nullptr || toTargetBufLen == 0 || +- fromTargetBuf == nullptr || fromTargetBufLen == 0) ++ if (toTargetBuf.empty() || fromTargetBuf.empty()) + { + return -1; + } + + constexpr size_t smbusWriteThenReadMsgCount = 2; + std::array<struct i2c_msg, smbusWriteThenReadMsgCount> msgs{}; +- struct i2c_rdwr_ioctl_data rdwr +- {}; ++ struct i2c_rdwr_ioctl_data rdwr{}; + + msgs[0].addr = address; + msgs[0].flags = 0; +- msgs[0].len = toTargetBufLen; +- msgs[0].buf = toTargetBuf; ++ msgs[0].len = toTargetBuf.size(); ++ msgs[0].buf = const_cast<uint8_t*>(toTargetBuf.data()); + msgs[1].addr = address; + msgs[1].flags = I2C_M_RD; +- msgs[1].len = fromTargetBufLen; +- msgs[1].buf = fromTargetBuf; ++ msgs[1].len = fromTargetBuf.size(); ++ msgs[1].buf = const_cast<uint8_t*>(fromTargetBuf.data()); + + rdwr.msgs = msgs.data(); + rdwr.nmsgs = msgs.size(); + +- int ret = ioctl(file, I2C_RDWR, &rdwr); +- ++ int ret = 0; ++ try ++ { ++ ret = fd->ioctl(I2C_RDWR, &rdwr); ++ } ++ catch (const std::system_error& e) ++ { ++ std::cerr << "failed call i2cSmbusWriteThenRead: " << e.what() << "\n"; ++ return -1; ++ } + return (ret == static_cast<int>(msgs.size())) ? msgs[1].len : -1; + } + +-static int64_t readData(bool is16bit, bool isBytewise, int file, +- uint16_t address, off_t offset, size_t len, +- uint8_t* buf) ++static int64_t readData(bool is16bit, bool isBytewise, ++ const std::shared_ptr<stdplus::ManagedFd>& fd, ++ uint16_t address, off_t offset, std::span<uint8_t> buf) + { + if (!is16bit) + { + if (!isBytewise) + { + return i2c_smbus_read_i2c_block_data( +- file, static_cast<uint8_t>(offset), len, buf); ++ fd->get(), static_cast<uint8_t>(offset), buf.size(), ++ const_cast<uint8_t*>(buf.data())); + } + +- std::span<uint8_t> bufspan{buf, len}; +- for (size_t i = 0; i < len; i++) ++ for (size_t i = 0; i < buf.size(); i++) + { + int byte = i2c_smbus_read_byte_data( +- file, static_cast<uint8_t>(offset + i)); ++ fd->get(), static_cast<uint8_t>(offset + i)); + if (byte < 0) + { + return static_cast<int64_t>(byte); + } +- bufspan[i] = static_cast<uint8_t>(byte); ++ buf[i] = static_cast<uint8_t>(byte); + } +- return static_cast<int64_t>(len); ++ return static_cast<int64_t>(buf.size()); + } + + offset = htobe16(offset); +- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) +- uint8_t* u8Offset = reinterpret_cast<uint8_t*>(&offset); +- return i2cSmbusWriteThenRead(file, address, u8Offset, 2, buf, len); ++ return i2cSmbusWriteThenRead(fd, address, ++ stdplus::raw::asSpan<uint8_t>(offset), buf); + } + + // TODO: This code is very similar to the non-eeprom version and can be merged +@@ -302,24 +324,29 @@ static int64_t readData(bool is16bit, bool isBytewise, int file, + static std::vector<uint8_t> processEeprom(int bus, int address) + { + auto path = getEepromPath(bus, address); +- +- int file = open(path.c_str(), O_RDONLY); +- if (file < 0) ++ std::shared_ptr<stdplus::ManagedFd> fd; ++ try ++ { ++ fd = std::make_shared<stdplus::ManagedFd>(stdplus::fd::open( ++ path.c_str(), ++ stdplus::fd::OpenFlags(stdplus::fd::OpenAccess::ReadOnly))); ++ } ++ catch (const std::system_error& e) + { +- std::cerr << "Unable to open eeprom file: " << path << "\n"; ++ std::cerr << "Unable to open eeprom file: " << path << " : " << e.what() ++ << "\n"; + return {}; + } + + std::string errorMessage = "eeprom at " + std::to_string(bus) + + " address " + std::to_string(address); +- auto readFunc = [file](off_t offset, size_t length, uint8_t* outbuf) { +- return readFromEeprom(file, offset, length, outbuf); ++ auto readFunc = [fd](off_t offset, std::span<uint8_t> outbuf) { ++ return readFromEeprom(fd, offset, outbuf); + }; + FRUReader reader(std::move(readFunc)); + std::pair<std::vector<uint8_t>, bool> pair = readFRUContents(reader, + errorMessage); + +- close(file); + return pair.first; + } + +@@ -364,7 +391,8 @@ std::set<size_t> findI2CEeproms(int i2cBus, size_t first, size_t last, + std::from_chars(addressStringView.begin(), addressStringView.end(), + address, 16); + +- if (address < first || address > last) { ++ if (address < first || address > last) ++ { + continue; + } + +@@ -396,9 +424,9 @@ std::set<size_t> findI2CEeproms(int i2cBus, size_t first, size_t last, + return foundList; + } + +-int getBusFRUs(int file, int first, int last, int bus, +- std::shared_ptr<DeviceMap> devices, const bool& powerIsOn, +- sdbusplus::asio::object_server& objServer) ++int getBusFRUs(const std::shared_ptr<stdplus::ManagedFd>& fd, int first, ++ int last, int bus, std::shared_ptr<DeviceMap> devices, ++ const bool& powerIsOn, sdbusplus::asio::object_server& objServer) + { + std::future<int> future = std::async(std::launch::async, [&]() { + // NOTE: When reading the devices raw on the bus, it can interfere with +@@ -467,14 +495,15 @@ int getBusFRUs(int file, int first, int last, int bus, + continue; + } + // Set target address +- if (ioctl(file, I2C_SLAVE, ii) < 0) ++ if (ioctl(fd->get(), I2C_SLAVE, ii) < 0) + { + std::cerr << "device at bus " << bus << " address " << ii + << " busy\n"; + continue; + } ++ + // probe +- if (i2c_smbus_read_byte(file) < 0) ++ if (i2c_smbus_read_byte(fd->get()) < 0) + { + continue; + } +@@ -502,7 +531,7 @@ int getBusFRUs(int file, int first, int last, int bus, + } + + /* Check for Device type if it is 8 bit or 16 bit */ +- std::optional<bool> is16Bit = isDevice16Bit(file); ++ std::optional<bool> is16Bit = isDevice16Bit(fd); + if (!is16Bit.has_value()) + { + std::cerr << "failed to read bus " << bus << " address " << ii +@@ -515,10 +544,9 @@ int getBusFRUs(int file, int first, int last, int bus, + } + bool is16BitBool{*is16Bit}; + +- auto readFunc = [is16BitBool, file, ii](off_t offset, size_t length, +- uint8_t* outbuf) { +- return readData(is16BitBool, false, file, ii, offset, length, +- outbuf); ++ auto readFunc = [is16BitBool, fd, ii](off_t offset, ++ std::span<uint8_t> outbuf) { ++ return readData(is16BitBool, false, fd, ii, offset, outbuf); + }; + FRUReader reader(std::move(readFunc)); + std::string errorMessage = "bus " + std::to_string(bus) + +@@ -532,11 +560,9 @@ int getBusFRUs(int file, int first, int last, int bus, + // certain FRU eeproms require bytewise reading. + // otherwise garbage is read. e.g. SuperMicro PWS 920P-SQ + +- auto readFunc = [is16BitBool, file, ii](off_t offset, +- size_t length, +- uint8_t* outbuf) { +- return readData(is16BitBool, true, file, ii, offset, length, +- outbuf); ++ auto readFunc = [is16BitBool, fd, ++ ii](off_t offset, std::span<uint8_t> outbuf) { ++ return readData(is16BitBool, true, fd, ii, offset, outbuf); + }; + FRUReader readerBytewise(std::move(readFunc)); + pair = readFRUContents(readerBytewise, errorMessage); +@@ -548,9 +574,9 @@ int getBusFRUs(int file, int first, int last, int bus, + } + if (!validateFRUAreas(pair.first)) + { +- std::cerr << "Checksum failure bus " << bus +- << " address " << ii << "\n"; +- continue; ++ std::cerr << "Checksum failure bus " << bus << " address " ++ << ii << "\n"; ++ continue; + } + + devices->emplace(ii, pair.first); +@@ -563,11 +589,9 @@ int getBusFRUs(int file, int first, int last, int bus, + if (status == std::future_status::timeout) + { + std::cerr << "Error reading bus " << bus << "\n"; +- close(file); + return -1; + } + +- close(file); + return future.get(); + } + +@@ -682,21 +706,25 @@ static void findI2CDevices(const std::vector<fs::path>& i2cBuses, + } + } + +- auto file = open(i2cBus.c_str(), O_RDWR); +- if (file < 0) ++ std::shared_ptr<stdplus::ManagedFd> fd; ++ try ++ { ++ fd = std::make_shared<stdplus::ManagedFd>(stdplus::fd::open( ++ i2cBus.c_str(), stdplus::fd::OpenAccess::ReadWrite)); ++ } ++ catch (const std::system_error& e) + { + std::cerr << "unable to open i2c device " << i2cBus.string() +- << "\n"; ++ << " : " << e.what() << "\n"; + continue; + } + unsigned long funcs = 0; + +- if (ioctl(file, I2C_FUNCS, &funcs) < 0) ++ if (ioctl(fd->get(), I2C_FUNCS, &funcs) < 0) + { + std::cerr + << "Error: Could not get the adapter functionality matrix bus " + << bus << "\n"; +- close(file); + continue; + } + if (((funcs & I2C_FUNC_SMBUS_READ_BYTE) == 0U) || +@@ -719,7 +747,7 @@ static void findI2CDevices(const std::vector<fs::path>& i2cBuses, + + // fd is closed in this function in case the bus locks up + // The FRU device is supposed to be in [0x50, 0x57] in Google. +- getBusFRUs(file, 0x50, 0x57, bus, device, powerIsOn, objServer); ++ getBusFRUs(fd, 0x50, 0x57, bus, device, powerIsOn, objServer); + + if (debug) + { +@@ -736,9 +764,8 @@ struct FindDevicesWithCallback : + BusMap& busmap, const bool& powerIsOn, + sdbusplus::asio::object_server& objServer, + std::function<void(void)>&& callback) : +- _i2cBuses(i2cBuses), +- _busMap(busmap), _powerIsOn(powerIsOn), _objServer(objServer), +- _callback(std::move(callback)) ++ _i2cBuses(i2cBuses), _busMap(busmap), _powerIsOn(powerIsOn), ++ _objServer(objServer), _callback(std::move(callback)) + {} + ~FindDevicesWithCallback() + { +@@ -865,7 +892,7 @@ static bool readBaseboardFRU(std::vector<uint8_t>& baseboardFRU) + return true; + } + +-bool writeFRU(uint8_t bus, uint8_t address, const std::vector<uint8_t>& fru) ++bool writeFRU(uint8_t bus, uint8_t address, std::span<const uint8_t> fru) + { + boost::container::flat_map<std::string, std::string> tmp; + if (fru.size() > maxFruSize) +@@ -898,40 +925,57 @@ bool writeFRU(uint8_t bus, uint8_t address, const std::vector<uint8_t>& fru) + if (hasEepromFile(bus, address)) + { + auto path = getEepromPath(bus, address); +- int eeprom = open(path.c_str(), O_RDWR | O_CLOEXEC); +- if (eeprom < 0) ++ std::shared_ptr<stdplus::ManagedFd> eeprom; ++ try + { +- std::cerr << "unable to open i2c device " << path << "\n"; ++ eeprom = std::make_shared<stdplus::ManagedFd>(stdplus::fd::open( ++ path.c_str(), ++ stdplus::BitFlags<stdplus::fd::OpenFlag>( ++ static_cast<int>(stdplus::fd::OpenAccess::ReadWrite) | ++ static_cast<int>(stdplus::fd::OpenFlag::CloseOnExec)))); ++ } ++ catch (const std::system_error& e) ++ { ++ std::cerr << "unable to open i2c device " << path << " : " ++ << e.what() << "\n"; + throw DBusInternalError(); + return false; + } + +- ssize_t writtenBytes = write(eeprom, fru.data(), fru.size()); +- if (writtenBytes < 0) ++ try + { +- std::cerr << "unable to write to i2c device " << path << "\n"; +- close(eeprom); ++ eeprom->write(stdplus::raw::asSpan<std::byte>(fru)); ++ } ++ catch (const std::system_error& e) ++ { ++ std::cerr << "unable to write to i2zc device " << path << " : " ++ << e.what() << "\n"; + throw DBusInternalError(); + return false; + } +- +- close(eeprom); + return true; + } + + std::string i2cBus = "/dev/i2c-" + std::to_string(bus); +- +- int file = open(i2cBus.c_str(), O_RDWR | O_CLOEXEC); +- if (file < 0) ++ std::shared_ptr<stdplus::ManagedFd> fd; ++ try ++ { ++ fd = std::make_shared<stdplus::ManagedFd>(stdplus::fd::open( ++ i2cBus.c_str(), ++ stdplus::BitFlags<stdplus::fd::OpenFlag>( ++ static_cast<int>(stdplus::fd::OpenAccess::ReadWrite) | ++ static_cast<int>(stdplus::fd::OpenFlag::CloseOnExec)))); ++ } ++ catch (const std::system_error& e) + { +- std::cerr << "unable to open i2c device " << i2cBus << "\n"; ++ std::cerr << "unable to open i2c device " << i2cBus << ": " << e.what() ++ << "\n"; + throw DBusInternalError(); + return false; + } +- if (ioctl(file, I2C_SLAVE_FORCE, address) < 0) ++ if (ioctl(fd->get(), I2C_SLAVE_FORCE, address) < 0) + { + std::cerr << "unable to set device address\n"; +- close(file); + throw DBusInternalError(); + return false; + } +@@ -946,22 +990,20 @@ bool writeFRU(uint8_t bus, uint8_t address, const std::vector<uint8_t>& fru) + { + // The 4K EEPROM only uses the A2 and A1 device address bits + // with the third bit being a memory page address bit. +- if (ioctl(file, I2C_SLAVE_FORCE, ++address) < 0) ++ if (ioctl(fd->get(), I2C_SLAVE_FORCE, ++address) < 0) + { + std::cerr << "unable to set device address\n"; +- close(file); + throw DBusInternalError(); + return false; + } + } + +- if (i2c_smbus_write_byte_data(file, static_cast<uint8_t>(index), ++ if (i2c_smbus_write_byte_data(fd->get(), static_cast<uint8_t>(index), + fru[index]) < 0) + { + if ((retries--) == 0U) + { + std::cerr << "error writing fru: " << strerror(errno) << "\n"; +- close(file); + throw DBusInternalError(); + return false; + } +@@ -974,7 +1016,6 @@ bool writeFRU(uint8_t bus, uint8_t address, const std::vector<uint8_t>& fru) + // most eeproms require 5-10ms between writes + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } +- close(file); + return true; + } + +@@ -1055,7 +1096,8 @@ void rescanBusses( + std::shared_ptr<sdbusplus::asio::dbus_interface>>& dbusInterfaceMap, + size_t& unknownBusObjectCount, const bool& powerIsOn, + sdbusplus::asio::object_server& objServer, +- std::shared_ptr<sdbusplus::asio::connection>& systemBus, std::chrono::seconds timeout = std::chrono::seconds(1)) ++ std::shared_ptr<sdbusplus::asio::connection>& systemBus, ++ std::chrono::seconds timeout = std::chrono::seconds(1)) + { + static boost::asio::steady_timer timer(io); + timer.expires_from_now(std::chrono::seconds(timeout)); +@@ -1168,8 +1210,7 @@ bool updateFRUProperty( + return false; + } + +- struct FruArea fruAreaParams +- {}; ++ struct FruArea fruAreaParams{}; + + if (!findFruAreaLocationAndField(fruData, propertyName, fruAreaParams)) + { +@@ -1388,7 +1429,8 @@ int main() + if (powerIsOn) + { + rescanBusses(busMap, dbusInterfaceMap, unknownBusObjectCount, +- powerIsOn, objServer, systemBus, std::chrono::seconds(3)); ++ powerIsOn, objServer, systemBus, ++ std::chrono::seconds(3)); + } + }; + +diff --git a/src/fru_reader.cpp b/src/fru_reader.cpp +index 5acfa65..750b0bc 100644 +--- a/src/fru_reader.cpp ++++ b/src/fru_reader.cpp +@@ -18,19 +18,19 @@ + + #include <cstring> + +-ssize_t FRUReader::read(off_t start, size_t len, uint8_t* outbuf) ++ssize_t FRUReader::read(off_t start, std::span<uint8_t> outbuf) + { + size_t done = 0; +- size_t remaining = len; ++ size_t remaining = outbuf.size(); + size_t cursor = start; +- while (done < len) ++ while (done < outbuf.size()) + { + if (eof.has_value() && cursor >= eof.value()) + { + break; + } + +- const uint8_t* blkData = nullptr; ++ std::span<uint8_t> blkData; + size_t available = 0; + size_t blk = cursor / cacheBlockSize; + size_t blkOffset = cursor % cacheBlockSize; +@@ -38,9 +38,8 @@ ssize_t FRUReader::read(off_t start, size_t len, uint8_t* outbuf) + if (findBlk == cache.end()) + { + // miss, populate cache +- uint8_t* newData = cache[blk].data(); +- int64_t ret = readFunc(blk * cacheBlockSize, cacheBlockSize, +- newData); ++ std::span<uint8_t> newBlkData = cache[blk]; ++ int64_t ret = readFunc(blk * cacheBlockSize, newBlkData); + + // if we've reached the end of the eeprom, record its size + if (ret >= 0 && static_cast<size_t>(ret) < cacheBlockSize) +@@ -55,13 +54,13 @@ ssize_t FRUReader::read(off_t start, size_t len, uint8_t* outbuf) + return done != 0U ? done : ret; + } + +- blkData = newData; ++ blkData = newBlkData; + available = ret; + } + else + { + // hit, use cached data +- blkData = findBlk->second.data(); ++ blkData = findBlk->second; + + // if the hit is to the block containing the (previously + // discovered on the miss that populated it) end of the eeprom, +@@ -81,7 +80,8 @@ ssize_t FRUReader::read(off_t start, size_t len, uint8_t* outbuf) + : std::min(available - blkOffset, remaining); + + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) +- memcpy(outbuf + done, blkData + blkOffset, toCopy); ++ memcpy(outbuf.data() + done, blkData.data() + blkOffset, toCopy); ++ + cursor += toCopy; + done += toCopy; + remaining -= toCopy; +diff --git a/src/fru_reader.hpp b/src/fru_reader.hpp +index f54d900..59bd859 100644 +--- a/src/fru_reader.hpp ++++ b/src/fru_reader.hpp +@@ -16,10 +16,13 @@ + + #pragma once + ++#include <unistd.h> ++ + #include <cstdint> + #include <functional> + #include <map> + #include <optional> ++#include <span> + #include <utility> + + extern "C" +@@ -33,7 +36,7 @@ extern "C" + // is not guaranteed to be) less than len if the read would go beyond the end + // of the FRU. + using ReadBlockFunc = +- std::function<int64_t(off_t offset, size_t len, uint8_t* outbuf)>; ++ std::function<int64_t(off_t offset, std::span<uint8_t> outbuf)>; + + // A caching wrapper around a ReadBlockFunc + class FRUReader +@@ -44,7 +47,7 @@ class FRUReader + // The ::read() operation here is analogous to ReadBlockFunc (with the same + // return value semantics), but is not subject to SMBus block size + // limitations; it can read as much data as needed in a single call. +- ssize_t read(off_t start, size_t len, uint8_t* outbuf); ++ ssize_t read(off_t start, std::span<uint8_t> outbuf); + + private: + static constexpr size_t cacheBlockSize = 32; +diff --git a/src/fru_utils.cpp b/src/fru_utils.cpp +index a5df59d..68aaecc 100644 +--- a/src/fru_utils.cpp ++++ b/src/fru_utils.cpp +@@ -25,6 +25,7 @@ + #include <iostream> + #include <numeric> + #include <set> ++#include <span> + #include <sstream> + #include <string> + #include <vector> +@@ -99,8 +100,8 @@ enum SubManagementAccessRecord : uint8_t + * iterator is no longer usable. + */ + std::pair<DecodeState, std::string> +- decodeFRUData(std::vector<uint8_t>::const_iterator& iter, +- const std::vector<uint8_t>::const_iterator& end, ++ decodeFRUData(std::span<const uint8_t>::const_iterator& iter, ++ const std::span<const uint8_t>::const_iterator& end, + bool isLangEng) + { + std::string value; +@@ -219,7 +220,7 @@ bool checkLangEng(uint8_t lang) + * len: Length of current area space and it is a multiple of 8 bytes + * as per specification + */ +-bool verifyOffset(const std::vector<uint8_t>& fruBytes, fruAreas currentArea, ++bool verifyOffset(std::span<const uint8_t> fruBytes, fruAreas currentArea, + uint8_t len) + { + unsigned int fruBytesSize = fruBytes.size(); +@@ -284,7 +285,7 @@ bool verifyOffset(const std::vector<uint8_t>& fruBytes, fruAreas currentArea, + return true; + } + +-bool validateFRUAreas(std::vector<uint8_t>& fruData) ++bool validateFRUAreas(std::span<const uint8_t> fruData) + { + for (fruAreas area = fruAreas::fruAreaChassis; + area <= fruAreas::fruAreaProduct; ++area) +@@ -295,8 +296,8 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + continue; + } + offset *= fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIter = fruData.begin() + +- offset; ++ std::span<const uint8_t>::const_iterator fruBytesIter = ++ fruData.begin() + offset; + if (fruBytesIter + fruBlockSize >= fruData.end()) + { + std::cerr << "Not enough data to parse \n"; +@@ -319,7 +320,7 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + } + + size_t fruAreaSize = *fruBytesIter * fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIterEndArea = ++ std::span<const uint8_t>::const_iterator fruBytesIterEndArea = + fruData.begin() + offset + fruAreaSize - 1; + ++fruBytesIter; + +@@ -338,7 +339,7 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + + // Validate MultiAreaRecord + size_t areaOffset = +- fruData.at(getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)); ++ fruData[getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)]; + + // No MultiAreaRecord + if (areaOffset == 0) +@@ -347,8 +348,8 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + } + + areaOffset *= fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIter = fruData.begin() + +- areaOffset; ++ std::span<const uint8_t>::const_iterator fruBytesIter = fruData.begin() + ++ areaOffset; + + /* Verify area offset */ + if (!verifyOffset(fruData, fruAreas::fruAreaMultirecord, *fruBytesIter)) +@@ -362,7 +363,7 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + + while (areaOffset + multiRecordHeaderLen <= fruData.size()) + { +- std::vector<uint8_t>::const_iterator multiAreaHeaderEnd = ++ std::span<const uint8_t>::const_iterator multiAreaHeaderEnd = + fruData.begin() + areaOffset + multiRecordHeaderLen - 1; + uint8_t fruComputedChecksum = + calculateChecksum(fruData.begin() + areaOffset, multiAreaHeaderEnd); +@@ -380,7 +381,7 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + /* Verify MultiAreaRecord */ + uint8_t multiAreaRecordChecksum = fruData[areaOffset + 3]; + size_t fruAreaSize = fruData[areaOffset + 2]; +- std::vector<uint8_t>::const_iterator fruBytesIterEndArea = ++ std::span<const uint8_t>::const_iterator fruBytesIterEndArea = + fruData.begin() + areaOffset + multiRecordHeaderLen + fruAreaSize; + + uint8_t fruComputedRecordChecksum = calculateChecksum( +@@ -409,21 +410,21 @@ bool validateFRUAreas(std::vector<uint8_t>& fruData) + } + + static void parseMultirecordOem( +- const std::vector<uint8_t>& device, ++ std::span<const uint8_t> device, + boost::container::flat_map<std::string, std::string>& result) + { + constexpr size_t multiRecordHeaderLen = 5; + constexpr size_t multiRecordEndOfListMask = 0x80; + uint32_t areaOffset = +- device.at(getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)); ++ device[getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)]; + + if (areaOffset == 0) + { + return; + } + areaOffset *= fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesOfRecordIter = device.begin() + +- areaOffset; ++ std::span<const uint8_t>::const_iterator fruBytesOfRecordIter = ++ device.begin() + areaOffset; + + /* Verify area offset */ + if (!verifyOffset(device, fruAreas::fruAreaMultirecord, +@@ -438,7 +439,7 @@ static void parseMultirecordOem( + { + uint8_t multiRecordId = *fruBytesOfRecordIter; + size_t recordLength = *(fruBytesOfRecordIter + 2); +- if(device.size() < areaOffset + multiRecordHeaderLen + recordLength) ++ if (device.size() < areaOffset + multiRecordHeaderLen + recordLength) + { + return; + } +@@ -467,7 +468,7 @@ static void parseMultirecordOem( + } + + static void parseMultirecordUUID( +- const std::vector<uint8_t>& device, ++ std::span<const uint8_t> device, + boost::container::flat_map<std::string, std::string>& result) + { + constexpr size_t uuidDataLen = 16; +@@ -482,7 +483,7 @@ static void parseMultirecordUUID( + const std::array<uint8_t, uuidDataLen> uuidCharOrder = { + 3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15}; + uint32_t areaOffset = +- device.at(getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)); ++ device[getHeaderAreaFieldOffset(fruAreas::fruAreaMultirecord)]; + + if (areaOffset == 0) + { +@@ -490,8 +491,8 @@ static void parseMultirecordUUID( + } + + areaOffset *= fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIter = device.begin() + +- areaOffset; ++ std::span<const uint8_t>::const_iterator fruBytesIter = device.begin() + ++ areaOffset; + + /* Verify area offset */ + if (!verifyOffset(device, fruAreas::fruAreaMultirecord, *fruBytesIter)) +@@ -547,7 +548,7 @@ static void parseMultirecordUUID( + } + + resCodes +- formatIPMIFRU(const std::vector<uint8_t>& fruBytes, ++ formatIPMIFRU(std::span<const uint8_t> fruBytes, + boost::container::flat_map<std::string, std::string>& result) + { + resCodes ret = resCodes::resOK; +@@ -571,8 +572,8 @@ resCodes + continue; + } + offset *= fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIter = fruBytes.begin() + +- offset; ++ std::span<const uint8_t>::const_iterator fruBytesIter = ++ fruBytes.begin() + offset; + if (fruBytesIter + fruBlockSize >= fruBytes.end()) + { + std::cerr << "Not enough data to parse \n"; +@@ -595,7 +596,7 @@ resCodes + } + + size_t fruAreaSize = *fruBytesIter * fruBlockSize; +- std::vector<uint8_t>::const_iterator fruBytesIterEndArea = ++ std::span<const uint8_t>::const_iterator fruBytesIterEndArea = + fruBytes.begin() + offset + fruAreaSize - 1; + ++fruBytesIter; + +@@ -756,15 +757,15 @@ resCodes + } + + // Calculate new checksum for fru info area +-uint8_t calculateChecksum(std::vector<uint8_t>::const_iterator iter, +- std::vector<uint8_t>::const_iterator end) ++uint8_t calculateChecksum(std::span<const uint8_t>::const_iterator iter, ++ std::span<const uint8_t>::const_iterator end) + { + constexpr int checksumMod = 256; + uint8_t sum = std::accumulate(iter, end, static_cast<uint8_t>(0)); + return (checksumMod - sum) % checksumMod; + } + +-uint8_t calculateChecksum(std::vector<uint8_t>& fruAreaData) ++uint8_t calculateChecksum(std::span<const uint8_t> fruAreaData) + { + return calculateChecksum(fruAreaData.begin(), fruAreaData.end()); + } +@@ -887,7 +888,8 @@ bool findFRUHeader(FRUReader& reader, const std::string& errorHelp, + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX>& blockData, + off_t& baseOffset) + { +- if (reader.read(baseOffset, 0x8, blockData.data()) < 0) ++ std::span<uint8_t> blockDataSpan(blockData); ++ if (reader.read(baseOffset, blockDataSpan.subspan(0, 0x8)) < 0) + { + std::cerr << "failed to read " << errorHelp << " base offset " + << baseOffset << "\n"; +@@ -931,6 +933,7 @@ std::pair<std::vector<uint8_t>, bool> + readFRUContents(FRUReader& reader, const std::string& errorHelp) + { + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; ++ std::span<uint8_t> blockDataSpan(blockData); + off_t baseOffset = 0x0; + + if (!findFRUHeader(reader, errorHelp, blockData, baseOffset)) +@@ -977,7 +980,8 @@ std::pair<std::vector<uint8_t>, bool> + + areaOffset *= fruBlockSize; + +- if (reader.read(baseOffset + areaOffset, 0x2, blockData.data()) < 0) ++ std::span<uint8_t> areaBlockData = blockDataSpan.subspan(0, 0x2); ++ if (reader.read(baseOffset + areaOffset, areaBlockData) < 0) + { + std::cerr << "failed to read " << errorHelp << " base offset " + << baseOffset << "\n"; +@@ -1007,7 +1011,8 @@ std::pair<std::vector<uint8_t>, bool> + { + // In multi-area, the area offset points to the 0th record, each + // record has 3 bytes of the header we care about. +- if (reader.read(baseOffset + areaOffset, 0x3, blockData.data()) < 0) ++ std::span<uint8_t> areaBlockData = blockDataSpan.subspan(0, 0x3); ++ if (reader.read(baseOffset + areaOffset, areaBlockData) < 0) + { + std::cerr << "failed to read " << errorHelp << " base offset " + << baseOffset << "\n"; +@@ -1038,8 +1043,8 @@ std::pair<std::vector<uint8_t>, bool> + size_t requestLength = + std::min(static_cast<size_t>(I2C_SMBUS_BLOCK_MAX), fruLength); + +- if (reader.read(baseOffset + readOffset, requestLength, +- blockData.data()) < 0) ++ if (reader.read(baseOffset + readOffset, ++ blockDataSpan.subspan(0, requestLength)) < 0) + { + std::cerr << "failed to read " << errorHelp << " base offset " + << baseOffset << "\n"; +diff --git a/src/fru_utils.hpp b/src/fru_utils.hpp +index e987f2a..457456c 100644 +--- a/src/fru_utils.hpp ++++ b/src/fru_utils.hpp +@@ -112,26 +112,26 @@ constexpr std::array<char, 6> bcdHighChars = { + + char bcdPlusToChar(uint8_t val); + +-bool verifyOffset(const std::vector<uint8_t>& fruBytes, fruAreas currentArea, ++bool verifyOffset(std::span<const uint8_t> fruBytes, fruAreas currentArea, + uint8_t len); + + std::pair<DecodeState, std::string> +- decodeFRUData(std::vector<uint8_t>::const_iterator& iter, +- const std::vector<uint8_t>::const_iterator& end, ++ decodeFRUData(std::span<const uint8_t>::const_iterator& iter, ++ const std::span<const uint8_t>::const_iterator& end, + bool isLangEng); + + bool checkLangEng(uint8_t lang); + + resCodes +- formatIPMIFRU(const std::vector<uint8_t>& fruBytes, ++ formatIPMIFRU(std::span<const uint8_t> fruBytes, + boost::container::flat_map<std::string, std::string>& result); + + std::vector<uint8_t>& getFRUInfo(const uint16_t& bus, const uint8_t& address); + +-uint8_t calculateChecksum(std::vector<uint8_t>::const_iterator iter, +- std::vector<uint8_t>::const_iterator end); ++uint8_t calculateChecksum(std::span<const uint8_t>::const_iterator iter, ++ std::span<const uint8_t>::const_iterator end); + +-uint8_t calculateChecksum(std::vector<uint8_t>& fruAreaData); ++uint8_t calculateChecksum(std::span<const uint8_t> fruAreaData); + + unsigned int updateFRUAreaLenAndChecksum(std::vector<uint8_t>& fruData, + size_t fruAreaStart, +@@ -163,7 +163,7 @@ std::pair<std::vector<uint8_t>, bool> + /// \brief Validate FRU Areas Checksum + /// \param fruData the bytes comprising the FRU + /// \return true if valid +-bool validateFRUAreas(std::vector<uint8_t>& fruData); ++bool validateFRUAreas(std::span<const uint8_t> fruData); + + /// \brief Validate an IPMI FRU common header + /// \param blockData the bytes comprising the common header +diff --git a/src/meson.build b/src/meson.build +index 4d090f2..9fa3b9c 100644 +--- a/src/meson.build ++++ b/src/meson.build +@@ -38,6 +38,7 @@ if get_option('fru-device') + i2c, + nlohmann_json_dep, + sdbusplus, ++ stdplus, + threads, + valijson, + ], +diff --git a/subprojects/stdplus.wrap b/subprojects/stdplus.wrap +new file mode 100644 +index 0000000..2f8a5f4 +--- /dev/null ++++ b/subprojects/stdplus.wrap +@@ -0,0 +1,6 @@ ++[wrap-git] ++url = https://github.com/openbmc/stdplus.git ++revision = HEAD ++ ++[provide] ++stdplus = stdplus_dep +diff --git a/test/test_fru-utils.cpp b/test/test_fru-utils.cpp +index 9470e4c..d490255 100644 +--- a/test/test_fru-utils.cpp ++++ b/test/test_fru-utils.cpp +@@ -147,22 +147,17 @@ TEST(VerifyChecksumTest, WrapBoundaryHigh) + EXPECT_EQ(calculateChecksum(data), 255); + } + +-int64_t getDataTempl(const std::vector<uint8_t>& data, off_t offset, +- size_t length, uint8_t* outBuf) ++int64_t getDataTempl(std::span<const uint8_t> data, off_t offset, ++ std::span<uint8_t> outbuf) + { + if (offset >= static_cast<off_t>(data.size())) + { + return 0; + } + +- uint16_t idx = offset; +- // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) +- for (; idx < std::min(data.size(), offset + length); ++idx, ++outBuf) +- { +- *outBuf = data[idx]; +- } +- +- return idx - offset; ++ size_t size = std::min(data.size() - offset, outbuf.size()); ++ std::memcpy(outbuf.data(), data.subspan(offset).data(), size); ++ return size; + } + + TEST(FRUReaderTest, ReadData) +@@ -173,20 +168,20 @@ TEST(FRUReaderTest, ReadData) + data.push_back(i); + } + std::array<uint8_t, blockSize * 2> rdbuf{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ std::span<uint8_t> rdbufSpan(rdbuf); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +- EXPECT_EQ(reader.read(0, data.size(), rdbuf.data()), +- static_cast<ssize_t>(data.size())); ++ EXPECT_EQ(reader.read(0, rdbufSpan), static_cast<ssize_t>(data.size())); + EXPECT_TRUE(std::equal(rdbuf.begin(), rdbuf.end(), data.begin())); + for (size_t i = 0; i < blockSize * 2; i++) + { +- EXPECT_EQ(reader.read(i, 1, rdbuf.data()), 1); ++ EXPECT_EQ(reader.read(i, rdbufSpan.subspan(0, 1)), 1); + EXPECT_EQ(rdbuf[i], i); + } +- EXPECT_EQ(reader.read(blockSize - 1, 2, rdbuf.data()), 2); ++ EXPECT_EQ(reader.read(blockSize - 1, rdbufSpan.subspan(0, 2)), 2); + EXPECT_EQ(rdbuf[0], blockSize - 1); + EXPECT_EQ(rdbuf[1], blockSize); + } +@@ -194,12 +189,13 @@ TEST(FRUReaderTest, ReadData) + TEST(FRUReaderTest, StartPastUnknownEOF) + { + const std::vector<uint8_t> data = {}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); ++ std::array<uint8_t, 0> buf{}; + +- EXPECT_EQ(reader.read(1, 1, nullptr), 0); ++ EXPECT_EQ(reader.read(1, buf), 0); + } + + TEST(FRUReaderTest, StartPastKnownEOF) +@@ -207,32 +203,32 @@ TEST(FRUReaderTest, StartPastKnownEOF) + std::vector<uint8_t> data = {}; + data.resize(blockSize / 2); + std::array<uint8_t, blockSize> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ std::array<uint8_t, 0> emptyBuf{}; ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +- EXPECT_EQ(reader.read(0, blockSize, blockData.data()), +- static_cast<ssize_t>(data.size())); +- EXPECT_EQ(reader.read(data.size(), 1, nullptr), 0); +- EXPECT_EQ(reader.read(data.size() + 1, 1, nullptr), 0); +- EXPECT_EQ(reader.read(blockSize, 1, nullptr), 0); +- EXPECT_EQ(reader.read(blockSize + 1, 1, nullptr), 0); ++ EXPECT_EQ(reader.read(0, blockData), static_cast<ssize_t>(data.size())); ++ EXPECT_EQ(reader.read(data.size(), emptyBuf), 0); ++ EXPECT_EQ(reader.read(data.size() + 1, emptyBuf), 0); ++ EXPECT_EQ(reader.read(blockSize, emptyBuf), 0); ++ EXPECT_EQ(reader.read(blockSize + 1, emptyBuf), 0); + } + + TEST(FRUReaderTest, DecreasingEOF) + { + const std::vector<uint8_t> data = {}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); +- +- EXPECT_EQ(reader.read(blockSize * 2, 1, nullptr), 0); +- EXPECT_EQ(reader.read(blockSize + (blockSize / 2), 1, nullptr), 0); +- EXPECT_EQ(reader.read(blockSize, 1, nullptr), 0); +- EXPECT_EQ(reader.read(blockSize / 2, 1, nullptr), 0); +- EXPECT_EQ(reader.read(0, 1, nullptr), 0); ++ std::vector<uint8_t> buf; ++ EXPECT_EQ(reader.read(blockSize * 2, buf), 0); ++ EXPECT_EQ(reader.read(blockSize + (blockSize / 2), buf), 0); ++ EXPECT_EQ(reader.read(blockSize, buf), 0); ++ EXPECT_EQ(reader.read(blockSize / 2, buf), 0); ++ EXPECT_EQ(reader.read(0, buf), 0); + } + + TEST(FRUReaderTest, CacheHit) +@@ -240,16 +236,16 @@ TEST(FRUReaderTest, CacheHit) + std::vector<uint8_t> data = {'X'}; + std::array<uint8_t, blockSize> read1{}; + std::array<uint8_t, blockSize> read2{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + + // cache hit should return the same data for the second read even if we + // change it behind the FRUReader's back after the first +- EXPECT_EQ(reader.read(0, blockSize, read1.data()), 1); ++ EXPECT_EQ(reader.read(0, read1), 1); + data[0] = 'Y'; +- EXPECT_EQ(reader.read(0, blockSize, read2.data()), 1); ++ EXPECT_EQ(reader.read(0, read2), 1); + EXPECT_EQ(read1[0], read2[0]); + } + +@@ -257,17 +253,15 @@ TEST(FRUReaderTest, ReadPastKnownEnd) + { + const std::vector<uint8_t> data = {'X', 'Y'}; + std::array<uint8_t, blockSize> rdbuf{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +- EXPECT_EQ(reader.read(0, data.size(), rdbuf.data()), +- static_cast<ssize_t>(data.size())); ++ EXPECT_EQ(reader.read(0, rdbuf), static_cast<ssize_t>(data.size())); + EXPECT_EQ(rdbuf[0], 'X'); + EXPECT_EQ(rdbuf[1], 'Y'); +- EXPECT_EQ(reader.read(1, data.size(), rdbuf.data()), +- static_cast<ssize_t>(data.size() - 1)); ++ EXPECT_EQ(reader.read(1, rdbuf), static_cast<ssize_t>(data.size() - 1)); + EXPECT_EQ(rdbuf[0], 'Y'); + } + +@@ -277,13 +271,12 @@ TEST(FRUReaderTest, MultiBlockRead) + data.resize(blockSize, 'X'); + data.resize(2 * blockSize, 'Y'); + std::array<uint8_t, 2 * blockSize> rdbuf{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +- EXPECT_EQ(reader.read(0, 2 * blockSize, rdbuf.data()), +- static_cast<ssize_t>(2 * blockSize)); ++ EXPECT_EQ(reader.read(0, rdbuf), static_cast<ssize_t>(2 * blockSize)); + EXPECT_TRUE(std::equal(rdbuf.begin(), rdbuf.end(), data.begin())); + } + +@@ -292,14 +285,14 @@ TEST(FRUReaderTest, ShrinkingEEPROM) + std::vector<uint8_t> data = {}; + data.resize(3 * blockSize, 'X'); + std::array<uint8_t, blockSize> rdbuf{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +- EXPECT_EQ(reader.read(data.size() - 1, 2, rdbuf.data()), 1); ++ EXPECT_EQ(reader.read(data.size() - 1, rdbuf), 1); + data.resize(blockSize); +- EXPECT_EQ(reader.read(data.size() - 1, 2, rdbuf.data()), 1); ++ EXPECT_EQ(reader.read(data.size() - 1, rdbuf), 1); + } + + TEST(FindFRUHeaderTest, InvalidHeader) +@@ -307,8 +300,8 @@ TEST(FindFRUHeaderTest, InvalidHeader) + const std::vector<uint8_t> data = {255, 16}; + off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +@@ -320,8 +313,8 @@ TEST(FindFRUHeaderTest, NoData) + const std::vector<uint8_t> data = {}; + off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +@@ -332,10 +325,10 @@ TEST(FindFRUHeaderTest, ValidHeader) + { + const std::vector<uint8_t> data = {0x01, 0x00, 0x01, 0x02, + 0x03, 0x04, 0x00, 0xf5}; +- off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ off_t offset = 0; ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +@@ -347,10 +340,10 @@ TEST(FindFRUHeaderTest, TyanInvalidHeader) + { + std::vector<uint8_t> data = {'$', 'T', 'Y', 'A', 'N', '$', 0, 0}; + data.resize(0x6000 + I2C_SMBUS_BLOCK_MAX); +- off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ off_t offset = 0; ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +@@ -360,10 +353,10 @@ TEST(FindFRUHeaderTest, TyanInvalidHeader) + TEST(FindFRUHeaderTest, TyanNoData) + { + const std::vector<uint8_t> data = {'$', 'T', 'Y', 'A', 'N', '$', 0, 0}; +- off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ off_t offset = 0; ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +@@ -378,10 +371,10 @@ TEST(FindFRUHeaderTest, TyanValidHeader) + 0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x00, 0xf5}; + copy(fruHeader.begin(), fruHeader.end(), back_inserter(data)); + +- off_t offset = 0; + std::array<uint8_t, I2C_SMBUS_BLOCK_MAX> blockData{}; +- auto getData = [&data](auto o, auto l, auto* b) { +- return getDataTempl(data, o, l, b); ++ off_t offset = 0; ++ auto getData = [&data](auto o, std::span<uint8_t> b) { ++ return getDataTempl(data, o, b); + }; + FRUReader reader(getData); + +-- +2.53.0.rc1.225.gd81095ad13-goog +
diff --git a/recipes-phosphor/configuration/entity-manager/0002-fru-device-Exit-gracefully-if-getBusFRUs-times-out.patch b/recipes-phosphor/configuration/entity-manager/0002-fru-device-Exit-gracefully-if-getBusFRUs-times-out.patch new file mode 100644 index 0000000..c72ef25 --- /dev/null +++ b/recipes-phosphor/configuration/entity-manager/0002-fru-device-Exit-gracefully-if-getBusFRUs-times-out.patch
@@ -0,0 +1,401 @@ +From 460ddf6f9f209022f6594384a0455de90f65dbf9 Mon Sep 17 00:00:00 2001 +From: Willy Tu <wltu@google.com> +Date: Thu, 29 Jan 2026 23:34:16 +0000 +Subject: [PATCH 2/5] fru-device: Exit gracefully if getBusFRUs times out + +After the std::async function timeout, the file descriptor for the i2c +bus will be closed once we exit the function. This change make sure to +not continue using the old fd when it was already closed. + +Currently, the std::future will continuing blocking the process because +std::async has a blocking destructor. Even if we timeout in `wait_for`, +it would still block until the async task completes. This change +refactored it to use std::thread, std::promise, and std::future manually +to allow us to detach() the thread if a timeout occurs, letting the +function continue what it needs to do while letting the sutck thread +continue/hang in the background. We will try to stop the thread early +oncce the weak_ptr of the managedFd cleans up. + + +Patch Tracking Bug: b/483088231 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/87122 +Upstream-Status: Submitted +Justification: Need cherry-pick since we are not rebasing to latest entity-manager. +Tested: + - Injected I2C error on bus 2 to stall it until timeout, everything + else continued even when bus 2 is stalled. Once we release the bus, + it exit out the thread when it realize that the fd is closed. +``` +Error reading bus 2 +device at bus 3 address 85 busy +device at bus 44 address 80 busy +device at bus 46 address 80 busy +device at bus 47 address 80 busy +device at bus 48 address 80 busy +device at bus 49 address 80 busy +device at bus 50 address 80 busy +device at bus 51 address 80 busy +device at bus 52 address 85 busy +device at bus 53 address 85 busy +device at bus 54 address 85 busy +device at bus 55 address 85 busy +device at bus 56 address 85 busy +device at bus 57 address 85 busy +device at bus 58 address 85 busy +device at bus 59 address 85 busy +File descriptor at bus 2 is closed due to timeout +``` +Change-Id: I71b80e204c04eed5d567b14c72943a5d01d5ff82 +Signed-off-by: Willy Tu <wltu@google.com> +--- + src/fru_device.cpp | 295 +++++++++++++++++++++++++-------------------- + 1 file changed, 166 insertions(+), 129 deletions(-) + +diff --git a/src/fru_device.cpp b/src/fru_device.cpp +index 3cc1718..284e123 100644 +--- a/src/fru_device.cpp ++++ b/src/fru_device.cpp +@@ -424,174 +424,211 @@ std::set<size_t> findI2CEeproms(int i2cBus, size_t first, size_t last, + return foundList; + } + +-int getBusFRUs(const std::shared_ptr<stdplus::ManagedFd>& fd, int first, +- int last, int bus, std::shared_ptr<DeviceMap> devices, +- const bool& powerIsOn, sdbusplus::asio::object_server& objServer) ++bool i2cFruScan(const std::weak_ptr<stdplus::ManagedFd>& weakFd, int first, ++ int last, int bus, const std::shared_ptr<DeviceMap>& devices, ++ const bool& powerIsOn, ++ sdbusplus::asio::object_server& objServer) + { +- std::future<int> future = std::async(std::launch::async, [&]() { +- // NOTE: When reading the devices raw on the bus, it can interfere with +- // the driver's ability to operate, therefore read eeproms first before +- // scanning for devices without drivers. Several experiments were run +- // and it was determined that if there were any devices on the bus +- // before the eeprom was hit and read, the eeprom driver wouldn't open +- // while the bus device was open. An experiment was not performed to see +- // if this issue was resolved if the i2c bus device was closed, but +- // hexdumps of the eeprom later were successful. +- +- // Scan for i2c eeproms loaded on this bus. +- std::set<size_t> skipList = findI2CEeproms(bus, first, last, devices); +- std::set<size_t>& failedItems = failedAddresses[bus]; +- std::set<size_t>& foundItems = fruAddresses[bus]; +- foundItems.clear(); +- +- auto busFind = busBlocklist.find(bus); +- if (busFind != busBlocklist.end()) ++ // NOTE: When reading the devices raw on the bus, it can interfere ++ // with the driver's ability to operate, therefore read eeproms ++ // first before scanning for devices without drivers. Several ++ // experiments were run and it was determined that if there were any ++ // devices on the bus before the eeprom was hit and read, the eeprom ++ // driver wouldn't open while the bus device was open. An experiment ++ // was not performed to see if this issue was resolved if the i2c ++ // bus device was closed, but hexdumps of the eeprom later were ++ // successful. ++ ++ // Scan for i2c eeproms loaded on this bus. ++ std::set<size_t> skipList = findI2CEeproms(bus, first, last, devices); ++ std::set<size_t>& failedItems = failedAddresses[bus]; ++ std::set<size_t>& foundItems = fruAddresses[bus]; ++ foundItems.clear(); ++ ++ auto busFind = busBlocklist.find(bus); ++ if (busFind != busBlocklist.end()) ++ { ++ const std::optional<std::set<size_t>>& busAddresses = busFind->second; ++ if (busAddresses.has_value()) + { +- if (busFind->second != std::nullopt) ++ for (const auto& address : *busAddresses) + { +- for (const auto& address : *(busFind->second)) +- { +- skipList.insert(address); +- } ++ skipList.insert(address); + } + } ++ } + +- std::set<size_t>* rootFailures = nullptr; +- int rootBus = getRootBus(bus); ++ std::set<size_t>* rootFailures = nullptr; ++ int rootBus = getRootBus(bus); + +- if (rootBus >= 0) ++ if (rootBus >= 0) ++ { ++ auto rootBusFind = busBlocklist.find(rootBus); ++ if (rootBusFind != busBlocklist.end()) + { +- auto rootBusFind = busBlocklist.find(rootBus); +- if (rootBusFind != busBlocklist.end()) ++ const std::optional<std::set<size_t>>& rootAddresses = ++ rootBusFind->second; ++ if (rootAddresses.has_value()) + { +- if (rootBusFind->second != std::nullopt) ++ for (const auto& rootAddress : *rootAddresses) + { +- for (const auto& rootAddress : *(rootBusFind->second)) +- { +- skipList.insert(rootAddress); +- } ++ skipList.insert(rootAddress); + } + } +- rootFailures = &(failedAddresses[rootBus]); +- foundItems = fruAddresses[rootBus]; + } ++ rootFailures = &(failedAddresses[rootBus]); ++ foundItems = fruAddresses[rootBus]; ++ } + +- constexpr int startSkipTargetAddr = 0; +- constexpr int endSkipTargetAddr = 12; ++ constexpr int startSkipTargetAddr = 0; ++ constexpr int endSkipTargetAddr = 12; + +- for (int ii = first; ii <= last; ii++) ++ for (int ii = first; ii <= last; ii++) ++ { ++ if (foundItems.find(ii) != foundItems.end()) + { +- if (foundItems.find(ii) != foundItems.end()) +- { +- continue; +- } +- if (skipList.find(ii) != skipList.end()) +- { +- continue; +- } +- // skipping since no device is present in this range +- if (ii >= startSkipTargetAddr && ii <= endSkipTargetAddr) +- { +- continue; +- } +- // Set target address +- if (ioctl(fd->get(), I2C_SLAVE, ii) < 0) +- { +- std::cerr << "device at bus " << bus << " address " << ii +- << " busy\n"; +- continue; +- } ++ continue; ++ } ++ if (skipList.find(ii) != skipList.end()) ++ { ++ continue; ++ } ++ // skipping since no device is present in this range ++ if (ii >= startSkipTargetAddr && ii <= endSkipTargetAddr) ++ { ++ continue; ++ } + +- // probe +- if (i2c_smbus_read_byte(fd->get()) < 0) +- { +- continue; +- } ++ std::shared_ptr<stdplus::ManagedFd> newFd = weakFd.lock(); ++ if (newFd == nullptr) ++ { ++ std::cerr << "File descriptor at bus " << bus ++ << " is closed due to timeout\n"; ++ return false; ++ } ++ // Set target address ++ if (ioctl(newFd->get(), I2C_SLAVE, ii) < 0) ++ { ++ std::cerr << "device at bus " << bus << " address " << ii ++ << " busy\n"; ++ continue; ++ } + +- if (debug) +- { +- std::cout << "something at bus " << bus << " addr " << ii +- << "\n"; +- } ++ // probe ++ if (i2c_smbus_read_byte(newFd->get()) < 0) ++ { ++ continue; ++ } + +- makeProbeInterface(bus, ii, objServer); ++ if (debug) ++ { ++ std::cout << "something at bus " << bus << " addr " << ii << "\n"; ++ } + +- if (failedItems.find(ii) != failedItems.end()) +- { +- // if we failed to read it once, unlikely we can read it later +- continue; +- } ++ makeProbeInterface(bus, ii, objServer); + +- if (rootFailures != nullptr) +- { +- if (rootFailures->find(ii) != rootFailures->end()) +- { +- continue; +- } +- } ++ if (failedItems.find(ii) != failedItems.end()) ++ { ++ // if we failed to read it once, unlikely we can read it ++ // later ++ continue; ++ } + +- /* Check for Device type if it is 8 bit or 16 bit */ +- std::optional<bool> is16Bit = isDevice16Bit(fd); +- if (!is16Bit.has_value()) ++ if (rootFailures != nullptr) ++ { ++ if (rootFailures->find(ii) != rootFailures->end()) + { +- std::cerr << "failed to read bus " << bus << " address " << ii +- << "\n"; +- if (powerIsOn) +- { +- failedItems.insert(ii); +- } + continue; + } +- bool is16BitBool{*is16Bit}; ++ } + +- auto readFunc = [is16BitBool, fd, ii](off_t offset, +- std::span<uint8_t> outbuf) { +- return readData(is16BitBool, false, fd, ii, offset, outbuf); +- }; +- FRUReader reader(std::move(readFunc)); +- std::string errorMessage = "bus " + std::to_string(bus) + +- " address " + std::to_string(ii); +- std::pair<std::vector<uint8_t>, bool> pair = +- readFRUContents(reader, errorMessage); +- const bool foundHeader = pair.second; +- +- if (!foundHeader && !is16BitBool) ++ /* Check for Device type if it is 8 bit or 16 bit */ ++ std::optional<bool> is16Bit = isDevice16Bit(newFd); ++ if (!is16Bit.has_value()) ++ { ++ std::cerr << "failed to read bus " << bus << " address " << ii ++ << "\n"; ++ if (powerIsOn) + { +- // certain FRU eeproms require bytewise reading. +- // otherwise garbage is read. e.g. SuperMicro PWS 920P-SQ +- +- auto readFunc = [is16BitBool, fd, +- ii](off_t offset, std::span<uint8_t> outbuf) { +- return readData(is16BitBool, true, fd, ii, offset, outbuf); +- }; +- FRUReader readerBytewise(std::move(readFunc)); +- pair = readFRUContents(readerBytewise, errorMessage); ++ failedItems.insert(ii); + } ++ continue; ++ } ++ bool is16BitBool{*is16Bit}; ++ ++ auto readFunc = [is16BitBool, newFd, ii](off_t offset, ++ std::span<uint8_t> outbuf) { ++ return readData(is16BitBool, false, newFd, ii, offset, outbuf); ++ }; ++ FRUReader reader(std::move(readFunc)); ++ std::string errorMessage = "bus " + std::to_string(bus) + " address " + ++ std::to_string(ii); ++ std::pair<std::vector<uint8_t>, bool> pair = ++ readFRUContents(reader, errorMessage); ++ const bool foundHeader = pair.second; ++ ++ if (!foundHeader && !is16BitBool) ++ { ++ // certain FRU eeproms require bytewise reading. ++ // otherwise garbage is read. e.g. SuperMicro PWS 920P-SQ + +- if (pair.first.empty()) +- { +- continue; +- } +- if (!validateFRUAreas(pair.first)) +- { +- std::cerr << "Checksum failure bus " << bus << " address " +- << ii << "\n"; +- continue; +- } ++ auto readFunc = [is16BitBool, newFd, ++ ii](off_t offset, std::span<uint8_t> outbuf) { ++ return readData(is16BitBool, true, newFd, ii, offset, outbuf); ++ }; ++ FRUReader readerBytewise(std::move(readFunc)); ++ pair = readFRUContents(readerBytewise, errorMessage); ++ } + +- devices->emplace(ii, pair.first); +- fruAddresses[bus].insert(ii); ++ if (pair.first.empty()) ++ { ++ continue; ++ } ++ if (!validateFRUAreas(pair.first)) ++ { ++ std::cerr << "Checksum failure bus " << bus << " address " << ii ++ << "\n"; ++ continue; ++ } ++ ++ devices->emplace(ii, pair.first); ++ fruAddresses[bus].insert(ii); ++ } ++ return true; ++} ++ ++bool getBusFRUs(const std::shared_ptr<stdplus::ManagedFd>& fd, int first, ++ int last, int bus, const std::shared_ptr<DeviceMap>& devices, ++ const bool& powerIsOn, ++ sdbusplus::asio::object_server& objServer) ++{ ++ std::weak_ptr<stdplus::ManagedFd> weakFd = fd; ++ auto promise = std::make_shared<std::promise<bool>>(); ++ std::future<bool> future = promise->get_future(); ++ ++ std::thread t( ++ [weakFd, first, last, bus, devices, &powerIsOn, &objServer, promise]() { ++ try ++ { ++ promise->set_value(i2cFruScan(weakFd, first, last, bus, devices, ++ powerIsOn, objServer)); ++ } ++ catch (...) ++ { ++ promise->set_exception(std::current_exception()); + } +- return 1; + }); ++ + std::future_status status = + future.wait_for(std::chrono::seconds(busTimeoutSeconds)); + if (status == std::future_status::timeout) + { + std::cerr << "Error reading bus " << bus << "\n"; +- return -1; ++ t.detach(); ++ return false; + } + ++ t.join(); + return future.get(); + } + +-- +2.53.0.rc1.225.gd81095ad13-goog +
diff --git a/recipes-phosphor/configuration/entity-manager/0003-fru-device-Add-retries-on-locked-up-buses.patch b/recipes-phosphor/configuration/entity-manager/0003-fru-device-Add-retries-on-locked-up-buses.patch new file mode 100644 index 0000000..cf4f5d2 --- /dev/null +++ b/recipes-phosphor/configuration/entity-manager/0003-fru-device-Add-retries-on-locked-up-buses.patch
@@ -0,0 +1,184 @@ +From a892b143df6961c954df712a71333522d902576b Mon Sep 17 00:00:00 2001 +From: Willy Tu <wltu@google.com> +Date: Thu, 29 Jan 2026 23:39:19 +0000 +Subject: [PATCH 3/5] fru-device: Add retries on locked up buses + +Try to rescan the buses that timed out to make sure we don't +accidentally drop the i2c bus when it was busy during rescan. + +Added meson options for the retry count and delays. + +Patch Tracking Bug: b/483087971 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/87123 +Upstream-Status: Submitted +Justification: Need cherry-pick since we are not rebasing to latest entity-manager. +Tested: +``` +Error reading bus 2 +device at bus 3 address 85 busy +device at bus 44 address 80 busy +... +Retrying /dev/i2c-2 in 5 seconds... +File descriptor at bus 2 is closed due to timeout +All I2C devices discovered successfully. +``` +Change-Id: I3ebe5bef9be3bf74292a34ee8b1c0881f5cb0a94 +Signed-off-by: Willy Tu <wltu@google.com> +--- + meson_options.txt | 12 +++++++++++ + src/fru_device.cpp | 53 +++++++++++++++++++++++++++++++++++++++------- + src/meson.build | 12 +++++++++++ + 3 files changed, 69 insertions(+), 8 deletions(-) + +diff --git a/meson_options.txt b/meson_options.txt +index c97b7f8..7cf9336 100644 +--- a/meson_options.txt ++++ b/meson_options.txt +@@ -13,3 +13,15 @@ option( + option( + 'fru-device-resizefru', value : false, type: 'boolean', description: 'Allow FruDevice to resize FRU areas.', + ) ++option( ++ 'fru-device-maxretries', ++ type: 'integer', ++ value: 0, ++ description: 'Maximum number of retries for I2C device discovery.', ++) ++option( ++ 'fru-device-retrydelay', ++ type: 'integer', ++ value: 5, ++ description: 'Delay in seconds between retries for I2C device discovery.', ++) +\ No newline at end of file +diff --git a/src/fru_device.cpp b/src/fru_device.cpp +index 284e123..740a539 100644 +--- a/src/fru_device.cpp ++++ b/src/fru_device.cpp +@@ -712,10 +712,12 @@ void loadBlocklist(const char* path) + } + } + +-static void findI2CDevices(const std::vector<fs::path>& i2cBuses, +- BusMap& busmap, const bool& powerIsOn, +- sdbusplus::asio::object_server& objServer) ++static std::vector<fs::path> ++ findI2CDevices(const std::vector<fs::path>& i2cBuses, BusMap& busmap, ++ const bool& powerIsOn, ++ sdbusplus::asio::object_server& objServer) + { ++ std::vector<fs::path> retryI2cBuses; + for (const auto& i2cBus : i2cBuses) + { + int bus = busStrToInt(i2cBus.string()); +@@ -783,14 +785,18 @@ static void findI2CDevices(const std::vector<fs::path>& i2cBuses, + } + + // fd is closed in this function in case the bus locks up +- // The FRU device is supposed to be in [0x50, 0x57] in Google. +- getBusFRUs(fd, 0x50, 0x57, bus, device, powerIsOn, objServer); ++ // If the bus is locked up, we will retry for up to 5 times. ++ if (!getBusFRUs(fd, 0x50, 0x57, bus, device, powerIsOn, objServer)) ++ { ++ retryI2cBuses.push_back(i2cBus); ++ } + + if (debug) + { + std::cerr << "Done scanning bus " << bus << "\n"; + } + } ++ return retryI2cBuses; + } + + // this class allows an async response after all i2c devices are discovered +@@ -800,17 +806,43 @@ struct FindDevicesWithCallback : + FindDevicesWithCallback(const std::vector<fs::path>& i2cBuses, + BusMap& busmap, const bool& powerIsOn, + sdbusplus::asio::object_server& objServer, +- std::function<void(void)>&& callback) : ++ std::function<void()>&& callback, ++ size_t retries = maxRetries) : + _i2cBuses(i2cBuses), _busMap(busmap), _powerIsOn(powerIsOn), +- _objServer(objServer), _callback(std::move(callback)) ++ _objServer(objServer), _callback(std::move(callback)), _retries(retries) + {} + ~FindDevicesWithCallback() + { + _callback(); ++ if (_retryI2cBuses.empty()) ++ { ++ std::cerr << "All I2C devices discovered successfully.\n"; ++ return; ++ } ++ if (maxRetries == 0) ++ { ++ return; ++ } ++ if (_retries == 0) ++ { ++ std::cerr << ++ "Failed to discover all I2C devices after " << maxRetries << " retries."; ++ ++ return; ++ } ++ ++ auto scan = std::make_shared<FindDevicesWithCallback>( ++ _i2cBuses, _busMap, _powerIsOn, _objServer, std::move(_callback), ++ _retries - 1); ++ auto timer = std::make_shared<boost::asio::steady_timer>(io); ++ timer->expires_after(retryDelay); ++ timer->async_wait( ++ [timer, scan](const boost::system::error_code&) { scan->run(); }); + } + void run() + { +- findI2CDevices(_i2cBuses, _busMap, _powerIsOn, _objServer); ++ _retryI2cBuses = findI2CDevices(_i2cBuses, _busMap, _powerIsOn, ++ _objServer); + } + + const std::vector<fs::path>& _i2cBuses; +@@ -818,6 +850,11 @@ struct FindDevicesWithCallback : + const bool& _powerIsOn; + sdbusplus::asio::object_server& _objServer; + std::function<void(void)> _callback; ++ size_t _retries; ++ std::vector<fs::path> _retryI2cBuses; ++ ++ static constexpr size_t maxRetries = FRU_DEVICE_MAXRETRIES; ++ static constexpr std::chrono::seconds retryDelay{FRU_DEVICE_RETRYDELAY}; + }; + + void addFruObjectToDbus( +diff --git a/src/meson.build b/src/meson.build +index 9fa3b9c..1ba7cd1 100644 +--- a/src/meson.build ++++ b/src/meson.build +@@ -20,11 +20,23 @@ executable( + install: true, + ) + ++max_retries = get_option('fru-device-maxretries') ++retry_delay = get_option('fru-device-retrydelay') ++if max_retries > 50 ++ error('fru-device-maxretries is too large') ++endif ++ ++if retry_delay < 1 ++ error('fru-device-retrydelay is too small') ++endif ++ + if get_option('fru-device') + cpp_args_fd = cpp_args + if get_option('fru-device-resizefru') + cpp_args_fd = cpp_args_fd + ['-DENABLE_FRU_AREA_RESIZE'] + endif ++ cpp_args_fd = cpp_args_fd + ['-DFRU_DEVICE_MAXRETRIES=' + max_retries.to_string()] ++ cpp_args_fd = cpp_args_fd + ['-DFRU_DEVICE_RETRYDELAY=' + retry_delay.to_string()] + executable( + 'fru-device', + 'expression.cpp', +-- +2.53.0.rc1.225.gd81095ad13-goog +
diff --git a/recipes-phosphor/configuration/entity-manager/0004-fru-device-Reuse-old-dbus-path-if-possible.patch b/recipes-phosphor/configuration/entity-manager/0004-fru-device-Reuse-old-dbus-path-if-possible.patch new file mode 100644 index 0000000..2a3c87b --- /dev/null +++ b/recipes-phosphor/configuration/entity-manager/0004-fru-device-Reuse-old-dbus-path-if-possible.patch
@@ -0,0 +1,125 @@ +From cec64a9aa9f81536615f1ad653cadafa39d1a347 Mon Sep 17 00:00:00 2001 +From: Willy Tu <wltu@google.com> +Date: Wed, 21 Jan 2026 07:50:13 +0000 +Subject: [PATCH 4/5] fru-device: Reuse old dbus path if possible + +There is a race condition between Entity Manager where call GetSubTree +for `xyz.openbmc_project.FruDevice` interfaces based on the Probe +condition and return a list of dbus object... and FruDevice rescaning +one bus before the dbus objects are processed. + +Example, +- Entity Manager GetSubTrees return [Fru, Fru_1] +- `/dev/i2c-2` which has Fru_1 gets removed/added, resulting in Fru0 + being removed and Fru_2 getting added. +- Entity Manager calls GetAll on Fru_1 and failed, retuling in Fru_1 + being removed from inventory. + +Error: +``` +Jan 20 00:25:02 bmc entity-manager[22410]: error calling getall on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:04 bmc entity-manager[22410]: error calling getall on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:06 bmc entity-manager[22410]: error calling getall on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:08 bmc entity-manager[22410]: error calling getall on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:10 bmc entity-manager[22410]: error calling getall on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:12 bmc entity-manager[22410]: retries exhausted on xyz.openbmc_project.FruDevice /xyz/openbmc_project/FruDevice/Fru_1 xyz.openbmc_project.FruDevice +Jan 20 00:25:33 bmc entity-manager[22410]: Inventory Removed: Fru_1 +``` + +Normally this wouldn't be an issue because something else is getting +added/removed in dbus resulting in `propertiesChangedCallback + +Patch Tracking Bug: b/483088223 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/86996 +Upstream-Status: Submitted +Justification: Need cherry-pick since we are not rebasing to latest entity-manager. +Tested: +With the new change, the name is reused and won't have this issue. +Change-Id: I77ffd1ebd3ed8e61a129f9d0203dcbfe3eccc83c +Signed-off-by: Willy Tu <wltu@google.com> +--- + src/fru_device.cpp | 31 ++++++++++++++++++++++--------- + 1 file changed, 22 insertions(+), 9 deletions(-) + +diff --git a/src/fru_device.cpp b/src/fru_device.cpp +index 72e9889..5a3e074 100644 +--- a/src/fru_device.cpp ++++ b/src/fru_device.cpp +@@ -861,7 +861,9 @@ void addFruObjectToDbus( + std::shared_ptr<sdbusplus::asio::dbus_interface>>& dbusInterfaceMap, + uint32_t bus, uint32_t address, size_t& unknownBusObjectCount, + const bool& powerIsOn, sdbusplus::asio::object_server& objServer, +- std::shared_ptr<sdbusplus::asio::connection>& systemBus) ++ std::shared_ptr<sdbusplus::asio::connection>& systemBus, ++ const boost::container::flat_map<std::pair<size_t, size_t>, std::string>& ++ oldNames = {}) + { + boost::container::flat_map<std::string, std::string> formattedFRU; + +@@ -876,16 +878,25 @@ void addFruObjectToDbus( + std::string productName = "/xyz/openbmc_project/FruDevice/" + + optionalProductName.value(); + +- std::optional<int> index = findIndexForFRU(dbusInterfaceMap, productName); +- if (index.has_value()) ++ auto key = std::pair<size_t, size_t>(bus, address); ++ auto oldName = oldNames.find(key); ++ if (oldName != oldNames.end()) + { +- productName += "_"; +- productName += std::to_string(++(*index)); ++ productName = oldName->second; ++ } ++ else ++ { ++ std::optional<int> index = findIndexForFRU(dbusInterfaceMap, ++ productName); ++ if (index.has_value()) ++ { ++ productName += "_"; ++ productName += std::to_string(++(*index)); ++ } + } +- + std::shared_ptr<sdbusplus::asio::dbus_interface> iface = + objServer.add_interface(productName, "xyz.openbmc_project.FruDevice"); +- dbusInterfaceMap[std::pair<size_t, size_t>(bus, address)] = iface; ++ dbusInterfaceMap[key] = iface; + + for (auto& property : formattedFRU) + { +@@ -1101,11 +1112,13 @@ void rescanOneBus( + sdbusplus::asio::object_server& objServer, + std::shared_ptr<sdbusplus::asio::connection>& systemBus) + { ++ boost::container::flat_map<std::pair<size_t, size_t>, std::string> oldNames; + for (auto device = foundDevices.begin(); device != foundDevices.end();) + { + if (device->first.first == static_cast<size_t>(busNum)) + { + objServer.remove_interface(device->second); ++ oldNames.emplace(device->first, device->second->get_object_path()); + device = foundDevices.erase(device); + } + else +@@ -1132,7 +1145,7 @@ void rescanOneBus( + auto scan = std::make_shared<FindDevicesWithCallback>( + i2cBuses, busmap, powerIsOn, objServer, + [busNum, &busmap, &dbusInterfaceMap, &unknownBusObjectCount, &powerIsOn, +- &objServer, &systemBus]() { ++ &objServer, &systemBus, oldNames{std::move(oldNames)}]() { + for (auto busIface = dbusInterfaceMap.begin(); + busIface != dbusInterfaceMap.end();) + { +@@ -1156,7 +1169,7 @@ void rescanOneBus( + addFruObjectToDbus(device.second, dbusInterfaceMap, + static_cast<uint32_t>(busNum), device.first, + unknownBusObjectCount, powerIsOn, objServer, +- systemBus); ++ systemBus, oldNames); + } + }); + scan->run(); +-- +2.53.0.rc1.225.gd81095ad13-goog +
diff --git a/recipes-phosphor/configuration/entity-manager/0005-fru_device-fix-I2C-functionality-checking-error.patch b/recipes-phosphor/configuration/entity-manager/0005-fru_device-fix-I2C-functionality-checking-error.patch new file mode 100644 index 0000000..76e2044 --- /dev/null +++ b/recipes-phosphor/configuration/entity-manager/0005-fru_device-fix-I2C-functionality-checking-error.patch
@@ -0,0 +1,34 @@ +From c3d544ac687220beb1a7ba0806b3500a59e20824 Mon Sep 17 00:00:00 2001 +From: Haiyue Wang <haiyuewa@163.com> +Date: Thu, 6 Nov 2025 09:27:59 +0800 +Subject: [PATCH 5/5] fru_device: fix I2C functionality checking error + +The I2C functionality 'I2C_FUNC_SMBUS_READ_I2C_BLOCK' checking missed +the logical AND '&' operation value. + +Patch Tracking Bug: b/483088677 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/86996 +Upstream-Status: Accepted +Justification: Need cherry-pick since we are not rebasing to latest entity-manager. +Change-Id: I0ce0842c9a5949d354446786e41ae84ba4ef0ea7 +Signed-off-by: Haiyue Wang <haiyuewa@163.com> +--- + src/fru_device.cpp | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/fru_device.cpp b/src/fru_device.cpp +index 6473243..db3efc0 100644 +--- a/src/fru_device.cpp ++++ b/src/fru_device.cpp +@@ -767,7 +767,7 @@ static std::vector<fs::path> + continue; + } + if (((funcs & I2C_FUNC_SMBUS_READ_BYTE) == 0U) || +- ((I2C_FUNC_SMBUS_READ_I2C_BLOCK) == 0)) ++ ((funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK) == 0)) + { + std::cerr << "Error: Can't use SMBus Receive Byte command bus " + << bus << "\n"; +-- +2.53.0.rc2.204.g2597b5adb4-goog +
diff --git a/recipes-phosphor/configuration/entity-manager_%.bbappend b/recipes-phosphor/configuration/entity-manager_%.bbappend index 7267afa..3479774 100644 --- a/recipes-phosphor/configuration/entity-manager_%.bbappend +++ b/recipes-phosphor/configuration/entity-manager_%.bbappend
@@ -18,8 +18,16 @@ file://0009-fru-device-Increase-FRU-scan-wait-on-Power-signal.patch \ file://scripts/config_preprocess.py \ file://scripts/config_preprocess_test.py \ + file://0001-fru-device-Use-stdplus-ManagedFd-for-fd-open.patch \ + file://0002-fru-device-Exit-gracefully-if-getBusFRUs-times-out.patch \ + file://0003-fru-device-Add-retries-on-locked-up-buses.patch \ + file://0004-fru-device-Reuse-old-dbus-path-if-possible.patch \ + file://0005-fru_device-fix-I2C-functionality-checking-error.patch \ " +DEPENDS:append:gbmc = " stdplus" +EXTRA_OEMESON:append:gbmc = " -Dfru-device-retrydelay=5 -Dfru-device-maxretries=5" + USB_DEVICE_OPTION:gbmc := "-Dusb-device=true, -Dusb-device=false, stdplus" PACKAGECONFIG[usb-device] := "${USB_DEVICE_OPTION}"