Link Down Debounce Feature

[JaiOCP]

Link down delay works by establishing a link down debounce/delay timer. In a link state transition from up -> down, the timer starts but the link down notification is suppressed until the timer expires.
Link down events are typically immediately notified for the short reach interconnects as protocols need to converge faster but for long distance cables, there is a ripple effect of link down and debounce feature is very useful without creating a churn in protocol states.

[rck-innovium]

Can we slightly reword as: A non-zero value of this port attribute overrides the switch level debounce timeout.
The reason being that the default value of this port attr is 0 and can cause confusion.

[JaiOCP]

I have kept the wording consistent with the port up debounce. If you feel strongly then probably would be a good idea to change there as well.

I feel the wording is ok: if port is not configured then default of 0 is a no op as an override.

[JaiOCP]

Return empty list or not supported attribute when continuous values are supported.
Look at a new data type if this can be accommodated

[j-bos]

Attribute code comment should describe behavior when the device would allow any value or range of values rather than limited set of specific values.

Though I would encourage implementors to support any value by extending any hardware/firmware capability with a SW timer as well.

[JaiOCP]

Based on Kamil's feedback. I am going to remove these attributes and let Kamil bring in new data types with tool chain changes.

[AnandhiDhanabalan]

The problem discussed for such switch capability query attributes is:
Some vendors may support discrete values and some may support range of values. Today there is no unified data type in SAI that address this requirement.
Defining separate attributes for discrete values and range of values is one option considered, but that would eventually make the switch attributes supported clumsy.

To address this issue I would like suggest a new data type that can:

1. Support only discreet values
2. Support only range values
3. Support both discreet and range values simultaneously.
4. Provide step option to be used in range.
5. Provide single model covering U8, U16, U32 and U64.

@JaiOCP @kcudnik @tjchadaga , Do let me know if this change needs to be raised as a separate PR for better focused discussion in the community?

Proposed Changes

/**
 * @brief Scalar data type tag used in generic capability constructs
 *
 * Describes the wire width of the values being represented.
 * All bounds and step values in a given sai_typed_capability_t instance
 * share the same type.
 */
typedef enum _sai_data_type_t
{
    /** Unsigned 8-bit  — valid value range 0 .. 255 */
    SAI_DATA_TYPE_UINT8,
 
    /** Unsigned 16-bit — valid value range 0 .. 65 535 */
    SAI_DATA_TYPE_UINT16,
 
    /** Unsigned 32-bit — valid value range 0 .. 4 294 967 295 */
    SAI_DATA_TYPE_UINT32,
 
    /** Unsigned 64-bit — valid value range 0 .. 2^64-1 */
    SAI_DATA_TYPE_UINT64,
 
} sai_data_type_t;

/**
 * @brief Inclusive stepped range or discrete value descriptor
 *
 * A list of these entries (see sai_range_step_list_t) forms the complete
 * capability set for a feature.  The full set is the UNION of all entries.
 */
typedef struct _sai_range_step_t
{
    /**
     * @brief Minimum value (inclusive lower bound)
     *
     * For a discrete entry (step == 0) this is the sole valid value.
     * Must fit within the data_type declared by the parent capability.
     */
    uint64_t min;
 
    /**
     * @brief Maximum value (inclusive upper bound)
     *
     * Ignored for discrete entries (step == 0); must equal min in that case.
     * Must fit within the data_type declared by the parent capability.
     */
    uint64_t max;
 
    /**
     * @brief Step granularity between consecutive valid values
     *
     *  0   Discrete entry — only `min` (== `max`) is valid.
     * >0   Range entry    — valid values are min, min+step, … ≤ max.
     *
     * Must fit within the data_type declared by the parent capability.
     */
    uint64_t step;
 
} sai_range_step_t;

/**
 * @brief Variable-length list of sai_range_step_t entries
 *
 * The capability set described by this list is the UNION of all entries.
 * Each entry is independently a range (step > 0) or a discrete value
 * (step == 0, min == max).
 */
typedef struct _sai_range_step_list_t
{
    /**
     * @brief Entry count
     *
     * IN  (before GET) — capacity of the caller-allocated `list` array.
     * OUT (after  GET) — number of entries written by the driver.
     *                    If > IN capacity, driver returns
     *                    SAI_STATUS_BUFFER_OVERFLOW and the caller should
     *                    reallocate and retry.
     */
    uint32_t count;
 
    /**
     * @brief Pointer to caller-allocated array of sai_range_step_t
     *
     * Must point to storage for at least `count` entries before the GET.
     */
    sai_range_step_t *list;
 
} sai_range_step_list_t;

/**
 * @brief Generic scalar capability descriptor
 *
 * Self-describing: data_type declares the wire width of every value in the
 * list; the list encodes all supported values as ranges and/or discrete
 * entries.
 *
 * Caller setup before GET:
 * @code
 *   sai_range_step_t  buf[16];
 *   sai_attribute_t   attr = { .id = SAI_SWITCH_ATTR_LINK_DOWN_DEBOUNCE_TIMEOUT_INTEVALS };
 *
 *   attr.value.typed_capability.data_type    = SAI_DATA_TYPE_UINT32;
 *   attr.value.typed_capability.values.count = 16;
 *   attr.value.typed_capability.values.list  = buf;
 *
 *   get_switch_attribute(switch_id, 1, &attr);
 * @endcode
 */
typedef struct _sai_typed_capability_t
{
    /**
     * @brief Wire width for all values in this capability instance
     *
     * The driver must ensure every min, max, and step value in `values`
     * fits within this type.  NOS consumers may use this to size display
     * formatting or range-check user input.
     *
     * The caller may optionally set this as a hint before the GET; the
     * driver always sets it on return to reflect the actual ASIC width.
     */
    sai_data_type_t data_type;
 
    /**
     * @brief List of capability entries
     *
     * The complete supported value set is the union of all entries.
     * Each entry is a stepped range (step > 0) or a discrete value
     * (step == 0, min == max).
     *
     * Caller must pre-allocate list.list and set list.count (capacity)
     * before issuing the GET.
     */
    sai_range_step_list_t values;
 
} sai_typed_capability_t;

This can be further simplified by always using only uint64_t or uint32_t and sai_data_type_t can be removed.

[kcudnik]

yes, this would make sense to make separate PR

[JaiOCP]

Agree. As mentioned before. I will remove the interval values and bring this changes with a new data type.

[AnandhiDhanabalan]

@JaiOCP @kcudnik , I will come-up with a formal proposal for the new data type. We can take it forward from there.

[Gnanapriya27]

If the DOWN DEBOUNCE mechanism only delays the NOS-level notification, upper-layer applications and protocols may continue to assume the port is operationally UP, but packets could start experiencing drops in the data path.
This introduces a challenge from the application perspective - the port remains up, but traffic loss is occurring. In such scenarios, is there any explicit drop reason, error indication, debug counters or any new operational state visibility exposed to help applications distinguish this condition ?

[JaiOCP]

I checked various implementation documentation across vendors and looks like there debug counters maintain for link flaps. If there is a link flap observed NOS knows that there is a traffic loss during debounce.

I will introduce debug counters for link flags during debounce.