From c76bd9e41b07253efa28e034cce42d4d48b756ba Mon Sep 17 00:00:00 2001
From: Hao Jiang <jianghao@google.com>
Date: Fri, 7 Apr 2023 23:51:14 +0000
Subject: [PATCH 27/44] nvmesensor: Check SCS for secondary controllers

The Secondary Controller State indicates if the secondary controller has
been enabled by CC.EN. For disabled controllers, the StorageController
and NVMeAdmin interface should not be created.

It is because the ctrl_list will list all controllers in the system
regardless of the enable status. The number of supported controllers can
be large compare to the active ones in the system. In our example
system, the total controllers in one SSD can live up to 128 but we
only enble one or two of them. The unused controller can seriously slow
down the performance of dbus if we chose to expose them.

Signed-off-by: Hao Jiang <jianghao@google.com>
Change-Id: I6fdbb503550968d013e16822f9d11e9962bb93a4
---
 src/NVMeController.cpp | 100 ++++++++++++++++++++++++++++++++---------
 src/NVMeController.hpp |  54 ++++++++++++++++++----
 src/NVMeSubsys.cpp     |  19 +++++++-
 3 files changed, 141 insertions(+), 32 deletions(-)

diff --git a/src/NVMeController.cpp b/src/NVMeController.cpp
index 7e26643..e40da17 100644
--- a/src/NVMeController.cpp
+++ b/src/NVMeController.cpp
@@ -14,35 +14,67 @@ using sdbusplus::xyz::openbmc_project::Inventory::Item::server::
     StorageController;
 using sdbusplus::xyz::openbmc_project::NVMe::server::NVMeAdmin;
 
-NVMeController::NVMeController(
+NVMeControllerEnabled::NVMeControllerEnabled(
     boost::asio::io_context& io, sdbusplus::asio::object_server& objServer,
     std::shared_ptr<sdbusplus::asio::connection> conn, std::string path,
     std::shared_ptr<NVMeMiIntf> nvmeIntf, nvme_mi_ctrl_t ctrl) :
+    NVMeController(io, objServer, conn, path, nvmeIntf, ctrl),
     StorageController(dynamic_cast<sdbusplus::bus_t&>(*conn), path.c_str()),
     NVMeAdmin(*conn, path.c_str(),
-              {{"FirmwareCommitStatus", {FwCommitStatus::Ready}}}),
-    io(io), objServer(objServer), conn(conn), path(path), nvmeIntf(nvmeIntf),
-    nvmeCtrl(ctrl)
+              {{"FirmwareCommitStatus", {FwCommitStatus::Ready}}})
 {
-    StorageController::emit_added();
-    NVMeAdmin::emit_added();
     assocIntf = objServer.add_interface(
         path, "xyz.openbmc_project.Association.Definitions");
 
     // regiester a property with empty association
     assocIntf->register_property("Associations", std::vector<Association>{});
     assocIntf->initialize();
+
+    StorageController::emit_added();
+    NVMeAdmin::emit_added();
 }
 
-void NVMeController::start(std::shared_ptr<NVMeControllerPlugin> nvmePlugin)
+NVMeControllerEnabled::NVMeControllerEnabled(NVMeController&& nvmeController) :
+    NVMeController(std::move(nvmeController)),
+    StorageController(
+        dynamic_cast<sdbusplus::bus_t&>(*this->NVMeController::conn),
+        this->NVMeController::path.c_str()),
+    NVMeAdmin(*this->NVMeController::conn, this->NVMeController::path.c_str(),
+              {{"FirmwareCommitStatus", {FwCommitStatus::Ready}}})
 {
-    plugin = nvmePlugin;
+    assocIntf = objServer.add_interface(
+        path, "xyz.openbmc_project.Association.Definitions");
+
+    // the association could have be set via NVMeController, set the association
+    // dbus property accordingly
+    std::vector<Association> associations;
+    for (const auto& subsys : subsystems)
+    {
+        associations.emplace_back("storage", "storage_controller", subsys);
+    }
+
+    for (const auto& cntrl : secondaryControllers)
+    {
+        associations.emplace_back("secondary", "primary", cntrl);
+    }
+
+    assocIntf->register_property("Associations", associations);
+    assocIntf->initialize();
+
+    StorageController::emit_added();
+    NVMeAdmin::emit_added();
+}
+
+void NVMeControllerEnabled::start(
+    std::shared_ptr<NVMeControllerPlugin> nvmePlugin)
+{
+    this->NVMeController::start(std::move(nvmePlugin));
 }
 
-sdbusplus::message::unix_fd NVMeController::getLogPage(uint8_t lid,
-                                                       uint32_t nsid,
-                                                       uint8_t lsp,
-                                                       uint16_t lsi)
+sdbusplus::message::unix_fd NVMeControllerEnabled::getLogPage(uint8_t lid,
+                                                              uint32_t nsid,
+                                                              uint8_t lsp,
+                                                              uint16_t lsi)
 {
     std::array<int, 2> pipe;
     if (::pipe(pipe.data()) < 0)
@@ -121,8 +153,8 @@ sdbusplus::message::unix_fd NVMeController::getLogPage(uint8_t lid,
     return sdbusplus::message::unix_fd{pipe[0]};
 }
 
-sdbusplus::message::unix_fd NVMeController::identify(uint8_t cns, uint32_t nsid,
-                                                     uint16_t cntid)
+sdbusplus::message::unix_fd
+    NVMeControllerEnabled::identify(uint8_t cns, uint32_t nsid, uint16_t cntid)
 {
     std::array<int, 2> pipe;
     if (::pipe(pipe.data()) < 0)
@@ -154,8 +186,8 @@ sdbusplus::message::unix_fd NVMeController::identify(uint8_t cns, uint32_t nsid,
     return sdbusplus::message::unix_fd{pipe[0]};
 }
 
-NVMeAdmin::FwCommitStatus
-    NVMeController::firmwareCommitStatus(NVMeAdmin::FwCommitStatus status)
+NVMeAdmin::FwCommitStatus NVMeControllerEnabled::firmwareCommitStatus(
+    NVMeAdmin::FwCommitStatus status)
 {
     auto commitStatus = this->NVMeAdmin::firmwareCommitStatus();
     // The function is only allowed to reset the status back to ready
@@ -168,8 +200,8 @@ NVMeAdmin::FwCommitStatus
     return this->NVMeAdmin::firmwareCommitStatus(status);
 }
 
-void NVMeController::firmwareCommitAsync(uint8_t commitAction,
-                                         uint8_t firmwareSlot, bool bpid)
+void NVMeControllerEnabled::firmwareCommitAsync(uint8_t commitAction,
+                                                uint8_t firmwareSlot, bool bpid)
 {
     auto commitStatus = this->NVMeAdmin::firmwareCommitStatus();
     if (commitStatus != FwCommitStatus::Ready)
@@ -197,13 +229,31 @@ void NVMeController::firmwareCommitAsync(uint8_t commitAction,
         });
 }
 
-NVMeController::~NVMeController()
+NVMeControllerEnabled::~NVMeControllerEnabled()
 {
-    objServer.remove_interface(assocIntf);
     NVMeAdmin::emit_removed();
     StorageController::emit_removed();
 }
 
+NVMeController::NVMeController(
+    boost::asio::io_context& io, sdbusplus::asio::object_server& objServer,
+    std::shared_ptr<sdbusplus::asio::connection> conn, std::string path,
+    std::shared_ptr<NVMeMiIntf> nvmeIntf, nvme_mi_ctrl_t ctrl) :
+    io(io),
+    objServer(objServer), conn(conn), path(path), nvmeIntf(nvmeIntf),
+    nvmeCtrl(ctrl)
+{}
+
+NVMeController::~NVMeController()
+{
+    objServer.remove_interface(assocIntf);
+}
+
+void NVMeController::start(std::shared_ptr<NVMeControllerPlugin> nvmePlugin)
+{
+    plugin = nvmePlugin;
+}
+
 void NVMeController::setSecAssoc(
     const std::vector<std::shared_ptr<NVMeController>> secCntrls)
 {
@@ -230,7 +280,10 @@ void NVMeController::setSecAssoc(
         associations.emplace_back("secondary", "primary", cntrl);
     }
 
-    assocIntf->set_property("Associations", associations);
+    if (assocIntf)
+    {
+        assocIntf->set_property("Associations", associations);
+    }
 }
 
 void NVMeController::addSubsystemAssociation(const std::string& subsysPath)
@@ -248,5 +301,8 @@ void NVMeController::addSubsystemAssociation(const std::string& subsysPath)
         associations.emplace_back("secondary", "primary", cntrl);
     }
 
-    assocIntf->set_property("Associations", associations);
+    if (assocIntf)
+    {
+        assocIntf->set_property("Associations", associations);
+    }
 }
diff --git a/src/NVMeController.hpp b/src/NVMeController.hpp
index c964b87..5a0f36f 100644
--- a/src/NVMeController.hpp
+++ b/src/NVMeController.hpp
@@ -12,12 +12,18 @@
 #include <utility>
 
 class NVMeControllerPlugin;
-class NVMeController :
-    private sdbusplus::xyz::openbmc_project::Inventory::Item::server::
-        StorageController,
-    private sdbusplus::xyz::openbmc_project::NVMe::server::NVMeAdmin,
-    public std::enable_shared_from_this<NVMeController>
 
+/**
+ * @brief A class to represent the NVMeController has not been enabled (CC.EN =
+ * 0)
+ *
+ * The disabled controllers still have cntrl_id and are listed in the
+ * cntrl_list. However the functionility has been disabled so neither
+ * StorageController nor NVMeAdmin interface should be exposed for the disabled
+ * controllers.
+ *
+ */
+class NVMeController
 {
   public:
     NVMeController(boost::asio::io_context& io,
@@ -26,9 +32,9 @@ class NVMeController :
                    std::string path, std::shared_ptr<NVMeMiIntf> nvmeIntf,
                    nvme_mi_ctrl_t ctrl);
 
-    ~NVMeController() override;
+    virtual ~NVMeController();
 
-    void start(std::shared_ptr<NVMeControllerPlugin> nvmePlugin);
+    virtual void start(std::shared_ptr<NVMeControllerPlugin> nvmePlugin);
 
     // setup association to the secondary controllers. Clear the Association if
     // empty.
@@ -60,7 +66,7 @@ class NVMeController :
      */
     void addSubsystemAssociation(const std::string& subsysPath);
 
-  private:
+  protected:
     friend class NVMeControllerPlugin;
 
     boost::asio::io_context& io;
@@ -80,7 +86,39 @@ class NVMeController :
 
     // NVMe Plug-in for vendor defined command/field
     std::weak_ptr<NVMeControllerPlugin> plugin;
+};
+
+/**
+ * @brief A class for the NVMe controller that has been enabled (CC.EN = 1)
+ *
+ * The premitted NVMe Admin cmds should be anable to processed via the enabled
+ * controller (e.g reading the temletries or other admin tasks). Thus the
+ * NVMeAmin and StorageController Dbus interface will be exposed via this class.
+ *
+ */
+class NVMeControllerEnabled :
+    public NVMeController,
+    private sdbusplus::xyz::openbmc_project::Inventory::Item::server::
+        StorageController,
+    private sdbusplus::xyz::openbmc_project::NVMe::server::NVMeAdmin,
+    public std::enable_shared_from_this<NVMeControllerEnabled>
+
+{
+  public:
+    NVMeControllerEnabled(boost::asio::io_context& io,
+                          sdbusplus::asio::object_server& objServer,
+                          std::shared_ptr<sdbusplus::asio::connection> conn,
+                          std::string path,
+                          std::shared_ptr<NVMeMiIntf> nvmeIntf,
+                          nvme_mi_ctrl_t ctrl);
 
+    NVMeControllerEnabled(NVMeController&& nvmeController);
+
+    ~NVMeControllerEnabled() override;
+
+    void start(std::shared_ptr<NVMeControllerPlugin> nvmePlugin) override;
+
+  private:
     /* NVMeAdmin method overload */
 
     /** @brief Implementation for GetLogPage
diff --git a/src/NVMeSubsys.cpp b/src/NVMeSubsys.cpp
index 90db473..aebfd14 100644
--- a/src/NVMeSubsys.cpp
+++ b/src/NVMeSubsys.cpp
@@ -194,6 +194,12 @@ void NVMeSubsystem::start(const SensorData& configData)
                               << std::endl;
                     return;
                 }
+
+                // Enable primary controller since they are required to work
+                auto& primaryController = findPrimary->second.first;
+                primaryController.reset(new NVMeControllerEnabled(
+                    std::move(*primaryController.get())));
+
                 std::vector<std::shared_ptr<NVMeController>> secCntrls;
                 for (int i = 0; i < listHdr.num; i++)
                 {
@@ -206,9 +212,18 @@ void NVMeSubsystem::start(const SensorData& configData)
                                   << std::endl;
                         break;
                     }
-                    secCntrls.push_back(findSecondary->second.first);
+
+                    auto& secondaryController = findSecondary->second.first;
+
+                    // Check Secondary Controller State
+                    if (listHdr.sc_entry[i].scs != 0)
+                    {
+                        secondaryController.reset(new NVMeControllerEnabled(
+                            std::move(*secondaryController.get())));
+                    }
+                    secCntrls.push_back(secondaryController);
                 }
-                findPrimary->second.first->setSecAssoc(secCntrls);
+                primaryController->setSecAssoc(secCntrls);
 
                 // start controller
                 for (auto& [_, pair] : self->controllers)
-- 
2.34.1