Skip to content

Update Media-based-Port-settings.md to support custom SerDes attributes#2173

Merged
prgeor merged 2 commits intosonic-net:masterfrom
longhuan-cisco:custom_serdes_attrs_hld
Feb 12, 2026
Merged

Update Media-based-Port-settings.md to support custom SerDes attributes#2173
prgeor merged 2 commits intosonic-net:masterfrom
longhuan-cisco:custom_serdes_attrs_hld

Conversation

@longhuan-cisco
Copy link
Copy Markdown
Contributor

@longhuan-cisco longhuan-cisco commented Jan 7, 2026

sonic-swss side change: sonic-net/sonic-swss#3764 (merged)
sonic-platform-daemons side change: sonic-net/sonic-platform-daemons#643 (under review)

Add custom SerDes attributes (SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION) support in media_settings.json

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

Signed-off-by: Longyin Huang <longhuan@cisco.com>
@longhuan-cisco longhuan-cisco force-pushed the custom_serdes_attrs_hld branch from b0f617c to 8cb1b04 Compare January 7, 2026 19:52
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@longhuan-cisco
Copy link
Copy Markdown
Contributor Author

@prgeor Please review, thanks


```
{
"GLOBAL_MEDIA_SETTINGS": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@longhuan-cisco can you introduce "CUSTOM_MEDIA_SETTINGS" so that we can have a separate parser for custom attributes. I know this will mandate to do the port range and per port attribute parsing again for custom attribute but the decision to introduce custom attributes deviates from general sonic requirements so ok to introduce this new section. This will help to contain the changes to this feature only.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@prgeor Thanks for the review and for raising the isolation / blast-radius concern — I agree we should keep the risk to existing platforms/vendors as low as possible, especially for those that don’t define any CUSTOM:XXX attributes in media_settings.json.

I considered the suggestion of introducing a dedicated CUSTOM_MEDIA_SETTINGS section. My main concern is that we’d either need to (a) replicate the existing parsing/matching behaviors (port ranges, DEFAULT_KEY, vendor+PN matching, regex key matching, media_lane_speed_key, etc.), which could become harder to maintain over time, or (b) do a larger refactor to share that logic, which may increase risk more than necessary for this feature.

As a small, low-impact compromise, I propose adding an early-return guard inside handle_custom_serdes_attrs():

  • If the resolved media_dict has no CUSTOM: keys, return immediately (no deepcopy, no conversion logic), so the legacy behavior stays unchanged for platforms/vendors that don’t opt in via CUSTOM: keys.
  • If CUSTOM: keys exist, proceed exactly as before for the opt-in case.

I updated sonic-net/sonic-platform-daemons#643 as the same, please refer:
https://github.com/longhuan-cisco/sonic-platform-daemons/blob/27dfe58d4668d678b2ad850b076194ca17891b76/sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py#L340-L358

def handle_custom_serdes_attrs(media_dict, lane_count, subport_num):
    """
    Handle custom SerDes attributes in the media_dict and convert them to JSON.

    Args:
        media_dict: dictionary containing SerDes settings for all lanes of the port
        lane_count: number of lanes for this subport
        subport_num: subport number (1-based), 0 for non-breakout case

    Returns:
        media_dict: updated media_dict with custom SerDes attributes converted to JSON,
                    or the original media_dict unchanged if no custom attributes present
    """
    # Keep the custom-SerDes-attributes feature fully opt-in.
    # If there are no "CUSTOM:" keys in the resolved media_dict, do not run any of the
    # new conversion logic (and avoid deepcopy). This minimizes blast radius for
    # existing platforms/vendors that don't define custom SerDes attributes.
    if not any(isinstance(k, str) and k.startswith(CUSTOM_SERDES_ATTR_PREFIX) for k in media_dict):
        return media_dict

If you’d still prefer an even stricter opt-in boundary, one option would be to do a one-time scan of media_settings.json (e.g., look for the substring "CUSTOM:") and only enable/invoke the custom handling when that marker exists in the file at all.

Copy link
Copy Markdown
Contributor Author

@longhuan-cisco longhuan-cisco Jan 10, 2026

Choose a reason for hiding this comment

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

@prgeor Let me know if this early-return guard addresses the isolation requirement. I’m happy to adjust further if you’d prefer an even stricter opt-in mechanism (e.g., a one-time scan of media_settings.json to detect "CUSTOM:" before enabling the custom handling), or if you still feel a dedicated CUSTOM_MEDIA_SETTINGS section is the better direction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@longhuan-cisco there is hardly 24 lines to duplicate which is already happening (between the parsing of GLOBAL_MEDIA_SETTINGS_KEY and PORT_MEDIA_SETTINGS_KEY) but you can use oops principal if you desire to minimize the duplication.

Copy link
Copy Markdown
Contributor Author

@longhuan-cisco longhuan-cisco Feb 5, 2026

Choose a reason for hiding this comment

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

@prgeor

Sure, I've implemented the dedicated CUSTOM_MEDIA_SETTINGS section as requested at sonic-net/sonic-platform-daemons#643. Please let me know if it looks ok. Thanks.

One thing to flag from the dedicated-CUSTOM_MEDIA_SETTINGS approach: the dedicated section adds a new selector/lookup path (is_port_selected,get_custom_media_settings_value) and an extra merge path in notify_media_setting. The earlier approach only added a single post‑processing helper (handle_custom_serdes_attrs) and reused the existing GLOBAL/PORT selection path, so it touched less of the parser.

  • old approach (no dedicated section) code reference
    • Adds handle_custom_serdes_attrs()
    • Single hook in notify_media_setting()
    • Reuses existing GLOBAL/PORT lookup path; no new selector logic
  • Current approach (dedicated CUSTOM_MEDIA_SETTINGS) code reference
    • Adds new path: get_custom_media_settings_value()
    • Adds selector parser: is_port_selected()
    • Adds get_custom_serdes_attrs()
    • Extra control flow in notify_media_setting() to merge custom JSON
    • Extracts get_media_settings_for_key() to reuse matching logic

I’m totally good sticking with the dedicated section if separation is the priority, but wanted to make the added maintenance surface and more touching of the existing code explicit now that we have code to compare.

Signed-off-by: Longyin Huang <longhuan@cisco.com>
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@longhuan-cisco longhuan-cisco requested a review from prgeor February 5, 2026 19:37
@prgeor prgeor merged commit a489131 into sonic-net:master Feb 12, 2026
2 checks passed
eddieruan-alibaba pushed a commit that referenced this pull request Mar 27, 2026
…es (#2173)

* Add custom SerDes attributes support in media_settings.json

Signed-off-by: Longyin Huang <longhuan@cisco.com>

* docs: specify dedicated CUSTOM_MEDIA_SETTINGS for custom attrs

Signed-off-by: Longyin Huang <longhuan@cisco.com>

---------

Signed-off-by: Longyin Huang <longhuan@cisco.com>
Signed-off-by: Eddie Ruan <eddie.ruan@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants