[orchagent]: admin-disable port before setPortSerdesAttribute() (#2831)

What I did Admin-disable port before applying media-based NPU serdes attributes from media_settings.json. Why I did it This fix is needed along with xcvrd changes #377. Maintain deterministic behavior of interface bring-up, by toggling host_tx_ready flag, which will trigger CMIS reinit for the module once NPU serdes params have been applied. How I verified it Validated media_settings being notified and applied on Cisco 8111 with subject changes combined with diffs from #360, #377 and #15453. Will update final results once #377 is frozen. Details if related Proposal for xcvrd changes: #356 Signed-off-by: Aman Singhal <[email protected]>
sonic-net · Aug 24, 2023 · e6f134f · e6f134f
1 parent a67d4a7
commit e6f134f
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 9 deletions.
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -3976,17 +3976,34 @@ void PortsOrch::doPortTask(Consumer &consumer)
                         p.m_preemphasis = serdes_attr;
                         m_portList[p.m_alias] = p;
                     }
-                    else if (setPortSerdesAttribute(p.m_port_id, gSwitchId, serdes_attr))
-                    {
-                        SWSS_LOG_NOTICE("Set port %s preemphasis is success", p.m_alias.c_str());
-                        p.m_preemphasis = serdes_attr;
-                        m_portList[p.m_alias] = p;
-                    }
                     else
                     {
-                        SWSS_LOG_ERROR("Failed to set port %s pre-emphasis", p.m_alias.c_str());
-                        it++;
-                        continue;
+                        if (p.m_admin_state_up)
+                        {
+                                /* Bring port down before applying serdes attribute*/
+                                if (!setPortAdminStatus(p, false))
+                                {
+                                    SWSS_LOG_ERROR("Failed to set port %s admin status DOWN to set serdes attr", p.m_alias.c_str());
+                                    it++;
+                                    continue;
+                                }
+
+                                p.m_admin_state_up = false;
+                                m_portList[p.m_alias] = p;
+                        }
+
+                        if (setPortSerdesAttribute(p.m_port_id, gSwitchId, serdes_attr))
+                        {
+                            SWSS_LOG_NOTICE("Set port %s SI settings is successful", p.m_alias.c_str());
+                            p.m_preemphasis = serdes_attr;
+                            m_portList[p.m_alias] = p;
+                        }
+                        else
+                        {
+                            SWSS_LOG_ERROR("Failed to set port %s SI settings", p.m_alias.c_str());
+                            it++;
+                            continue;
+                        }
                     }
                 }
 

diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp
@@ -71,6 +71,8 @@ namespace portsorch_test
     uint32_t _sai_set_port_fec_count;
     int32_t _sai_port_fec_mode;
     uint32_t _sai_set_pfc_mode_count;
+    uint32_t _sai_set_admin_state_up_count;
+    uint32_t _sai_set_admin_state_down_count;
     sai_status_t _ut_stub_sai_set_port_attribute(
         _In_ sai_object_id_t port_id,
         _In_ const sai_attribute_t *attr)
@@ -89,6 +91,14 @@ namespace portsorch_test
 	{
 	    _sai_set_pfc_mode_count++;
         }
+	else if (attr[0].id == SAI_PORT_ATTR_ADMIN_STATE)
+	{
+            if (attr[0].value.booldata) {
+	        _sai_set_admin_state_up_count++;
+            } else {
+	        _sai_set_admin_state_down_count++;
+            }
+        }
         return pold_sai_port_api->set_port_attribute(port_id, attr);
     }
 
@@ -760,6 +770,95 @@ namespace portsorch_test
         cleanupPorts(gPortsOrch);
     }
 
+    /**
+     * Test that verifies admin-disable then admin-enable during setPortSerdesAttribute()
+     */
+    TEST_F(PortsOrchTest, PortSerdesConfig)
+    {
+        auto portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
+
+        // Get SAI default ports
+        auto &ports = defaultPortList;
+        ASSERT_TRUE(!ports.empty());
+
+        // Generate port config
+        for (const auto &cit : ports)
+        {
+            portTable.set(cit.first, cit.second);
+        }
+
+        // Set PortConfigDone
+        portTable.set("PortConfigDone", { { "count", std::to_string(ports.size()) } });
+
+        // Refill consumer
+        gPortsOrch->addExistingData(&portTable);
+
+        // Apply configuration
+        static_cast<Orch*>(gPortsOrch)->doTask();
+
+        // Generate basic port config
+        std::deque<KeyOpFieldsValuesTuple> kfvBasic = {{
+            "Ethernet0",
+            SET_COMMAND, {
+                { "speed",               "100000"            },
+                { "fec",                 "rs"                },
+                { "mtu",                 "9100"              },
+                { "admin_status",        "up"                }
+            }
+        }};
+
+        // Refill consumer
+        auto consumer = dynamic_cast<Consumer*>(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME));
+        consumer->addToSync(kfvBasic);
+
+        // Apply configuration
+        static_cast<Orch*>(gPortsOrch)->doTask();
+
+        // Get port and verify admin status
+        Port p;
+        ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p));
+        ASSERT_TRUE(p.m_admin_state_up);
+
+        // Generate port serdes config
+        std::deque<KeyOpFieldsValuesTuple> kfvSerdes = {{
+            "Ethernet0",
+            SET_COMMAND, {
+                { "admin_status", "up"              },
+                { "idriver"     , "0x6,0x6,0x6,0x6" }
+            }
+        }};
+
+        // Refill consumer
+        consumer->addToSync(kfvSerdes);
+
+        _hook_sai_port_api();
+        uint32_t current_sai_api_call_count = _sai_set_admin_state_down_count;
+
+        // Apply configuration
+        static_cast<Orch*>(gPortsOrch)->doTask();
+
+        _unhook_sai_port_api();
+
+        ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p));
+        ASSERT_TRUE(p.m_admin_state_up);
+
+        // Verify idriver
+        std::vector<std::uint32_t> idriver = { 0x6, 0x6, 0x6, 0x6 };
+        ASSERT_EQ(p.m_preemphasis.at(SAI_PORT_SERDES_ATTR_IDRIVER), idriver);
+
+        // Verify admin-disable then admin-enable
+        ASSERT_EQ(_sai_set_admin_state_down_count, ++current_sai_api_call_count);
+        ASSERT_EQ(_sai_set_admin_state_up_count, current_sai_api_call_count);
+
+        // Dump pending tasks
+        std::vector<std::string> taskList;
+        gPortsOrch->dumpPendingTasks(taskList);
+        ASSERT_TRUE(taskList.empty());
+
+        // Cleanup ports
+        cleanupPorts(gPortsOrch);
+    }
+
     /**
      * Test that verifies PortsOrch::getPort() on a port that has been deleted
      */