Re: [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status

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

On 9/9/2019 12:58 PM, Andrew Rybchenko wrote:
> Enabling/disabling of promiscuous mode is not always successful and
> it should be taken into account to be able to handle it properly.
> 
> When correct return status is unclear from driver code, -EAGAIN is used.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

<...>

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b97dd8aa85..f2e6b4c83b 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1892,30 +1892,38 @@ int
>  rte_eth_promiscuous_enable(uint16_t port_id)
>  {
>  	struct rte_eth_dev *dev;
> +	uint8_t old_promiscuous;
> +	int diag;
>  
>  	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->promiscuous_enable, -ENOTSUP);
> -	(*dev->dev_ops->promiscuous_enable)(dev);
> -	dev->data->promiscuous = 1;
> +	old_promiscuous = dev->data->promiscuous;
> +	diag = (*dev->dev_ops->promiscuous_enable)(dev);
> +	dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
>  
> -	return 0;
> +	return eth_err(port_id, diag);
>  }
>  
>  int
>  rte_eth_promiscuous_disable(uint16_t port_id)
>  {
>  	struct rte_eth_dev *dev;
> +	uint8_t old_promiscuous;
> +	int diag;
>  
>  	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->promiscuous_disable, -ENOTSUP);
> +	old_promiscuous = dev->data->promiscuous;
>  	dev->data->promiscuous = 0;
> -	(*dev->dev_ops->promiscuous_disable)(dev);
> +	diag = (*dev->dev_ops->promiscuous_disable)(dev);
> +	if (diag != 0)
> +		dev->data->promiscuous = old_promiscuous;

Not a real issue, but the enable/disable does exact same thing, slightly
different way, it makes double check if logic is different,
can you adapt one of them for both please.

"
diag = foo();
dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous;
"
vs

"
dev->data->promiscuous = 0;
diag = bar();
if (diag != 0)
	dev->data->promiscuous = old_promiscuous;
"

>  
> -	return 0;
> +	return eth_err(port_id, diag);
>  }
>  
>  int
> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
> index 2394b32c83..6322348d17 100644
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -52,10 +52,10 @@ typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev);
>  typedef int (*eth_is_removed_t)(struct rte_eth_dev *dev);
>  /**< @internal Function used to detect an Ethernet device removal. */
>  
> -typedef void (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
> +typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
>  /**< @internal Function used to enable the RX promiscuous mode of an Ethernet device. */
>  
> -typedef void (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
> +typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
>  /**< @internal Function used to disable the RX promiscuous mode of an Ethernet device. */

Should we add a note what is expected return value? This information is not
available anyplace, it may help driver developers.