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

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

在 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
>>
>> ---
>>   doc/guides/rel_notes/release_20_11.rst   | 13 +++++
>>   lib/librte_ethdev/rte_ethdev.c           | 50 +++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev.h           | 83 ++++++++++++++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_core.h      | 77 +++++++++++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_version.map |  5 ++
>>   5 files changed, 228 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
>> index cc72609..e4f0587 100644
>> --- a/doc/guides/rel_notes/release_20_11.rst
>> +++ b/doc/guides/rel_notes/release_20_11.rst
>> @@ -55,6 +55,19 @@ New Features
>>        Also, make sure to start the actual text at the margin.
>>        =======================================================
>>   
>> +* **Added the FEC Library, a generic FEC query and config library.**
>> +
>> +  Added the FEC library which provides an API for query FEC capabilities and
>> +  FEC mode from device. Also, API for configuring FEC mode is also provided.
> 
> The patch does not add a library. It adds an API get FEC
> capabilities, get current configuration and allows to set
> a new configuration.
I fixed it in V6，thanks.

> 
>> +
>> +  Added hns3 FEC PMD, for supporting query and config FEC mode.
> 
> If required, it should be later in release notes in accordance
> with defined order.
> 
I fixed it in V6，thanks.

>> +
>> +* **Updated testpmd with a command for FEC.**
>> +
>> +  Added a FEC command to testpmd app,
>> +  ``show port <port_id> fec capabilities`` which queries FEC capabilities device supports.
>> +  ``show port <port_id> fec_mode`` which queries FEC mode from device.
>> +  ``set port <port_id> fec_mode <auto|off|rs|baser>`` which configures FEC mode to device.
> 
> IMHO, it is not much details for release notes.
> As I understand it is assumed that testpmd must be
> updated for any new ethdev feature, so, the information
> about testpmd is not really required.
> 
I fixed it in V6，thanks.
>>   
>>   Removed Items
>>   -------------
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 7858ad5..fde77c1 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -3642,6 +3642,56 @@ rte_eth_led_off(uint16_t port_id)
>>   	return eth_err(port_id, (*dev->dev_ops->dev_led_off)(dev));
>>   }
>>   
>> +int
>> +rte_eth_fec_get_capability(uint16_t port_id, uint32_t *fec_cap)
>> +{
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +	dev = &rte_eth_devices[port_id];
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_get_capability, -ENOTSUP);
>> +	return eth_err(port_id, (*dev->dev_ops->fec_get_capability)(dev,
>> +								fec_cap));
>> +}
>> +
>> +int
>> +rte_eth_fec_get(uint16_t port_id, enum rte_fec_mode *mode)
>> +{
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +	dev = &rte_eth_devices[port_id];
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_get, -ENOTSUP);
>> +	return eth_err(port_id, (*dev->dev_ops->fec_get)(dev, mode));
>> +}
>> +
>> +int
>> +rte_eth_fec_set(uint16_t port_id, enum rte_fec_mode mode)
>> +{
>> +	struct rte_eth_dev *dev;
>> +	uint32_t fec_mode_mask;
>> +	int ret;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> Basically the check is duplicated since get_capabilities
> does it. However, there is no much harm to keep. If so,
> I'd move it below just before port_id usage as an index
> in rte_eth_devices array.
> 
I fixed it in V6，just delete the duplicated check, thanks.
>> +
>> +	ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
>> +	if (ret)
> 
> Please, compare with 0 explicitly as specified in DPDK coding
> style.
> 
I fixed it in V6，thanks.

>> +		return ret;
>> +
>> +	/*
>> +	 * Check whether the configured mode is within the FEC capability.
>> +	 * If not, the configured mode will not be supported.
>> +	 */
>> +	if (!(fec_mode_mask & (1U << (uint32_t)mode))) {
>> +		RTE_ETHDEV_LOG(ERR, "unsupported fec mode=%d\n", mode);
> 
> I'd use "FEC" (in capitals) in log message as well.
> Also I think it would be useful to log port_id.
I fixed it in V6，thanks.

> .
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev = &rte_eth_devices[port_id];
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_set, -ENOTSUP);
>> +	return eth_err(port_id, (*dev->dev_ops->fec_set)(dev, mode));
>> +}
>> +
>>   /*
>>    * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
>>    * an empty spot.
>> 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.
  in other ways, AUTO is flexible mode, which should not be differently 
    treated,the location in last item of enum is ok.

>> +	RTE_ETH_FEC_NUM
> 
> I'm not about about current policy for such items. Is it really
> required? Addition of more FEC modes will be ABI breakage.

  it does not matter even though RTE_ETH_FEC_NUM value is changed when 
adding new element to that enum.
  RTE_ETH_FEC_NUM is used as the MAX vlaue of FEC modes, not one FEC 
mode itself.
  One of the application scenarios is as follows,set "testpmd" as an 
example:
show_fec_capability(uint32_t fec_cap)
{
     uint32_t i;

     if (fec_cap == 0) {
         printf("FEC is not supported\n");
         return;
     }

     printf("FEC capabilities: ");
     for (i = RTE_ETH_FEC_BASER; i < RTE_ETH_FEC_NUM; i++) {
         if (fec_cap & 1U << i)
             printf("%s ", fec_mode_name[i].name);
     }
     printf("\n");
}
>> +};
>> +
>> +/* This indicates FEC capabilities */
>> +#define RTE_ETH_FEC_CAPA_NOFEC  (1U << RTE_ETH_FEC_NOFEC)
>> +#define RTE_ETH_FEC_CAPA_BASER  (1U << RTE_ETH_FEC_BASER)
>> +#define RTE_ETH_FEC_CAPA_RS     (1U << RTE_ETH_FEC_RS)
>> +#define RTE_ETH_FEC_CAPA_AUTO   (1U << RTE_ETH_FEC_AUTO)
>> +
>> +
>>   #define RTE_ETH_ALL RTE_MAX_ETHPORTS
>>   
>>   /* Macros to check for valid port */
>> @@ -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;
	}

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