Add custom serdes attributes for saiport.h#2156
Add custom serdes attributes for saiport.h#2156tjchadaga merged 1 commit intoopencomputeproject:masterfrom
Conversation
longhuan-cisco
force-pushed
the
serdes_attrs_ext
branch
from
a592ded to
9e05d9e
Compare
|
custom attributes are per vendor, and are not meant to be public, please remove those, and each vendor should implement its own custom attributes, please reffer to this PR: #2122 and take a look at its documentation |
|
cc @prgeor |
Thanks for the pointer. But the situation here is: The SAI requestor side, such as SONiC’s orchagent (which is public, platform agnostic and independently built as separate docker), might not be able to include vendor specific headers. If today's enum range |
|
your assumption is false, if OA will require custom headers then it will need to include them as metadata generated from headers is per vendor, so if 1st vendor will have custom attribute let say int, and 2nd vendor the same attribute as oid list, you will get crash right away. if OA want to use custom headers, it need to include them per vendor, and it needs to be compiled per vendor. Adding unnamed custom attributes make no sense, and it's unsolvable. that's why #2122 is introduced |
longhuan-cisco
force-pushed
the
serdes_attrs_ext
branch
from
5ac25c6 to
b8fbbcc
Compare
In this particular case here (sai_port_serdes_attr_t), the values of SAI_PORT_SERDES_ATTR are always of type sai_s32_list_t where the count is number lanes in a port and the list specifies list of values to be applied to each lane, regardless of vendors. (I updated the diff by adding comments indicating the value type as And OA is platform agnostic, it would not be able to include any vendor specific headers. Any particular reasons to not be able to introduce dummy serdes attributes with the same value type? Is this a deviation from SAI design? Add @prgeor for comments also |
|
Custom range is for each vendor to use, and they are not compatoble, for public interface attributes should be added to main range. Lepsze discuss this on todays sai community meeting |
I understood the And, unlike most other types of attributes, a port serdes attribute is always inherently meant to be a list of integers (each integer represents a value for each serdes lane of the specific port), regardless of whether it's common or vendor-specific. That's why I predefined the type of all public custom serdes attributers as On the other hand, in today's SONiC, the serdes attributes are applied to SAI/SDK by OA on a per-platform basis: each platform/vendor defines its own attribute abbreviation->attribute value in
(Not sure if I'm understanding correctly) Or were you suggesting to put all the public custom serdes attributers at main range right after |
kcudnik
left a comment
kcudnik
left a comment
There was a problem hiding this comment.
lets have a live discussion since i dont see point of introducing arbitrary incompatible attributes for all vendors
|
@kcudnik, @longhuan-cisco - do you want to discuss this in this week's SAI community meeting? |
|
sure, let's discuss, could you please send me ([email protected]) the link and include @prgeor also. |
|
my proposal: /**
* @brief custom vendor intersface
*
* vendor have 3 attributes: custom_1, custom_2, custom_3
*
* we carete json/ini/or vendor format:
*
* std::string s1 =
* {
* "custom_1": "value1"
* }
*
*
*
* sai_attribute_t a;
* a.id = SAI_PORT_SERDES_ATTR_CUSTOM_INTERFACE;
* a.value.s32list.count = s1.lenght();
* a.value.s32list.data = s1.data()
* portserdesapi->set_port_serteds_attribute(&a); // this will set custom_1
*
*
*
* std::string s2 =
* {
* "custom_1": "value1" // string, int, bool
* "custom_2": "value2"
* }
*
* foo=bar // old ini file format
*
*
* a.value.s32list.count = s2.lenght();
* a.value.s32list.data = s2.data()
* portserdesapi->set_port_serteds_attribute(&a); // this will set custom_1 and custom_@
*
* @type sai_s32_list_t
* @flags CREATE_AND_SET
* @default internal
*/
SAI_PORT_SERDES_ATTR_CUSTOM_INTERFACE, // etc |
|
Thanks @kcudnik for joining today's discussion and the proposal. BTW. Since it's your idea, just feel free to close mine, in case if you feel more comfortable to raise your own PR, |
im ok for you to drive this change |
|
@prgeor , can you take a look? |
prgeor
left a comment
prgeor
left a comment
There was a problem hiding this comment.
can we hold the PR merge. @longhuan-cisco will sync with you offline.
|
Hi @prgeor, |
|
/azp run |
|
Committed change to:
|
|
I saw CodeQL run got canceled due to below two errors, which doesn't seem to be related to my change Wondering if there's a way to re-run it? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 2156 in repo opencomputeproject/SAI |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Build complained about But none of my commits (including a2eb597) touched Wondering if I shall do a force-push anyway to squash everything into one commit to give a try. |
|
yes, you need to force push squashed |
longhuan-cisco
force-pushed
the
serdes_attrs_ext
branch
from
1dd1356 to
03a9a17
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I did force-push squashed. |
|
you will need to squash your commits and rebase to master and then force pushed, since it seems like you are behind master |
Signed-off-by: Longyin Huang <[email protected]>
longhuan-cisco
force-pushed
the
serdes_attrs_ext
branch
from
03a9a17 to
481768a
Compare
Thanks, rebased to latest master |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Longyin Huang <[email protected]>
Signed-off-by: Longyin Huang <[email protected]>
6 participants
Problem description:
Today, saiport.h and SONiC only supports 10+ NPU SI serdes attributes, but there are below situations for certain vendors/platforms:
Proposal:
(credit to @kcudnik for his idea #2156 (comment))
Add one single attribute, whose value is a string that includes a JSON object with key-value pairs. Each pair's key represents the serdes attribute name, and each pair's value is a list of values for every serdes lane. This allows vendor-specific serdes attributes to be forwarded in a JSON string without the sender needing to know the details. The sender simply passes along the data, vendor-defined rules (e.g. for SONiC, it's media_settings.json on platform side) determine which key-value pairs to include, and the vendor SDK interprets the JSON accordingly.