[DPDK/ethdev Bug 2100] net/ice: ice_get_module_eeprom() silently returns zeroed data when all I2C read retries fail

[bugzilla@dpdk.org]

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: dev@dpdk.org
          Reporter: marcel@telka.sk
  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.