Re: Clarify FEC capabilities

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 4/7/2023 11:16 AM, Denis Pryazhennikov wrote:
> Hi,
> 

Hi Denis,

cc'ed Connor as author of the feature and Andrew & Ajit as reviewers of it.

> We are currently working on implementing the Forward Error Correction
> (FEC) feature in our driver, but we have encountered some difficulties
> in understanding the interpretation of the semantics of 'fec_capa', a
> bitmask of allowed FEC modes. Specifically, we are unclear about the
> meaning of various bit combinations.
> 
> In rte_ethdev.h, 'fec_capa' is defined as follows:
> 
> ```
>  * @param fec_capa
>  *   A bitmask of allowed FEC modes. If AUTO bit is set, other
>  *   bits specify FEC modes which may be negotiated. If AUTO
>  *   bit is clear, specify FEC modes to be used (only one valid
>  *   mode per speed may be set).
> ```
> 

As far as I can see above comment is from `rte_eth_fec_get()` and
`rte_eth_fec_set()` APIs, but I think comment doesn't make sense for
`rte_eth_fec_get()`, if agreed it can be cleared.


Following comments/requests during review may help to clarify more:
https://inbox.dpdk.org/dev/c2654fcf-8ce9-e480-2f18-e6daea200350@solarflare.com/
https://inbox.dpdk.org/dev/65aadb61-4798-a428-1340-83bedb252ccd@solarflare.com/

Please find answers according my understanding:

> We have a few questions regarding this definition:
> * Is it possible for the AUTO bit to be set without any other bits?

For `rte_eth_fec_set()`, not clear but I think it should be supported to
set AUTO without other bits.
The intention to set other bits is to have ability to exclude unset
bits, if application is OK to set any supported mode, it is easier for
app to just set AUTO bit.

'hns3' on the other hand seems doesn't support auto selecting from given
mode list, and it enforces application to a single mode selection, not
sure how compatible it is to the API spec as it is.

> * Can both RS and BASER bits be set together without the AUTO bit?
> 

Again for `rte_eth_fec_set()`, No, "only one valid mode per speed may be
set".


> Based on our understanding, we have come up with the following
> interpretations for different bit combinations:
> 
> * NOFEC overrides any other bits, and means "disable all FEC";

Ack

> * AUTO on its own implies using cable requirements and link partner
> autoneg with firmware-default preferences for the cable type;

I didn't get this one, please describe more

> * AUTO combined with either RS or BASER signifies using the specified
> FEC type if the cable and link partner support it; otherwise,
> autoneg/firmware-default is used;

"AUTO combined with either RS or BASER", application leaves the decision
to HW/FW/driver to pick RS or BASER.

> * RS or BASER alone means using the specified FEC type if the cable and
> link partner support it and either requests it; otherwise, no FEC is
> employed;

Ack

> * Both RS and BASER, with or without AUTO, indicate using FEC if the
> cable and link partner support it, preferring RS to BASER.
> 

Can't have both RS and BASER without AUTO, and already described above
how to behave with AUTO.

> We would greatly appreciate any assistance in clarifying our
> understanding and ensuring that our interpretations are accurate.
> 
> Thank you in advance for your help.
> 

Can you please make a patch to clarify API documentation as above, and
we can continue discussion based on that patch, it is more practical
that way.

Thanks,
Ferruh