https://bugs.dpdk.org/show_bug.cgi?id=2100
Bug ID: 2100
Summary: net/ice: ice_get_module_eeprom() silently returns
zeroed data when all I2C read retries fail
Product: DPDK
Version: unspecified
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: ethdev
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
https://github.com/DPDK/dpdk/blob/a13c1053360ee26bbacca9f883d3f3cb83a0f366/drivers/net/intel/ice/ice_ethdev.c#L6742-L6767
for (j = 0; j < I2C_BUSY_TRY_TIMES; j++) {
status = ice_aq_sff_eeprom(hw, 0, addr, offset,
page, !is_sfp, value,
SFF_READ_BLOCK_SIZE,
0, NULL);
PMD_DRV_LOG(DEBUG, "SFF %02X %02X %02X %X = "
"%02X%02X%02X%02X."
"%02X%02X%02X%02X (%X)",
addr, offset, page, is_sfp,
value[0], value[1],
value[2], value[3],
value[4], value[5],
value[6], value[7],
status);
if (status) {
usleep_range(I2C_USLEEP_MIN_TIME,
I2C_USLEEP_MAX_TIME);
memset(value, 0, SFF_READ_BLOCK_SIZE);
continue;
}
break;
}
/* Make sure we have enough room for the new block */
if ((i + SFF_READ_BLOCK_SIZE) <= info->length)
memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
Two separate problems in this retry loop's failure handling:
1. Zeroed data accepted as a successful read. status is never checked
after the loop exits. If all I2C_BUSY_TRY_TIMES (4) attempts fail, value
has been memset to zero by the last failed attempt, and that zeroed
block is still unconditionally copied into the output buffer as if
valid. The function falls through to return 0 (success) regardless.
A persistent I2C/admin-queue failure for any single 8-byte block -- the
comment directly above this code acknowledges I2C bus contention with
NIC link management is "normal" -- is therefore silently reported as a
successful read, with that block's bytes coming back all-zero,
indistinguishable from genuine zero-valued EEPROM content.
2. Debug log prints unread data as if it were the read result.
PMD_DRV_LOG fires unconditionally right after the ice_aq_sff_eeprom()
call, before the if (status) check and before the failure-path
memset(value, 0, ...). So on a failed attempt, the log line prints
value alongside the (correctly) failed status -- but value at that
point holds stale data: leftover from the previous block's last
successful read on a block's first failed attempt, or zeros from the
prior attempt's own memset on any subsequent retry of that same block.
Either way, it's never the result of the failed call being logged,
which can mislead debugging by making a failed read look like a
plausible real value.
Suggested fix:
bool block_ok = false;
for (j = 0; j < I2C_BUSY_TRY_TIMES; j++) {
status = ice_aq_sff_eeprom(...);
if (status) {
PMD_DRV_LOG(DEBUG, "SFF %02X %02X %02X %X read failed (%X)",
addr, offset, page, is_sfp, status);
usleep_range(...);
memset(value, 0, SFF_READ_BLOCK_SIZE);
continue;
}
PMD_DRV_LOG(DEBUG, "SFF %02X %02X %02X %X = "
"%02X%02X%02X%02X.%02X%02X%02X%02X",
addr, offset, page, is_sfp,
value[0], value[1], value[2], value[3],
value[4], value[5], value[6], value[7]);
block_ok = true;
break;
}
if (!block_ok)
return -EIO;
--
You are receiving this mail because:
You are the assignee for the bug.
