Re: [dpdk-dev] [V5 1/3] ethdev: introduce FEC API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/18/20 12:28 PM, Min Hu (Connor) wrote:
> 
> 
> 在 2020/9/17 17:58, Andrew Rybchenko 写道:
>> On 9/17/20 11:52 AM, Min Hu (Connor) wrote:
>>> This patch adds Forward error correction(FEC) support for ethdev.
>>> Introduce APIs which support query and config FEC information in
>>> hardware.
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Reviewed-by: Chengchang Tang <tangchengchang@huawei.com>
>>> ---
>>> v4->v5:
>>> Modifies FEC capa definitions using macros.
>>> Add RTE_ prefix for public FEC mode enum.
>>> add release notes about FEC for dpdk20_11.
>>>
>>> ---
>>> v2->v3:
>>> add function return value "-ENOTSUP" for API

[snip]

>>> diff --git a/lib/librte_ethdev/rte_ethdev.h 
>>> b/lib/librte_ethdev/rte_ethdev.h
>>> index 70295d7..aa79326 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1511,6 +1511,25 @@ struct rte_eth_dcb_info {
>>>       struct rte_eth_dcb_tc_queue_mapping tc_queue;
>>>   };
>>> +/**
>>> + * This enum indicates the possible (forward error correction)FEC modes
>>> + * of an ethdev port.
>>> + */
>>> +enum rte_fec_mode {
>>
>> enum rte_eth_fec_mode
>> to be consistent with other enums and to have library prefix.
>>
> I fixed it in V6，thanks.
>>> +    RTE_ETH_FEC_NOFEC = 0,      /**< FEC is off */
>>> +    RTE_ETH_FEC_BASER,          /**< FEC using common algorithm */
>>> +    RTE_ETH_FEC_RS,             /**< FEC using RS algorithm */
>>> +    RTE_ETH_FEC_AUTO,           /**< FEC autonegotiation modes */
>>
>> May I suggest to put AUTO just after NOFEC. It would look move
>> logical in the future when more FEC modes are added. It would
>> look strange with AUTO in the middle of the list.
>>
>   hns3 PMD is the first to implement FEC function. The FEC mode order
>   is in accordance with the bit definition in hardware FEC module.

Sorry, it is not a good justification.
It does not matter who is the first and who will be the last.

>   in other ways, AUTO is flexible mode, which should not be differently 
>     treated,the location in last item of enum is ok.

I still disagree. If it is OK for other ethdev maintainer - no problem,
but I'd like to see explicit confirmation.

[snip]

>>> @@ -3328,6 +3347,70 @@ int  rte_eth_led_on(uint16_t port_id);
>>>   int  rte_eth_led_off(uint16_t port_id);
>>>   /**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change, or be removed, without 
>>> prior notice
>>> + *
>>> + * Get Forward Error Correction(FEC) capability.
>>> + *
>>> + * @param port_id
>>> + *   The port identifier of the Ethernet device.
>>> + * @param fec_cap
>>> + *   returns the FEC capability from the device, as follows:
>>> + *   RTE_ETH_FEC_CAPA_NOFEC
>>> + *   RTE_ETH_FEC_CAPA_BASER
>>> + *   RTE_ETH_FEC_CAPA_RS
>>> + *   RTE_ETH_FEC_CAPA_AUTO
>>
>> I'd like to see thoughts about different FEC modes for
>> different link speed. How to report it?
>> (sorry if I missed it before)
> the relation between FEC mode and link speed, we can refer to
> hns3 PMD code, as follows:
> 
>      switch (mac->link_speed) {
>      case ETH_SPEED_NUM_10G:
>      case ETH_SPEED_NUM_40G:
>          mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
>              RTE_ETH_FEC_CAPA_AUTO;
>          break;
>      case ETH_SPEED_NUM_25G:
>      case ETH_SPEED_NUM_50G:
>          mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
>              RTE_ETH_FEC_CAPA_RS | RTE_ETH_FEC_CAPA_AUTO;
>          break;
>      case ETH_SPEED_NUM_100G:
>      case ETH_SPEED_NUM_200G:
>          mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_RS |
>              RTE_ETH_FEC_CAPA_AUTO;
>          break;
>      default:
>          mode = 0;
>          break;
>      }

I'm sorry, but it does not answer my question.
I think we should change the API to report supported FEC modes per
supported link speed.

>>> + * @return
>>> + *   - (0) if successful.
>>> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
>>> + *     that operation.
>>> + *   - (-EIO) if device is removed.
>>> + *   - (-ENODEV)  if *port_id* invalid.
>>> + */
>>> +__rte_experimental
>>> +int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *fec_cap);
>>
>> [snip]
>> .
>>