Add custom serdes attributes for saiport.h

[longhuan-cisco]

Problem description:

Today, saiport.h and SONiC only supports 10+ NPU SI serdes attributes, but there are below situations for certain vendors/platforms:

1. The total number of attributes can be way more than 10+ for certain platforms/vendors (e.g. 40+ attrs), and the number can keep growing even more for future platforms and new ASIC board versions for existing platforms.
2. Attributes can be vendor/platform specific. Different platforms may have totally different sets of attributes (or only very few attributes are common). And SONiC’s orchagent (which is platform agnostic and independently built) should not include a platform-specific extension header file containing those attributes enums.
3. Attributes can be proprietary, which means original names for those attributes cannot be exposed to public, they cannot be upstreamed as it is, unless as dummy/alias attributes.

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.

[kcudnik]

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

[longhuan-cisco]

cc @prgeor

[longhuan-cisco]

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

Thanks for the pointer.
PR #2122 seems to introduce a way to support custom attributes by allowing vendors to put vendor specific headers (containing custom attributes) under SAI/custom folder.

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.
We would need a way to support custom attributes without putting vendor specific headers at SAI/custom folder of the requestor side (e.g. SONiC orchagent). Thus raised the proposal of adding public dummy/placeholder serdes attributes in saiport.h to allow vendors to use them for whatever attributes if needed.

If today's enum range SAI_PORT_SERDES_ATTR_CUSTOM_RANGE_START is reserved only for the custom attributes supposed to be included in SAI/custom/<headers>. Then, I'm wondering if we could add another enum range (e.g. SAI_PORT_SERDES_ATTR_PUBLIC_CUSTOM_RANGE_START = 0x20000000) dedicated for those dummy/placeholder serdes attributes?

[kcudnik]

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]

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

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 sai_s32_list_t for all the new attributes, and making the enum range starting from SAI_PORT_SERDES_ATTR_PUBLIC_CUSTOM_RANGE_START = 0x20000000 to avoid conflicts with #2122)

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

[kcudnik]

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

[longhuan-cisco]

Custom range is for each vendor to use, and they are not compatoble,

I understood the SAI_PORT_SERDES_ATTR_CUSTOM_RANGE_START = 0x10000000 is reserved for each vendor to use. That's why I updated my diff to put all the public custom serdes attributers in a separate range starting from SAI_PORT_SERDES_ATTR_PUBLIC_CUSTOM_RANGE_START = 0x20000000.

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 sai_s32_list_t, where I didn't see compatibility problem.

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 /usr/share/sonic/device/<platform>/media_settings.json, and only the ones defined in this file will get applied.
With these proposed public custom serdes attributers, platform/vendor would be able to easily support large number of newly added proprietary vendor-specific serdes attributes by just updating media_settings.json (plus corresponding SDK support), without having to expose the original real names of the attributes to public, without having to change SONiC and SAI.

for public interface attributes should be added to main range. Lepsze discuss this on todays sai community meeting

(Not sure if I'm understanding correctly) Or were you suggesting to put all the public custom serdes attributers at main range right after SAI_PORT_SERDES_ATTR_RX_PRECODING?

[kcudnik]

this range is reserved for DASH

[longhuan-cisco]

Changed to 0x30000000 to avoid conflict with DASH.

And will discuss more in the meeting.

[kcudnik]

lets have a live discussion since i dont see point of introducing arbitrary incompatible attributes for all vendors

[tjchadaga]

@kcudnik, @longhuan-cisco - do you want to discuss this in this week's SAI community meeting?

[longhuan-cisco]

sure, let's discuss, could you please send me (longhuan@cisco.com) the link and include @prgeor also.

[kcudnik]

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

[longhuan-cisco]

Thanks @kcudnik for joining today's discussion and the proposal.
I updated the diff as what you proposed, and modified the PR description to reflect the latest.

BTW. Since it's your idea, just feel free to close mine, in case if you feel more comfortable to raise your own PR,