[cbf] Fix max FC value (sonic-net#2049)

What I did
Fixed the maximum FC value allowed by a switch.

Why I did it
There was a bit of confusion when I first wrote this, thinking the attribute should return the maximum FC value allowed by the switch, not the maximum number of FC classes. Because of this, the actual maximum FC value allowed is one less than the current one because we're counting from 0. As such, if the switch reports the maximum number of FCs is 255, the actual FC value must be in the 0-254 range.

How I verified it
Updated the existing UTs

orchagent/cbf/cbfnhgorch.cpp

@@ -308,7 +308,7 @@ bool CbfNhg::sync()
     nhg_attr.value.u32 = static_cast<sai_uint32_t>(m_members.size());
     nhg_attrs.push_back(move(nhg_attr));
 
-    if (nhg_attr.value.u32 > gNhgMapOrch->getMaxFcVal())
+    if (nhg_attr.value.u32 > gNhgMapOrch->getMaxNumFcs())
     {
         /* If there are more members than FCs then this may be an error, as some members won't be used. */
         SWSS_LOG_WARN("More CBF NHG members configured than supported Forwarding Classes");

orchagent/cbf/nhgmaporch.cpp

@@ -294,34 +294,34 @@ void NhgMapOrch::decRefCount(const string &index)
 }
 
 /*
- * Get the max FC value supported by the switch.
+ * Get the maximum number of FC classes supported by the switch.
  */
-sai_uint8_t NhgMapOrch::getMaxFcVal()
+sai_uint8_t NhgMapOrch::getMaxNumFcs()
 {
     SWSS_LOG_ENTER();
 
-    static int max_fc_val = -1;
+    static int max_num_fcs = -1;
 
     /*
-     * Get the maximum value allowed for FC if it wasn't already initialized.
+     * Get the maximum number of FC classes if it wasn't already initialized.
      */
-    if (max_fc_val == -1)
+    if (max_num_fcs == -1)
     {
         sai_attribute_t attr;
         attr.id = SAI_SWITCH_ATTR_MAX_NUMBER_OF_FORWARDING_CLASSES;
 
         if (sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr) == SAI_STATUS_SUCCESS)
         {
-            max_fc_val = attr.value.u8;
+            max_num_fcs = attr.value.u8;
         }
         else
         {
             SWSS_LOG_WARN("Switch does not support FCs");
-            max_fc_val = 0;
+            max_num_fcs = 0;
         }
     }
 
-    return static_cast<sai_uint8_t>(max_fc_val);
+    return static_cast<sai_uint8_t>(max_num_fcs);
 }
 
 /*
@@ -343,7 +343,7 @@ pair<bool, unordered_map<sai_uint32_t, sai_int32_t>> NhgMapOrch::getMap(const ve
     }
 
     unordered_map<sai_uint32_t, sai_int32_t> fc_map;
-    sai_uint8_t max_fc_val = getMaxFcVal();
+    sai_uint8_t max_num_fcs = getMaxNumFcs();
 
     /*
     * Create the map while validating that the values are positive
@@ -353,13 +353,13 @@ pair<bool, unordered_map<sai_uint32_t, sai_int32_t>> NhgMapOrch::getMap(const ve
         try
         {
             /*
-             * Check the FC value is valid.
+             * Check the FC value is valid. FC value must be in range [0, max_num_fcs).
              */
             auto fc = stoi(fvField(*it));
 
-            if ((fc < 0) || (fc > max_fc_val))
+            if ((fc < 0) || (fc >= max_num_fcs))
             {
-                SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_fc_val);
+                SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_num_fcs - 1);
                 success = false;
                 break;
             }

orchagent/cbf/nhgmaporch.h

@@ -43,9 +43,9 @@ class NhgMapOrch : public Orch
     void decRefCount(const string &key);
 
     /*
-     * Get the max FC value supported by the switch.
+     * Get the maximum number of FC classes supported by the switch.
      */
-    static sai_uint8_t getMaxFcVal();
+    static sai_uint8_t getMaxNumFcs();
 
 private:
     /*

orchagent/qosorch.cpp

@@ -840,7 +840,7 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
 {
     SWSS_LOG_ENTER();
 
-    sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
+    sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
 
     sai_attribute_t list_attr;
     list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -867,10 +867,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
             }
             list_attr.value.qosmap.list[ind].key.dscp = static_cast<sai_uint8_t>(value);
 
+            // FC value must be in range [0, max_num_fcs)
             value = stoi(fvValue(*i));
-            if ((value < 0) || (value > max_fc_val))
+            if ((value < 0) || (value >= max_num_fcs))
             {
-                SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_fc_val);
+                SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_num_fcs - 1);
                 delete[] list_attr.value.qosmap.list;
                 return false;
             }
@@ -933,7 +934,7 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
 {
     SWSS_LOG_ENTER();
 
-    sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
+    sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
 
     sai_attribute_t list_attr;
     list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -960,10 +961,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
             }
             list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast<sai_uint8_t>(value);
 
+            // FC value must be in range [0, max_num_fcs)
             value = stoi(fvValue(*i));
-            if ((value < 0) || (value > max_fc_val))
+            if ((value < 0) || (value >= max_num_fcs))
             {
-                SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_fc_val);
+                SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_num_fcs - 1);
                 delete[] list_attr.value.qosmap.list;
                 return false;
             }
@@ -1692,7 +1694,7 @@ task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer)
         }
 
         sai_uint8_t old_pfc_enable = 0;
-        if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable)) 
+        if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable))
         {
             SWSS_LOG_ERROR("Failed to retrieve PFC bits on port %s", port_name.c_str());
         }

tests/test_nhg.py

@@ -1980,7 +1980,7 @@ def data_validation_test():
             # Test validation errors
             nhg_maps = [
                 ('-1', '0'), # negative FC
-                ('64', '0'), # greater than max FC value
+                ('63', '0'), # greater than max FC value
                 ('a', '0'), # non-integer FC
                 ('0', '-1'), # negative NH index
                 ('0', 'a'), # non-integer NH index

tests/test_qos_map.py

@@ -139,7 +139,7 @@ def test_dscp_to_fc(self, dvs):
         self.init_test(dvs)
 
         # Create a DSCP_TO_FC map
-        dscp_map = [(str(i), str(i)) for i in range(0, 64)]
+        dscp_map = [(str(i), str(i)) for i in range(0, 63)]
         self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
 
         self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1)
@@ -153,7 +153,7 @@ def test_dscp_to_fc(self, dvs):
         assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_FORWARDING_CLASS")
 
         # Modify the map
-        dscp_map = [(str(i), '0') for i in range(0, 64)]
+        dscp_map = [(str(i), '0') for i in range(0, 63)]
         self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
         time.sleep(1)
 
@@ -174,7 +174,7 @@ def test_dscp_to_fc(self, dvs):
             ('-1', '0'), # negative DSCP
             ('64', '0'), # DSCP greater than max value
             ('0', '-1'), # negative FC
-            ('0', '64'), # FC greater than max value
+            ('0', '63'), # FC greater than max value
             ('a', '0'), # non-integer DSCP
             ('0', 'a'), # non-integet FC
         ]
@@ -228,7 +228,7 @@ def test_exp_to_fc(self, dvs):
             ('-1', '0'), # negative EXP
             ('8', '0'), # EXP greater than max value
             ('0', '-1'), # negative FC
-            ('0', '64'), # FC greater than max value
+            ('0', '63'), # FC greater than max value
             ('a', '0'), # non-integer EXP
             ('0', 'a'), # non-integet FC
         ]
@@ -258,7 +258,7 @@ def test_per_port_cbf_binding(self, dvs):
         self.init_test(dvs)
 
         # Create a DSCP_TO_FC map
-        dscp_map = [(str(i), str(i)) for i in range(0, 64)]
+        dscp_map = [(str(i), str(i)) for i in range(0, 63)]
         self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
         self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1)
         dscp_map_id = self.get_qos_id()