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

["Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>]

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Monday, September 9, 2019 8:59 PM
[...]
> Subject: [PATCH v2 04/13] ethdev: change promiscuous callbacks to return
> status
> 
> 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>
> ---
[...]
>  drivers/net/enic/enic.h                   |  2 +-
>  drivers/net/enic/enic_ethdev.c            | 22 +++++++---
>  drivers/net/enic/enic_main.c              |  4 +-
[...]
>  static void
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index 5a92508f00..72b1e7956b 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -305,7 +305,7 @@ int enic_get_link_status(struct enic *enic);
>  int enic_dev_stats_get(struct enic *enic,
>  		       struct rte_eth_stats *r_stats);
>  void enic_dev_stats_clear(struct enic *enic);
> -void enic_add_packet_filter(struct enic *enic);
> +int enic_add_packet_filter(struct enic *enic);
>  int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
>  int enic_del_mac_address(struct enic *enic, int mac_index);
>  unsigned int enic_cleanup_wq(struct enic *enic, struct vnic_wq *wq);
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 90fdeda901..5d48930a9d 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -603,29 +603,39 @@ static const uint32_t
> *enicpmd_dev_supported_ptypes_get(struct rte_eth_dev *dev)
>  	return NULL;
>  }
> 
> -static void enicpmd_dev_promiscuous_enable(struct rte_eth_dev
> *eth_dev)
> +static int enicpmd_dev_promiscuous_enable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
> +	int ret;
> 
>  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> -		return;
> +		return -ENOTSUP;

Should return -E_RTE_SECONDARY to be consistent with other handlers
that check primary/secondary.

> 
>  	ENICPMD_FUNC_TRACE();
> 
>  	enic->promisc = 1;
> -	enic_add_packet_filter(enic);
> +	ret = enic_add_packet_filter(enic);
> +	if (ret != 0)
> +		enic->promisc = 0;
> +
> +	return ret;
>  }
> 
> -static void enicpmd_dev_promiscuous_disable(struct rte_eth_dev
> *eth_dev)
> +static int enicpmd_dev_promiscuous_disable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
> +	int ret;
> 
>  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> -		return;
> +		return -ENOTSUP;

Should return -E_RTE_SECONDARY here too.

>  	ENICPMD_FUNC_TRACE();
>  	enic->promisc = 0;
> -	enic_add_packet_filter(enic);
> +	ret = enic_add_packet_filter(enic);
> +	if (ret != 0)
> +		enic->promisc = 1;
> +
> +	return ret;
>  }
> 
>  static void enicpmd_dev_allmulticast_enable(struct rte_eth_dev *eth_dev)
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 40af3781b3..f4e76a057a 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -1364,10 +1364,10 @@ int enic_set_vlan_strip(struct enic *enic)
>  			       enic->rss_enable);
>  }
> 
> -void enic_add_packet_filter(struct enic *enic)
> +int enic_add_packet_filter(struct enic *enic)
>  {
>  	/* Args -> directed, multicast, broadcast, promisc, allmulti */
> -	vnic_dev_packet_filter(enic->vdev, 1, 1, 1,
> +	return vnic_dev_packet_filter(enic->vdev, 1, 1, 1,
>  		enic->promisc, enic->allmulti);
>  }
> 

A couple minor comments above. Other than those, patch works fine for enic.
Feel free to add my ack on v2..

Acked-by: Hyong Youb Kim <hyonkim@cisco.com>

Thanks.
-Hyong