Re: [dpdk-dev] [PATCH v3 3/8] net/failsafe: support probed sub-devices getting

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Matan,

I'n not fond of the commit title, how about:

[PATCH v3 3/8] net/failsafe: add probed etherdev capture

?

On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> Previous fail-safe code didn't support getting probed sub-devices and
> failed when it tried to probe them.
> 
> Skip fail-safe sub-device probing when it already was probed.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  doc/guides/nics/fail_safe.rst       |  5 ++++
>  drivers/net/failsafe/failsafe_eal.c | 60 ++++++++++++++++++++++++-------------
>  2 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst
> index 5b1b47e..b89e53b 100644
> --- a/doc/guides/nics/fail_safe.rst
> +++ b/doc/guides/nics/fail_safe.rst
> @@ -115,6 +115,11 @@ Fail-safe command line parameters
>    order to take only the last line into account (unlike ``exec()``) at every
>    probe attempt.
>  
> +.. note::
> +
> +   In case of whitelist sub-device probed by EAL, fail-safe PMD will take the device
> +   as is, which means that EAL device options are taken in this case.
> +
>  - **mac** parameter [MAC address]
>  
>    This parameter allows the user to set a default MAC address to the fail-safe
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index 19d26f5..7bc7453 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -36,39 +36,59 @@
>  #include "failsafe_private.h"
>  
>  static int
> +fs_get_port_by_device_name(const char *name, uint16_t *port_id)

The naming convention for the failsafe driver is

      namespace_object_sub-object_action()

With an ordering of objects by their scope (std, rte, failsafe, file).
Also, "get" as an action is not descriptive enough.

static int
fs_ethdev_capture(const char *name, uint16_t *port_id);

> +{
> +	uint16_t pid;
> +	size_t len;
> +
> +	if (name == NULL) {
> +		DEBUG("Null pointer is specified\n");
> +		return -EINVAL;
> +	}
> +	len = strlen(name);
> +	RTE_ETH_FOREACH_DEV(pid) {
> +		if (!strncmp(name, rte_eth_devices[pid].device->name, len)) {
> +			*port_id = pid;
> +			return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> +
> +static int
>  fs_bus_init(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
>  	struct rte_devargs *da;
>  	uint8_t i;
> -	uint16_t j;
> +	uint16_t pid;
>  	int ret;
>  
>  	FOREACH_SUBDEV(sdev, i, dev) {
>  		if (sdev->state != DEV_PARSED)
>  			continue;
>  		da = &sdev->devargs;
> -		ret = rte_eal_hotplug_add(da->bus->name,
> -					  da->name,
> -					  da->args);
> -		if (ret) {
> -			ERROR("sub_device %d probe failed %s%s%s", i,
> -			      rte_errno ? "(" : "",
> -			      rte_errno ? strerror(rte_errno) : "",
> -			      rte_errno ? ")" : "");
> -			continue;
> -		}
> -		RTE_ETH_FOREACH_DEV(j) {
> -			if (strcmp(rte_eth_devices[j].device->name,
> -				    da->name) == 0) {
> -				ETH(sdev) = &rte_eth_devices[j];
> -				break;
> +		if (fs_get_port_by_device_name(da->name, &pid) != 0) {
> +			ret = rte_eal_hotplug_add(da->bus->name,
> +						  da->name,
> +						  da->args);
> +			if (ret) {
> +				ERROR("sub_device %d probe failed %s%s%s", i,
> +				      rte_errno ? "(" : "",
> +				      rte_errno ? strerror(rte_errno) : "",
> +				      rte_errno ? ")" : "");
> +				continue;
>  			}
> +			if (fs_get_port_by_device_name(da->name, &pid) != 0) {
> +				ERROR("sub_device %d init went wrong", i);
> +				return -ENODEV;
> +			}
> +		} else {
> +			/* Take control of device probed by EAL options. */
> +			DEBUG("Taking control of a probed sub device"
> +			      " %d named %s", i, da->name);

In this case, the devargs of the probed device must be copied within the
sub-device definition and removed from the EAL using the proper
rte_devargs API.

Note that there is no rte_devargs copy function. You can use
rte_devargs_parse instead, "parsing" again the original devargs into the
sub-device one. It is necessary for complying with internal rte_devargs
requirements (da->args being malloc-ed, at the moment, but may evolve).

The rte_eal_devargs_parse function is not easy enough to use right now,
you will have to build a devargs string (using snprintf) and submit it.
I proposed a change this release for it but it will not make it for
18.02, that would have simplified your implementation.

-- 
Gaëtan Rivet
6WIND