[Nexthop][m4340nhp] Create ports the way the Qumran4D SAI requires

fboss/agent/hw/sai/api/PortApi.h

@@ -15,11 +15,12 @@
 #include "fboss/agent/hw/sai/api/SaiVersion.h"
 #include "fboss/agent/hw/sai/api/Types.h"
 
-#include <folly/logging/xlog.h>
-
+#include <algorithm>
+#include <iterator>
 #include <limits>
 #include <optional>
 #include <tuple>
+#include <vector>
 
 extern "C" {
 #include <sai.h>
@@ -790,15 +791,14 @@ struct SaiPortTraits {
    *   - Enhancing AdapterHostKey as well as CreateAttribute means that the
    *     AdatperHostKey remains computable from AdaterKey, CreateAttributes: a
    *     requirement for our design.
-   *   - However, this also means that attempt to create port will pass Type (a
-   *     Read Only attribute today) and that call will fail.
-   *   - This is not a problem at the moment since passing Type is a
-   *     requirement only on DNX and ports are always pre-created on this
-   *     platform and loaded by SaiStore during init and never created.
-   *   - In future, if we support breakout ports on DNX, we would need to call
-   *     create port which would fail: but as mentioned above, we plan to
-   *     enhance the SAI spec and work with SAI vendors to support passing port
-   *     type during creation.
+   *   - It also means create_port would receive Type, a read-only attribute,
+   *     and some adapters reject it only after creating the port, leaving a
+   *     half-created port behind. Ports created at runtime (breakout) hit
+   *     this, so PortApi::_create leaves a LOGICAL Type out of the attribute
+   *     list it passes to create_port; the other types (fabric, recycle,
+   *     eventor, management, hyper) are still passed, and Type still keys the
+   *     SaiStore.
+   *   - Long term the fix is a SAI spec change allowing Type on create.
    */
 
   using AdapterHostKey = std::tuple<
@@ -1910,7 +1910,24 @@ class PortApi : public SaiApi<PortApi> {
       sai_object_id_t switch_id,
       size_t count,
       sai_attribute_t* attr_list) const {
-    return api_->create_port(rawSaiId(id), switch_id, count, attr_list);
+    // SAI_PORT_ATTR_TYPE is read-only in the SAI spec. It is part of the DNX
+    // CreateAttributes so that SaiStore can key port types apart; the logical
+    // (front-panel) type is not sent to the adapter. An adapter that rejects it
+    // only after creating the port (the Broadcom DNX SAI) would otherwise leave
+    // a half-created port behind that no retry can replace. Other types
+    // (fabric, recycle, eventor, management, hyper port) are still passed.
+    std::vector<sai_attribute_t> attrs;
+    attrs.reserve(count);
+    std::copy_if(
+        attr_list,
+        attr_list + count,
+        std::back_inserter(attrs),
+        [](const sai_attribute_t& attr) {
+          return attr.id != SAI_PORT_ATTR_TYPE ||
+              attr.value.s32 != SAI_PORT_TYPE_LOGICAL;
+        });
+    return api_->create_port(
+        rawSaiId(id), switch_id, attrs.size(), attrs.data());
   }
   sai_status_t _remove(PortSaiId key) const {
     return api_->remove_port(key);

fboss/agent/hw/sai/switch/npu/SaiPortManager.cpp

@@ -1634,17 +1634,30 @@ void SaiPortManager::programSerdes(
      * Serdes objects are no longer created beforehand. Therefore, agent only
      * needs to create a new serdes object without knowing the serdes Id or
      * reloading the object to sai store.
+     *
+     * On the Broadcom DNX SAI that holds only where the ASIC does not claim
+     * PORT_SERDES_CREATED_WITH_PORT: the Qumran4D SAI attaches a default
+     * serdes object to every port it creates and answers a second
+     * create_port_serdes with SAI_STATUS_ITEM_ALREADY_EXISTS, so a port
+     * created at runtime (breakout) must adopt it, as on the other SAIs.
      */
-#if !defined(BRCM_SAI_SDK_XGS_AND_DNX)
-    // serdes is not yet programmed or reloaded from adapter
-    std::optional<SaiPortTraits::Attributes::SerdesId> serdesAttr{};
-    auto serdesId = SaiApiTable::getInstance()->portApi().getAttribute(
-        saiPort->adapterKey(), serdesAttr);
-    if (serdesId.has_value() && serdesId.value() != SAI_NULL_OBJECT_ID) {
-      // but default serdes exists in the adapter.
-      serdes =
-          store.reloadObject(static_cast<PortSerdesSaiId>(serdesId.value()));
+#if !defined(BRCM_SAI_SDK_XGS_AND_DNX) || defined(BRCM_SAI_SDK_DNX)
+#if defined(BRCM_SAI_SDK_DNX)
+    if (platform_->getAsic()->isSupported(
+            HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT)) {
+#endif
+      // serdes is not yet programmed or reloaded from adapter
+      std::optional<SaiPortTraits::Attributes::SerdesId> serdesAttr{};
+      auto serdesId = SaiApiTable::getInstance()->portApi().getAttribute(
+          saiPort->adapterKey(), serdesAttr);
+      if (serdesId.has_value() && serdesId.value() != SAI_NULL_OBJECT_ID) {
+        // but default serdes exists in the adapter.
+        serdes =
+            store.reloadObject(static_cast<PortSerdesSaiId>(serdesId.value()));
+      }
+#if defined(BRCM_SAI_SDK_DNX)
     }
+#endif
 #endif
   } else {
     // ensure warm boot handles are reclaimed removed

fboss/agent/hw/switch_asics/ChenabAsic.cpp

@@ -272,6 +272,7 @@ bool ChenabAsic::isSupportedNonFabric(Feature feature) const {
     case HwAsic::Feature::PBR_ACL_BYTE_COUNTER:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/EbroAsic.cpp

@@ -283,6 +283,7 @@ bool EbroAsic::isSupportedNonFabric(Feature feature) const {
     case HwAsic::Feature::MY_MAC:
     case HwAsic::Feature::ISOLATION_GROUP:
     case HwAsic::Feature::CSIG:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
     case HwAsic::Feature::SAI_ACL_MPLS_LABEL0_TTL:
     case HwAsic::Feature::SAI_MPLS_NEXTHOP_OUTSEG_TYPE:
     case HwAsic::Feature::ACL_ENTRY_OUT_PORT_QUALIFIER:

fboss/agent/hw/switch_asics/G202xAsic.cpp

@@ -262,6 +262,7 @@ bool G202xAsic::isSupportedNonFabric(Feature feature) const {
     case HwAsic::Feature::MY_MAC:
     case HwAsic::Feature::ISOLATION_GROUP:
     case HwAsic::Feature::CSIG:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
     case HwAsic::Feature::SWITCH_WIDE_LINK_UP_DEBOUNCE:
     case HwAsic::Feature::ACL_ENTRY_OUT_PORT_QUALIFIER:
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:

fboss/agent/hw/switch_asics/HwAsic.h

@@ -649,6 +649,9 @@ class HwAsic {
     CSIG,
     // TTL control on the aggregate's shared forwarding service.
     LAG_TTL_DECREMENT_DISABLE,
+    // The adapter attaches a default serdes object to every port it creates
+    // and rejects a second create_port_serdes; the agent adopts that object
+    PORT_SERDES_CREATED_WITH_PORT,
   };
 
   enum class AsicMode {

fboss/agent/hw/switch_asics/Jericho2Asic.cpp

@@ -271,6 +271,7 @@ bool Jericho2Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/Jericho3Asic.cpp

@@ -307,6 +307,7 @@ bool Jericho3Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/Jericho4Asic.cpp

@@ -307,6 +307,7 @@ bool Jericho4Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/P200Asic.cpp

@@ -284,6 +284,7 @@ bool P200Asic::isSupportedNonFabric(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
     case HwAsic::Feature::SAI_ACL_MPLS_LABEL0_TTL:
       // Leaba does not implement the MPLS label0 TTL ACL qualifier yet.

fboss/agent/hw/switch_asics/Qumran4DAsic.cpp

@@ -80,6 +80,7 @@ bool Qumran4DAsic::isSupported(Feature feature) const {
     case HwAsic::Feature::ACL_METADATA_QUALIFER:
     case HwAsic::Feature::EVENTOR_PORT_FOR_SFLOW:
     case HwAsic::Feature::SFLOWv6:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
     case HwAsic::Feature::ZERO_SDK_WRITE_WARMBOOT:
     case HwAsic::Feature::SWITCH_REACHABILITY_CHANGE_NOTIFY:
     case HwAsic::Feature::ACL_ENTRY_ETHER_TYPE:

fboss/agent/hw/switch_asics/Tomahawk3Asic.cpp

@@ -111,6 +111,7 @@ bool Tomahawk3Asic::isSupported(Feature feature) const {
       return true;
     case HwAsic::Feature::ISOLATION_GROUP:
     case HwAsic::Feature::CSIG:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
     case HwAsic::Feature::QCM:
     case HwAsic::Feature::SMAC_EQUALS_DMAC_CHECK_ENABLED:
     case HwAsic::Feature::PORT_TTL_DECREMENT_DISABLE:

fboss/agent/hw/switch_asics/Tomahawk4Asic.cpp

@@ -280,6 +280,7 @@ bool Tomahawk4Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::CSIG:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/Tomahawk5Asic.cpp

@@ -263,6 +263,7 @@ bool Tomahawk5Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::CSIG:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/Tomahawk6Asic.cpp

@@ -270,6 +270,7 @@ bool Tomahawk6Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/TomahawkAsic.cpp

@@ -261,6 +261,7 @@ bool TomahawkAsic::isSupported(Feature feature) const {
     case HwAsic::Feature::CSIG:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/TomahawkUltra1Asic.cpp

@@ -266,6 +266,7 @@ bool TomahawkUltra1Asic::isSupported(Feature feature) const {
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3:
     case HwAsic::Feature::LAG_TTL_DECREMENT_DISABLE:
     case HwAsic::Feature::PORT_LATCH_STATUS:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
       return false;
   }
   return false;

fboss/agent/hw/switch_asics/YubaAsic.cpp

@@ -276,6 +276,7 @@ bool YubaAsic::isSupportedNonFabric(Feature feature) const {
     case HwAsic::Feature::RX_PACKET_TYPE:
     case HwAsic::Feature::MY_MAC:
     case HwAsic::Feature::ISOLATION_GROUP:
+    case HwAsic::Feature::PORT_SERDES_CREATED_WITH_PORT:
     case HwAsic::Feature::SWITCH_WIDE_LINK_UP_DEBOUNCE:
     case HwAsic::Feature::ACL_ENTRY_OUT_PORT_QUALIFIER:
     case HwAsic::Feature::XPHY_SERDES_TX_FIR_POST3: