address review comments

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

Author: ds952811

orchagent/port.h

@@ -178,7 +178,8 @@ class Port
 
     bool m_fec_cfg = false;
     bool m_an_cfg = false;
-    bool m_port_cap_lt = false; /* Port Capability - LinkTraining */
+
+    int m_cap_lt = -1; /* Capability - LinkTraining, -1 means not set */
 };
 
 }

orchagent/portsorch.cpp

@@ -61,7 +61,7 @@ extern string gMyAsicName;
 #define DEFAULT_VLAN_ID     1
 #define MAX_VALID_VLAN_ID   4094
 
-#define PORT_STAT_POLLING_SEC                             3
+#define PORT_STATE_POLLING_SEC                            5
 #define PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS     1000
 #define PORT_BUFFER_DROP_STAT_POLLING_INTERVAL_MS     60000
 #define QUEUE_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS   10000
@@ -320,7 +320,7 @@ PortsOrch::PortsOrch(DBConnector *db, DBConnector *stateDb, vector<table_name_wi
         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),
-        m_timer(new SelectableTimer(timespec { .tv_sec = PORT_STAT_POLLING_SEC, .tv_nsec = 0 }))
+        m_timer(new SelectableTimer(timespec { .tv_sec = PORT_STATE_POLLING_SEC, .tv_nsec = 0 }))
 {
     SWSS_LOG_ENTER();
 
@@ -1894,16 +1894,21 @@ void PortsOrch::initPortCapLinkTraining(Port &port)
 #ifdef SAI_PORT_ATTR_SUPPORTED_LINK_TRAINING_MODE
     attr.id = SAI_PORT_ATTR_SUPPORTED_LINK_TRAINING_MODE;
 #else
+    /*
+     * Fallback to autoneg upon legacy SAI implementation
+     * In the case of SFP/QSFP ports, link-training is most likely available
+     * if autoneg is supported.
+     */
     attr.id = SAI_PORT_ATTR_SUPPORTED_AUTO_NEG_MODE;
 #endif
     status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
     if (status == SAI_STATUS_SUCCESS)
     {
-        port.m_port_cap_lt = attr.value.booldata;
+        port.m_cap_lt = attr.value.booldata ? 1 : 0;
     }
     else
     {
-        port.m_port_cap_lt = true; /* This is for vstest */
+        port.m_cap_lt = 1; /* Default to 1 for vstest */
         SWSS_LOG_NOTICE("Unable to get %s LT capability", port.m_alias.c_str());
     }
 }
@@ -2426,9 +2431,6 @@ bool PortsOrch::initPort(const string &alias, const string &role, const int inde
                 /* Create associated Gearbox lane mapping */
                 initGearboxPort(p);
 
-                /* Initialize port capabilities */
-                initPortCapLinkTraining(p);
-
                 /* Add port to port list */
                 m_portList[alias] = p;
                 saiOidToAlias[id] = alias;
@@ -3028,7 +3030,12 @@ void PortsOrch::doPortTask(Consumer &consumer)
                     lt = link_training_mode_map[lt_str];
                     if (lt != p.m_link_training)
                     {
-                        if (!p.m_port_cap_lt)
+                        if (p.m_cap_lt < 0)
+                        {
+                            initPortCapLinkTraining(p);
+                            m_portList[alias] = p;
+                        }
+                        if (p.m_cap_lt < 1)
                         {
                             SWSS_LOG_ERROR("%s: LT is not supported by the ASIC", alias.c_str());
                             // Don't retry
@@ -7097,7 +7104,7 @@ void PortsOrch::updatePortStateLinkTraining(const Port &port)
 
     string status = "off";
 
-    if ((port.m_link_training > 0) && port.m_port_cap_lt)
+    if ((port.m_link_training > 0) && (port.m_cap_lt > 0))
     {
         sai_port_link_training_rx_status_t rx_status;
         sai_port_link_training_failure_status_t failure;
@@ -7127,13 +7134,13 @@ void PortsOrch::updatePortStateLinkTraining(const Port &port)
     m_portStateTable.hset(port.m_alias, "link_training_status", status);
 }
 
-void PortsOrch::updatePortStatePoll(const Port &port, port_state_poll_t type, bool set)
+void PortsOrch::updatePortStatePoll(const Port &port, port_state_poll_t type, bool active)
 {
     if (type == PORT_STATE_POLL_NONE)
     {
         return;
     }
-    if (set)
+    if (active)
     {
         m_port_state_poll[port.m_alias] |= type;
         m_timer->start();
@@ -7155,7 +7162,11 @@ void PortsOrch::doTask(swss::SelectableTimer &timer)
             m_port_state_poll.erase(it);
             continue;
         }
-        if (port.m_admin_state_up && (it->second & PORT_STATE_POLL_LT))
+        if (!port.m_admin_state_up)
+        {
+            continue;
+        }
+        if (it->second & PORT_STATE_POLL_LT)
         {
             updatePortStateLinkTraining(port);
         }

orchagent/portsorch.h

@@ -347,7 +347,7 @@ class PortsOrch : public Orch, public Subject
     } 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 updatePortStatePoll(const Port &port, port_state_poll_t type, bool active);
     void updatePortStateLinkTraining(const Port &port);
 
     void getPortSerdesVal(const std::string& s, std::vector<uint32_t> &lane_values);