[CMIS] Return the CDB status value for the caller to check the status… #485

xinyulin · 2024-07-19T08:54:49Z

Description

This PR unifies all CDB API calls to return CDB status, allowing the caller to check the command status returned from the module and perform corresponding actions.

Motivation and Context

Modified the following CDB APIs to return a dictionary with CDB status as the first element and RPL result as the second element.

query_cdb_status()
get_module_feature()
get_fw_management_features()
get_fw_info()

The caller can first check the CDB status to ensure the command was processed successfully, then retrieve the data from the reply payload (RPL).

How Has This Been Tested?

Tested with Cisco8111 and Credo C1 cable

mihirpat1 · 2024-07-25T05:14:36Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

@@ -1282,7 +1282,7 @@ 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()
+        rpllen, rpl_chkcode, rpl = self.cdb.get_fw_info()['rpl']


@xinyulin Can we check for the value of self.cdb.get_fw_info()['status'] and take appropriate action?

@mihirpat1 I have added the check and corresponding actions in the get_module_fw_mgmt_feature() and get_module_fw_info() functions. Could you please review them again? Thanks.

rqx110 · 2024-07-30T02:33:45Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

@@ -1281,7 +1284,7 @@ def get_module_fw_mgmt_feature(self, verbose = False):
        elapsedtime = time.time()-starttime
        logger.info('Get module FW upgrade features time: %.2f s\n' %elapsedtime)
        logger.info(txt)
-        return {'status': True, 'info': txt, 'feature': (startLPLsize, maxblocksize, lplonly_flag, autopaging_flag, writelength)}
+        return {'status': True, 'info': txt, 'result': (startLPLsize, maxblocksize, lplonly_flag, autopaging_flag, writelength)}


change the field from feature to result will break the method caller!
e.g. https://github.com/sonic-net/sonic-utilities/blob/84cb00a4b2d7e8fb2bcab259367836fa11a17d0a/sfputil/main.py#L1528

@rqx110 thanks for your review and comment, I will change it back to feature.

xinyulin · 2024-08-01T05:37:18Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

@@ -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}


@mihirpat1 As suggested by rqx110, I should ensure that get_module_fw_mgmt_feature() returns a dictionary with the key 'feature' instead of 'result'; otherwise, other callers may not function properly. To ensure consistency in the output dictionary keys, I have also corrected the key to 'feature' for error cases.

mihirpat1

@xinyulin Can you please test FW download + run + commit if not already tested with the changes?

mihirpat1 · 2024-08-02T16:20:52Z

sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py

        '''
        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):


@xinyulin Can you please revert this unrelated change to avoid increasing line history (agreed that the version without parenthesis is more pythonic though). Please do the same for similar changes in the diff.

Sure, I have reverted it.

mihirpat1 · 2024-08-02T16:37:45Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

@@ -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}


@xinyulin Can we log an error with the value of status here?

This error will be logged with the following format in the latest commit

else: txt += 'Status or reply payload check code error\n' logger.info(txt) logger.info('Fail to get fw mgnt feature, cdb status: 0x{}\n'.format(status)) return {'status': False, 'info': txt, 'feature': None}

The new commit has been tested with firmware download, run, and commit flows without any errors. Could you please review it to see if anything is missing.

In my opinion, there is no need to log here. The method caller will make the judgment and log the result.
for example:
https://github.com/sonic-net/sonic-utilities/blob/f50587a1ff65bb489231bfe45cec805fb32dbf00/sfputil/main.py#L1526-L1531

@rqx110 I think capturing the status and rpl_chkcode as part of logs will help in providing more datapoints to debug in case of an error.

… and perform the corresponding actions Signed-off-by: xinyu <[email protected]>

…ent feature command Signed-off-by: xinyu <[email protected]>

rqx110 · 2024-08-05T07:52:41Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

-            return {'status': False, 'info': txt, 'result': None}
+            txt += 'Status or reply payload check code error\n'
+            logger.info(txt)
+            logger.info('Fail to get fw mgnt feature, cdb status: 0x{}\n'.format(status))


Suggested change

logger.info('Fail to get fw mgnt feature, cdb status: 0x{}\n'.format(status))

logger.info('Fail to get fw mgmt feature, cdb status: {:#x}\n'.format(status))

@xinyulin Can you please log the value of rpl_chkcode?

Sure. both rpl_chkcode and check result are recorded in the new commit.

rqx110 · 2024-08-06T03:32:23Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

-            return {'status': False, 'info': txt, 'result': None}
+            txt += 'Status or reply payload check code error\n'
+            logger.info(txt)
+            logger.info('Fail to get fw mgnt feature, cdb status: {:#x} rpl_chkcode: {:#x}, {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))


First, mgnt should mgmt
Second, missing ',' after cdb status: {:#x}
Third, the message rpl_chkcode: {:#x}, {:#x} is not very clear. When leaving the code, do you know what the first and second values represent?

@rqx110 Thank you for the review. I have added more details and corrected the incorrect abbreviation.

…er data when an error occurs Signed-off-by: xinyu <[email protected]>

mihirpat1 · 2024-08-12T17:02:59Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

-            return {'status': False, 'info': txt, 'result': None}
+            txt += 'Status or reply payload check code error\n'
+            logger.info(txt)
+            logger.info('Fail to get fw mgmt feature, cdb status: {:#x}, cdb_chkcode: {:#x}, rpl_chkcode: {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))


Suggested change

logger.info('Fail to get fw mgmt feature, cdb status: {:#x}, cdb_chkcode: {:#x}, rpl_chkcode: {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))

logger.error('Fail to get fw mgmt feature, cdb status: {:#x}, cdb_chkcode: {:#x}, rpl_chkcode: {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))

@mihirpat1 Thanks for the suggestion, I have modified it accordingly.

…ature Signed-off-by: xinyu <[email protected]>

prgeor · 2024-09-09T04:44:59Z

@bingwang-ms could you take this to 202405. This is for improved debugging.

mihirpat1 · 2024-09-09T04:51:55Z

@bingwang-ms MSFT ADO - 26804772

sonic-net#485) * [CMIS] Return the CDB status value for the caller to check the status and perform the corresponding actions Signed-off-by: xinyu <[email protected]> * [CMIS] Log an error when failing to retrieve the CDB firmware management feature command Signed-off-by: xinyu <[email protected]> * [CMIS] Log get fw mgmt feature RPL check code result to provide clearer data when an error occurs Signed-off-by: xinyu <[email protected]> * [CMIS] Chagne log level from info to error for fail to get fw mgmt feature Signed-off-by: xinyu <[email protected]> --------- Signed-off-by: xinyu <[email protected]>

mssonicbld · 2024-09-09T15:42:30Z

Cherry-pick PR to 202405: #496

#485) * [CMIS] Return the CDB status value for the caller to check the status and perform the corresponding actions Signed-off-by: xinyu <[email protected]> * [CMIS] Log an error when failing to retrieve the CDB firmware management feature command Signed-off-by: xinyu <[email protected]> * [CMIS] Log get fw mgmt feature RPL check code result to provide clearer data when an error occurs Signed-off-by: xinyu <[email protected]> * [CMIS] Chagne log level from info to error for fail to get fw mgmt feature Signed-off-by: xinyu <[email protected]> --------- Signed-off-by: xinyu <[email protected]>

mihirpat1 reviewed Jul 25, 2024

View reviewed changes

xinyulin force-pushed the cmis_cdb_status branch 8 times, most recently from 04560e3 to a66b757 Compare July 28, 2024 15:11

rqx110 reviewed Jul 30, 2024

View reviewed changes

xinyulin force-pushed the cmis_cdb_status branch 13 times, most recently from 2556588 to ef46a57 Compare July 30, 2024 15:40

xinyulin commented Aug 1, 2024

View reviewed changes

mihirpat1 reviewed Aug 2, 2024

View reviewed changes

xinyulin added 2 commits August 3, 2024 13:14

[CMIS] Return the CDB status value for the caller to check the status…

a3dde34

… and perform the corresponding actions Signed-off-by: xinyu <[email protected]>

[CMIS] Log an error when failing to retrieve the CDB firmware managem…

7b098c7

…ent feature command Signed-off-by: xinyu <[email protected]>

xinyulin force-pushed the cmis_cdb_status branch from ef46a57 to 7b098c7 Compare August 3, 2024 05:29

rqx110 reviewed Aug 5, 2024

View reviewed changes

rqx110 reviewed Aug 6, 2024

View reviewed changes

[CMIS] Log get fw mgmt feature RPL check code result to provide clear…

4d919c8

…er data when an error occurs Signed-off-by: xinyu <[email protected]>

xinyulin force-pushed the cmis_cdb_status branch from 4a1b685 to 4d919c8 Compare August 6, 2024 08:34

mihirpat1 approved these changes Aug 12, 2024

View reviewed changes

[CMIS] Chagne log level from info to error for fail to get fw mgmt fe…

ea7ee0d

…ature Signed-off-by: xinyu <[email protected]>

prgeor approved these changes Sep 9, 2024

View reviewed changes

prgeor merged commit 174bbd4 into sonic-net:master Sep 9, 2024
5 checks passed

prgeor added the Request for 202405 Branch label Sep 9, 2024

bingwang-ms added the Approved for 202405 Branch label Sep 9, 2024

mssonicbld mentioned this pull request Sep 9, 2024

[action] [PR:485] [CMIS] Return the CDB status value for the caller to check the status… (#485) #496

Merged

mssonicbld added the Created PR to 202405 Branch label Sep 9, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Sep 9, 2024

xinyulin deleted the cmis_cdb_status branch September 12, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMIS] Return the CDB status value for the caller to check the status… #485

[CMIS] Return the CDB status value for the caller to check the status… #485

xinyulin commented Jul 19, 2024 •

edited

Loading

mihirpat1 Jul 25, 2024

xinyulin Aug 1, 2024

rqx110 Jul 30, 2024

xinyulin Jul 30, 2024

xinyulin Aug 1, 2024

mihirpat1 left a comment

mihirpat1 Aug 2, 2024

xinyulin Aug 5, 2024

mihirpat1 Aug 2, 2024

xinyulin Aug 5, 2024

rqx110 Aug 5, 2024

mihirpat1 Aug 5, 2024

rqx110 Aug 5, 2024 •

edited

Loading

mihirpat1 Aug 5, 2024

xinyulin Aug 6, 2024

rqx110 Aug 6, 2024

xinyulin Aug 6, 2024

mihirpat1 Aug 12, 2024

xinyulin Aug 13, 2024

prgeor commented Sep 9, 2024

mihirpat1 commented Sep 9, 2024

mssonicbld commented Sep 9, 2024

	logger.info('Fail to get fw mgnt feature, cdb status: 0x{}\n'.format(status))
	logger.info('Fail to get fw mgmt feature, cdb status: {:#x}\n'.format(status))

	logger.info('Fail to get fw mgmt feature, cdb status: {:#x}, cdb_chkcode: {:#x}, rpl_chkcode: {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))
	logger.error('Fail to get fw mgmt feature, cdb status: {:#x}, cdb_chkcode: {:#x}, rpl_chkcode: {:#x}\n'.format(status, self.cdb.cdb_chkcode(rpl), rpl_chkcode))

[CMIS] Return the CDB status value for the caller to check the status… #485

[CMIS] Return the CDB status value for the caller to check the status… #485

Conversation

xinyulin commented Jul 19, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihirpat1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rqx110 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prgeor commented Sep 9, 2024

mihirpat1 commented Sep 9, 2024

mssonicbld commented Sep 9, 2024

xinyulin commented Jul 19, 2024 •

edited

Loading

rqx110 Aug 5, 2024 •

edited

Loading