From 817b984814d2f5306b84952c5148407d1eef087d Mon Sep 17 00:00:00 2001 From: xinyu Date: Tue, 30 Jul 2024 22:04:54 +0800 Subject: [PATCH] [VSC] Fix infinite loop issue caused by unexpected large ext_length in Vsc_CompleteCmd Signed-off-by: xinyu --- .../sonic_xcvr/api/public/cmis.py | 27 +++++++---- .../sonic_xcvr/api/public/cmisCDB.py | 31 ++++++++----- tests/sonic_xcvr/test_cmis.py | 46 +++++++++++++++---- tests/sonic_xcvr/test_cmisCDB.py | 8 ++-- 4 files changed, 78 insertions(+), 34 deletions(-) diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmis.py b/sonic_platform_base/sonic_xcvr/api/public/cmis.py index cc6cd5920..68fbdb0b9 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmis.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmis.py @@ -1240,7 +1240,7 @@ def get_module_fw_mgmt_feature(self, verbose = False): """ txt = '' if self.cdb is None: - return {'status': False, 'info': "CDB Not supported", 'result': None} + return {'status': False, 'info': "CDB Not supported", 'feature': None} # get fw upgrade features (CMD 0041h) starttime = time.time() @@ -1252,8 +1252,11 @@ def get_module_fw_mgmt_feature(self, verbose = False): writelength = (writelength_raw + 1) * 8 txt += 'Auto page support: %s\n' %autopaging_flag txt += 'Max write length: %d\n' %writelength - rpllen, rpl_chkcode, rpl = self.cdb.get_fw_management_features() - if self.cdb.cdb_chkcode(rpl) == rpl_chkcode: + result = self.cdb.get_fw_management_features() + status = result['status'] + _, rpl_chkcode, rpl = result['rpl'] + + if status == 1 and self.cdb.cdb_chkcode(rpl) == rpl_chkcode: startLPLsize = rpl[2] txt += 'Start payload size %d\n' % startLPLsize maxblocksize = (rpl[4] + 1) * 8 @@ -1277,7 +1280,7 @@ def get_module_fw_mgmt_feature(self, verbose = False): else: txt += 'Reply payload check code error\n' - return {'status': False, 'info': txt, 'result': None} + return {'status': False, 'info': txt, 'feature': None} elapsedtime = time.time()-starttime logger.info('Get module FW upgrade features time: %.2f s\n' %elapsedtime) logger.info(txt) @@ -1297,20 +1300,26 @@ def get_module_fw_info(self): return {'status': False, 'info': "CDB Not supported", 'result': None} # get fw info (CMD 0100h) - rpllen, rpl_chkcode, rpl = self.cdb.get_fw_info() + result = self.cdb.get_fw_info() + status = result['status'] + rpllen, rpl_chkcode, rpl = result['rpl'] + # Interface NACK or timeout if (rpllen is None) or (rpl_chkcode is None): - return {'status': False, 'info': "Interface fail", 'result': 0} # Return result 0 for distinguishing CDB is maybe in busy or failure. + # Return result 0 for distinguishing CDB is maybe in busy or failure. + return {'status': False, 'info': "Interface fail", 'result': 0} # password issue - if self.cdb.cdb_chkcode(rpl) != rpl_chkcode: + if status == 0x46: string = 'Get module FW info: Need to enter password\n' logger.info(string) # Reset password for module using CMIS 4.0 self.cdb.module_enter_password(0) - rpllen, rpl_chkcode, rpl = self.cdb.get_fw_info() + result = self.cdb.get_fw_info() + status = result['status'] + rpllen, rpl_chkcode, rpl = result['rpl'] - if self.cdb.cdb_chkcode(rpl) == rpl_chkcode: + if status == 1 and self.cdb.cdb_chkcode(rpl) == rpl_chkcode: # Regiter 9Fh:136 fwStatus = rpl[0] ImageARunning = (fwStatus & 0x01) # bit 0 - image A is running diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py b/sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py index b21f1f651..96d30db71 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py @@ -155,13 +155,13 @@ def query_cdb_status(self): ''' This QUERY Status command may be used to retrieve the password acceptance status and to perform a test of the CDB interface. - It returns the reply message of this CDB command 0000h. + It returns the status and reply message of this CDB command 0000h. ''' cmd = bytearray(b'\x00\x00\x00\x00\x02\x00\x00\x00\x00\x10') cmd[133-INIT_OFFSET] = self.cdb_chkcode(cmd) self.write_cdb(cmd) status = self.cdb1_chkstatus() - if (status != 0x1): + if status != 0x1: if status > 127: txt = 'Query CDB status: Busy' else: @@ -170,7 +170,8 @@ def query_cdb_status(self): else: txt = 'Query CDB status: Success' logger.info(txt) - return self.read_cdb() + rpl = self.read_cdb() + return {'status': status, 'rpl': rpl} # Enter password def module_enter_password(self, psw = 0x00001011): @@ -199,13 +200,13 @@ def module_enter_password(self, psw = 0x00001011): def get_module_feature(self): ''' This command is used to query which CDB commands are supported. - It returns the reply message of this CDB command 0040h. + It returns the status and reply message of this CDB command 0040h. ''' cmd = bytearray(b'\x00\x40\x00\x00\x00\x00\x00\x00') cmd[133-INIT_OFFSET] = self.cdb_chkcode(cmd) self.write_cdb(cmd) status = self.cdb1_chkstatus() - if (status != 0x1): + if status != 0x1: if status > 127: txt = 'Get module feature status: Busy' else: @@ -214,19 +215,21 @@ def get_module_feature(self): else: txt = 'Get module feature status: Success' logger.info(txt) - return self.read_cdb() + + rpl = self.read_cdb() + return {'status': status, 'rpl': rpl} # Firmware Update Features Supported def get_fw_management_features(self): ''' This command is used to query supported firmware update features - It returns the reply message of this CDB command 0041h. + It returns the status and reply message of this CDB command 0041h. ''' cmd = bytearray(b'\x00\x41\x00\x00\x00\x00\x00\x00') cmd[133-INIT_OFFSET] = self.cdb_chkcode(cmd) self.write_cdb(cmd) status = self.cdb1_chkstatus() - if (status != 0x1): + if status != 0x1: if status > 127: txt = 'Get firmware management feature status: Busy' else: @@ -235,20 +238,22 @@ def get_fw_management_features(self): else: txt = 'Get firmware management feature status: Success' logger.info(txt) - return self.read_cdb() + + rpl = self.read_cdb() + return {'status': status, 'rpl': rpl} # Get FW info def get_fw_info(self): ''' This command returns the firmware versions and firmware default running images that reside in the module - It returns the reply message of this CDB command 0100h. + It returns the status and reply message of this CDB command 0100h. ''' cmd = bytearray(b'\x01\x00\x00\x00\x00\x00\x00\x00') cmd[133-INIT_OFFSET] = self.cdb_chkcode(cmd) self.write_cdb(cmd) status = self.cdb1_chkstatus() - if (status != 0x1): + if status != 0x1: if status > 127: txt = 'Get firmware info status: Busy' else: @@ -257,7 +262,9 @@ def get_fw_info(self): else: txt = 'Get firmware info status: Success' logger.info(txt) - return self.read_cdb() + + rpl = self.read_cdb() + return {'status': status, 'rpl': rpl} # Start FW download def start_fw_download(self, startLPLsize, header, imagesize): diff --git a/tests/sonic_xcvr/test_cmis.py b/tests/sonic_xcvr/test_cmis.py index 466621183..fa58a1b0e 100644 --- a/tests/sonic_xcvr/test_cmis.py +++ b/tests/sonic_xcvr/test_cmis.py @@ -1171,14 +1171,22 @@ def test_get_module_level_flag(self, mock_response, expected): assert result == expected @pytest.mark.parametrize("mock_response, expected", [ - ((128, 1, [0] * 128), {'status': True, 'info': "", 'result': 0}), - ((None, 1, [0] * 128), {'status': False, 'info': "", 'result': 0}), - ((128, None, [0] * 128), {'status': False, 'info': "", 'result': 0}), - ((128, 0, [0] * 128), {'status': False, 'info': "", 'result': None}), - ((128, 1, [67, 3, 2, 2, 3, 183] + [0] * 104), {'status': True, 'info': "", 'result': None}), - ((128, 1, [52, 3, 2, 2, 3, 183] + [0] * 104), {'status': True, 'info': "", 'result': None}), - ((110, 1, [3, 3, 2, 2, 3, 183] + [0] * 104), {'status': True, 'info': "", 'result': None}), - ((110, 1, [48, 3, 2, 2, 3, 183] + [0] * 104), {'status': True, 'info': "", 'result': None}), + ({'status':1, 'rpl':(128, 1, [0] * 128)}, + {'status': True, 'info': "", 'result': 0}), + ({'status':1, 'rpl':(None, 1, [0] * 128)}, + {'status': False, 'info': "", 'result': 0}), + ({'status':1, 'rpl':(128, None, [0] * 128)}, + {'status': False, 'info': "", 'result': 0}), + ({'status':1, 'rpl':(128, 0, [0] * 128)}, + {'status': False, 'info': "", 'result': None}), + ({'status':1, 'rpl':(128, 1, [67, 3, 2, 2, 3, 183] + [0] * 104)}, + {'status': True, 'info': "", 'result': None}), + ({'status':1, 'rpl':(128, 1, [52, 3, 2, 2, 3, 183] + [0] * 104)}, + {'status': True, 'info': "", 'result': None}), + ({'status':1, 'rpl':(110, 1, [3, 3, 2, 2, 3, 183] + [0] * 104)}, + {'status': True, 'info': "", 'result': None}), + ({'status':1, 'rpl':(110, 1, [48, 3, 2, 2, 3, 183] + [0] * 104)}, + {'status': True, 'info': "", 'result': None}), ]) def test_get_module_fw_info(self, mock_response, expected): self.api.cdb = MagicMock() @@ -1191,10 +1199,28 @@ def test_get_module_fw_info(self, mock_response, expected): self.api.cdb.get_fw_info = MagicMock() self.api.cdb.get_fw_info.return_value = mock_response result = self.api.get_module_fw_info() - if result['status'] == False: # Check 'result' when 'status' == False for distinguishing error type. + if result['status'] is False: # Check 'result' when 'status' == False + # for distinguishing error type. assert result['result'] == expected['result'] assert result['status'] == expected['status'] + @pytest.mark.parametrize("mock_response, expected", [ + ({'status':0, 'rpl':(18, 0, [0] * 18)}, {'status': False, 'info': "", 'feature': None}), + ({'status':1, 'rpl':(18, 1, [0] * 18)}, {'status': True, 'info': "", 'feature': None}) + ]) + def test_get_module_fw_mgmt_feature(self, mock_response, expected): + self.api.cdb = MagicMock() + self.api.cdb.cdb_chkcode = MagicMock() + self.api.cdb.cdb_chkcode.return_value = 1 + self.api.xcvr_eeprom.read = MagicMock() + self.api.xcvr_eeprom.read.side_effect = [1, 1] + self.api.cdb.get_fw_management_features = MagicMock() + self.api.cdb.get_fw_management_features.return_value = mock_response + result = self.api.get_module_fw_mgmt_feature() + if result['status'] is False: + assert result['feature'] == expected['feature'] + assert result['status'] == expected['status'] + @pytest.mark.parametrize("input_param, mock_response, expected", [ (1, 1, (True, 'Module FW run: Success\n')), (1, 64, (False, 'Module FW run: Fail\nFW_run_status 64\n')), @@ -1242,6 +1268,7 @@ def test_module_fw_commit(self, mock_response, expected): def test_module_fw_upgrade(self, input_param, mock_response, expected): self.api.get_module_fw_info = MagicMock() self.api.get_module_fw_info.return_value = mock_response[0] + original_function = self.api.get_module_fw_mgmt_feature self.api.get_module_fw_mgmt_feature = MagicMock() self.api.get_module_fw_mgmt_feature.return_value = mock_response[1] self.api.module_fw_download = MagicMock() @@ -1249,6 +1276,7 @@ def test_module_fw_upgrade(self, input_param, mock_response, expected): self.api.module_fw_switch = MagicMock() self.api.module_fw_switch.return_value = mock_response[3] result = self.api.module_fw_upgrade(input_param) + self.api.get_module_fw_mgmt_feature = original_function assert result == expected @pytest.mark.parametrize("mock_response, expected",[ diff --git a/tests/sonic_xcvr/test_cmisCDB.py b/tests/sonic_xcvr/test_cmisCDB.py index 6973e0245..c0ae07b77 100644 --- a/tests/sonic_xcvr/test_cmisCDB.py +++ b/tests/sonic_xcvr/test_cmisCDB.py @@ -78,7 +78,7 @@ def test_query_cdb_status(self, mock_response, expected): self.api.cdb1_chkstatus.return_value = mock_response[0] self.api.read_cdb = MagicMock() self.api.read_cdb.return_value = mock_response[1] - result = self.api.query_cdb_status() + result = self.api.query_cdb_status()['rpl'] assert result == expected @pytest.mark.parametrize("mock_response, expected", [ @@ -102,7 +102,7 @@ def test_get_module_feature(self, mock_response, expected): self.api.cdb1_chkstatus.return_value = mock_response[0] self.api.read_cdb = MagicMock() self.api.read_cdb.return_value = mock_response[1] - result = self.api.get_module_feature() + result = self.api.get_module_feature()['rpl'] assert result == expected @pytest.mark.parametrize("mock_response, expected", [ @@ -115,7 +115,7 @@ def test_get_fw_management_features(self, mock_response, expected): self.api.cdb1_chkstatus.return_value = mock_response[0] self.api.read_cdb = MagicMock() self.api.read_cdb.return_value = mock_response[1] - result = self.api.get_fw_management_features() + result = self.api.get_fw_management_features()['rpl'] assert result == expected @pytest.mark.parametrize("mock_response, expected", [ @@ -128,7 +128,7 @@ def test_get_fw_info(self, mock_response, expected): self.api.cdb1_chkstatus.return_value = mock_response[0] self.api.read_cdb = MagicMock() self.api.read_cdb.return_value = mock_response[1] - result = self.api.get_fw_info() + result = self.api.get_fw_info()['rpl'] assert result == expected @pytest.mark.parametrize("input_param, mock_response, expected", [