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.

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

Port auto negotiation high level design document #732

Merged
merged 13 commits into from
May 23, 2021

Conversation

Junchao-Mellanox
Copy link
Contributor

@Junchao-Mellanox Junchao-Mellanox commented Jan 12, 2021

Why I did this?

The IEEE 802.3 standard defines a set of Ethernet protocols that are comprised of speed rate and interface type. It allows for configuring multiple values at the same time for port provisioning and advertising to the remote side. However, on SONiC, user can configure the speed of port, and user can configure auto negotiation mode via config DB. Port attributes such as interface type, advertised speeds, advertised interface types are not supported.

The feature in this document is to address the above issues.

Related PRs:

PR title state context
[YANG] Enhance the port yang model with new port fields: adv_speeds, interface_type and adv_interface_types GitHub issue/pull request detail GitHub pull request check contexts
[sonic-swss] Add port auto negotiation support to swss GitHub issue/pull request detail GitHub pull request check contexts
Add test cases for port auto negotiation feature GitHub issue/pull request detail GitHub pull request check contexts
[sonic-utilities] CLI support for port auto negotiation GitHub issue/pull request detail GitHub pull request check contexts

- Allow user to configure auto negotiation via CLI
- Allow user to configure advertised speeds
- Allow user to configure interface type
- Allow user to configure advertised interface types
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add link training into the requirement. enable/disable link training.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need collect information, and this will be added to phase 2.


This command always replace the advertised speeds instead of append. For example, say the current advertised speeds value are "10000,25000", if user configure it with `config interface advertised-speeds Ethernet0 40000,100000`, the advertised speeds value will be changed to "40000,100000".

##### Config interface type

Choose a reason for hiding this comment

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

The interface type should be determined by the media that is plugged in (e.g., copper DAC, optical transceiver, etc.). Hence SONiC (i.e., xcvrd) should be able to dynamically set the interface type based on the detected media using the media_settings.json framework. That framework today is used to set pre-emphasis values, but it can/should be extended to perform other default settings such as interface type, FEC, auto-negotiation, etc.

This config could override what is defined in the platform's media_settings.json file, but care should be taken when the configured interface type differs from what is actually supported by the media (e.g., setting CR (copper type) for an optical transceiver).

Copy link
Contributor Author

@Junchao-Mellanox Junchao-Mellanox Jan 20, 2021

Choose a reason for hiding this comment

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

Hi @jeff-yin, thanks for the comments. As you mentioned, media_settings.json is used to set the pre-emphasis values for ports. And it handles it this way:

  1. When xcvrd daemon start, it reads the media_setting.json and set pre-emphasis values to APPL_DB for each port
  2. When a new cable is inserted, xcvrd will use the value in media_setting.json to set pre-emphasis value to APPL_DB for this port.

As xcvrd just reads the configuration from media_setting.json and set value to APPL_DB, xcvrd does not care what the configuration is. So if you want to use media_setting.json to specify the default value for autoneg, adv_speeds, fec and so on, nothing needs to be changed. However, there is something that user need to understand:

If we have autoneg, adv_speeds or other port configuration both in CONFIG_DB and media_settings.json, the value in media_settings.json will be used if user reboot switch, restart pmon docker or remove/insert the cable.

I suppose it means that, if user choose to configure port autoneg in media_settings.json, he/she should not configure port autoneg via CLI, or what he/she configured in CLI will lose effect after rebooting/restart pmon/re-insert SFP.

IMO, it is always good practice to use CLI/CONFIG_DB to configure port auto negotiation attributes. But user still got the option to use media_setings.json to configure them. I will add this discussion as a separate section to this design document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jeff-yin, the problem i see with setting default based on standards in media_settings.json is it would break the backward compatibility. Currently SONiC is deployed with autoneg and media type not being set. Setting this automatically via media_settings.json would cause interop issues and in many cases the link will not even come up. Hence currently we can go with user configuration approach alone and later have discussion on how to implement based on standards

Interface Auto-Neg Mode Speed Adv Speeds Type Adv Types
----------- --------------- ------- ------------ ------ -----------
Ethernet0 enabled 100G 40G,100G N/A CR4,KR4
Ethernet32 disabled 40G N/A N/A N/A
Copy link
Contributor

Choose a reason for hiding this comment

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

can we show the cable interface type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lguohan , could you please elaborate more about the "cable interface type" here? I guess maybe you are talking about the "type" column in the output of show interfaces status. And its value is usually something like "QSFP28 or later" or "QSFP+ or later". Or is it something in the SFP eeprom data:

Ethernet100: SFP EEPROM detected
        Application Advertisement: N/A
        Connector: No separable connector
        Encoding: 64B66B
        Extended Identifier: Power Class 3(2.5W max), CDR present in Rx Tx
        Extended RateSelect Compliance: QSFP+ Rate Select Version 1
        Identifier: QSFP28 or later
        Length Cable Assembly(m): 3
        Nominal Bit Rate(100Mbs): 255
        Specification compliance:
                Extended Specification compliance: 100G AOC (Active Optical Cable) or 25GAUI C2M AOC
        Vendor Date Code(YYYY-MM-DD Lot): 2019-02-15
        Vendor Name: Mellanox
        Vendor OUI: 00-02-c9
        Vendor PN: MFA1A00-C003
        Vendor Rev: B2
        Vendor SN: MT1907FT08074

Thanks.

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Looks good from my perspective. @lguohan: can you please review the responses to your comments and resolve if OK?

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

as discussed in the sonic yang subgroup, all new hld will need sub pr to add yang model in src/sonic-yang-model, the hld approved is contingent on the approval of yang model pr.

@Junchao-Mellanox Junchao-Mellanox requested a review from lguohan March 3, 2021 01:43
@liat-grozovik
Copy link
Collaborator

as discussed in the sonic yang subgroup, all new hld will need sub pr to add yang model in src/sonic-yang-model, the hld approved is contingent on the approval of yang model pr.

Please check sonic-net/sonic-buildimage#6948

@liat-grozovik
Copy link
Collaborator

@dgsudharsan, @jeff-yin and @praveen-li any further comments or can we approve the HLD?
the PRs should be ready soon for review as well and better to agree on the HLD befor ethat.

@jeff-yin
Copy link

jeff-yin commented Mar 30, 2021

@liat-grozovik @Junchao-Mellanox -- no further comments since we reviewed the doc in the community and that was my only question from the review. Implicit approval from my side (I don't have a review/approve button here).

lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 20, 2021
…interface_type and adv_interface_types (#6948)

Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types
Refer to HLD sonic-net/SONiC#732
@Junchao-Mellanox
Copy link
Contributor Author

Hi @lguohan, the yang model PR has been merged, could you please let me know if there is any further comment on this PR? Thanks.

@liat-grozovik liat-grozovik merged commit f5ea881 into sonic-net:master May 23, 2021
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…interface_type and adv_interface_types (sonic-net#6948)

Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types
Refer to HLD sonic-net/SONiC#732
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.

7 participants