Re: [dpdk-dev] [PATCH 4/7] ethdev: change allmulticast callbacks to return status

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

On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> 
> Enabling/disabling of allmulticast 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: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

<...>

> @@ -1227,23 +1227,27 @@ atl_dev_promiscuous_disable(struct rte_eth_dev *dev)
>  	return 0;
>  }
>  
> -static void
> +static int
>  atl_dev_allmulticast_enable(struct rte_eth_dev *dev)
>  {
>  	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  
>  	hw_atl_rpfl2_accept_all_mc_packets_set(hw, true);
> +
> +	return 0;
>  }
>  
> -static void
> +static int
>  atl_dev_allmulticast_disable(struct rte_eth_dev *dev)
>  {
>  	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  
>  	if (dev->data->promiscuous == 1)
> -		return; /* must remain in all_multicast mode */
> +		return 0; /* must remain in all_multicast mode */

What about making this change in the API level, so all dev_ops not need to do
the similar check.
Indeed I can see you are already adding this to API in following patches, but we
can document and guarantee this behavior in API level, so driver developers can
know and rely on this behavior, what do you think?

And this can be a general guideline for all enable/disable APIs. If the feature
already enabled, detect in the API and don't call underlying enable dev_ops,
same for the disable. This saves doing the checks by multiple drivers.

<...>

> @@ -58,10 +58,10 @@ typedef int (*eth_promiscuous_enable_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. */
>  
> -typedef void (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev);
> +typedef int (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev);
>  /**< @internal Enable the receipt of all multicast packets by an Ethernet device. */
>  
> -typedef void (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev);
> +typedef int (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev);
>  /**< @internal Disable the receipt of all multicast packets by an Ethernet device. */

Can you please document what a driver should return? As done in other patches.