Skip to content

Add enable/disable option for forwarding pause frame to SAI port#1262

Merged
lguohan merged 15 commits intoopencomputeproject:masterfrom
Pterosaur:macsec_pfc
Aug 19, 2021
Merged

Add enable/disable option for forwarding pause frame to SAI port#1262
lguohan merged 15 commits intoopencomputeproject:masterfrom
Pterosaur:macsec_pfc

Conversation

@Pterosaur
Copy link
Collaborator

@Pterosaur Pterosaur commented Jun 30, 2021

Enhance the flow control APIs including enable/disable forwarding flow control frame.

Signed-off-by: Ze Gan [email protected]

@Pterosaur Pterosaur force-pushed the macsec_pfc branch 2 times, most recently from 4a5230d to 6bae158 Compare June 30, 2021 04:58
@Pterosaur Pterosaur force-pushed the macsec_pfc branch 2 times, most recently from 98cd371 to ec8cbb4 Compare June 30, 2021 11:09
@Pterosaur Pterosaur marked this pull request as ready for review June 30, 2021 11:38
@Pterosaur Pterosaur requested a review from kcudnik June 30, 2021 11:38
@Pterosaur
Copy link
Collaborator Author

This PR need to be discussed with Guohan further, so just draft it again.

@Pterosaur Pterosaur marked this pull request as draft July 1, 2021 00:44
@Pterosaur Pterosaur force-pushed the macsec_pfc branch 3 times, most recently from 4f95e72 to 5d5758f Compare July 5, 2021 08:24
Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur changed the title Add PFC control in MACsec Add the control of pause frame to SAI port Jul 8, 2021
@Pterosaur Pterosaur changed the title Add the control of pause frame to SAI port Add control of pause frame to SAI port Jul 8, 2021
inc/saiport.h Outdated
* @flags CREATE_AND_SET
* @default true
*/
SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_ALL_BITS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need this attribute.

just add a new enum.

SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_DISABLED around line 192.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Pterosaur Pterosaur requested a review from lguohan July 9, 2021 13:53
inc/saiport.h Outdated
typedef enum _sai_port_priority_flow_control_mode_t
{
/** Disable PFC for both rx and tx */
SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_DISABLED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, attributes are added in the middle of the enum will cause to shift other attributes and not be binary backward compatible

Copy link
Collaborator

Choose a reason for hiding this comment

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

move to last

Copy link
Collaborator

Choose a reason for hiding this comment

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

for the attribute, i think we need to explicitly add the enum value as we have discussed @kcudnik it helps to group enum together meanwhile maintain backward compatibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make sense, should I also reorder other attributes to the end of the enum. But I think it will break a kind of readability.

Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur marked this pull request as ready for review July 13, 2021 00:50
inc/saiport.h Outdated
SAI_PORT_ATTR_GLOBAL_FLOW_CONTROL_MODE,

/**
* @brief Forward or not forward the global flow control frame
Copy link
Contributor

@dipankar-ba dipankar-ba Jul 13, 2021

Choose a reason for hiding this comment

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

When not forwarded, I suppose the flow control frame is honored by the Tx MAC. Perhaps better to make that clear.

Copy link
Collaborator

@lguohan lguohan Jul 13, 2021

Choose a reason for hiding this comment

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

you mean the rx flow control frame is owner by the Rx MAC?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the forward means, when it is gearbox, it is forward the flow control frame from one side of the port to the other side of the port, it could be lineside to the systemside or vice versa.

when it is asic, it means the flow control frame will be asic to the packet core from the serdes block and asic can process the flow control frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree - it would be less confusing to simply say the ASIC or the port terminates and honors the flow control frame.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dipankar-ba - do you approve this pr or anything to be revisited?

inc/saiport.h Outdated
SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_TX,

/**
* @brief Forward or not forward the PFC frame
Copy link
Contributor

@dipankar-ba dipankar-ba Jul 13, 2021

Choose a reason for hiding this comment

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

When not forwarded, I suppose the PFC frames are terminated and honored by the switch ASIC. Perhaps it is better to make that clear.

inc/saiport.h Outdated
SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE,

/** Disable PFC for both rx and tx */
SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_DISABLED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem here is for gearbox, they do not control per bitmap. they can do disable for all priority or none. the per bit control is not supported on the gearbox.

@rlhui
Copy link
Collaborator

rlhui commented Jul 15, 2021

Discussed in community meeting that #1256 should be merged in before this one.

Signed-off-by: Ze Gan <[email protected]>
@Pterosaur Pterosaur changed the title Add control of pause frame to SAI port Add switch of forwarding pause frame to SAI port Jul 27, 2021
@Pterosaur Pterosaur changed the title Add switch of forwarding pause frame to SAI port Add enable/disable option for forwarding pause frame to SAI port Jul 27, 2021
@kcudnik
Copy link
Collaborator

kcudnik commented Jul 29, 2021

attributes inserted in the middle and not in the end, maybe dont merge this until we add enum values enforcement

@dipankar-ba
Copy link
Contributor

dipankar-ba commented Jul 30, 2021 via email

Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

Hi Kamil, I tried moving all attributes to the end of enums. If you notice anything else, please let me know.

both attributes are in this PR are still added int the middle of PORT enum, please add them at the end. We will allow explicit enum values to place them in the middle, probably in this week, since that change hangs for over a month, so you can ether wait or move them and the end, and after that, bringthem back to where you want

@Pterosaur Pterosaur requested a review from kcudnik August 17, 2021 15:13
@Pterosaur
Copy link
Collaborator Author

Hi Kamil, I tried moving all attributes to the end of enums. If you notice anything else, please let me know.

both attributes are in this PR are still added int the middle of PORT enum, please add them at the end. We will allow explicit enum values to place them in the middle, probably in this week, since that change hangs for over a month, so you can ether wait or move them and the end, and after that, bringthem back to where you want

Hi Kamil, I moved them to the end of enums. Please review it.

@lguohan lguohan merged commit 82f9e4f into opencomputeproject:master Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants