From cb310a0d87cc4680fd5651233555b9c68a5084b2 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 12 Mar 2025 12:10:31 +0000 Subject: [PATCH 01/15] Platform changes to handle PCIE detach/attach --- sonic_platform_base/module_base.py | 142 ++++++++++++++++++++++++++++- tests/module_base_test.py | 131 ++++++++++++++++++++++++++ 2 files changed, 272 insertions(+), 1 deletion(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index ad88a0177..3431203fb 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -7,7 +7,14 @@ import sys from . import device_base +import swsscommon +import json +import threading +# PCI state database constants +PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" +PCIE_OPERATION_DETACHING = "detaching" +PCIE_OPERATION_ATTACHING = "attaching" class ModuleBase(device_base.DeviceBase): """ @@ -17,6 +24,8 @@ class ModuleBase(device_base.DeviceBase): # Device type definition. Note, this is a constant. DEVICE_TYPE = "module" + _pci_operation_lock = threading.Lock() + # Possible card types for modular chassis MODULE_TYPE_SUPERVISOR = "SUPERVISOR" MODULE_TYPE_LINE = "LINE-CARD" @@ -73,6 +82,8 @@ def __init__(self): self._thermal_list = [] self._voltage_sensor_list = [] self._current_sensor_list = [] + self.state_db_connector = None + self.pci_bus_info = None # List of SfpBase-derived objects representing all sfps # available on the module @@ -81,6 +92,22 @@ def __init__(self): # List of ASIC-derived objects representing all ASICs # visibile in PCI domain on the module self._asic_list = [] + + def __del__(self): + """ + Destructor - ensures any PCI state DB entries are cleaned up when the module is deleted + """ + try: + bus_info_list = self.get_pci_bus_info() + for bus in bus_info_list: + self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) + except NotImplementedError: + pci_bus = self.get_pci_bus_from_platform_json() + if pci_bus: + self.pci_entry_state_db(pci_bus, PCIE_OPERATION_ATTACHING) + except Exception: + pass + def get_base_mac(self): """ @@ -271,10 +298,123 @@ def get_pci_bus_info(self): Retrieves the bus information. Returns: - Returns the PCI bus information in BDF format like "[DDDD:]BB:SS:F" + Returns the PCI bus information in list of BDF format like "[DDDD:]BB:SS:F" """ raise NotImplementedError + def get_pci_bus_from_platform_json(self): + """ + Retrieves the PCI bus information from platform.json file. + + Returns: + str: PCI bus information if found in platform.json + None: If module is not found in platform.json or any error occurs + """ + if self.pci_bus_info: + return self.pci_bus_info + try: + with open("/usr/share/sonic/platform/platform.json", 'r') as f: + platform_data = json.load(f) + module_name = self.get_name() + if module_name in platform_data["DPUS"]: + self.pci_bus_info = platform_data["DPUS"][module_name]["bus_info"] + return self.pci_bus_info + return None + except Exception: + return None + + def pci_removal_from_platform_json(self): + """ + Generic function to handle PCI device removal. + + Returns: + bool: True if operation was successful, False otherwise + """ + pci_bus = self.get_pci_bus_from_platform_json() + if pci_bus: + with self._pci_operation_lock: + self.pci_entry_state_db(pci_bus, PCIE_OPERATION_DETACHING) + with open(f"/sys/bus/pci/devices/{pci_bus}/remove", 'w') as f: + f.write("1") + return True + return False + + 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. + + Returns: + bool: True if operation was successful, False otherwise + """ + try: + bus_info_list = self.get_pci_bus_info() + with self._pci_operation_lock: + for bus in bus_info_list: + self.pci_entry_state_db(bus, PCIE_OPERATION_DETACHING) + return self.pci_detach() + except NotImplementedError: + return self.pci_removal_from_platform_json() + except Exception: + return False + + def pci_entry_state_db(self, pcie_string, operation): + """ + Adds the STATE DB entry for pcied so that it can ignore warnings + due to expected PCIE removal. + + Args: + pcie_string (str): The PCI device identifier in BDF format + operation (str): The operation being performed ("detaching" or "attaching") + + Raises: + RuntimeError: If state database connection fails + """ + try: + if not self.state_db_connector: + self.state_db_connector = swsscommon.DBConnector("STATE_DB", 0) + if operation == PCIE_OPERATION_ATTACHING: + self.state_db_connector.hdel(PCIE_DETACH_INFO_TABLE, pcie_string) + return + self.state_db_connector.hset(PCIE_DETACH_INFO_TABLE, pcie_string, operation) + 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): + """ + Generic function to handle PCI device rescan. + + Returns: + bool: True if operation was successful, False otherwise + """ + pci_bus = self.get_pci_bus_from_platform_json() + if pci_bus: + with self._pci_operation_lock: + self.pci_entry_state_db(pci_bus, PCIE_OPERATION_ATTACHING) + with open("/sys/bus/pci/rescan", 'w') as f: + f.write("1") + return True + return False + + 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. + + Returns: + bool: True if operation was successful, False otherwise + """ + try: + bus_info_list = self.get_pci_bus_info() + with self._pci_operation_lock: + for bus in bus_info_list: + self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) + return self.pci_reattach() + except NotImplementedError: + return self.pci_reattach_from_platform_json() + except Exception: + return False + def pci_detach(self): """ Detaches the PCI device. diff --git a/tests/module_base_test.py b/tests/module_base_test.py index e760eabbd..f5f65605f 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -1,4 +1,8 @@ from sonic_platform_base.module_base import ModuleBase +import pytest +import json +import os +from unittest.mock import patch, mock_open, MagicMock class TestModuleBase: @@ -39,3 +43,130 @@ def test_sensors(self): assert(module.get_all_current_sensors() == ["s1"]) assert(module.get_current_sensor(0) == "s1") + def test_get_pci_bus_from_platform_json(self): + module = ModuleBase() + module.pci_bus_info = "0000:00:00.0" + assert module.get_pci_bus_from_platform_json() == "0000:00:00.0" + mock_json_data = { + "DPUS": { + "DPU0": {"bus_info": "0000:01:00.0"} + } + } + module.pci_bus_info = None + platform_json_path = "/usr/share/sonic/platform/platform.json" + with patch('builtins.open', mock_open(read_data=json.dumps(mock_json_data))) as mock_open_call, \ + patch.object(module, 'get_name', return_value="DPU0"): + assert module.get_pci_bus_from_platform_json() == "0000:01:00.0" + mock_open_call.assert_called_once_with(platform_json_path, 'r') + assert module.pci_bus_info == "0000:01:00.0" + + module.pci_bus_info = None + with patch('builtins.open', mock_open(read_data=json.dumps(mock_json_data))) as mock_open_call, \ + patch.object(module, 'get_name', return_value="ABC"): + assert module.get_pci_bus_from_platform_json() is None + mock_open_call.assert_called_once_with(platform_json_path, 'r') + + with patch('builtins.open', side_effect=Exception()) as mock_open_call: + assert module.get_pci_bus_from_platform_json() is None + + def test_pci_entry_state_db(self): + module = ModuleBase() + mock_connector = MagicMock() + module.state_db_connector = mock_connector + + module.pci_entry_state_db("0000:00:00.0", "detaching") + mock_connector.hset.assert_called_with("PCIE_DETACH_INFO", "0000:00:00.0", "detaching") + + module.pci_entry_state_db("0000:00:00.0", "attaching") + mock_connector.hdel.assert_called_with("PCIE_DETACH_INFO", "0000:00:00.0") + + mock_connector.hset.side_effect = Exception("DB Error") + module.pci_entry_state_db("0000:00:00.0", "detaching") + + @patch('builtins.open') + def test_pci_removal_from_platform_json(self, mock_open): + module = ModuleBase() + mock_file = MagicMock() + mock_open.return_value.__enter__.return_value = mock_file + + with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ + patch.object(module, 'pci_entry_state_db') as mock_db: + assert module.pci_removal_from_platform_json() is True + mock_db.assert_called_with("0000:00:00.0", "detaching") + mock_file.write.assert_called_with("1") + mock_open.assert_called_once_with("/sys/bus/pci/devices/0000:00:00.0/remove", 'w') + + mock_open.reset_mock() + with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): + assert module.pci_removal_from_platform_json() is False + mock_open.assert_not_called() + + @patch('builtins.open') + def test_pci_reattach_from_platform_json(self, mock_open): + module = ModuleBase() + mock_file = MagicMock() + mock_open.return_value.__enter__.return_value = mock_file + + with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ + patch.object(module, 'pci_entry_state_db') as mock_db: + assert module.pci_reattach_from_platform_json() is True + mock_db.assert_called_with("0000:00:00.0", "attaching") + mock_file.write.assert_called_with("1") + mock_open.assert_called_once_with("/sys/bus/pci/rescan", 'w') + + + mock_open.reset_mock() + with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): + assert module.pci_reattach_from_platform_json() is False + mock_open.assert_not_called() + + def test_handle_pci_removal(self): + module = ModuleBase() + + with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ + patch.object(module, 'pci_entry_state_db') as mock_db, \ + patch.object(module, 'pci_detach', return_value=True): + assert module.handle_pci_removal() is True + mock_db.assert_called_with("0000:00:00.0", "detaching") + + with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ + patch.object(module, 'pci_removal_from_platform_json', return_value=True): + assert module.handle_pci_removal() is True + + with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): + assert module.handle_pci_removal() is False + + def test_handle_pci_rescan(self): + module = ModuleBase() + + with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ + patch.object(module, 'pci_entry_state_db') as mock_db, \ + patch.object(module, 'pci_reattach', return_value=True): + assert module.handle_pci_rescan() is True + mock_db.assert_called_with("0000:00:00.0", "attaching") + + with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ + patch.object(module, 'pci_reattach_from_platform_json', return_value=True): + assert module.handle_pci_rescan() is True + + with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): + assert module.handle_pci_rescan() is False + + def test_module_cleanup(self): + module = ModuleBase() + + with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ + patch.object(module, 'pci_entry_state_db') as mock_db: + module.__del__() + mock_db.assert_called_with("0000:00:00.0", "attaching") + + with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ + patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:01:00.0"), \ + patch.object(module, 'pci_entry_state_db') as mock_db: + module.__del__() + mock_db.assert_called_with("0000:01:00.0", "attaching") + + with patch.object(module, 'get_pci_bus_info', side_effect=Exception()), \ + patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): + module.__del__() + From 6117834803501e5bac1a35f977a463c96162b42b Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 13 Mar 2025 15:49:59 +0000 Subject: [PATCH 02/15] Use file lock instead of thread lock --- sonic_platform_base/module_base.py | 24 +++++++++++---- tests/module_base_test.py | 48 ++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 3431203fb..8dd0162fd 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -6,10 +6,13 @@ """ import sys +import os +import fcntl from . import device_base import swsscommon import json import threading +import contextlib # PCI state database constants PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" @@ -23,8 +26,7 @@ class ModuleBase(device_base.DeviceBase): """ # Device type definition. Note, this is a constant. DEVICE_TYPE = "module" - - _pci_operation_lock = threading.Lock() + PCI_OPERATION_LOCK_FILE_PATH = "/tmp/{}_pci.lock" # Possible card types for modular chassis MODULE_TYPE_SUPERVISOR = "SUPERVISOR" @@ -108,6 +110,16 @@ def __del__(self): except Exception: pass + @contextlib.contextmanager + def _pci_operation_lock(self): + """File-based lock for PCI operations using flock""" + lock_file_path = self.PCI_OPERATION_LOCK_FILE_PATH.format(self.get_name()) + with open(lock_file_path, 'w') as f: + try: + fcntl.flock(f.fileno(), fcntl.LOCK_EX) + yield + finally: + fcntl.flock(f.fileno(), fcntl.LOCK_UN) def get_base_mac(self): """ @@ -332,7 +344,7 @@ def pci_removal_from_platform_json(self): """ pci_bus = self.get_pci_bus_from_platform_json() if pci_bus: - with self._pci_operation_lock: + with self._pci_operation_lock(): self.pci_entry_state_db(pci_bus, PCIE_OPERATION_DETACHING) with open(f"/sys/bus/pci/devices/{pci_bus}/remove", 'w') as f: f.write("1") @@ -349,7 +361,7 @@ def handle_pci_removal(self): """ try: bus_info_list = self.get_pci_bus_info() - with self._pci_operation_lock: + with self._pci_operation_lock(): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_DETACHING) return self.pci_detach() @@ -389,7 +401,7 @@ def pci_reattach_from_platform_json(self): """ pci_bus = self.get_pci_bus_from_platform_json() if pci_bus: - with self._pci_operation_lock: + with self._pci_operation_lock(): self.pci_entry_state_db(pci_bus, PCIE_OPERATION_ATTACHING) with open("/sys/bus/pci/rescan", 'w') as f: f.write("1") @@ -406,7 +418,7 @@ def handle_pci_rescan(self): """ try: bus_info_list = self.get_pci_bus_info() - with self._pci_operation_lock: + with self._pci_operation_lock(): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) return self.pci_reattach() diff --git a/tests/module_base_test.py b/tests/module_base_test.py index f5f65605f..6f7d2da17 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -2,7 +2,8 @@ import pytest import json import os -from unittest.mock import patch, mock_open, MagicMock +import fcntl +from unittest.mock import patch, mock_open, MagicMock, call class TestModuleBase: @@ -83,6 +84,24 @@ def test_pci_entry_state_db(self): mock_connector.hset.side_effect = Exception("DB Error") module.pci_entry_state_db("0000:00:00.0", "detaching") + def test_pci_operation_lock(self): + module = ModuleBase() + mock_file = MagicMock() + mock_open_obj = mock_open() + mock_open_obj.reset_mock() + with patch('builtins.open', mock_open_obj) as mock_file_open, \ + patch('fcntl.flock') as mock_flock, \ + patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.makedirs') as mock_makedirs: + + with module._pci_operation_lock(): + mock_flock.assert_called_with(mock_file_open().fileno(), fcntl.LOCK_EX) + + mock_flock.assert_has_calls([ + call(mock_file_open().fileno(), fcntl.LOCK_EX), + call(mock_file_open().fileno(), fcntl.LOCK_UN) + ]) + @patch('builtins.open') def test_pci_removal_from_platform_json(self, mock_open): module = ModuleBase() @@ -90,12 +109,14 @@ def test_pci_removal_from_platform_json(self, mock_open): mock_open.return_value.__enter__.return_value = mock_file with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ - patch.object(module, 'pci_entry_state_db') as mock_db: + patch.object(module, 'pci_entry_state_db') as mock_db, \ + patch.object(module, '_pci_operation_lock') as mock_lock, \ + patch.object(module, 'get_name', return_value="DPU0"): assert module.pci_removal_from_platform_json() is True mock_db.assert_called_with("0000:00:00.0", "detaching") mock_file.write.assert_called_with("1") - mock_open.assert_called_once_with("/sys/bus/pci/devices/0000:00:00.0/remove", 'w') - + mock_open.assert_called_with("/sys/bus/pci/devices/0000:00:00.0/remove", 'w') + mock_lock.assert_called_once() mock_open.reset_mock() with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): assert module.pci_removal_from_platform_json() is False @@ -108,26 +129,32 @@ def test_pci_reattach_from_platform_json(self, mock_open): mock_open.return_value.__enter__.return_value = mock_file with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ - patch.object(module, 'pci_entry_state_db') as mock_db: + patch.object(module, 'pci_entry_state_db') as mock_db, \ + patch.object(module, '_pci_operation_lock') as mock_lock, \ + patch.object(module, 'get_name', return_value="DPU0"): assert module.pci_reattach_from_platform_json() is True mock_db.assert_called_with("0000:00:00.0", "attaching") mock_file.write.assert_called_with("1") - mock_open.assert_called_once_with("/sys/bus/pci/rescan", 'w') - + mock_open.assert_called_with("/sys/bus/pci/rescan", 'w') + mock_lock.assert_called_once() mock_open.reset_mock() with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): assert module.pci_reattach_from_platform_json() is False mock_open.assert_not_called() + mock_open.reset_mock() def test_handle_pci_removal(self): module = ModuleBase() with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ patch.object(module, 'pci_entry_state_db') as mock_db, \ - patch.object(module, 'pci_detach', return_value=True): + patch.object(module, 'pci_detach', return_value=True), \ + patch.object(module, '_pci_operation_lock') as mock_lock, \ + patch.object(module, 'get_name', return_value="DPU0"): assert module.handle_pci_removal() is True mock_db.assert_called_with("0000:00:00.0", "detaching") + mock_lock.assert_called_once() with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ patch.object(module, 'pci_removal_from_platform_json', return_value=True): @@ -141,9 +168,12 @@ def test_handle_pci_rescan(self): with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ patch.object(module, 'pci_entry_state_db') as mock_db, \ - patch.object(module, 'pci_reattach', return_value=True): + patch.object(module, 'pci_reattach', return_value=True), \ + patch.object(module, '_pci_operation_lock') as mock_lock, \ + patch.object(module, 'get_name', return_value="DPU0"): assert module.handle_pci_rescan() is True mock_db.assert_called_with("0000:00:00.0", "attaching") + mock_lock.assert_called_once() with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ patch.object(module, 'pci_reattach_from_platform_json', return_value=True): From 8beb2d645da3b56c378d5390b35e512305ae07dd Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Tue, 25 Mar 2025 19:56:49 +0000 Subject: [PATCH 03/15] Change path --- sonic_platform_base/module_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 8dd0162fd..71d04587b 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -26,7 +26,7 @@ class ModuleBase(device_base.DeviceBase): """ # Device type definition. Note, this is a constant. DEVICE_TYPE = "module" - PCI_OPERATION_LOCK_FILE_PATH = "/tmp/{}_pci.lock" + PCI_OPERATION_LOCK_FILE_PATH = "/var/lock/{}_pci.lock" # Possible card types for modular chassis MODULE_TYPE_SUPERVISOR = "SUPERVISOR" From 76c45f66dc9b3c83f2f51043e1eb3dfccf01a988 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 9 Apr 2025 16:49:41 +0000 Subject: [PATCH 04/15] Changes based on test --- sonic_platform_base/module_base.py | 13 ++--- tests/module_base_test.py | 84 +++++++++++++++++++----------- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 71d04587b..d849ba1cc 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -372,23 +372,24 @@ def handle_pci_removal(self): def pci_entry_state_db(self, pcie_string, operation): """ - Adds the STATE DB entry for pcied so that it can ignore warnings - due to expected PCIE removal. + Generic function to handle PCI device state database entry. Args: - pcie_string (str): The PCI device identifier in BDF format + pcie_string (str): The PCI bus string to be written to state database operation (str): The operation being performed ("detaching" or "attaching") Raises: RuntimeError: If state database connection fails """ try: + PCIE_DETACH_INFO_TABLE_KEY = PCIE_DETACH_INFO_TABLE+"|"+pcie_string if not self.state_db_connector: - self.state_db_connector = swsscommon.DBConnector("STATE_DB", 0) + self.state_db_connector = swsscommon.swsscommon.DBConnector("STATE_DB", 0) if operation == PCIE_OPERATION_ATTACHING: - self.state_db_connector.hdel(PCIE_DETACH_INFO_TABLE, pcie_string) + self.state_db_connector.delete(PCIE_DETACH_INFO_TABLE_KEY) return - self.state_db_connector.hset(PCIE_DETACH_INFO_TABLE, pcie_string, operation) + 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))) diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 6f7d2da17..635a0b4cb 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -3,7 +3,32 @@ import json import os import fcntl -from unittest.mock import patch, mock_open, MagicMock, call +from unittest.mock import patch, MagicMock, call +from io import StringIO + +class MockFile: + def __init__(self, data=None): + self.data = data + self.written_data = None + self.closed = False + self.fileno_called = False + + def __enter__(self): + return self + + def __exit__(self, *args): + self.closed = True + + def read(self): + return self.data + + def write(self, data): + self.written_data = data + + def fileno(self): + self.fileno_called = True + return 123 + class TestModuleBase: @@ -55,19 +80,20 @@ def test_get_pci_bus_from_platform_json(self): } module.pci_bus_info = None platform_json_path = "/usr/share/sonic/platform/platform.json" - with patch('builtins.open', mock_open(read_data=json.dumps(mock_json_data))) as mock_open_call, \ + + with patch('builtins.open', return_value=MockFile(json.dumps(mock_json_data))) as mock_open_call, \ patch.object(module, 'get_name', return_value="DPU0"): assert module.get_pci_bus_from_platform_json() == "0000:01:00.0" mock_open_call.assert_called_once_with(platform_json_path, 'r') assert module.pci_bus_info == "0000:01:00.0" module.pci_bus_info = None - with patch('builtins.open', mock_open(read_data=json.dumps(mock_json_data))) as mock_open_call, \ + with patch('builtins.open', return_value=MockFile(json.dumps(mock_json_data))) as mock_open_call, \ patch.object(module, 'get_name', return_value="ABC"): assert module.get_pci_bus_from_platform_json() is None mock_open_call.assert_called_once_with(platform_json_path, 'r') - with patch('builtins.open', side_effect=Exception()) as mock_open_call: + with patch('builtins.open', side_effect=Exception()): assert module.get_pci_bus_from_platform_json() is None def test_pci_entry_state_db(self): @@ -76,73 +102,69 @@ def test_pci_entry_state_db(self): module.state_db_connector = mock_connector module.pci_entry_state_db("0000:00:00.0", "detaching") - mock_connector.hset.assert_called_with("PCIE_DETACH_INFO", "0000:00:00.0", "detaching") + mock_connector.hset.assert_has_calls([ + call("PCIE_DETACH_INFO|0000:00:00.0", "bus_info", "0000:00:00.0"), + call("PCIE_DETACH_INFO|0000:00:00.0", "dpu_state", "detaching") + ]) module.pci_entry_state_db("0000:00:00.0", "attaching") - mock_connector.hdel.assert_called_with("PCIE_DETACH_INFO", "0000:00:00.0") + mock_connector.delete.assert_called_with("PCIE_DETACH_INFO|0000:00:00.0") mock_connector.hset.side_effect = Exception("DB Error") module.pci_entry_state_db("0000:00:00.0", "detaching") def test_pci_operation_lock(self): module = ModuleBase() - mock_file = MagicMock() - mock_open_obj = mock_open() - mock_open_obj.reset_mock() - with patch('builtins.open', mock_open_obj) as mock_file_open, \ + mock_file = MockFile() + + with patch('builtins.open', return_value=mock_file) as mock_file_open, \ patch('fcntl.flock') as mock_flock, \ patch.object(module, 'get_name', return_value="DPU0"), \ patch('os.makedirs') as mock_makedirs: with module._pci_operation_lock(): - mock_flock.assert_called_with(mock_file_open().fileno(), fcntl.LOCK_EX) + mock_flock.assert_called_with(123, fcntl.LOCK_EX) mock_flock.assert_has_calls([ - call(mock_file_open().fileno(), fcntl.LOCK_EX), - call(mock_file_open().fileno(), fcntl.LOCK_UN) + call(123, fcntl.LOCK_EX), + call(123, fcntl.LOCK_UN) ]) + assert mock_file.fileno_called - @patch('builtins.open') - def test_pci_removal_from_platform_json(self, mock_open): + def test_pci_removal_from_platform_json(self): module = ModuleBase() - mock_file = MagicMock() - mock_open.return_value.__enter__.return_value = mock_file - - with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ + mock_file = MockFile() + with patch('builtins.open', return_value=mock_file) as mock_open, \ + patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ patch.object(module, 'pci_entry_state_db') as mock_db, \ patch.object(module, '_pci_operation_lock') as mock_lock, \ patch.object(module, 'get_name', return_value="DPU0"): assert module.pci_removal_from_platform_json() is True mock_db.assert_called_with("0000:00:00.0", "detaching") - mock_file.write.assert_called_with("1") + assert mock_file.written_data == "1" mock_open.assert_called_with("/sys/bus/pci/devices/0000:00:00.0/remove", 'w') mock_lock.assert_called_once() - mock_open.reset_mock() + with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): assert module.pci_removal_from_platform_json() is False - mock_open.assert_not_called() - @patch('builtins.open') - def test_pci_reattach_from_platform_json(self, mock_open): + def test_pci_reattach_from_platform_json(self): module = ModuleBase() - mock_file = MagicMock() - mock_open.return_value.__enter__.return_value = mock_file + mock_file = MockFile() - with patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ + with patch('builtins.open', return_value=mock_file) as mock_open, \ + patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ patch.object(module, 'pci_entry_state_db') as mock_db, \ patch.object(module, '_pci_operation_lock') as mock_lock, \ patch.object(module, 'get_name', return_value="DPU0"): assert module.pci_reattach_from_platform_json() is True mock_db.assert_called_with("0000:00:00.0", "attaching") - mock_file.write.assert_called_with("1") + assert mock_file.written_data == "1" mock_open.assert_called_with("/sys/bus/pci/rescan", 'w') mock_lock.assert_called_once() - mock_open.reset_mock() with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): assert module.pci_reattach_from_platform_json() is False - mock_open.assert_not_called() - mock_open.reset_mock() def test_handle_pci_removal(self): module = ModuleBase() From 451bdfa7f9aa9855e99bb3e617f618e0ab7a1650 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 10 Apr 2025 14:54:03 +0000 Subject: [PATCH 05/15] Removed deletion and added fixed import --- sonic_platform_base/module_base.py | 18 ++---------------- tests/module_base_test.py | 19 ------------------- 2 files changed, 2 insertions(+), 35 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index d849ba1cc..26c999599 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -9,7 +9,6 @@ import os import fcntl from . import device_base -import swsscommon import json import threading import contextlib @@ -95,21 +94,6 @@ def __init__(self): # visibile in PCI domain on the module self._asic_list = [] - def __del__(self): - """ - Destructor - ensures any PCI state DB entries are cleaned up when the module is deleted - """ - try: - bus_info_list = self.get_pci_bus_info() - for bus in bus_info_list: - self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) - except NotImplementedError: - pci_bus = self.get_pci_bus_from_platform_json() - if pci_bus: - self.pci_entry_state_db(pci_bus, PCIE_OPERATION_ATTACHING) - except Exception: - pass - @contextlib.contextmanager def _pci_operation_lock(self): """File-based lock for PCI operations using flock""" @@ -382,6 +366,8 @@ def pci_entry_state_db(self, pcie_string, operation): RuntimeError: If state database connection fails """ try: + # Do not use import if swsscommon is not needed + 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) diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 635a0b4cb..82f9330a8 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -203,22 +203,3 @@ def test_handle_pci_rescan(self): with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): assert module.handle_pci_rescan() is False - - def test_module_cleanup(self): - module = ModuleBase() - - with patch.object(module, 'get_pci_bus_info', return_value=["0000:00:00.0"]), \ - patch.object(module, 'pci_entry_state_db') as mock_db: - module.__del__() - mock_db.assert_called_with("0000:00:00.0", "attaching") - - with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ - patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:01:00.0"), \ - patch.object(module, 'pci_entry_state_db') as mock_db: - module.__del__() - mock_db.assert_called_with("0000:01:00.0", "attaching") - - with patch.object(module, 'get_pci_bus_info', side_effect=Exception()), \ - patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): - module.__del__() - From 1708c0f5be12652ada613f3b3d083b93e3df8d82 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 7 May 2025 20:47:40 +0000 Subject: [PATCH 06/15] Removal of platform independen changes --- sonic_platform_base/module_base.py | 36 ------------------------------ tests/module_base_test.py | 35 ----------------------------- 2 files changed, 71 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 26c999599..808c88f4c 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -318,22 +318,6 @@ def get_pci_bus_from_platform_json(self): return None except Exception: return None - - def pci_removal_from_platform_json(self): - """ - Generic function to handle PCI device removal. - - Returns: - bool: True if operation was successful, False otherwise - """ - pci_bus = self.get_pci_bus_from_platform_json() - if pci_bus: - with self._pci_operation_lock(): - self.pci_entry_state_db(pci_bus, PCIE_OPERATION_DETACHING) - with open(f"/sys/bus/pci/devices/{pci_bus}/remove", 'w') as f: - f.write("1") - return True - return False def handle_pci_removal(self): """ @@ -349,8 +333,6 @@ def handle_pci_removal(self): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_DETACHING) return self.pci_detach() - except NotImplementedError: - return self.pci_removal_from_platform_json() except Exception: return False @@ -379,22 +361,6 @@ def pci_entry_state_db(self, pcie_string, operation): 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): - """ - Generic function to handle PCI device rescan. - - Returns: - bool: True if operation was successful, False otherwise - """ - pci_bus = self.get_pci_bus_from_platform_json() - if pci_bus: - with self._pci_operation_lock(): - self.pci_entry_state_db(pci_bus, PCIE_OPERATION_ATTACHING) - with open("/sys/bus/pci/rescan", 'w') as f: - f.write("1") - return True - return False - def handle_pci_rescan(self): """ Handles PCI device rescan by updating state database and reattaching device. @@ -409,8 +375,6 @@ def handle_pci_rescan(self): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) return self.pci_reattach() - except NotImplementedError: - return self.pci_reattach_from_platform_json() except Exception: return False diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 82f9330a8..207fe8c1d 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -131,41 +131,6 @@ def test_pci_operation_lock(self): ]) assert mock_file.fileno_called - def test_pci_removal_from_platform_json(self): - module = ModuleBase() - mock_file = MockFile() - with patch('builtins.open', return_value=mock_file) as mock_open, \ - patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ - patch.object(module, 'pci_entry_state_db') as mock_db, \ - patch.object(module, '_pci_operation_lock') as mock_lock, \ - patch.object(module, 'get_name', return_value="DPU0"): - assert module.pci_removal_from_platform_json() is True - mock_db.assert_called_with("0000:00:00.0", "detaching") - assert mock_file.written_data == "1" - mock_open.assert_called_with("/sys/bus/pci/devices/0000:00:00.0/remove", 'w') - mock_lock.assert_called_once() - - with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): - assert module.pci_removal_from_platform_json() is False - - def test_pci_reattach_from_platform_json(self): - module = ModuleBase() - mock_file = MockFile() - - with patch('builtins.open', return_value=mock_file) as mock_open, \ - patch.object(module, 'get_pci_bus_from_platform_json', return_value="0000:00:00.0"), \ - patch.object(module, 'pci_entry_state_db') as mock_db, \ - patch.object(module, '_pci_operation_lock') as mock_lock, \ - patch.object(module, 'get_name', return_value="DPU0"): - assert module.pci_reattach_from_platform_json() is True - mock_db.assert_called_with("0000:00:00.0", "attaching") - assert mock_file.written_data == "1" - mock_open.assert_called_with("/sys/bus/pci/rescan", 'w') - mock_lock.assert_called_once() - - with patch.object(module, 'get_pci_bus_from_platform_json', return_value=None): - assert module.pci_reattach_from_platform_json() is False - def test_handle_pci_removal(self): module = ModuleBase() From e4d32a9956f8911dcfb45783f0aea639f631640c Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 7 May 2025 20:49:28 +0000 Subject: [PATCH 07/15] Fix spelling mistake --- sonic_platform_base/module_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 808c88f4c..3665face2 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -359,7 +359,7 @@ def pci_entry_state_db(self, pcie_string, operation): 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))) + sys.stderr.write("Failed to write pcie bus info to state database: {}\n".format(str(e))) def handle_pci_rescan(self): """ From 6cfe8e94ddb62bf97b2ac3f90a3cd2afaab2e121 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 7 May 2025 22:15:17 +0000 Subject: [PATCH 08/15] Fix test --- tests/module_base_test.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 207fe8c1d..8e6e77eba 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -143,10 +143,6 @@ def test_handle_pci_removal(self): mock_db.assert_called_with("0000:00:00.0", "detaching") mock_lock.assert_called_once() - with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ - patch.object(module, 'pci_removal_from_platform_json', return_value=True): - assert module.handle_pci_removal() is True - with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): assert module.handle_pci_removal() is False @@ -162,9 +158,5 @@ def test_handle_pci_rescan(self): mock_db.assert_called_with("0000:00:00.0", "attaching") mock_lock.assert_called_once() - with patch.object(module, 'get_pci_bus_info', side_effect=NotImplementedError()), \ - patch.object(module, 'pci_reattach_from_platform_json', return_value=True): - assert module.handle_pci_rescan() is True - with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): assert module.handle_pci_rescan() is False From e3c1654999544f3e672cac7754e98d810d850fce Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 1 May 2025 18:26:51 +0000 Subject: [PATCH 09/15] Added sensord changes --- sonic_platform_base/module_base.py | 54 ++++++++++++++++++++++++++++++ tests/module_base_test.py | 50 +++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 3665face2..02bf63bf0 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -790,3 +790,57 @@ def get_all_asics(self): And '0000:05:00.0' is its PCI address. """ return self._asic_list + + def handle_sensor_removal(self): + """ + Handles sensor removal by copying ignore configuration file from platform folder + to sensors.d directory and restarting sensord if the file exists. + + Returns: + bool: True if operation was successful, False otherwise + """ + try: + module_name = self.get_name() + source_file = f"/usr/share/sonic/platform/dpu_ignore_conf/ignore_{module_name}.conf" + target_file = f"/etc/sensors.d/ignore_{module_name}.conf" + + # If source file does not exist, we dont need to copy it and restart sensord + if not os.path.exists(source_file): + return True + + # Copy the file + import shutil + shutil.copy2(source_file, target_file) + + # Restart sensord + os.system("service sensord restart") + + return True + except Exception as e: + return False + + def handle_sensor_addition(self): + """ + Handles sensor addition by removing the ignore configuration file from + sensors.d directory and restarting sensord. + + Returns: + bool: True if operation was successful, False otherwise + """ + try: + module_name = self.get_name() + target_file = f"/etc/sensors.d/ignore_{module_name}.conf" + + # If target file does not exist, we dont need to remove it and restart sensord + if not os.path.exists(target_file): + return True + + # Remove the file + os.remove(target_file) + + # Restart sensord + os.system("service sensord restart") + + return True + except Exception as e: + return False \ No newline at end of file diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 8e6e77eba..514f7d2de 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -5,6 +5,7 @@ import fcntl from unittest.mock import patch, MagicMock, call from io import StringIO +import shutil class MockFile: def __init__(self, data=None): @@ -160,3 +161,52 @@ def test_handle_pci_rescan(self): with patch.object(module, 'get_pci_bus_info', side_effect=Exception()): assert module.handle_pci_rescan() is False + + def test_handle_sensor_removal(self): + module = ModuleBase() + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=True), \ + patch('shutil.copy2') as mock_copy, \ + patch('os.system') as mock_system: + assert module.handle_sensor_removal() is True + mock_copy.assert_called_once_with("/usr/share/sonic/platform/dpu_ignore_conf/ignore_DPU0.conf", + "/etc/sensors.d/ignore_DPU0.conf") + mock_system.assert_called_once_with("service sensord restart") + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=False), \ + patch('shutil.copy2') as mock_copy, \ + patch('os.system') as mock_system: + assert module.handle_sensor_removal() is True + mock_copy.assert_not_called() + mock_system.assert_not_called() + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=True), \ + patch('shutil.copy2', side_effect=Exception("Copy failed")): + assert module.handle_sensor_removal() is False + + def test_handle_sensor_addition(self): + module = ModuleBase() + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=True), \ + patch('os.remove') as mock_remove, \ + patch('os.system') as mock_system: + assert module.handle_sensor_addition() is True + mock_remove.assert_called_once_with("/etc/sensors.d/ignore_DPU0.conf") + mock_system.assert_called_once_with("service sensord restart") + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=False), \ + patch('os.remove') as mock_remove, \ + patch('os.system') as mock_system: + assert module.handle_sensor_addition() is True + mock_remove.assert_not_called() + mock_system.assert_not_called() + + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('os.path.exists', return_value=True), \ + patch('os.remove', side_effect=Exception("Remove failed")): + assert module.handle_sensor_addition() is False From 57ca44e741387cfd05db60e5da59b5352f318a48 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Fri, 2 May 2025 16:14:49 +0000 Subject: [PATCH 10/15] Change path --- sonic_platform_base/module_base.py | 9 ++++----- tests/module_base_test.py | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 02bf63bf0..6910e3c10 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -12,6 +12,7 @@ import json import threading import contextlib +import shutil # PCI state database constants PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" @@ -801,15 +802,13 @@ def handle_sensor_removal(self): """ try: module_name = self.get_name() - source_file = f"/usr/share/sonic/platform/dpu_ignore_conf/ignore_{module_name}.conf" + source_file = f"/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_{module_name}.conf" target_file = f"/etc/sensors.d/ignore_{module_name}.conf" # If source file does not exist, we dont need to copy it and restart sensord if not os.path.exists(source_file): return True - # Copy the file - import shutil shutil.copy2(source_file, target_file) # Restart sensord @@ -829,7 +828,7 @@ def handle_sensor_addition(self): """ try: module_name = self.get_name() - target_file = f"/etc/sensors.d/ignore_{module_name}.conf" + target_file = f"/etc/sensors.d/ignore_sensors_{module_name}.conf" # If target file does not exist, we dont need to remove it and restart sensord if not os.path.exists(target_file): @@ -843,4 +842,4 @@ def handle_sensor_addition(self): return True except Exception as e: - return False \ No newline at end of file + return False diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 514f7d2de..c444621f7 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -170,8 +170,8 @@ def test_handle_sensor_removal(self): patch('shutil.copy2') as mock_copy, \ patch('os.system') as mock_system: assert module.handle_sensor_removal() is True - mock_copy.assert_called_once_with("/usr/share/sonic/platform/dpu_ignore_conf/ignore_DPU0.conf", - "/etc/sensors.d/ignore_DPU0.conf") + mock_copy.assert_called_once_with("/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf", + "/etc/sensors.d/ignore_sensors_DPU0.conf") mock_system.assert_called_once_with("service sensord restart") with patch.object(module, 'get_name', return_value="DPU0"), \ @@ -195,7 +195,7 @@ def test_handle_sensor_addition(self): patch('os.remove') as mock_remove, \ patch('os.system') as mock_system: assert module.handle_sensor_addition() is True - mock_remove.assert_called_once_with("/etc/sensors.d/ignore_DPU0.conf") + mock_remove.assert_called_once_with("/etc/sensors.d/ignore_sensors_DPU0.conf") mock_system.assert_called_once_with("service sensord restart") with patch.object(module, 'get_name', return_value="DPU0"), \ From 74380d10e472000b71fe3a61364fca686242c42e Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 8 May 2025 17:15:47 +0000 Subject: [PATCH 11/15] Add single function for sensord and pcie --- sonic_platform_base/module_base.py | 24 ++++++++++++++++++++ tests/module_base_test.py | 36 ++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 6910e3c10..f23a0c79c 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -843,3 +843,27 @@ def handle_sensor_addition(self): return True except Exception as e: return False + + def module_pre_shutdown(self): + """ + Handles module pre-shutdown operations by detaching PCI devices and handling sensor removal. + This function should be called before shutting down a module. + + Returns: + bool: True if all operations were successful, False otherwise + """ + sensor_result = self.handle_sensor_removal() + pci_result = self.handle_pci_removal() + return pci_result and sensor_result + + def module_post_startup(self): + """ + Handles module post-startup operations by reattaching PCI devices and handling sensor addition. + This function should be called after a module has started up. + + Returns: + bool: True if all operations were successful, False otherwise + """ + pci_result = self.handle_pci_rescan() + sensor_result = self.handle_sensor_addition() + return pci_result and sensor_result diff --git a/tests/module_base_test.py b/tests/module_base_test.py index c444621f7..de1c462a0 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -210,3 +210,39 @@ def test_handle_sensor_addition(self): patch('os.path.exists', return_value=True), \ patch('os.remove', side_effect=Exception("Remove failed")): assert module.handle_sensor_addition() is False + + def test_module_pre_shutdown(self): + module = ModuleBase() + + # Test successful case + with patch.object(module, 'handle_pci_removal', return_value=True), \ + patch.object(module, 'handle_sensor_removal', return_value=True): + assert module.module_pre_shutdown() is True + + # Test PCI removal failure + with patch.object(module, 'handle_pci_removal', return_value=False), \ + patch.object(module, 'handle_sensor_removal', return_value=True): + assert module.module_pre_shutdown() is False + + # Test sensor removal failure + with patch.object(module, 'handle_pci_removal', return_value=True), \ + patch.object(module, 'handle_sensor_removal', return_value=False): + assert module.module_pre_shutdown() is False + + def test_module_post_startup(self): + module = ModuleBase() + + # Test successful case + with patch.object(module, 'handle_pci_rescan', return_value=True), \ + patch.object(module, 'handle_sensor_addition', return_value=True): + assert module.module_post_startup() is True + + # Test PCI rescan failure + with patch.object(module, 'handle_pci_rescan', return_value=False), \ + patch.object(module, 'handle_sensor_addition', return_value=True): + assert module.module_post_startup() is False + + # Test sensor addition failure + with patch.object(module, 'handle_pci_rescan', return_value=True), \ + patch.object(module, 'handle_sensor_addition', return_value=False): + assert module.module_post_startup() is False From d4ffc7c91b0d30770ec17dcd261a82780138359f Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 8 May 2025 17:32:25 +0000 Subject: [PATCH 12/15] Fix path --- sonic_platform_base/module_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index f23a0c79c..214a24363 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -803,7 +803,7 @@ def handle_sensor_removal(self): try: module_name = self.get_name() source_file = f"/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_{module_name}.conf" - target_file = f"/etc/sensors.d/ignore_{module_name}.conf" + target_file = f"/etc/sensors.d/ignore_sensors_{module_name}.conf" # If source file does not exist, we dont need to copy it and restart sensord if not os.path.exists(source_file): From 07808d87e36941d48c00f241e896538aad236863 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Thu, 22 May 2025 20:31:59 +0000 Subject: [PATCH 13/15] Fix review comments --- sonic_platform_base/module_base.py | 24 ++---------------------- tests/module_base_test.py | 27 --------------------------- 2 files changed, 2 insertions(+), 49 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 214a24363..520bfec44 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -299,27 +299,6 @@ def get_pci_bus_info(self): """ raise NotImplementedError - def get_pci_bus_from_platform_json(self): - """ - Retrieves the PCI bus information from platform.json file. - - Returns: - str: PCI bus information if found in platform.json - None: If module is not found in platform.json or any error occurs - """ - if self.pci_bus_info: - return self.pci_bus_info - try: - with open("/usr/share/sonic/platform/platform.json", 'r') as f: - platform_data = json.load(f) - module_name = self.get_name() - if module_name in platform_data["DPUS"]: - self.pci_bus_info = platform_data["DPUS"][module_name]["bus_info"] - return self.pci_bus_info - return None - except Exception: - return None - def handle_pci_removal(self): """ Handles PCI device removal by updating state database and detaching device. @@ -373,9 +352,10 @@ def handle_pci_rescan(self): try: bus_info_list = self.get_pci_bus_info() with self._pci_operation_lock(): + return_value = self.pci_reattach() for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) - return self.pci_reattach() + return return_value except Exception: return False diff --git a/tests/module_base_test.py b/tests/module_base_test.py index de1c462a0..b041f4349 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -70,33 +70,6 @@ def test_sensors(self): assert(module.get_all_current_sensors() == ["s1"]) assert(module.get_current_sensor(0) == "s1") - def test_get_pci_bus_from_platform_json(self): - module = ModuleBase() - module.pci_bus_info = "0000:00:00.0" - assert module.get_pci_bus_from_platform_json() == "0000:00:00.0" - mock_json_data = { - "DPUS": { - "DPU0": {"bus_info": "0000:01:00.0"} - } - } - module.pci_bus_info = None - platform_json_path = "/usr/share/sonic/platform/platform.json" - - with patch('builtins.open', return_value=MockFile(json.dumps(mock_json_data))) as mock_open_call, \ - patch.object(module, 'get_name', return_value="DPU0"): - assert module.get_pci_bus_from_platform_json() == "0000:01:00.0" - mock_open_call.assert_called_once_with(platform_json_path, 'r') - assert module.pci_bus_info == "0000:01:00.0" - - module.pci_bus_info = None - with patch('builtins.open', return_value=MockFile(json.dumps(mock_json_data))) as mock_open_call, \ - patch.object(module, 'get_name', return_value="ABC"): - assert module.get_pci_bus_from_platform_json() is None - mock_open_call.assert_called_once_with(platform_json_path, 'r') - - with patch('builtins.open', side_effect=Exception()): - assert module.get_pci_bus_from_platform_json() is None - def test_pci_entry_state_db(self): module = ModuleBase() mock_connector = MagicMock() From e65cb1b205637e173a67fe9c813c3ba913d9a282 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Tue, 27 May 2025 18:00:23 +0000 Subject: [PATCH 14/15] Added error log --- sonic_platform_base/module_base.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 520bfec44..619e7c883 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -313,7 +313,8 @@ def handle_pci_removal(self): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_DETACHING) return self.pci_detach() - except Exception: + except Exception as e: + sys.stderr.write("Failed to handle PCI removal: {}\n".format(str(e))) return False def pci_entry_state_db(self, pcie_string, operation): @@ -356,7 +357,8 @@ def handle_pci_rescan(self): for bus in bus_info_list: self.pci_entry_state_db(bus, PCIE_OPERATION_ATTACHING) return return_value - except Exception: + except Exception as e: + sys.stderr.write("Failed to handle PCI rescan: {}\n".format(str(e))) return False def pci_detach(self): @@ -796,6 +798,7 @@ def handle_sensor_removal(self): return True except Exception as e: + sys.stderr.write("Failed to handle sensor removal: {}\n".format(str(e))) return False def handle_sensor_addition(self): @@ -822,6 +825,7 @@ def handle_sensor_addition(self): return True except Exception as e: + sys.stderr.write("Failed to handle sensor addition: {}\n".format(str(e))) return False def module_pre_shutdown(self): From 68495798210ade9e89c29622b8f6c3e34b232f71 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Wed, 28 May 2025 16:12:19 +0000 Subject: [PATCH 15/15] Removed extra comment --- sonic_platform_base/module_base.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 619e7c883..d4d0ba448 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -302,7 +302,6 @@ def get_pci_bus_info(self): 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. Returns: bool: True if operation was successful, False otherwise @@ -345,7 +344,6 @@ def pci_entry_state_db(self, pcie_string, operation): 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. Returns: bool: True if operation was successful, False otherwise