Read/write module EEPROM via SDK provided sys fs #152
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I am reviewing your PR. Will provide feedback in the nearest hour |
fastiuk
suggested changes
Oct 14, 2022
| @@ -23,9 +23,10 @@ | |||
| ############################################################################# | |||
|
|
|||
| try: | |||
| import ctypes | |||
| import glob | |||
There was a problem hiding this comment.
I don't see you are using it in your code. Remove it please
Comment on lines
130
to
131
| SFP_PAGE_SIZE = 256 # page size of page0h | ||
| SFP_UPPER_PAGE_OFFSET = 128 # page size of other pages |
There was a problem hiding this comment.
Optional:
IMO it is easier to read when comments are aligned.
SFP_PAGE_SIZE = 256 # page size of page0h
SFP_UPPER_PAGE_OFFSET = 128 # page size of other pages
| return bytearray(content) | ||
| except (OSError, IOError) as e: | ||
| if log_on_error: | ||
| logger.log_error('Failed to read sfp={} EEPROM page={}, page_offset={}, size={}, offset={}, exception = {}'.format( |
There was a problem hiding this comment.
- Use f-strings instead of format. It is linear and you can see all variables you are printing in place. Change it everywhere in the new code.
- Use logger.exception
There was a problem hiding this comment.
- will fix
- logger is an instance of sonic_py_common.logger, it has no method "exception"
Comment on lines
345
to
348
| logger.log_error('Failed to read sfp={} EEPROM page={}, page_offset={}, size={}, offset={}, errno = {}'.format( | ||
| self.sdk_index, page, page_offset, num_bytes, offset, os.strerror(ctypes.get_errno()))) | ||
| return None | ||
| return bytearray(content) |
There was a problem hiding this comment.
- let's minimize exit points
1.1. Ifif ctypes.get_errno() != 0raise an exception. You will be able to handle them together with(OSError, IOError)
1.2.return bytearray(content)- move it after try-except
In conclusion: you will have two exit points here in regards how many errors you handle. If something bad happen you will exit in exception, otherwise you will return data after try-except block. - Another story is log duplication. Logs are the same. Leave it in the exception only.
Comment on lines
383
to
390
| if ret != num_bytes: | ||
| logger.log_error('Failed to write EEPROM data sfp={} EEPROM page={}, page_offset={}, size={}, offset={}, data = {}, ret = {}'.format( | ||
| self.sdk_index, page, page_offset, num_bytes, offset, ''.join('{:02x}'.format(x) for x in write_buffer), ret)) | ||
| return False | ||
| if ctypes.get_errno() != 0: | ||
| logger.log_error('Failed to write EEPROM data sfp={} EEPROM page={}, page_offset={}, size={}, offset={}, data = {}, errno = {}'.format( | ||
| self.sdk_index, page, page_offset, num_bytes, offset, ''.join('{:02x}'.format(x) for x in write_buffer), os.strerror(ctypes.get_errno()))) | ||
| return False |
There was a problem hiding this comment.
The same here. 4 exit points, very hard to read. A lot of duplications in the comments. Use the same advice I provided for the "read" function.
| return None | ||
| return self._sfp_type_str | ||
|
|
||
| def _is_limited_eeprom(self, page, page_offset, num_bytes): |
There was a problem hiding this comment.
_is_limited_eeprom - limited doesn't explain what exactly is limited.
It is better to name it _is_read_only or _is_writable or _is_write_protected (Used to name corresponding EEPROM chip pin)
| device_address = (offset - SFP_PAGE_SIZE) % SFP_UPPER_PAGE_OFFSET + SFP_UPPER_PAGE_OFFSET | ||
| return self._read_eeprom(offset, num_bytes) | ||
|
|
||
| def _read_eeprom(self, offset, num_bytes, log_on_error=True): |
There was a problem hiding this comment.
I think this is not the best place to put EEPROM API.
This API can be reusable for other EEPROM devices not only for SFP.
I would put this API into another module like eeprom.py and use it here.
There was a problem hiding this comment.
It is only for SFP EEPROM reading. eeprom.py is for system EEPROM.
fastiuk
approved these changes
Oct 14, 2022
Junchao-Mellanox
force-pushed
the
sysfs_eeprom
branch
from
7f26bbe
to
e77aa82
Compare
Junchao-Mellanox
pushed a commit
that referenced
this pull request
Nov 4, 2022
…n] update submodule head (sonic-net#12594) linkmgrd: * 8e9e49b 2022-11-02 | [active-standby][active-active] update link prober stats updating frequency to 30s (#152) (HEAD -> 202205) [Jing Zhang] * 1ad3f18 2022-11-01 | [Active-Active] periodically re-sync soc side admin forwarding state (#151) [Jing Zhang] * cfcdb76 2022-11-01 | [202205] incrementing icmp buffer size (#150) [Jing Zhang] swss: * ac7570a 2022-11-03 | [Dynamic buffer calculation][Mellanox] Enhance the logic to identify buffer pools and profiles (sonic-net#2498) (HEAD -> 202205) [Stephen Sun] swss-common: * 300fc8f 2022-10-31 | [sonic-db-cli] Fix sonic-db-cli crash when database config file not ready issue. (sonic-net#701) (HEAD -> 202205) [Hua Liu] platform-daemon: * cd1608a 2022-10-28 | [ycabled] add support for detach mode in 'active-active' topology (sonic-net#309) (HEAD -> 202205) [vdahiya12] * 2a5f0f4 2022-10-27 | Added filtering logic to send filtered fields from DB event (sonic-net#307) [mihirpat1] platform-common: * b521882 2022-10-30 | CmisApi::get_application_advertisement catch AttributeError as well (sonic-net#316) (HEAD -> 202205) [Stephen Sun] Signed-off-by: Ying Xie <ying.xie@microsoft.com> Signed-off-by: Ying Xie <ying.xie@microsoft.com>
Junchao-Mellanox
pushed a commit
that referenced
this pull request
Nov 11, 2022
[sonic-linkmgrd][master] submodule update b3501d2 Jing Zhang Wed Nov 2 22:22:45 2022 -0700 [active-standby][active-active] update link prober stats updating frequency to 30s (#152) 5d546ec Jing Zhang Tue Nov 1 16:12:17 2022 -0700 [202205] incrementing icmp buffer size (#150) 76b128a Jing Zhang Tue Nov 1 12:06:21 2022 -0700 [Active-Active] periodically re-sync soc side admin forwarding state (#151) sign-off: Jing Zhang zhangjing@microsoft.com
Junchao-Mellanox
pushed a commit
that referenced
this pull request
Jun 5, 2024
…ically (sonic-net#19169) #### Why I did it src/sonic-restapi ``` * 4a564ba - (HEAD -> master, origin/master, origin/HEAD) Fix buster-backports debian mirror issue. (#152) (4 days ago) [Liu Shilong] ``` #### How I did it #### How to verify it #### Description for the changelog
Junchao-Mellanox
pushed a commit
that referenced
this pull request
Jul 5, 2024
…ically (sonic-net#19166) #### Why I did it src/sonic-restapi ``` * 5e39692 - (HEAD -> 202311, origin/202311) Fix buster-backports debian mirror issue. (#152) (4 hours ago) [Liu Shilong] ``` #### How I did it #### How to verify it #### Description for the changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I did it
Currently, implementation:
The problem is that spawning new process of ethtool/mlxreg is very slow. To optimize the performance, this PR will use SDK provided sys fs for EEPROM reading/writing.
How I did it
Replace the logic of
sfp.read_eepromandsfp.write_eepromby SDK sys fs.How to verify it
Manual test
Unit test cases (new test case added)
sonic-mgmt regression
Which release branch to backport (provide reason below if selected)
Description for the changelog
Ensure to add label/tag for the feature raised. example - PR#2174 where, Generic Config and Update feature has been labelled as GCU.
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)