SONiC TPID Configuration Support

[gechiang]

This is the HLD proposal for the implementation of TPID configuration in SONiC.

Please also refer to the following SAI Proposal that is depended by this TPID configuration feature:
opencomputeproject/SAI#1089

PR title	state	context
[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes(sonic-net/sonic-buildimage#7630)
[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG(sonic-net/sonic-swss#1747)
[TPID CONFIG] Added TPID configuration CLI suppor(sonic-net/sonic-utilities#1618)

[gechiang]

During the review there was one question raised by Ravi from Innovium in regard to the handling of adding a port to LAG when the port already has some non-default TPID and how should that be handled.
Here are 2 options:

1. After the port passes all current LAG membership add pre-check (if this is present today) we shall allow the add to proceed by first implicitly modify the port's TPID value to 0x8100 right before the Add LAG member continue to execute.
2. Reject the request due to Port TPID is NOT set at "default TPID" value. This forces the user to have to explicitly restore the port's TPID to 8100 first. After the TPID is restored to default, the user can retry the add LAG member again and it should now able to proceed if TPID was the only blocking constraint check.

Option 1 has a potential race condition issue where if the implicit set Port TPID to default arrived later than the Add member to LAG. So the code handling need to be aware of this race condition and only update the port's config DB, App DB to the default TPID value only and NOT program this TPID with SAI through syncd when such race condition is detected.

option 2 is the safest thing to do as it also draws attention to the user that issued the command. Perhaps it was a typo where user specified the wrong interface... OR the error generated by this option can serves as a reminder to the user that the port being requested to add to the port channel may had some other usage intended. It gives the user a chance to ensure it is desirable configuration that the user wants to perform. The draw back of option 2 is that it prevented the user who know what he/she is doing and just want to quickly add the port to port-channel ignoring what its previous TPID maybe...

Given the two options above, I think we should go with option 2 for now... If anyone else has comments over this, please raise a comment.
Thanks!

[rck-innovium]

Agree that option#2 is safer.
And some more things to consider for these checks: SAI allows a platform to specify the capability independently for a physical port and PortChannel. So we need to handle the theoretical possibility where the TPID is supported on the port and not on the PortChannel. And vice-versa.

[gechiang]

@rck-innovium
You are correct that the capability are independent and this HLD does not assume that the HW vendor is able to support both. That is why in the State DB there are 2 separate attributes indicating whether TPID can be configured at PORT level and/or at LAG level.
The requirement for support at LAG level actually placed more responsibility on the HW SAI vendor to be able to apply the TPID on each new joining member as well as reset to default TPID when a port is deleted from the LAG to become an individual port. So my expectation is most vendor should have no problem to support at PORT level at first and later to support LAG but NOT vice versa as it make no sense to be able to support LAG but not able to support at port level...
Rest assure this HLD when implemented will not make any assumptions for this.

[msreddypaluru]

How does an application know what are the supported TPID's by ASIC?  Is there any SAI support to pull these capabilities? Should it be part of switch capabilities in addition to TPID port & port channel capability?

[gechiang]

Please refer to the SAI proposal document for details.
you can find it here:
opencomputeproject/SAI#1089
In summary: The SAI proposal details that the SAI HW vendor when replying to the sai_query_attribute_capability() API that indicate it is capable of supporting the TPID attribute on PORT on or LAG means that it is able to support TPID values of 0x8100, 0x9100, 0x9200, and 0x88A8.
If the SAI HW vendor is not able to support these, then it should indicate it is NOT capable to handle this attribute to the Port/LAG object.
Hope this answered your question.

[msreddypaluru]

sounds good.

[sandeep-kulambi]

Hi Gen-Hwa,

In case user configures a non default TPID value (say 0x9100) in such a case what happens to the control packets in the kernel will they be accepted or dropped (ex STP, ARP, ICMP etc). If they are dropped it will break the functionality of these features right ?

Thanks

[gechiang]

@sandeep-kulambi ,
This HLD was to address the specific use case mentioned in the fanout switch scenario. It also serves as a starting point for support of 802.q tunneling. STP BPDU is untagged so it should not matter which TPID is configured. But if you need to run PVST which needs to be tagged then you will need to re-evaluate why there is a need to change TPID for this port as the kernel does not support non-default TPID values.
Hope this clarify the expectation of this HLD.

[sandeep-kulambi]

Thanks Gen-Hwa !!

[rck-innovium]

What will be the relationship of the port level TPID setting with respect to SAI_SWITCH_ATTR_TPID_OUTER_VLAN and SAI_SWITCH_ATTR_TPID_INNER_VLAN? Does the port level TPID override the switch setting? If so, is it the outer or inner VLAN TPID?

[gechiang]

@rck-innovium
The TPID setting is always applied on the OUTER TPID. It does not change the inner TPID specification.
I believe some SAI vendors do allow setting of inner TPID but this is rarely used by most applications and is usually default to 0x8100.
The TPID setting in the SAI proposal as well as in this HLD is applicable at PORT/LAG level. It is not at Switch level. So having this configured at PORT/LAG level, it overrides what is set at switch level for that specified PORT/LAG.
Hope this help clarify your question.