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

["Min Hu (Connor)" <humin29@huawei.com>]

在 2020/9/19 16:42, Andrew Rybchenko 写道:
> 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.
  OK,thanks, I will try to fix it as you suggested.

> 
> [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.
  Sorry, maybe I do not get your meaning. By this API 
(rte_eth_fec_get_capability)，we can get FEC capabilityes from device.
we (as user )do not care about the link speed, because this is done in PMD.
  So, how to change the API. Could your make it more clearer?
  Hope for your reply.

> 
>>>> + * @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]
>>> .
>>>
> 
> .