address review comments

Signed-off-by: Dante Su <[email protected]>

Author: ds952811

orchagent/port.h

@@ -176,6 +176,7 @@ class Port
 
     bool m_fec_cfg = false;
     bool m_an_cfg = false;
+    bool m_port_cap_lt = false; /* Port Capability - LinkTraining */
 };
 
 }

orchagent/portsorch.cpp

@@ -325,7 +325,6 @@ static bool isValidPortTypeForLagMember(const Port& port)
 PortsOrch::PortsOrch(DBConnector *db, DBConnector *stateDb, vector<table_name_with_pri_t> &tableNames, DBConnector *chassisAppDb) :
         Orch(db, tableNames),
         m_portStateTable(stateDb, STATE_PORT_TABLE_NAME),
-        m_stateLinkTrainingTable(stateDb, STATE_LINK_TRAINING_TABLE_NAME),
         port_stat_manager(PORT_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, false),
         port_buffer_drop_stat_manager(PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_BUFFER_DROP_STAT_POLLING_INTERVAL_MS, false),
         queue_stat_manager(QUEUE_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, QUEUE_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, false),
@@ -1932,7 +1931,7 @@ void PortsOrch::initPortSupportedSpeeds(const std::string& alias, sai_object_id_
     m_portStateTable.set(alias, v);
 }
 
-void PortsOrch::initPortCapabilityLinkTraining(const Port &port)
+void PortsOrch::initPortCapLinkTraining(Port &port)
 {
     sai_status_t status;
     sai_attribute_t attr;
@@ -1941,11 +1940,11 @@ void PortsOrch::initPortCapabilityLinkTraining(const Port &port)
     status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
     if (status == SAI_STATUS_SUCCESS)
     {
-        string supp = attr.value.booldata ? "yes" : "no";
-        m_stateLinkTrainingTable.hset(port.m_alias, "supported", supp);
+        port.m_port_cap_lt = attr.value.booldata;
     }
     else
     {
+        port.m_port_cap_lt = true; /* This is for vstest */
         SWSS_LOG_NOTICE("Unable to get %s LT capability", port.m_alias.c_str());
     }
 }
@@ -2218,17 +2217,6 @@ task_process_status PortsOrch::setPortAutoNeg(sai_object_id_t id, int an)
     return task_success;
 }
 
-bool PortsOrch::getPortCapabilityLinkTraining(const Port& port)
-{
-    string supp;
-
-    if (m_stateLinkTrainingTable.hget(port.m_alias, "supported", supp))
-    {
-        return (supp == "yes") ? true : false;
-    }
-    return false;
-}
-
 task_process_status PortsOrch::setPortLinkTraining(const Port &port, bool state)
 {
     SWSS_LOG_ENTER();
@@ -2480,7 +2468,7 @@ bool PortsOrch::initPort(const string &alias, const string &role, const int inde
                 initGearboxPort(p);
 
                 /* Initialize port capabilities */
-                initPortCapabilityLinkTraining(p);
+                initPortCapLinkTraining(p);
 
                 /* Add port to port list */
                 m_portList[alias] = p;
@@ -3081,7 +3069,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
                     lt = link_training_mode_map[lt_str];
                     if (lt != p.m_link_training)
                     {
-                        if (!getPortCapabilityLinkTraining(p))
+                        if (!p.m_port_cap_lt)
                         {
                             SWSS_LOG_ERROR("%s: LT is not supported by the ASIC", alias.c_str());
                             // Don't retry
@@ -3103,10 +3091,11 @@ void PortsOrch::doPortTask(Consumer &consumer)
                             }
                             continue;
                         }
-                        m_stateLinkTrainingTable.hset(p.m_alias, "status", lt > 0 ? "on" : "off");
+                        m_portStateTable.hset(alias, "link_training_status", lt_str);
                         SWSS_LOG_NOTICE("Set port %s LT from %d to %d", alias.c_str(), p.m_link_training, lt);
                         p.m_link_training = lt;
                         m_portList[alias] = p;
+                        updatePortStatePoll(p, PORT_STATE_POLL_LT, (lt > 0));
                     }
                 }
 
@@ -5938,6 +5927,19 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status)
     if (port.m_type == Port::PHY)
     {
         updateDbPortOperStatus(port, status);
+        if (port.m_admin_state_up && port.m_link_training > 0)
+        {
+            updatePortStateLinkTraining(port);
+            if (status == SAI_PORT_OPER_STATUS_UP)
+            {
+                updatePortStatePoll(port, PORT_STATE_POLL_LT, false);
+            }
+            else
+            {
+                /* Restart port state polling upon link down event */
+                updatePortStatePoll(port, PORT_STATE_POLL_LT, true);
+            }
+        }
     }
     port.m_oper_status = status;
 
@@ -7127,7 +7129,7 @@ bool PortsOrch::decrFdbCount(const std::string& alias, int count)
     return true;
 }
 
-void PortsOrch::updatePortLinkTrainingStatus(const Port &port)
+void PortsOrch::updatePortStateLinkTraining(const Port &port)
 {
     SWSS_LOG_ENTER();
 
@@ -7138,7 +7140,7 @@ void PortsOrch::updatePortLinkTrainingStatus(const Port &port)
 
     string status = "off";
 
-    if ((port.m_link_training > 0) && getPortCapabilityLinkTraining(port))
+    if ((port.m_link_training > 0) && port.m_port_cap_lt)
     {
         sai_port_link_training_rx_status_t rx_status;
         sai_port_link_training_failure_status_t failure;
@@ -7164,15 +7166,45 @@ void PortsOrch::updatePortLinkTrainingStatus(const Port &port)
             }
         }
     }
-    m_stateLinkTrainingTable.hset(port.m_alias, "status", status);
+
+    m_portStateTable.hset(port.m_alias, "link_training_status", status);
+}
+
+void PortsOrch::updatePortStatePoll(const Port &port, port_state_poll_t type, bool set)
+{
+    if (type == PORT_STATE_POLL_NONE)
+    {
+        return;
+    }
+    if (set)
+    {
+        m_port_state_poll[port.m_alias] |= type;
+        m_timer->start();
+    }
+    else
+    {
+        m_port_state_poll[port.m_alias] &= ~type;
+    }
 }
 
 void PortsOrch::doTask(swss::SelectableTimer &timer)
 {
-    SWSS_LOG_ENTER();
+    Port port;
 
-    for (auto it = m_portList.begin(); it != m_portList.end(); ++it)
+    for (auto it = m_port_state_poll.begin(); it != m_port_state_poll.end(); ++it)
+    {
+        if ((it->second == 0) || !getPort(it->first, port))
+        {
+            m_port_state_poll.erase(it);
+            continue;
+        }
+        if (port.m_admin_state_up && (it->second & PORT_STATE_POLL_LT))
+        {
+            updatePortStateLinkTraining(port);
+        }
+    }
+    if (m_port_state_poll.size() == 0)
     {
-        updatePortLinkTrainingStatus(it->second);
+        m_timer->stop();
     }
 }

orchagent/portsorch.h

@@ -185,7 +185,6 @@ class PortsOrch : public Orch, public Subject
     unique_ptr<ProducerTable> m_flexCounterTable;
     unique_ptr<ProducerTable> m_flexCounterGroupTable;
     Table m_portStateTable;
-    Table m_stateLinkTrainingTable;
 
     std::string getQueueWatermarkFlexCounterTableKey(std::string s);
     std::string getPriorityGroupWatermarkFlexCounterTableKey(std::string s);
@@ -294,8 +293,7 @@ class PortsOrch : public Orch, public Subject
     bool initPort(const string &alias, const string &role, const int index, const set<int> &lane_set);
     void deInitPort(string alias, sai_object_id_t port_id);
 
-    void initPortCapabilityLinkTraining(const Port &port);
-    bool getPortCapabilityLinkTraining(const Port& port);
+    void initPortCapLinkTraining(Port &port);
 
     bool setPortAdminStatus(Port &port, bool up);
     bool getPortAdminStatus(sai_object_id_t id, bool& up);
@@ -345,7 +343,16 @@ class PortsOrch : public Orch, public Subject
 
     bool getPortLinkTrainingRxStatus(const Port &port, sai_port_link_training_rx_status_t &rx_status);
     bool getPortLinkTrainingFailure(const Port &port, sai_port_link_training_failure_status_t &failure);
-    void updatePortLinkTrainingStatus(const Port &port);
+
+    typedef enum {
+        PORT_STATE_POLL_NONE = 0,
+        PORT_STATE_POLL_AN   = 0x00000001, /* Auto Negotiation */
+        PORT_STATE_POLL_LT   = 0x00000002  /* Link Trainig */
+    } port_state_poll_t;
+
+    map<string, uint32_t> m_port_state_poll;
+    void updatePortStatePoll(const Port &port, port_state_poll_t type, bool set);
+    void updatePortStateLinkTraining(const Port &port);
 
     void getPortSerdesVal(const std::string& s, std::vector<uint32_t> &lane_values);
     bool getPortAdvSpeedsVal(const std::string &s, std::vector<uint32_t> &speed_values);

tests/test_port_lt.py

@@ -19,12 +19,12 @@ def test_PortLinkTrainingForce(self, dvs, testlog):
         fvs = swsscommon.FieldValuePairs([("link_training","on")])
         tbl.set("Ethernet4", fvs)
 
-        # validate if autoneg false is pushed to asic db when set first time
+        # validate if link_training false is pushed to asic db when set first time
         port_oid = adb.port_name_map["Ethernet0"]
         expected_fields = {"SAI_PORT_ATTR_LINK_TRAINING_ENABLE":"false"}
         adb.wait_for_field_match("ASIC_STATE:SAI_OBJECT_TYPE_PORT", port_oid, expected_fields)
 
-        # validate if autoneg true is pushed to asic db when set first time
+        # validate if link_training true is pushed to asic db when set first time
         port_oid = adb.port_name_map["Ethernet4"]
         expected_fields = {"SAI_PORT_ATTR_LINK_TRAINING_ENABLE":"true"}
         adb.wait_for_field_match("ASIC_STATE:SAI_OBJECT_TYPE_PORT", port_oid, expected_fields)