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.

Sign up for GitHub

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

Jump to bottom

Extended port operational status with various error and fault status #2060

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Extended port operational status with various error and fault status #2060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions inc/saiport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,57 @@ typedef enum _sai_port_oper_status_t

} sai_port_oper_status_t;

/**
* @brief Attribute data for #SAI_PORT_ATTR_ERROR_STATUS
* Note enum values must be powers of 2 to be used as Bit mask to query multiple errors
*
* @flags free
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be changed to "strict" then it will force validator to chack if all enum are power of 2 to catch user mistakes

*/
typedef enum _sai_port_error_status_t
Copy link
Contributor

Choose a reason for hiding this comment

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

are any other values from sai_port_err_status_t needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeberesford may I know which one you are interested to be added here? I am not sure if I can duplicate the attributes here but I can try...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally mostly interested in local/remote fault, just want to make sure the others are not useful or are covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to Local/Remote fault, SAI_PORT_ERR_STATUS_CRC_RATE would also be good to include 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.

@harshitgulati18 added now

{
/** No errors */
SAI_PORT_ERROR_STATUS_CLEAR = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@prgeor @lguohan I added exception for that, but i would vote that this flag should be removed, since its not real flag, usually flags are used like this:

 if (x & FLAG) == FLAG)

checks if flag is set, in this case if you use CLEAR value then this condidtion will be always true, and CLEAR is not part of actual flags that can be set, it seems like intention is to do this:

if (x == CLEAR) ...

but i think that is a bad design, more natural is first code snipper or eventually it could be done like this:

if (x) ... // some flags are set

do we need a live discussion on this or i can just remove this entry ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik sai_port_error_status_t being used as enum or flag is up to the implementation. I can use enum value like below:-

I can use the enum value like so

if (port_error_status) {
// Found Errors
}
else {
// No Errors
}
`

In another implementation I can look for only interested errors where flag could be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's the same as i wrote in my comment, but there eis no need for SAI_PORT_ERROR_STATUS_CLEAR enum value zero, im proposing to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik why do you say there is no need for SAI_PORT_ERROR_STATUS_CLEAR ? I am planning to use it when there is port UP event and gave the example above .

Copy link
Collaborator

Choose a reason for hiding this comment

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

because that enum is not an actual flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik as I mentioned I am planning to use sai_port_error_status_t as both as an enum value and also also as a flag. I don't want to check each and every flag rather at time I am interested only in few flags in which case I know the enum value and in some case I may be interested only to check one flag. It not just about CLEAR flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

CLEAR is not a flag is definition of absence of any flags,, read my first comment in this thread


/** MAC Local fault asserted */
SAI_PORT_ERROR_STATUS_MAC_LOCAL_FAULT = 1 << 0,

/** MAC Remote fault asserted */
SAI_PORT_ERROR_STATUS_MAC_REMOTE_FAULT = 1 << 1,

/** FEC loss of sync asserted */
SAI_PORT_ERROR_STATUS_FEC_SYNC_LOSS = 1 << 2,

/** FEC loss of alignment marker asserted */
SAI_PORT_ERROR_STATUS_FEC_LOSS_ALIGNMENT_MARKER = 1 << 3,

/** High SER asserted */
SAI_PORT_ERROR_STATUS_HIGH_SER = 1 << 4,

/** High BER asserted */
SAI_PORT_ERROR_STATUS_HIGH_BER = 1 << 5,

/** Rate of data units with CRC errors passed its threshold */
SAI_PORT_ERROR_STATUS_CRC_RATE = 1 << 6,

/** Data Unit CRC Error */
SAI_PORT_ERROR_STATUS_DATA_UNIT_CRC_ERROR = 1 << 7,

/** Data Unit Size Error */
SAI_PORT_ERROR_STATUS_DATA_UNIT_SIZE = 1 << 8,

/** Data Unit Misalignment Error */
SAI_PORT_ERROR_STATUS_DATA_UNIT_MISALIGNMENT_ERROR = 1 << 9,

/** Uncorrectable RS-FEC code word error */
SAI_PORT_ERROR_STATUS_CODE_GROUP_ERROR = 1 << 10,

/** SerDes Signal is out of sync */
SAI_PORT_ERROR_STATUS_SIGNAL_LOCAL_ERROR = 1 << 11,

/** Port is not accepting reachability data units */
SAI_PORT_ERROR_STATUS_NO_RX_REACHABILITY = 1 << 12
} sai_port_error_status_t;
Copy link

Choose a reason for hiding this comment

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

please add SAI_PORT_ERROR_STATUS_HIGH_SER and SAI_PORT_ERROR_STATUS_HIGH_BER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyk-nvidia added


/**
* @brief Defines the operational status of the port
*/
Expand All @@ -89,6 +140,8 @@ typedef struct _sai_port_oper_status_notification_t
/** Port operational status */
sai_port_oper_status_t port_state;

/** Bitmap of various port error or fault status */
sai_port_error_status_t port_error_status;
prgeor marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is breaking change not backward compatible, could cause some issues on sonic when deserialize

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of adding this extra field we should make this RO attribute per port, and if user would be interested in port status, then it could query that attribute in notification using SAI api, then everything would be backward compatible, we can still move this around

} sai_port_oper_status_notification_t;

/**
Expand Down Expand Up @@ -1935,6 +1988,7 @@ typedef enum _sai_port_attr_t
*
* @type sai_port_err_status_list_t
* @flags READ_ONLY
* @deprecated true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeberesford now added here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

*/
SAI_PORT_ATTR_ERR_STATUS_LIST,

Expand Down Expand Up @@ -2537,6 +2591,18 @@ typedef enum _sai_port_attr_t
*/
SAI_PORT_ATTR_UNRELIABLE_LOS,

/**
* @brief Various port error status
*
* Attribute to query the capability of the Switch to report
* various port error and fault status. The attribute can also
* be used to query the current port error and fault status.
*
* @type sai_port_error_status_t
* @flags READ_ONLY
*/
SAI_PORT_ATTR_ERROR_STATUS,

/**
* @brief End of attributes
*/
Expand Down
4 changes: 3 additions & 1 deletion inc/saitypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1373,7 +1373,9 @@ typedef struct _sai_fabric_port_reachability_t
} sai_fabric_port_reachability_t;

/**
* @brief Port error status
* @brief Port error status. This attribute is to be deprecated. Use sai_port_error_status_t instead.
*
* @deprecated true
*/
typedef enum _sai_port_err_status_t
{
Expand Down
1 change: 1 addition & 0 deletions meta/acronyms.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ API - Application Program Interface
ARP - Address Resolution Protocol
ARS - Adaptiver Routing and Switching
ASIC - Application Specific Integrated Circuit
BER - Bit Error Rate
BFD - Bidirectional Forwarding Detection
BFDV6 - Bidirectional Forwarding Detection for IPv6
BGP - Border Gateway Protocol
Expand Down
2 changes: 1 addition & 1 deletion meta/saiserializetest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,7 +1412,7 @@ void test_serialize_notifications()
memset(&data1, 0, sizeof(data1));

res = sai_serialize_port_state_change_notification(buf, 1, &data1);
ret = "{\"count\":1,\"data\":[{\"port_id\":\"oid:0x0\",\"port_state\":\"SAI_PORT_OPER_STATUS_UNKNOWN\"}]}";
ret = "{\"count\":1,\"data\":[{\"port_id\":\"oid:0x0\",\"port_state\":\"SAI_PORT_OPER_STATUS_UNKNOWN\",\"port_error_status\":\"SAI_PORT_ERROR_STATUS_CLEAR\"}]}";
ASSERT_STR_EQ(buf, ret , res);

sai_queue_deadlock_notification_data_t data2;
Expand Down
3 changes: 2 additions & 1 deletion meta/structs.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ sub BuildCommitHistory
# check is performed by sai sanity check

if ($currCount != $histCount and not $structTypeName =~ /^sai_\w+_api_t$/
and $structTypeName ne "sai_switch_health_data_t")
and $structTypeName ne "sai_switch_health_data_t"
and $structTypeName ne "sai_port_oper_status_notification_t")
{
LogError "FATAL: struct $structTypeName member count differs, was $histCount but is $currCount on commit $commit" if $type eq "struct";
}
Expand Down
Loading