[Smartswitch] Add module specific pcie attach/detach functions for smartswitch platforms by gpunathilell · Pull Request #557 · sonic-net/sonic-platform-common

gpunathilell · 2025-04-01T16:35:33Z

Description

As there could be platforms which have multiple PCIE devices per dpu, the module_base implementation is handling the PCIE removal and attachment along with adding the entry details in the PCIE table, so that the pcie daemon which is running will ignore the errors generated from the DPUs,

Motivation and Context

This was done because there are multiple PCIE devices in some platforms and only one in others, we need to have a platform independent method for removal and attach algorithms along with state db entries

How Has This Been Tested?

Additional Information (Optional)

mssonicbld · 2025-04-01T16:35:41Z

/azp run

azure-pipelines · 2025-04-01T16:35:51Z

Azure Pipelines successfully started running 1 pipeline(s).

gpunathilell · 2025-04-01T16:36:18Z

@vvolam @rameshraghupathy Please review

gpunathilell · 2025-04-02T17:46:32Z

/azpw run

mssonicbld · 2025-04-02T17:46:35Z

/AzurePipelines run

azure-pipelines · 2025-04-02T17:46:43Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-04-09T16:49:57Z

/azp run

azure-pipelines · 2025-04-09T16:50:06Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-04-10T14:54:23Z

/azp run

azure-pipelines · 2025-04-10T14:54:32Z

Azure Pipelines successfully started running 1 pipeline(s).

rameshraghupathy

@vvolam @gpunathilell Adding @bmridul to review

Copilot

Pull Request Overview

This PR introduces new PCI attach/detach functions and related state database operations for smartswitch platforms, along with comprehensive tests for these new functionalities.

Added new methods to ModuleBase for handling PCI removal and reattach operations via platform.json.
Introduced a file-based locking mechanism for PCI operations and state database updates.
Extended test coverage for PCI-related functionalities.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
tests/module_base_test.py	Added tests for PCI bus retrieval, state DB entry, locking & PCI operations
sonic_platform_base/module_base.py	Added functions for PCI removal/reattach and file-based PCI locking

Copilot · 2025-04-24T20:29:18Z

sonic_platform_base/module_base.py

+            self.state_db_connector.hset(PCIE_DETACH_INFO_TABLE_KEY, "bus_info", pcie_string)
+            self.state_db_connector.hset(PCIE_DETACH_INFO_TABLE_KEY, "dpu_state", operation)
+        except Exception as e:
+            sys.stderr.write("Failed to write pcie bus infoto state database: {}\n".format(str(e)))


There is a spelling error in the error message ('infoto' should be 'info to'). Please correct the typo.

Suggested change

sys.stderr.write("Failed to write pcie bus infoto state database: {}\n".format(str(e)))

sys.stderr.write("Failed to write pcie bus info to state database: {}\n".format(str(e)))

rameshraghupathy · 2025-04-30T14:54:27Z

sonic_platform_base/module_base.py

+        except Exception as e:
+            sys.stderr.write("Failed to write pcie bus infoto state database: {}\n".format(str(e)))
+
+    def pci_reattach_from_platform_json(self):


@vvolam @gpunathilell This class is a module specific class and we are trying to rescan not only all the modules but also other PCI devices on the box which are not even considered as modules. Can you move this to your module.py ?

@rameshraghupathy Correct me if I am wrong, but there is no other way of performing the rescan (unless the DPUs are under the same parent bus, but doing it in the current method would work even if the DPUs are not under the same bus, so the current implementation is the most generic)
https://github.com/torvalds/linux/blob/7a13c14ee59d4f6c5f4277a86516cbc73a1383a8/Documentation/ABI/testing/sysfs-bus-pci#L74
If that doesn't work then we should be implementing a platform specific pcie_reattach() function.

As discussed in the HLD meeting, we will look into moving the rescan for the whole PCIE tree to chassis instead of module

@rameshraghupathy I had a discussion with Vasundhara, we can move the implementation for the rescan to chassis, but we would have to call both chassis rescan and module rescan separately each time (since the pci bus removal is module specific). So the plan is to remove the independent implementation currently present pci_reattach_from_platform_json and pci_removal_from_platform_json. So pci_detach() and pci_rescan() have to be implemented by vendor.

@vvolam @rameshraghupathy I have removed the relevant pcie removal code, Please re review

mssonicbld · 2025-05-07T20:48:02Z

/azp run

azure-pipelines · 2025-05-07T20:48:11Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-05-07T20:49:42Z

/azp run

azure-pipelines · 2025-05-07T20:49:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-05-07T22:15:42Z

/azp run

azure-pipelines · 2025-05-07T22:15:50Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-05-08T17:32:39Z

/azp run

azure-pipelines · 2025-05-08T17:32:48Z

Azure Pipelines successfully started running 1 pipeline(s).

vvolam

LGTM, other than these minor query/comment

vvolam · 2025-05-20T20:30:03Z

sonic_platform_base/module_base.py

    """
    # Device type definition. Note, this is a constant.
    DEVICE_TYPE = "module"
+    PCI_OPERATION_LOCK_FILE_PATH = "/var/lock/{}_pci.lock"


Query: Is this file separate for each module?

the lock should be valid across all modules, so we are using a generic lock file

As discussed offline, could you check if this lock is still required?

Clarification, File lock is required, and the lock is applicable per module, just to prevent reattach of the module while we are removing the module

vvolam · 2025-05-20T20:36:35Z

sonic_platform_base/module_base.py

+        try:
+            bus_info_list = self.get_pci_bus_info()
+            with self._pci_operation_lock():
+                for bus in bus_info_list:


should we do the attach first and then remove the entry just to be safe?

Order is changed

rameshraghupathy · 2025-05-21T22:20:34Z

sonic_platform_base/module_base.py

+        if self.pci_bus_info:
+            return self.pci_bus_info
+        try:
+            with open("/usr/share/sonic/platform/platform.json", 'r') as f:


shouldn't this be "/usr/share/sonic/device/{platform}/platform.json" ?

Function removed

rameshraghupathy · 2025-05-21T22:20:55Z

sonic_platform_base/module_base.py

        """
        raise NotImplementedError

+    def get_pci_bus_from_platform_json(self):


This function is not used anywhere in this file. This looks like an utility function. Shouldn't this be under utilities?

Unused function, this is removed

rameshraghupathy · 2025-05-21T22:27:52Z

sonic_platform_base/module_base.py

+            with self._pci_operation_lock():
+                for bus in bus_info_list:
+                    self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING)
+                return self.pci_reattach()


Is the intent to attach one module at a time?

In case of our platform, we have multiple buses, so the state db entry is added in series, but the reattach function is called only once

mssonicbld · 2025-05-22T20:33:19Z

/azp run

azure-pipelines · 2025-05-22T20:33:29Z

Azure Pipelines successfully started running 1 pipeline(s).

vvolam · 2025-05-22T22:48:45Z

sonic_platform_base/module_base.py

+            import swsscommon
+            PCIE_DETACH_INFO_TABLE_KEY = PCIE_DETACH_INFO_TABLE+"|"+pcie_string
+            if not self.state_db_connector:
+                self.state_db_connector = swsscommon.swsscommon.DBConnector("STATE_DB", 0)


Check if state_db connection is successful?

rameshraghupathy · 2025-05-27T15:01:43Z

sonic_platform_base/module_base.py

+                    self.pci_entry_state_db(bus, PCIE_OPERATION_DETACHING)
+                return self.pci_detach()
+        except Exception:
+            return False


Can you add an error message in case of failure?

rameshraghupathy · 2025-05-27T15:01:58Z

sonic_platform_base/module_base.py

+                    self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING)
+                return return_value
+        except Exception:
+            return False


Can you add an error message in case of failure?

rameshraghupathy · 2025-05-27T15:02:11Z

sonic_platform_base/module_base.py

+            os.system("service sensord restart")
+
+            return True
+        except Exception as e:


Can you add an error message in case of failure?

rameshraghupathy · 2025-05-27T15:02:24Z

sonic_platform_base/module_base.py

+            os.system("service sensord restart")
+
+            return True
+        except Exception as e:


Can you add an error message in case of failure?

mssonicbld · 2025-05-27T18:00:49Z

/azp run

azure-pipelines · 2025-05-27T18:00:58Z

Azure Pipelines successfully started running 1 pipeline(s).

vvolam · 2025-05-28T16:03:25Z

sonic_platform_base/module_base.py

+    def handle_pci_removal(self):
+        """
+        Handles PCI device removal by updating state database and detaching device.
+        If pci_detach is not implemented, falls back to platform.json based removal.


This comment needs to be fixed as we are not having any fallback mechanism now.

vvolam · 2025-05-28T16:05:08Z

sonic_platform_base/module_base.py

+    def handle_pci_rescan(self):
+        """
+        Handles PCI device rescan by updating state database and reattaching device.
+        If pci_reattach is not implemented, falls back to platform.json based rescan.


Same comment, fallback comment needs to be fixed.

mssonicbld · 2025-05-28T16:13:03Z

/azp run

azure-pipelines · 2025-05-28T16:13:13Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-06-09T04:00:49Z

Cherry-pick PR to 202505: #576

gpunathilell added 3 commits March 12, 2025 22:35

Platform changes to handle PCIE detach/attach

cb310a0

Use file lock instead of thread lock

6117834

Change path

8beb2d6

Changes based on test

76c45f6

Removed deletion and added fixed import

451bdfa

rameshraghupathy reviewed Apr 24, 2025

View reviewed changes

vvolam requested a review from Copilot April 24, 2025 20:28

Copilot AI reviewed Apr 24, 2025

View reviewed changes

wenchungw approved these changes Apr 26, 2025

View reviewed changes

rameshraghupathy reviewed Apr 30, 2025

View reviewed changes

gpunathilell mentioned this pull request Apr 30, 2025

[Smartswitch][Pmon]Changes for Post startup and pre shutdown sonic-net/SONiC#1980

Merged

Removal of platform independen changes

1708c0f

Fix spelling mistake

e4d32a9

Fix test

6cfe8e9

vvolam reviewed May 20, 2025

View reviewed changes

rameshraghupathy reviewed May 21, 2025

View reviewed changes

Fix review comments

07808d8

vvolam approved these changes May 22, 2025

View reviewed changes

rameshraghupathy reviewed May 27, 2025

View reviewed changes

Added error log

e65cb1b

rameshraghupathy approved these changes May 27, 2025

View reviewed changes

vvolam reviewed May 28, 2025

View reviewed changes

Removed extra comment

6849579

judyjoseph approved these changes May 28, 2025

View reviewed changes

judyjoseph merged commit 44ad17b into sonic-net:master May 28, 2025
5 checks passed

oleksandrivantsiv added the Request for 202505 Branch label Jun 6, 2025

yejianquan added the Approved for 202505 Branch label Jun 9, 2025

mssonicbld mentioned this pull request Jun 9, 2025

[action] [PR:557] [Smartswitch] Add module specific pcie attach/detach functions for smartswitch platforms #576

Merged

mssonicbld added the Created PR to 202505 Branch label Jun 9, 2025

mssonicbld added Included in 202505 Branch and removed Created PR to 202505 Branch labels Jun 9, 2025

	sys.stderr.write("Failed to write pcie bus infoto state database: {}\n".format(str(e)))
	sys.stderr.write("Failed to write pcie bus info to state database: {}\n".format(str(e)))

Conversation

gpunathilell commented Apr 1, 2025

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Apr 1, 2025

Uh oh!

azure-pipelines bot commented Apr 1, 2025

Uh oh!

gpunathilell commented Apr 1, 2025

Uh oh!

gpunathilell commented Apr 2, 2025

Uh oh!

mssonicbld commented Apr 2, 2025

Uh oh!

azure-pipelines bot commented Apr 2, 2025

Uh oh!

mssonicbld commented Apr 9, 2025

Uh oh!

azure-pipelines bot commented Apr 9, 2025

Uh oh!

mssonicbld commented Apr 10, 2025

Uh oh!

azure-pipelines bot commented Apr 10, 2025

Uh oh!

rameshraghupathy left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rameshraghupathy Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented May 7, 2025

Uh oh!

azure-pipelines bot commented May 7, 2025

Uh oh!

mssonicbld commented May 7, 2025

Uh oh!

azure-pipelines bot commented May 7, 2025

Uh oh!

mssonicbld commented May 7, 2025

Uh oh!

azure-pipelines bot commented May 7, 2025

Uh oh!

mssonicbld commented May 8, 2025

Uh oh!

azure-pipelines bot commented May 8, 2025

Uh oh!

vvolam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rameshraghupathy Apr 30, 2025 •

edited

Loading