From 3b06d2d0794589e4bd53169236f01f23e20c0e31 Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Wed, 29 Apr 2026 17:13:52 +0800 Subject: [PATCH] [syncd] Fix PORT_PHY_ATTR::collectData to check initAttrData return value (#1872) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Why I did it PORT_PHY_ATTR::collectData in FlexCounter.cpp calls `initAttrData` without checking its return value. When Broadcom SAI does not support attributes like `SAI_PORT_ATTR_RX_SIGNAL_DETECT` (attribute 149), the subsequent `sai_get` call fails with `Feature unavailable (0xfffffff0)`, generating ERR-level syslog entries every ~10 seconds (FlexCounter polling interval). This causes **all tests using loganalyzer** to fail at teardown on Arista-7260CX3 (Broadcom) platforms. On 202511 image `20251110.20`, this produced **8,700+ test failures**. The regression is triggered by the combination of: - sonic-sairedis #1674 / #1785 (adds PORT_PHY_ATTR FlexCounter polling support) - sonic-swss #4223 / #4407 (activates PORT_PHY_ATTR polling via FLEX_COUNTER_DB entries) A partial fix was applied in #1799 / #1815 for PORT_PHY_SERDES_ATTR::collectData, but the same fix was **not** applied to PORT_PHY_ATTR::collectData. #### How I did it - Changed `initAttrData` return type from `void` to `bool` - Each switch case now returns `true` on success, error paths return `false` - In `collectData`, check the return value of `initAttrData` in the init loop - If any attribute initialization fails, log a WARN and skip the entire object (`continue`) - This matches the pattern already used in PORT_PHY_SERDES_ATTR::collectData (fixed by #1799) #### How to verify it 1. Deploy on an Arista-7260CX3 (Broadcom) testbed with 202511 image 2. Verify no more `RX signal detect` ERR syslog entries from syncd 3. Run any test with loganalyzer enabled — teardown should pass #### Related PRs - #1674 — PORT_PHY_ATTR flex counter support (master) - #1785 — cherry-pick to 202511 - #1799 — Partial fix for PORT_PHY_SERDES_ATTR (master) - #1815 — cherry-pick of #1799 to 202511 - #1855 — Same fix targeting 202511 branch - sonic-swss #4223 / #4407 — Activation trigger #### ADO Reference - Microsoft ADO PBI: 37325902 - Arista issue: https://github.com/aristanetworks/sonic-qual.msft/issues/1232 - Public tracking: https://github.com/sonic-net/sonic-mgmt/issues/24023 Signed-off-by: Sonic Build Admin --- syncd/FlexCounter.cpp | 30 ++++++--- tests/aspell.en.pws | 1 + unittest/syncd/TestPortPhyAttr.cpp | 98 ++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 9eb23ca1..c7f7700f 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1844,7 +1844,7 @@ class PortPhyAttrContext : public AttrContextid : %d", rid, attr->id); - return; + return false; } const auto &attrLaneCountMap = outer_it->second; @@ -1871,7 +1871,7 @@ class PortPhyAttrContext : public AttrContextid, rid); - return; + return false; } auto portLaneCount = inner_it->second; @@ -1883,23 +1883,23 @@ class PortPhyAttrContext : public AttrContextrxSignalDetectData.resize(portLaneCount); attr->value.portlanelatchstatuslist.count = portLaneCount; attr->value.portlanelatchstatuslist.list = data->rxSignalDetectData.data(); - break; + return true; case SAI_PORT_ATTR_FEC_ALIGNMENT_LOCK: data->fecAlignmentLockData.resize(portLaneCount); attr->value.portlanelatchstatuslist.count = portLaneCount; attr->value.portlanelatchstatuslist.list = data->fecAlignmentLockData.data(); - break; + return true; case SAI_PORT_ATTR_RX_SNR: data->rxSnrData.resize(portLaneCount); attr->value.portsnrlist.count = portLaneCount; attr->value.portsnrlist.list = data->rxSnrData.data(); - break; + return true; default: SWSS_LOG_ERROR("PORT_PHY_ATTR: initAttrData: Unsupported attr-id : %d", attr->id); - break; + return false; } } @@ -1981,10 +1981,22 @@ class PortPhyAttrContext : public AttrContextremoveCounter(testPortOid); } + +/** + * Test that collectData() gracefully skips objects when initAttrData() fails. + * + * This simulates the Broadcom 7260CX3 scenario where SAI does not support + * SAI_PORT_ATTR_RX_SIGNAL_DETECT ΓÇö the lane count query returns NOT_SUPPORTED + * instead of BUFFER_OVERFLOW, so m_portLaneCountMap has no entry for the + * attribute, and initAttrData() returns false. + * + * Before the fix, collectData() would ignore the initAttrData() return value + * and call sai_get() anyway, which would fail and emit ERR syslog every 10s. + * After the fix, collectData() skips the object entirely. + * + * See: https://github.com/sonic-net/sonic-mgmt/issues/24023 + */ +TEST_F(TestPortPhyAttr, CollectDataSkipsWhenInitAttrDataFails) +{ + // Use a different port OID to avoid picking up stale Redis data + // from the previous test (CollectDataAndValidateCountersDB). + sai_object_id_t failPortOid = 0x1000000000099; + sai_object_id_t failPortRid = 0x1000000000099; + + int getCallCount = 0; + + // Mock SAI: return NOT_SUPPORTED for the lane count query + // This means m_portLaneCountMap will NOT have an entry for this RID/attr, + // so initAttrData() will return false when collectData() calls it. + sai->mock_get = [&getCallCount](sai_object_type_t object_type, + sai_object_id_t object_id, + uint32_t attr_count, + sai_attribute_t *attr_list) -> sai_status_t + { + if (object_type != SAI_OBJECT_TYPE_PORT) { + return SAI_STATUS_INVALID_PARAMETER; + } + + // Track calls after addObject's lane count query phase + getCallCount++; + + // Return NOT_SUPPORTED for the lane count query (attr_count == 1) + // This simulates Broadcom SAI on 7260CX3 not supporting RX_SIGNAL_DETECT + if (attr_count == 1) { + return SAI_STATUS_NOT_SUPPORTED; + } + + // If collectData() reaches sai_get with multiple attrs, that's a bug + // The fix should prevent this from happening + return SAI_STATUS_FAILURE; + }; + + vector portPhyAttrValues; + std::string attrIds = "SAI_PORT_ATTR_RX_SIGNAL_DETECT,SAI_PORT_ATTR_FEC_ALIGNMENT_LOCK,SAI_PORT_ATTR_RX_SNR"; + portPhyAttrValues.emplace_back(PORT_PHY_ATTR_ID_LIST, attrIds); + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + + // addObject will call updatePortLaneCountMap which queries SAI for lane count. + // Since SAI returns NOT_SUPPORTED, m_portLaneCountMap will be empty. + flexCounter->addCounter(failPortOid, failPortRid, portPhyAttrValues); + + int getCallsAfterAdd = getCallCount; + + vector pluginValues; + pluginValues.emplace_back(POLL_INTERVAL_FIELD, "1000"); + pluginValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + pluginValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + flexCounter->addCounterPlugin(pluginValues); + + usleep(1000 * 1050); // 1.05 seconds - one poll cycle + + // Verify that collectData did NOT make additional sai_get calls + // beyond what addObject's updatePortLaneCountMap already made. + // If initAttrData fails, collectData should skip the object entirely. + EXPECT_EQ(getCallCount, getCallsAfterAdd) + << "collectData should not call sai_get when initAttrData fails. " + << "Additional sai_get calls indicate the initAttrData return value was ignored."; + + // Verify no data was written to COUNTERS_DB + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, PORT_PHY_ATTR_TABLE, false); + + std::string expectedKey = toOid(failPortOid); + + std::string rxSignalDetectValue; + bool found = countersTable.hget(expectedKey, "phy_rx_signal_detect", rxSignalDetectValue); + EXPECT_FALSE(found) << "phy_rx_signal_detect should NOT be in COUNTERS_DB when initAttrData fails"; + + std::string fecAlignmentValue; + found = countersTable.hget(expectedKey, "pcs_fec_lane_alignment_lock", fecAlignmentValue); + EXPECT_FALSE(found) << "pcs_fec_lane_alignment_lock should NOT be in COUNTERS_DB when initAttrData fails"; + + std::string rxSnrValue; + found = countersTable.hget(expectedKey, "rx_snr", rxSnrValue); + EXPECT_FALSE(found) << "rx_snr should NOT be in COUNTERS_DB when initAttrData fails"; + + flexCounter->removeCounter(failPortOid); +}