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

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

Hi, Andrew,
	I have fixed it about FEC in V11 according to your suggestion.
	could your please check it out.

	thanks.

在 2020/9/22 20:18, Andrew Rybchenko 写道:
> On 9/22/20 2:06 PM, Min Hu (Connor) wrote:
>>
>>
>> 在 2020/9/22 16:02, Andrew Rybchenko 写道:
>>> On 9/22/20 7:58 AM, Min Hu (Connor) wrote:
>>>>
>>>>
>>>> 在 2020/9/21 21:39, Andrew Rybchenko 写道:
>>>>> On 9/21/20 9:13 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>
>>>>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> [snip]
> 
>>>>>> @@ -3328,6 +3349,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_AUTO
>>>>>> + *   RTE_ETH_FEC_CAPA_BASER
>>>>>> + *   RTE_ETH_FEC_CAPA_RS
>>>>>> + * @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);
>>>>>
>>>>> The API does not allow to report capabilities per link speed:
>>>>> which FEC mode is supported at which link speed?
>>>>>
>>>>> What about something like:
>>>>>
>>>>> struct rte_eth_fec_capa {
>>>>>      uint32_t speed; /**< Link speed (see ETH_SPEED_NUM_*) */
>>>>>      uint32_t capa;  /**< FEC capabilities bitmask (see
>>>>> RTE_FEC_CAPA_*) */
>>>>> };
>>>>>
>>>>> __rte_experimental
>>>>> int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *num, struct
>>>>> rte_eth_fec_capa *speed_capa);
>>>>>
>>>>> where:
>>>>>     - num is in/out with a number of elements in an array
>>>>>     - speed_capa is out only with per-speed capabilities
>>>>>
>>>> There is no need to report capabilities per link speed, because
>>>> relastionship between the link speed and fec mode is fixed. The
>>>> infomations can be referred to in official documents or internet.
>>>
>>> Should an application download documents and search for it? :)
>>
>> OK， I will report capabilities per link speed in V11.
>> By the way,
>>>>> where:
>>>>>      - num is in/out with a number of elements in an array
>>
>> could you describe "num" more detailedly, how to use this value?
> 
> On input, num should specify a number of elements in speed_capa
> array provided by the caller to get FEC capabilities.
> If the number is insufficient to, error should be returned and
> the number should contain required number of elements.
> If sufficient, on output driver should return a number of
> filled in array elements.
> 
>>>
>>>>
>>>> A ethernet port may have various link speed in diffrent situations(for
>>>> example, optical module with different speed is used). But we do not
>>>> care about capabilities per link speed. We only care about FEC capa of
>>>> the ethernet device at a specific moment, because set FEC mode also
>>>> depend on the current FEC capa.
>>>
>>> Capabilities should not be for a specific moment. Capabilities
>>> should be fixed and stable (if transceiver is not replaced).
>>> Capabilities should not depend on a link speed or link status.
>>> Otherwise an application can't use it in a reliable way.
>>>
>>>>
>>>> By the way, we can also get link speed of the device by API
>>>> "rte_eth_link_get" in the same time.
>>>>
>>>> thanks.
>>>>
>>>>>> +
>>>>>> +/**
>>>>>> + * @warning
>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without
>>>>>> prior notice
>>>>>> + *
>>>>>> + * Get current Forward Error Correction(FEC) mode.
>>>>>> + *
>>>>>> + * @param port_id
>>>>>> + *   The port identifier of the Ethernet device.
>>>>>> + * @param mode
>>>>>> + *   returns the FEC mode from the device.
>>>>>> + * @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(uint16_t port_id, enum rte_eth_fec_mode *mode);
>>>>>
>>>>> Please, specify what should be reported if link is down.
>>>>> E.g. if set to RS, but link is down.
>>>>>
>>>>> Does AUTO make sense here?
>>>>>
>>>> OK, I will add the information in the function header comment:
>>>> when link down，None AUTO mode(RS, BASER. NOFEC) keep as it is when link
>>>> up, AUTO mode will change from rs，baser to nofec when quering the mode.
>>>
>>> I'll take a look at the patch, above text is hardly readable.
> 
>> (1). If the current mode of device is one of these modes:
>> RS, BASER. NOFEC.
>> when link up, for example, the mode is RS. when the device is linked
>> down, the mode is always RS.
> 
>> (2). If the current mode of device is AUTO:
>> when the device is linked down, the mode varies in this order:
>> rs->baser->nofec, until Auto-negotiation success (link shoud be
>> up first).
>>
>> But this is defined in our hardware. I think the first feature(1) are
>> common and can be adopted.
> 
> Sorry, I don't understand.
> What abgot if link is down and AUTO is enabled, AUTO is
> returned, otherwise, configured FEC mode is returned.
> If link is up, current FEC mode is returned (i.e. not AUTO,
> either NOFEC, or RS, or BASER).
> 
>>>>>> +
>>>>>> +/**
>>>>>> + * @warning
>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without
>>>>>> prior notice
>>>>>> + *
>>>>>> + * Set Forward Error Correction(FEC) mode.
>>>>>> + *
>>>>>> + * @param port_id
>>>>>> + *   The port identifier of the Ethernet device.
>>>>>> + * @param mode
>>>>>> + *   the FEC mode.
>>>>>> + * @return
>>>>>> + *   - (0) if successful.
>>>>>> + *   - (-EINVAL) if the FEC mode is not valid.
>>>>>> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
>>>>>> + *   - (-EIO) if device is removed.
>>>>>> + *   - (-ENODEV)  if *port_id* invalid.
>>>>>> + */
>>>>>> +__rte_experimental
>>>>>> +int rte_eth_fec_set(uint16_t port_id, enum rte_eth_fec_mode mode);
>>>>>
>>>>> It does not allow to tweak autoneg facilities.
>>>>> E.g. "I know that RS is buggy, so I want to exclude it from
>>>>> auto-negotiation".
>>>>> So, I suggest to change mode to capa bitmask.
>>>>> If AUTO is set, other bits may be set and specify allowed
>>>>> options. E.g. AUTO|RS|BASER will require FEC, i.e. NOFEC is
>>>> The two FEC modes cannot be configured for hardware at the same time,
>>>> including AUTO and other FEC modes. This is determined by Hardware
>>>> itself.
>>>
>>> Which HW? Yours? If so, it does not matter. The patch adds
>>> generic API. My comments are not abstract thoughts. There
>>> are requirements and capabilities behind.
>>
>> yes, it is in my HW. But I think the feature of FEC will exist in other
>> HW: the two FEC modes cannot be configured for hardware at the same time.
>> By the way, if set two FEC mode in our HW, the result will be unknown.
>> I also test X710 nic device, it does not support that feature.
>> I do not support that solutions. thanks.
> 
> I'm not trying to say that two FEC modes could be running
> simultaneously. I'm trying to say that in the future a PHY
> could support more than one FEC mode and autonegotiation
> could make a choice which FEC mode to use.
> E.g.
> NOFEC, FOO and BAR modes supported
> set (AUTO|FOO|BAR) will require either FOO or BAR to be
> negotiated and does not allow NOFEC.
> 
>>>> Thanks.
>>>>
>>>>> not allowed. If just RS, it means that auto-negotiation is
>>>>> disabled and RS must be used.
>>>>> If AUTO is unset, only one bit may be set in capabilities.
>>>>> Since we don't do it per speed, I think it is safe to ignore
>>>>> unsupported mode bits. I.e. do not return error if unsupported
>>>>> capa is requested to together with AUTO, however it could be
>> Why? if the mode is unsupported (not in capa),why we can configure
>> the the mode to the device? Because this is unreaonable. Also,
>> the configure will not be ineffective, and the hardware will return
>> error back to the driver.
> 
> I agree that requested mode should be in capabilities for at
> least some speed, but not required to be applicable to
> currently negotiated and running speed. May be it is obvious.
> I.e. if I set AUTO|NOFEC|RS when capabilities are
> 25G(NOFEC,AUTO,BASER) and 100G(NOFEC,AUTO,RS), it will
> enforce NOFEC for 25G (since BASER is disabled) and allow
> either NOFEC or RS for 100G.
> 
>>>>> a problem if no modes are allowed for negotiated link speed.
>>>>> Thoughts are welcome.
>>>>>
>>>>>> +
>>>>>> +/**
>>>>>>      * Get current status of the Ethernet link flow control for
>>>>>> Ethernet device
>>>>>>      *
>>>>>>      * @param port_id
>>>>>
>>>>> [snip]
>>>>>
>>>>> .
>>>>>
>>>
>>> .
>>>
> 
> .
>