Re: [dpdk-dev] [PATCH v2] drivers/net/nfb: add timestamp support

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

On 7/2/2019 11:32 AM, Rastislav Cernay wrote:
> From: Rastislav Cernay <cernay@netcope.com>
> 
> This patch adds timestamping support to nfb driver.
> 
> Signed-off-by: Rastislav Cernay <cernay@netcope.com>

<...>

> @@ -142,3 +142,22 @@ Example output:
>       TX threshold registers: pthresh=0 hthresh=0 wthresh=0
>       TX RS bit threshold=0 - TXQ flags=0x0
>     testpmd>
> +
> +Timestamp
> +----------------
> +
> +The PMD supports hardware timestamps of frame receipt on physical network interface. In order to use
> +the timestamps, the hardware timestamping unit must be enabled (follow the documentation of the NFB
> +products) and the device argument `timestamp=1` must be used.
> +
> +.. code-block:: console
> +
> +    $RTE_TARGET/app/testpmd -w b3:00.0,timestamp=1 <other EAL params> -- <testpmd params>
> +
> +When the timestamps are enabled with the *devarg*, a timestamp validity flag is set in the MBUFs
> +containing received frames and timestamp is inserted into the `rte_mbuf` struct.
> +
> +The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
> +(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
> +*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
> +second in the *seconds* portion.

Can you please move this block of document under "Configuration" (because this
is runtime configuration) instead of end of the document. It is good to make two
different sections for config, please check i40e.rst for sample.

> diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
> index 9147b00..805d893 100644
> --- a/drivers/net/nfb/nfb_rx.c
> +++ b/drivers/net/nfb/nfb_rx.c
> @@ -4,9 +4,53 @@
>   * All rights reserved.
>   */
>  
> +#include <rte_kvargs.h>
> +

Shared build fails, can you please add "-lrte_kvargs" to "LDLIBS"

<...>

> +static int
> +nfb_check_timestamp(struct rte_devargs *devargs)
> +{
> +	struct rte_kvargs *kvlist;
> +	const char *timestamp_key = "timestamp";

This is device level argument, I think it is better to make it more visible more
than a string in a function. Common approach is define macros for devargs at the
begging of the file.

> +
> +	if (devargs == NULL)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, NULL);
> +	if (kvlist == NULL)
> +		return 0;

It is common approach to provide valid_keys to parse, for your case there is
single command which is checked below, but in case user provide more (invalid)
arguments and expect them work, it is good to catch it and return error.

And if there will be a valid_keys check it is better to be out of timestamp
checks, in device init phase makes more sense.

> +
> +	if (!rte_kvargs_count(kvlist, timestamp_key)) {
> +		rte_kvargs_free(kvlist);
> +		return 0;
> +	}
> +	/* Timestamps are enabled when there is
> +	 * key-value pair: enable_timestamp=1
> +	 */
> +	if (rte_kvargs_process(kvlist, timestamp_key,
> +		timestamp_check_handler, NULL) < 0) {
> +		rte_kvargs_free(kvlist);
> +		return 0;
> +	}
> +	rte_kvargs_free(kvlist);
> +
> +	return 1;
> +}

Can you please add "RTE_PMD_REGISTER_PARAM_STRING" macro to list the device
arguments?

<...>