Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add high power class support for non-CMIS modules in xcvrd #574

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add high power class support for non-CMIS modules in xcvrd #574

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c5d0c23
Add high power class handling in sff_mgr
longhuan-cisco Nov 12, 2024
20cba36
Fix test_xcvrd
longhuan-cisco Nov 12, 2024
c01b549
Add UT coverage and rename sff_mgr enable flag to enable_sff_mgr_cont…
longhuan-cisco Dec 12, 2024
db2a06f
Keep set_high_power_class raw and put handling logic in sff_mgr
longhuan-cisco Mar 9, 2025
682cef9
Merge branch 'master' into high_power_handling
longhuan-cisco Mar 11, 2025
9a45c17
Combine with merged code from master
longhuan-cisco Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions sonic-xcvrd/tests/test_xcvrd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ class TestXcvrdThreadException(object):
@patch('xcvrd.sff_mgr.PortChangeObserver', MagicMock(side_effect=NotImplementedError))
def test_SffManagerTask_task_run_with_exception(self):
stop_event = threading.Event()
sff_mgr = SffManagerTask(DEFAULT_NAMESPACE, stop_event, MagicMock(), helper_logger)
sff_mgr = SffManagerTask(True, DEFAULT_NAMESPACE, stop_event, MagicMock(), helper_logger)
exception_received = None
trace = None
try:
Expand Down Expand Up @@ -1432,7 +1432,7 @@ def test_DaemonXcvrd_run(self, mock_task_stop1, mock_task_stop2, mock_task_run1,

def test_SffManagerTask_handle_port_change_event(self):
stop_event = threading.Event()
task = SffManagerTask(DEFAULT_NAMESPACE, stop_event, MagicMock(), helper_logger)
task = SffManagerTask(True, DEFAULT_NAMESPACE, stop_event, MagicMock(), helper_logger)

port_change_event = PortChangeEvent('PortConfigDone', -1, 0, PortChangeEvent.PORT_SET)
task.on_port_update_event(port_change_event)
Expand Down Expand Up @@ -1470,7 +1470,7 @@ def test_SffManagerTask_handle_port_change_event(self):
assert len(task.port_dict) == 0

def test_SffManagerTask_get_active_lanes_for_lport(self):
sff_manager_task = SffManagerTask(DEFAULT_NAMESPACE,
sff_manager_task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
MagicMock(),
helper_logger)
Expand Down Expand Up @@ -1524,7 +1524,7 @@ def test_SffManagerTask_get_active_lanes_for_lport(self):
assert result == expected_result

def test_SffManagerTask_get_active_lanes_for_lport_with_invalid_input(self):
sff_manager_task = SffManagerTask(DEFAULT_NAMESPACE,
sff_manager_task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
MagicMock(),
helper_logger)
Expand All @@ -1547,7 +1547,7 @@ def test_SffManagerTask_get_active_lanes_for_lport_with_invalid_input(self):
def test_SffManagerTask_get_host_tx_status(self, mock_get_state_port_tbl):
mock_get_state_port_tbl.return_value.hget.return_value = (True, 'true')

sff_manager_task = SffManagerTask(DEFAULT_NAMESPACE,
sff_manager_task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
MagicMock(),
helper_logger)
Expand All @@ -1561,7 +1561,7 @@ def test_SffManagerTask_get_host_tx_status(self, mock_get_state_port_tbl):
def test_SffManagerTask_get_admin_status(self, mock_get_cfg_port_tbl):
mock_get_cfg_port_tbl.return_value.hget.return_value = (True, 'up')

sff_manager_task = SffManagerTask(DEFAULT_NAMESPACE,
sff_manager_task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
MagicMock(),
helper_logger)
Expand All @@ -1571,6 +1571,48 @@ def test_SffManagerTask_get_admin_status(self, mock_get_cfg_port_tbl):
mock_get_cfg_port_tbl.assert_called_once_with(0)
mock_get_cfg_port_tbl.return_value.hget.assert_called_once_with(lport, 'admin_status')

def test_SffManagerTask_handle_high_power_class(self):
mock_xcvr_api = MagicMock()
mock_xcvr_api.get_power_class = MagicMock(return_value=5)
mock_xcvr_api.set_high_power_class = MagicMock(return_value=True)
lport = 'Ethernet0'

sff_manager_task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
MagicMock(),
helper_logger)

# Test with normal case
sff_manager_task.handle_high_power_class(mock_xcvr_api, lport)
assert mock_xcvr_api.get_power_class.call_count == 1
assert mock_xcvr_api.set_high_power_class.call_count == 1

# Test with get_power_class failed
mock_xcvr_api.get_power_class.return_value = None
sff_manager_task.handle_high_power_class(mock_xcvr_api, lport)
assert mock_xcvr_api.get_power_class.call_count == 2
assert mock_xcvr_api.set_high_power_class.call_count == 1

# Test for no need to set high power class
mock_xcvr_api.get_power_class.return_value = 4
sff_manager_task.handle_high_power_class(mock_xcvr_api, lport)
assert mock_xcvr_api.get_power_class.call_count == 3
assert mock_xcvr_api.set_high_power_class.call_count == 1

# Test for set_high_power_class failed
mock_xcvr_api.get_power_class.return_value = 5
mock_xcvr_api.set_high_power_class.return_value = False
sff_manager_task.handle_high_power_class(mock_xcvr_api, lport)
assert mock_xcvr_api.get_power_class.call_count == 4
assert mock_xcvr_api.set_high_power_class.call_count == 2

# Test for set_high_power_class not supported
mock_xcvr_api.get_power_class.return_value = 5
mock_xcvr_api.set_high_power_class = MagicMock(side_effect=AttributeError("Attribute not found"))
sff_manager_task.handle_high_power_class(mock_xcvr_api, lport)
assert mock_xcvr_api.get_power_class.call_count == 5
assert mock_xcvr_api.set_high_power_class.call_count == 1

@patch('xcvrd.xcvrd.platform_chassis')
@patch('xcvrd.sff_mgr.PortChangeObserver', MagicMock(handle_port_update_event=MagicMock()))
def test_SffManagerTask_task_worker(self, mock_chassis):
Expand All @@ -1579,6 +1621,7 @@ def test_SffManagerTask_task_worker(self, mock_chassis):
mock_xcvr_api.is_flat_memory = MagicMock(return_value=False)
mock_xcvr_api.is_copper = MagicMock(return_value=False)
mock_xcvr_api.get_tx_disable_support = MagicMock(return_value=True)
mock_xcvr_api.get_power_class = MagicMock(return_value=1)

mock_sfp = MagicMock()
mock_sfp.get_presence = MagicMock(return_value=True)
Expand All @@ -1587,7 +1630,7 @@ def test_SffManagerTask_task_worker(self, mock_chassis):
mock_chassis.get_all_sfps = MagicMock(return_value=[mock_sfp])
mock_chassis.get_sfp = MagicMock(return_value=mock_sfp)

task = SffManagerTask(DEFAULT_NAMESPACE,
task = SffManagerTask(True, DEFAULT_NAMESPACE,
threading.Event(),
mock_chassis,
helper_logger)
Expand Down
38 changes: 37 additions & 1 deletion sonic-xcvrd/xcvrd/sff_mgr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,10 @@ class SffManagerTask(threading.Thread):

SFF_LOGGER_PREFIX = "SFF-MAIN: "

def __init__(self, namespaces, main_thread_stop_event, platform_chassis, helper_logger):
def __init__(self, enable_sff_mgr_controlled_tx, namespaces, main_thread_stop_event, platform_chassis, helper_logger):
threading.Thread.__init__(self)
self.name = "SffManagerTask"
self.enable_sff_mgr_controlled_tx = enable_sff_mgr_controlled_tx
self.exc = None
self.task_stopping_event = threading.Event()
self.main_thread_stop_event = main_thread_stop_event
Expand All @@ -96,6 +97,9 @@ def __init__(self, namespaces, main_thread_stop_event, platform_chassis, helper_
self.xcvr_table_helper = XcvrTableHelper(namespaces)
self.namespaces = namespaces

if not self.enable_sff_mgr_controlled_tx:
self.log_notice("enable_sff_mgr_controlled_tx is False, skipping controlling module TX based on host_tx_ready")

def log_notice(self, message):
self.helper_logger.log_notice("{}{}".format(self.SFF_LOGGER_PREFIX, message))

Expand Down Expand Up @@ -284,6 +288,32 @@ def convert_bool_array_to_bit_mask(self, bool_array):
mask += (1 << i if flag else 0)
return mask

def handle_high_power_class(self, xcvr_api, lport):
"""
Enable high power class for the transceiver.

Args:
xcvr_api (XcvrApi): The XcvrApi instance for the transceiver.
lport (str): Logical port name.
"""
try:
power_class = xcvr_api.get_power_class()
if power_class is None:
self.log_error("{}: failed to get power class".format(lport))
return

if power_class < 5:
# No action needed for power class < 5
return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prgeor
Continue the discussion of sonic-net/sonic-platform-common#521 (comment) here at sff_mgr PR, regarding whether setting power_override bit should be included as part of handle_high_power_class:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high power class enabling (added by this PR) and lpmode turning-off are two different independent steps. Not all modules need high power class enabling(only for power_class>=5), while most of the optical modules need lpmode turning-off.

Today, power_override bit setting has already been done by sff_mgr's lpmode turning-off (as part of which, xcvr API set_lpmode() gets called setting power_override bit). There's no need to do extra setting for that bit as part of high power class enabling.

image
Also from functionality POV, would it make more sense to decouple power_override bit from handling of high_power_class? Because, according to the truth table 6-11, once high power class (byte 93 bit 2 or/and bit 3) gets set properly, the module will function in the desired power class after the module is brought out of lpmode by either HW control or SW control.

if xcvr_api.set_high_power_class(power_class, True):
self.log_notice("{}: done enabling high power class".format(lport))
else:
self.log_error("{}: failed to enable high power class".format(lport))

except (AttributeError, NotImplementedError):
pass

def task_worker(self):
'''
The main goal of sff_mgr is to make sure SFF compliant modules are
Expand Down Expand Up @@ -435,6 +465,12 @@ def task_worker(self):
# Skip if these essential routines are not available
continue

if xcvr_inserted:
self.handle_high_power_class(api, lport)

if not self.enable_sff_mgr_controlled_tx:
continue

if active_lanes is None:
active_lanes = self.get_active_lanes_for_lport(lport, subport_idx,
len(lanes_list),
Expand Down
18 changes: 7 additions & 11 deletions sonic-xcvrd/xcvrd/xcvrd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2052,12 +2052,12 @@ def retry_eeprom_reading(self):


class DaemonXcvrd(daemon_base.DaemonBase):
def __init__(self, log_identifier, skip_cmis_mgr=False, enable_sff_mgr=False):
def __init__(self, log_identifier, skip_cmis_mgr=False, enable_sff_mgr_controlled_tx=False):
super(DaemonXcvrd, self).__init__(log_identifier, enable_runtime_log_config=True)
self.stop_event = threading.Event()
self.sfp_error_event = threading.Event()
self.skip_cmis_mgr = skip_cmis_mgr
self.enable_sff_mgr = enable_sff_mgr
self.enable_sff_mgr_controlled_tx = enable_sff_mgr_controlled_tx
self.namespaces = ['']
self.threads = []

Expand Down Expand Up @@ -2222,13 +2222,9 @@ def run(self):
port_mapping_data = self.init()

# Start the SFF manager
sff_manager = None
if self.enable_sff_mgr:
sff_manager = SffManagerTask(self.namespaces, self.stop_event, platform_chassis, helper_logger)
sff_manager.start()
self.threads.append(sff_manager)
else:
self.log_notice("Skipping SFF Task Manager")
sff_manager = SffManagerTask(self.enable_sff_mgr_controlled_tx, self.namespaces, self.stop_event, platform_chassis, helper_logger)
sff_manager.start()
self.threads.append(sff_manager)

# Start the CMIS manager
cmis_manager = None
Expand Down Expand Up @@ -2308,10 +2304,10 @@ def run(self):
def main():
parser = argparse.ArgumentParser()
parser.add_argument('--skip_cmis_mgr', action='store_true')
parser.add_argument('--enable_sff_mgr', action='store_true')
parser.add_argument('--enable_sff_mgr_controlled_tx', action='store_true')

args = parser.parse_args()
xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER, args.skip_cmis_mgr, args.enable_sff_mgr)
xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER, args.skip_cmis_mgr, args.enable_sff_mgr_controlled_tx)
xcvrd.run()


Expand Down
Loading