diff mbox

[dpdk-dev,v4] ethdev: allow returning error on VLAN offload configuration

Message ID 20170901023628.21308-1-dharton@cisco.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

David Harton Sept. 1, 2017, 2:36 a.m. UTC 
  Some devices may not support or fail setting VLAN offload
configuration based on dynamic circurmstances so the
vlan_offload_set_t vector is modified to return an int so
the caller can determine success or not.

rte_eth_dev_set_vlan_offload is updated to return the
value provided by the vector when called along with restoring
the original offload configs on failure.

Existing vlan_offload_set_t vectors are modified to return
an int.  Majority of cases return 0 but a few that actually
can fail now return their failure codes.

Finally, a vlan_offload_set_t vector is added to virtio
to facilitate dynamically turning VLAN strip on or off.

Signed-off-by: David Harton <dharton@cisco.com>
---

v4
* Modified commit message heading
* Moved rel_note comments from ABI to API section
* Renamed locals of rte_eth_dev_set_vlan_offload from 'org*' to 'orig*'

v3
* Fixed a format error.
* Apologies...need to figure out why checkpatches.pl keeps saying
  valid patch when I've got soft tabs.

v2
* Fixed a missed format error.
* Removed vlan offload vector call casts and replaced with checks 
  for return values.

v1
* This is an ABI breakage that has been previously negotiated
  with Thomas and the proposed rel note change is included as well.

 doc/guides/rel_notes/release_17_11.rst |  8 +++++++-
 drivers/net/avp/avp_ethdev.c           | 12 +++++++++---
 drivers/net/bnxt/bnxt_ethdev.c         |  9 ++++++---
 drivers/net/dpaa2/dpaa2_ethdev.c       | 13 ++++++++++---
 drivers/net/e1000/em_ethdev.c          | 12 +++++++++---
 drivers/net/e1000/igb_ethdev.c         | 12 +++++++++---
 drivers/net/enic/enic_ethdev.c         |  8 +++++---
 drivers/net/fm10k/fm10k_ethdev.c       |  3 ++-
 drivers/net/i40e/i40e_ethdev.c         | 11 ++++++++---
 drivers/net/i40e/i40e_ethdev_vf.c      |  9 ++++++---
 drivers/net/ixgbe/ixgbe_ethdev.c       | 25 ++++++++++++++++++-------
 drivers/net/mlx5/mlx5.h                |  2 +-
 drivers/net/mlx5/mlx5_vlan.c           |  3 ++-
 drivers/net/nfp/nfp_net.c              | 13 +++++++------
 drivers/net/qede/qede_ethdev.c         |  9 +++++++--
 drivers/net/virtio/virtio_ethdev.c     | 21 +++++++++++++++++++++
 drivers/net/vmxnet3/vmxnet3_ethdev.c   | 10 +++++++---
 lib/librte_ether/rte_ethdev.c          | 28 ++++++++++++++++++++--------
 lib/librte_ether/rte_ethdev.h          |  2 +-
 19 files changed, 155 insertions(+), 55 deletions(-)

Comments

Hemant Agrawal Sept. 1, 2017, 7:41 a.m. UTC | #1 
On 9/1/2017 8:06 AM, David Harton wrote:
> Some devices may not support or fail setting VLAN offload
> configuration based on dynamic circurmstances so the
> vlan_offload_set_t vector is modified to return an int so
> the caller can determine success or not.
>
> rte_eth_dev_set_vlan_offload is updated to return the
> value provided by the vector when called along with restoring
> the original offload configs on failure.
>
> Existing vlan_offload_set_t vectors are modified to return
> an int.  Majority of cases return 0 but a few that actually
> can fail now return their failure codes.
>
> Finally, a vlan_offload_set_t vector is added to virtio
> to facilitate dynamically turning VLAN strip on or off.
>
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
>
> v4
> * Modified commit message heading
> * Moved rel_note comments from ABI to API section
> * Renamed locals of rte_eth_dev_set_vlan_offload from 'org*' to 'orig*'
>
> v3
> * Fixed a format error.
> * Apologies...need to figure out why checkpatches.pl keeps saying
>   valid patch when I've got soft tabs.
>
> v2
> * Fixed a missed format error.
> * Removed vlan offload vector call casts and replaced with checks
>   for return values.
>
> v1
> * This is an ABI breakage that has been previously negotiated
>   with Thomas and the proposed rel note change is included as well.
>
>  doc/guides/rel_notes/release_17_11.rst |  8 +++++++-
>  drivers/net/avp/avp_ethdev.c           | 12 +++++++++---
>  drivers/net/bnxt/bnxt_ethdev.c         |  9 ++++++---
>  drivers/net/dpaa2/dpaa2_ethdev.c       | 13 ++++++++++---
>  drivers/net/e1000/em_ethdev.c          | 12 +++++++++---
>  drivers/net/e1000/igb_ethdev.c         | 12 +++++++++---
>  drivers/net/enic/enic_ethdev.c         |  8 +++++---
>  drivers/net/fm10k/fm10k_ethdev.c       |  3 ++-
>  drivers/net/i40e/i40e_ethdev.c         | 11 ++++++++---
>  drivers/net/i40e/i40e_ethdev_vf.c      |  9 ++++++---
>  drivers/net/ixgbe/ixgbe_ethdev.c       | 25 ++++++++++++++++++-------
>  drivers/net/mlx5/mlx5.h                |  2 +-
>  drivers/net/mlx5/mlx5_vlan.c           |  3 ++-
>  drivers/net/nfp/nfp_net.c              | 13 +++++++------
>  drivers/net/qede/qede_ethdev.c         |  9 +++++++--
>  drivers/net/virtio/virtio_ethdev.c     | 21 +++++++++++++++++++++
>  drivers/net/vmxnet3/vmxnet3_ethdev.c   | 10 +++++++---
>  lib/librte_ether/rte_ethdev.c          | 28 ++++++++++++++++++++--------
>  lib/librte_ether/rte_ethdev.h          |  2 +-
>  19 files changed, 155 insertions(+), 55 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
> index 170f4f9..22df4fd 100644
> --- a/doc/guides/rel_notes/release_17_11.rst
> +++ b/doc/guides/rel_notes/release_17_11.rst
> @@ -110,6 +110,13 @@ API Changes
>     Also, make sure to start the actual text at the margin.
>     =========================================================
>
> +* **Modified the vlan_offload_set_t function prototype in the ethdev library.**
> +
> +  Changed the function prototype of ``vlan_offload_set_t``.  The return value
> +  has been changed from ``void`` to ``int`` so the caller to knows whether
> +  the backing device supports the operation or if the operation was
> +  successfully performed.
> +
>
>  ABI Changes
>  -----------
> @@ -125,7 +132,6 @@ ABI Changes
>     =========================================================
>
>
> -
>  Shared Library Versions
>  -----------------------
>
> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
> index c746a0e..4011dfa 100644
> --- a/drivers/net/avp/avp_ethdev.c
> +++ b/drivers/net/avp/avp_ethdev.c
> @@ -70,7 +70,7 @@ static int avp_dev_create(struct rte_pci_device *pci_dev,
>  static void avp_dev_close(struct rte_eth_dev *dev);
>  static void avp_dev_info_get(struct rte_eth_dev *dev,
>  			     struct rte_eth_dev_info *dev_info);
> -static void avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static int avp_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete);
>  static void avp_dev_promiscuous_enable(struct rte_eth_dev *dev);
>  static void avp_dev_promiscuous_disable(struct rte_eth_dev *dev);
> @@ -2031,7 +2031,12 @@ struct avp_queue {
>  	mask = (ETH_VLAN_STRIP_MASK |
>  		ETH_VLAN_FILTER_MASK |
>  		ETH_VLAN_EXTEND_MASK);
> -	avp_vlan_offload_set(eth_dev, mask);
> +	ret = avp_vlan_offload_set(eth_dev, mask);
> +	if (ret < 0) {
> +		PMD_DRV_LOG(ERR, "VLAN offload set failed by host, ret=%d\n",
> +			    ret);
> +		goto unlock;
> +	}
>
>  	/* update device config */
>  	memset(&config, 0, sizeof(config));
> @@ -2214,7 +2219,7 @@ struct avp_queue {
>  	}
>  }
>
> -static void
> +static int
>  avp_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
>  {
>  	struct avp_dev *avp = AVP_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
> @@ -2239,6 +2244,7 @@ struct avp_queue {
>  		if (eth_dev->data->dev_conf.rxmode.hw_vlan_extend)
>  			PMD_DRV_LOG(ERR, "VLAN extend offload not supported\n");
>  	}
> +	return 0;
>  }
>
>  static void
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index c9d1122..547bd55 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -144,7 +144,7 @@
>  	ETH_RSS_NONFRAG_IPV6_TCP |	\
>  	ETH_RSS_NONFRAG_IPV6_UDP)
>
> -static void bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask);
> +static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask);
>
>  /***********************/
>
> @@ -522,7 +522,9 @@ static int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
>  		vlan_mask |= ETH_VLAN_FILTER_MASK;
>  	if (eth_dev->data->dev_conf.rxmode.hw_vlan_strip)
>  		vlan_mask |= ETH_VLAN_STRIP_MASK;
> -	bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
> +	rc = bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
> +	if (rc)
> +		goto error;
>
>  	return 0;
>
> @@ -1275,7 +1277,7 @@ static int bnxt_vlan_filter_set_op(struct rte_eth_dev *eth_dev,
>  		return bnxt_del_vlan_filter(bp, vlan_id);
>  }
>
> -static void
> +static int
>  bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask)
>  {
>  	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> @@ -1307,6 +1309,7 @@ static int bnxt_vlan_filter_set_op(struct rte_eth_dev *eth_dev,
>
>  	if (mask & ETH_VLAN_EXTEND_MASK)
>  		RTE_LOG(ERR, PMD, "Extend VLAN Not supported\n");
> +	return 0;
>  }
>
>  static void
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index 429b3a0..3390cb3 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -138,7 +138,7 @@
>  	return ret;
>  }
>
> -static void
> +static int
>  dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct dpaa2_dev_priv *priv = dev->data->dev_private;
> @@ -158,6 +158,7 @@
>  			RTE_LOG(ERR, PMD, "Unable to set vlan filter = %d\n",
>  				ret);
>  	}
> +	return 0;
>  }
>
>  static int
> @@ -643,8 +644,14 @@
>  		return ret;
>  	}
>  	/* VLAN Offload Settings */
> -	if (priv->max_vlan_filters)
> -		dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
> +	if (priv->max_vlan_filters) {
> +		ret = dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
> +		if (ret) {
> +			PMD_INIT_LOG(ERR, "Error to dpaa2_vlan_offload_set:"
> +				     "code = %d\n", ret);
> +			return ret;
> +		}
> +	}
>
>  	return 0;
>  }
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index 3d4ab93..51f49d8 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -99,7 +99,7 @@ static int eth_em_interrupt_action(struct rte_eth_dev *dev,
>
>  static int eth_em_vlan_filter_set(struct rte_eth_dev *dev,
>  		uint16_t vlan_id, int on);
> -static void eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static void em_vlan_hw_filter_enable(struct rte_eth_dev *dev);
>  static void em_vlan_hw_filter_disable(struct rte_eth_dev *dev);
>  static void em_vlan_hw_strip_enable(struct rte_eth_dev *dev);
> @@ -668,7 +668,12 @@ static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
>
>  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
>  			ETH_VLAN_EXTEND_MASK;
> -	eth_em_vlan_offload_set(dev, mask);
> +	ret = eth_em_vlan_offload_set(dev, mask);
> +	if (ret) {
> +		PMD_INIT_LOG(ERR, "Unable to update vlan offload");
> +		em_dev_clear_queues(dev);
> +		return ret;
> +	}
>
>  	/* Set Interrupt Throttling Rate to maximum allowed value. */
>  	E1000_WRITE_REG(hw, E1000_ITR, UINT16_MAX);
> @@ -1447,7 +1452,7 @@ static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
>  	E1000_WRITE_REG(hw, E1000_CTRL, reg);
>  }
>
> -static void
> +static int
>  eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	if(mask & ETH_VLAN_STRIP_MASK){
> @@ -1463,6 +1468,7 @@ static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
>  		else
>  			em_vlan_hw_filter_disable(dev);
>  	}
> +	return 0;
>  }
>
>  /*
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index e4f7a9f..fa15ee9 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -157,7 +157,7 @@ static int eth_igb_vlan_filter_set(struct rte_eth_dev *dev,
>  static int eth_igb_vlan_tpid_set(struct rte_eth_dev *dev,
>  				 enum rte_vlan_type vlan_type,
>  				 uint16_t tpid_id);
> -static void eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>
>  static void igb_vlan_hw_filter_enable(struct rte_eth_dev *dev);
>  static void igb_vlan_hw_filter_disable(struct rte_eth_dev *dev);
> @@ -1400,7 +1400,12 @@ static int eth_igbvf_pci_remove(struct rte_pci_device *pci_dev)
>  	 */
>  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
>  			ETH_VLAN_EXTEND_MASK;
> -	eth_igb_vlan_offload_set(dev, mask);
> +	ret = eth_igb_vlan_offload_set(dev, mask);
> +	if (ret) {
> +		PMD_INIT_LOG(ERR, "Unable to set vlan offload");
> +		igb_dev_clear_queues(dev);
> +		return ret;
> +	}
>
>  	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
>  		/* Enable VLAN filter since VMDq always use VLAN filter */
> @@ -2715,7 +2720,7 @@ static int eth_igbvf_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  						2 * VLAN_TAG_SIZE);
>  }
>
> -static void
> +static int
>  eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	if(mask & ETH_VLAN_STRIP_MASK){
> @@ -2738,6 +2743,7 @@ static int eth_igbvf_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  		else
>  			igb_vlan_hw_extend_disable(dev);
>  	}
> +	return 0;
>  }
>
>
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index da8fec2..fc1eac2 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -347,7 +347,7 @@ static int enicpmd_vlan_filter_set(struct rte_eth_dev *eth_dev,
>  	return err;
>  }
>
> -static void enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
> +static int enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>
> @@ -371,6 +371,8 @@ static void enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
>  		dev_warning(enic,
>  			"Configuration of extended VLAN is not supported\n");
>  	}
> +
> +	return 0;
>  }
>
>  static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
> @@ -392,9 +394,9 @@ static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
>  			eth_dev->data->dev_conf.rxmode.split_hdr_size);
>  	}
>
> -	enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
> +	ret = enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
>  	enic->hw_ip_checksum = eth_dev->data->dev_conf.rxmode.hw_ip_checksum;
> -	return 0;
> +	return ret;
>  }
>
>  /* Start the device.
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index e60d3a3..f4626f7 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1590,7 +1590,7 @@ static int fm10k_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	return 0;
>  }
>
> -static void
> +static int
>  fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	if (mask & ETH_VLAN_STRIP_MASK) {
> @@ -1609,6 +1609,7 @@ static int fm10k_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  		if (!dev->data->dev_conf.rxmode.hw_vlan_filter)
>  			PMD_INIT_LOG(ERR, "VLAN filter is always on in fm10k");
>  	}
> +	return 0;
>  }
>
>  /* Add/Remove a MAC address, and update filters to main VSI */
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 00b6082..d03a44b 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -278,7 +278,7 @@ static int i40e_vlan_filter_set(struct rte_eth_dev *dev,
>  static int i40e_vlan_tpid_set(struct rte_eth_dev *dev,
>  			      enum rte_vlan_type vlan_type,
>  			      uint16_t tpid);
> -static void i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static void i40e_vlan_strip_queue_set(struct rte_eth_dev *dev,
>  				      uint16_t queue,
>  				      int on);
> @@ -3130,7 +3130,7 @@ static int i40e_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	return ret;
>  }
>
> -static void
> +static int
>  i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> @@ -3163,6 +3163,7 @@ static int i40e_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  		else
>  			i40e_vsi_config_double_vlan(vsi, FALSE);
>  	}
> +	return 0;
>  }
>
>  static void
> @@ -5216,7 +5217,11 @@ struct i40e_vsi *
>
>  	/* Apply vlan offload setting */
>  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK;
> -	i40e_vlan_offload_set(dev, mask);
> +	ret = i40e_vlan_offload_set(dev, mask);
> +	if (ret) {
> +		PMD_DRV_LOG(INFO, "Failed to update vlan offload");
> +		return ret;
> +	}
>
>  	/* Apply double-vlan setting, not implemented yet */
>
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index f6d8293..f7fffc2 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -118,7 +118,7 @@ static int i40evf_dev_xstats_get_names(struct rte_eth_dev *dev,
>  static void i40evf_dev_xstats_reset(struct rte_eth_dev *dev);
>  static int i40evf_vlan_filter_set(struct rte_eth_dev *dev,
>  				  uint16_t vlan_id, int on);
> -static void i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static int i40evf_vlan_pvid_set(struct rte_eth_dev *dev, uint16_t pvid,
>  				int on);
>  static void i40evf_dev_close(struct rte_eth_dev *dev);
> @@ -1634,7 +1634,9 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>  	int ret;
>
>  	/* Apply vlan offload setting */
> -	i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
> +	ret = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
> +	if (ret)
> +		return ret;
>
>  	/* Apply pvid setting */
>  	ret = i40evf_vlan_pvid_set(dev, data->dev_conf.txmode.pvid,
> @@ -1642,7 +1644,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>  	return ret;
>  }
>
> -static void
> +static int
>  i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> @@ -1655,6 +1657,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>  		else
>  			i40evf_disable_vlan_strip(dev);
>  	}
> +	return 0;
>  }
>
>  static int
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 22171d8..1ec5aaf 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -218,7 +218,7 @@ static void ixgbe_vlan_hw_strip_bitmap_set(struct rte_eth_dev *dev,
>  		uint16_t queue, bool on);
>  static void ixgbe_vlan_strip_queue_set(struct rte_eth_dev *dev, uint16_t queue,
>  		int on);
> -static void ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static void ixgbe_vlan_hw_strip_enable(struct rte_eth_dev *dev, uint16_t queue);
>  static void ixgbe_vlan_hw_strip_disable(struct rte_eth_dev *dev, uint16_t queue);
>  static void ixgbe_vlan_hw_extend_enable(struct rte_eth_dev *dev);
> @@ -274,7 +274,7 @@ static int ixgbevf_vlan_filter_set(struct rte_eth_dev *dev,
>  		uint16_t vlan_id, int on);
>  static void ixgbevf_vlan_strip_queue_set(struct rte_eth_dev *dev,
>  		uint16_t queue, int on);
> -static void ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on);
>  static int ixgbevf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev,
>  					    uint16_t queue_id);
> @@ -2125,7 +2125,7 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
>  	 */
>  }
>
> -static void
> +static int
>  ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	if (mask & ETH_VLAN_STRIP_MASK) {
> @@ -2148,6 +2148,7 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
>  		else
>  			ixgbe_vlan_hw_extend_disable(dev);
>  	}
> +	return 0;
>  }
>
>  static void
> @@ -2568,9 +2569,13 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
>  		goto error;
>  	}
>
> -    mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
> +	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
>  		ETH_VLAN_EXTEND_MASK;
> -	ixgbe_vlan_offload_set(dev, mask);
> +	err = ixgbe_vlan_offload_set(dev, mask);
> +	if (err) {
> +		PMD_INIT_LOG(ERR, "Unable to set VLAN offload");
> +		goto error;
> +	}
>
>  	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
>  		/* Enable vlan filtering for VMDq */
> @@ -4993,7 +4998,12 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	/* Set HW strip */
>  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
>  		ETH_VLAN_EXTEND_MASK;
> -	ixgbevf_vlan_offload_set(dev, mask);
> +	err = ixgbevf_vlan_offload_set(dev, mask);
> +	if (err) {
> +		PMD_INIT_LOG(ERR, "Unable to set VLAN offload (%d)", err);
> +		ixgbe_dev_clear_queues(dev);
> +		return err;
> +	}
>
>  	ixgbevf_dev_rxtx_start(dev);
>
> @@ -5153,7 +5163,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
>  	ixgbe_vlan_hw_strip_bitmap_set(dev, queue, on);
>  }
>
> -static void
> +static int
>  ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct ixgbe_hw *hw =
> @@ -5168,6 +5178,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
>  		for (i = 0; i < hw->mac.max_rx_queues; i++)
>  			ixgbevf_vlan_strip_queue_set(dev, i, on);
>  	}
> +	return 0;
>  }
>
>  int
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 43c5384..93e71be 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -283,7 +283,7 @@ int mlx5_xstats_get_names(struct rte_eth_dev *,
>  /* mlx5_vlan.c */
>
>  int mlx5_vlan_filter_set(struct rte_eth_dev *, uint16_t, int);
> -void mlx5_vlan_offload_set(struct rte_eth_dev *, int);
> +int mlx5_vlan_offload_set(struct rte_eth_dev *, int);
>  void mlx5_vlan_strip_queue_set(struct rte_eth_dev *, uint16_t, int);
>
>  /* mlx5_trigger.c */
> diff --git a/drivers/net/mlx5/mlx5_vlan.c b/drivers/net/mlx5/mlx5_vlan.c
> index 1b0fa40..7215909 100644
> --- a/drivers/net/mlx5/mlx5_vlan.c
> +++ b/drivers/net/mlx5/mlx5_vlan.c
> @@ -210,7 +210,7 @@
>   * @param mask
>   *   VLAN offload bit mask.
>   */
> -void
> +int
>  mlx5_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct priv *priv = dev->data->dev_private;
> @@ -230,4 +230,5 @@
>  			priv_vlan_strip_queue_set(priv, i, hw_vlan_strip);
>  		priv_unlock(priv);
>  	}
> +	return 0;
>  }
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index 92b03c4..6473edc 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -2149,11 +2149,12 @@ uint32_t nfp_net_txq_full(struct nfp_net_txq *txq)
>  	return i;
>  }
>
> -static void
> +static int
>  nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	uint32_t new_ctrl, update;
>  	struct nfp_net_hw *hw;
> +	int ret;
>
>  	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	new_ctrl = 0;
> @@ -2174,14 +2175,14 @@ uint32_t nfp_net_txq_full(struct nfp_net_txq *txq)
>  		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
>
>  	if (new_ctrl == 0)
> -		return;
> +		return 0;
>
>  	update = NFP_NET_CFG_UPDATE_GEN;
>
> -	if (nfp_net_reconfig(hw, new_ctrl, update) < 0)
> -		return;
> -
> -	hw->ctrl = new_ctrl;
> +	ret = nfp_net_reconfig(hw, new_ctrl, update);
> +	if (!ret)
> +		hw->ctrl = new_ctrl;
> +	return ret;
>  }
>
>  /* Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device */
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index 0e05989..644f69d 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -975,7 +975,7 @@ static int qede_vlan_filter_set(struct rte_eth_dev *eth_dev,
>  	return rc;
>  }
>
> -static void qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
> +static int qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
>  {
>  	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
>  	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
> @@ -1013,6 +1013,8 @@ static void qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
>
>  	DP_INFO(edev, "vlan offload mask %d vlan-strip %d vlan-filter %d\n",
>  		mask, rxmode->hw_vlan_strip, rxmode->hw_vlan_filter);
> +
> +	return 0;
>  }
>
>  static void qede_prandom_bytes(uint32_t *buff)
> @@ -1157,6 +1159,7 @@ static int qede_dev_configure(struct rte_eth_dev *eth_dev)
>  	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
>  	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
>  	struct rte_eth_rxmode *rxmode = &eth_dev->data->dev_conf.rxmode;
> +	int ret;
>
>  	PMD_INIT_FUNC_TRACE(edev);
>
> @@ -1237,9 +1240,11 @@ static int qede_dev_configure(struct rte_eth_dev *eth_dev)
>  	qdev->enable_lro = rxmode->enable_lro;
>
>  	/* Enable VLAN offloads by default */
> -	qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
> +	ret = qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
>  			ETH_VLAN_FILTER_MASK |
>  			ETH_VLAN_EXTEND_MASK);
> +	if (ret)
> +		return ret;
>
>  	DP_INFO(edev, "Device configured with RSS=%d TSS=%d\n",
>  			QEDE_RSS_COUNT(qdev), QEDE_TSS_COUNT(qdev));
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e320811..72b4248 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -72,6 +72,7 @@ static void virtio_dev_info_get(struct rte_eth_dev *dev,
>  				struct rte_eth_dev_info *dev_info);
>  static int virtio_dev_link_update(struct rte_eth_dev *dev,
>  	int wait_to_complete);
> +static int virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>
>  static void virtio_set_hwaddr(struct virtio_hw *hw);
>  static void virtio_get_hwaddr(struct virtio_hw *hw);
> @@ -779,6 +780,7 @@ struct rte_virtio_xstats_name_off {
>  	.stats_reset             = virtio_dev_stats_reset,
>  	.xstats_reset            = virtio_dev_stats_reset,
>  	.link_update             = virtio_dev_link_update,
> +	.vlan_offload_set        = virtio_dev_vlan_offload_set,
>  	.rx_queue_setup          = virtio_dev_rx_queue_setup,
>  	.rx_queue_intr_enable    = virtio_dev_rx_queue_intr_enable,
>  	.rx_queue_intr_disable   = virtio_dev_rx_queue_intr_disable,
> @@ -1875,6 +1877,25 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>  	return (old.link_status == link.link_status) ? -1 : 0;
>  }
>
> +static int
> +virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> +{
> +	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +	struct virtio_hw *hw = dev->data->dev_private;
> +
> +	if (rxmode->hw_vlan_filter &&
> +	    !vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VLAN)) {
> +		PMD_DRV_LOG(NOTICE,
> +			    "vlan filtering not available on this host");
> +		return -ENOTSUP;
> +	}
> +
> +	if (mask & ETH_VLAN_STRIP_MASK)
> +		hw->vlan_strip = rxmode->hw_vlan_strip;
> +
> +	return 0;
> +}
> +
>  static void
>  virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  {
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index 3910991..06735dd 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> @@ -100,7 +100,7 @@ static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
>  vmxnet3_dev_supported_ptypes_get(struct rte_eth_dev *dev);
>  static int vmxnet3_dev_vlan_filter_set(struct rte_eth_dev *dev,
>  				       uint16_t vid, int on);
> -static void vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static int vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
>  static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
>  				 struct ether_addr *mac_addr);
>  static void vmxnet3_interrupt_handler(void *param);
> @@ -730,8 +730,10 @@ static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
>  		devRead->rssConfDesc.confPA  = hw->rss_confPA;
>  	}
>
> -	vmxnet3_dev_vlan_offload_set(dev,
> +	ret = vmxnet3_dev_vlan_offload_set(dev,
>  				     ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK);
> +	if (ret)
> +		return ret;
>
>  	vmxnet3_write_mac(hw, dev->data->mac_addrs->addr_bytes);
>
> @@ -1275,7 +1277,7 @@ static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
>  	return 0;
>  }
>
> -static void
> +static int
>  vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  {
>  	struct vmxnet3_hw *hw = dev->data->dev_private;
> @@ -1301,6 +1303,8 @@ static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
>  		VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
>  				       VMXNET3_CMD_UPDATE_VLAN_FILTERS);
>  	}
> +
> +	return 0;
>  }
>
>  static void
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0597641..05e52b8 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
>  	struct rte_eth_dev *dev;
>  	int ret = 0;
>  	int mask = 0;
> -	int cur, org = 0;
> +	int cur, orig = 0;
> +	uint8_t orig_strip, orig_filter, orig_extend;
>
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
>
> +	/* save original values in case of failure */
> +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
> +
>  	/*check which option changed by application*/
>  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> -	if (cur != org) {
> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> +	if (cur != orig) {
>  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
>  		mask |= ETH_VLAN_STRIP_MASK;
>  	}
>
>  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> -	if (cur != org) {
> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> +	if (cur != orig) {
>  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
>  		mask |= ETH_VLAN_FILTER_MASK;
>  	}
>
>  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> -	if (cur != org) {
> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> +	if (cur != orig) {
>  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
>  		mask |= ETH_VLAN_EXTEND_MASK;
>  	}
> @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
>  		return ret;
>
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
> -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
> +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> +	if (ret) {
> +		/* hit an error restore  original values */
> +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
> +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
> +		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
> +	}
>
Currently vlan offload is combining three functions:
strip, filter and extend.

Not all PMDs in DPDK support all of three.
Should not the error we extended to reflect, which of the VLAN offload 
is not supported?

>  	return ret;
>  }
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 0adf327..7254fd0 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1245,7 +1245,7 @@ typedef int (*vlan_tpid_set_t)(struct rte_eth_dev *dev,
>  			       enum rte_vlan_type type, uint16_t tpid);
>  /**< @internal set the outer/inner VLAN-TPID by an Ethernet device. */
>
> -typedef void (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
> +typedef int (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
>  /**< @internal set VLAN offload function by an Ethernet device. */
>
>  typedef int (*vlan_pvid_set_t)(struct rte_eth_dev *dev,
>
David Harton Sept. 1, 2017, 12:54 p.m. UTC | #2 
> -----Original Message-----
> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]
> Sent: Friday, September 01, 2017 3:41 AM
> To: David Harton (dharton) <dharton@cisco.com>; thomas@monjalon.net;
> ferruh.yigit@intel.com; ajit.khaparde@broadcom.com; John Daley (johndale)
> <johndale@cisco.com>; konstantin.ananyev@intel.com; jingjing.wu@intel.com;
> beilei.xing@intel.com; jing.d.chen@intel.com; adrien.mazarguil@6wind.com;
> nelio.laranjeiro@6wind.com; alejandro.lucero@netronome.com;
> rasesh.mody@cavium.com; harish.patil@cavium.com; skhare@vmware.com;
> yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> allain.legacy@windriver.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v4] ethdev: allow returning error on VLAN offload
> configuration
> 
> On 9/1/2017 8:06 AM, David Harton wrote:
> > Some devices may not support or fail setting VLAN offload
> > configuration based on dynamic circurmstances so the
> > vlan_offload_set_t vector is modified to return an int so the caller
> > can determine success or not.
> >
> > rte_eth_dev_set_vlan_offload is updated to return the value provided
> > by the vector when called along with restoring the original offload
> > configs on failure.
> >
> > Existing vlan_offload_set_t vectors are modified to return an int.
> > Majority of cases return 0 but a few that actually can fail now return
> > their failure codes.
> >
> > Finally, a vlan_offload_set_t vector is added to virtio to facilitate
> > dynamically turning VLAN strip on or off.
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> >
> > v4
> > * Modified commit message heading
> > * Moved rel_note comments from ABI to API section
> > * Renamed locals of rte_eth_dev_set_vlan_offload from 'org*' to 'orig*'
> >
> > v3
> > * Fixed a format error.
> > * Apologies...need to figure out why checkpatches.pl keeps saying
> >   valid patch when I've got soft tabs.
> >
> > v2
> > * Fixed a missed format error.
> > * Removed vlan offload vector call casts and replaced with checks
> >   for return values.
> >
> > v1
> > * This is an ABI breakage that has been previously negotiated
> >   with Thomas and the proposed rel note change is included as well.
> >
> >  doc/guides/rel_notes/release_17_11.rst |  8 +++++++-
> >  drivers/net/avp/avp_ethdev.c           | 12 +++++++++---
> >  drivers/net/bnxt/bnxt_ethdev.c         |  9 ++++++---
> >  drivers/net/dpaa2/dpaa2_ethdev.c       | 13 ++++++++++---
> >  drivers/net/e1000/em_ethdev.c          | 12 +++++++++---
> >  drivers/net/e1000/igb_ethdev.c         | 12 +++++++++---
> >  drivers/net/enic/enic_ethdev.c         |  8 +++++---
> >  drivers/net/fm10k/fm10k_ethdev.c       |  3 ++-
> >  drivers/net/i40e/i40e_ethdev.c         | 11 ++++++++---
> >  drivers/net/i40e/i40e_ethdev_vf.c      |  9 ++++++---
> >  drivers/net/ixgbe/ixgbe_ethdev.c       | 25 ++++++++++++++++++-------
> >  drivers/net/mlx5/mlx5.h                |  2 +-
> >  drivers/net/mlx5/mlx5_vlan.c           |  3 ++-
> >  drivers/net/nfp/nfp_net.c              | 13 +++++++------
> >  drivers/net/qede/qede_ethdev.c         |  9 +++++++--
> >  drivers/net/virtio/virtio_ethdev.c     | 21 +++++++++++++++++++++
> >  drivers/net/vmxnet3/vmxnet3_ethdev.c   | 10 +++++++---
> >  lib/librte_ether/rte_ethdev.c          | 28 ++++++++++++++++++++-------
> -
> >  lib/librte_ether/rte_ethdev.h          |  2 +-
> >  19 files changed, 155 insertions(+), 55 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_17_11.rst
> > b/doc/guides/rel_notes/release_17_11.rst
> > index 170f4f9..22df4fd 100644
> > --- a/doc/guides/rel_notes/release_17_11.rst
> > +++ b/doc/guides/rel_notes/release_17_11.rst
> > @@ -110,6 +110,13 @@ API Changes
> >     Also, make sure to start the actual text at the margin.
> >     =========================================================
> >
> > +* **Modified the vlan_offload_set_t function prototype in the ethdev
> > +library.**
> > +
> > +  Changed the function prototype of ``vlan_offload_set_t``.  The
> > + return value  has been changed from ``void`` to ``int`` so the
> > + caller to knows whether  the backing device supports the operation
> > + or if the operation was  successfully performed.
> > +
> >
> >  ABI Changes
> >  -----------
> > @@ -125,7 +132,6 @@ ABI Changes
> >     =========================================================
> >
> >
> > -
> >  Shared Library Versions
> >  -----------------------
> >
> > diff --git a/drivers/net/avp/avp_ethdev.c
> > b/drivers/net/avp/avp_ethdev.c index c746a0e..4011dfa 100644
> > --- a/drivers/net/avp/avp_ethdev.c
> > +++ b/drivers/net/avp/avp_ethdev.c
> > @@ -70,7 +70,7 @@ static int avp_dev_create(struct rte_pci_device
> > *pci_dev,  static void avp_dev_close(struct rte_eth_dev *dev);  static
> > void avp_dev_info_get(struct rte_eth_dev *dev,
> >  			     struct rte_eth_dev_info *dev_info); -static void
> > avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> > +static int avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> >  static int avp_dev_link_update(struct rte_eth_dev *dev, int
> > wait_to_complete);  static void avp_dev_promiscuous_enable(struct
> > rte_eth_dev *dev);  static void avp_dev_promiscuous_disable(struct
> > rte_eth_dev *dev); @@ -2031,7 +2031,12 @@ struct avp_queue {
> >  	mask = (ETH_VLAN_STRIP_MASK |
> >  		ETH_VLAN_FILTER_MASK |
> >  		ETH_VLAN_EXTEND_MASK);
> > -	avp_vlan_offload_set(eth_dev, mask);
> > +	ret = avp_vlan_offload_set(eth_dev, mask);
> > +	if (ret < 0) {
> > +		PMD_DRV_LOG(ERR, "VLAN offload set failed by host, ret=%d\n",
> > +			    ret);
> > +		goto unlock;
> > +	}
> >
> >  	/* update device config */
> >  	memset(&config, 0, sizeof(config));
> > @@ -2214,7 +2219,7 @@ struct avp_queue {
> >  	}
> >  }
> >
> > -static void
> > +static int
> >  avp_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)  {
> >  	struct avp_dev *avp =
> > AVP_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
> > @@ -2239,6 +2244,7 @@ struct avp_queue {
> >  		if (eth_dev->data->dev_conf.rxmode.hw_vlan_extend)
> >  			PMD_DRV_LOG(ERR, "VLAN extend offload not supported\n");
> >  	}
> > +	return 0;
> >  }
> >
> >  static void
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> > b/drivers/net/bnxt/bnxt_ethdev.c index c9d1122..547bd55 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -144,7 +144,7 @@
> >  	ETH_RSS_NONFRAG_IPV6_TCP |	\
> >  	ETH_RSS_NONFRAG_IPV6_UDP)
> >
> > -static void bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int
> > mask);
> > +static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int
> > +mask);
> >
> >  /***********************/
> >
> > @@ -522,7 +522,9 @@ static int bnxt_dev_start_op(struct rte_eth_dev
> *eth_dev)
> >  		vlan_mask |= ETH_VLAN_FILTER_MASK;
> >  	if (eth_dev->data->dev_conf.rxmode.hw_vlan_strip)
> >  		vlan_mask |= ETH_VLAN_STRIP_MASK;
> > -	bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
> > +	rc = bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
> > +	if (rc)
> > +		goto error;
> >
> >  	return 0;
> >
> > @@ -1275,7 +1277,7 @@ static int bnxt_vlan_filter_set_op(struct
> rte_eth_dev *eth_dev,
> >  		return bnxt_del_vlan_filter(bp, vlan_id);  }
> >
> > -static void
> > +static int
> >  bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask)  {
> >  	struct bnxt *bp = (struct bnxt *)dev->data->dev_private; @@ -1307,6
> > +1309,7 @@ static int bnxt_vlan_filter_set_op(struct rte_eth_dev
> > *eth_dev,
> >
> >  	if (mask & ETH_VLAN_EXTEND_MASK)
> >  		RTE_LOG(ERR, PMD, "Extend VLAN Not supported\n");
> > +	return 0;
> >  }
> >
> >  static void
> > diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c
> > b/drivers/net/dpaa2/dpaa2_ethdev.c
> > index 429b3a0..3390cb3 100644
> > --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> > @@ -138,7 +138,7 @@
> >  	return ret;
> >  }
> >
> > -static void
> > +static int
> >  dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	struct dpaa2_dev_priv *priv = dev->data->dev_private; @@ -158,6
> > +158,7 @@
> >  			RTE_LOG(ERR, PMD, "Unable to set vlan filter = %d\n",
> >  				ret);
> >  	}
> > +	return 0;
> >  }
> >
> >  static int
> > @@ -643,8 +644,14 @@
> >  		return ret;
> >  	}
> >  	/* VLAN Offload Settings */
> > -	if (priv->max_vlan_filters)
> > -		dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
> > +	if (priv->max_vlan_filters) {
> > +		ret = dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
> > +		if (ret) {
> > +			PMD_INIT_LOG(ERR, "Error to dpaa2_vlan_offload_set:"
> > +				     "code = %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> >
> >  	return 0;
> >  }
> > diff --git a/drivers/net/e1000/em_ethdev.c
> > b/drivers/net/e1000/em_ethdev.c index 3d4ab93..51f49d8 100644
> > --- a/drivers/net/e1000/em_ethdev.c
> > +++ b/drivers/net/e1000/em_ethdev.c
> > @@ -99,7 +99,7 @@ static int eth_em_interrupt_action(struct
> > rte_eth_dev *dev,
> >
> >  static int eth_em_vlan_filter_set(struct rte_eth_dev *dev,
> >  		uint16_t vlan_id, int on);
> > -static void eth_em_vlan_offload_set(struct rte_eth_dev *dev, int
> > mask);
> > +static int eth_em_vlan_offload_set(struct rte_eth_dev *dev, int
> > +mask);
> >  static void em_vlan_hw_filter_enable(struct rte_eth_dev *dev);
> > static void em_vlan_hw_filter_disable(struct rte_eth_dev *dev);
> > static void em_vlan_hw_strip_enable(struct rte_eth_dev *dev); @@
> > -668,7 +668,12 @@ static int eth_em_pci_remove(struct rte_pci_device
> > *pci_dev)
> >
> >  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
> >  			ETH_VLAN_EXTEND_MASK;
> > -	eth_em_vlan_offload_set(dev, mask);
> > +	ret = eth_em_vlan_offload_set(dev, mask);
> > +	if (ret) {
> > +		PMD_INIT_LOG(ERR, "Unable to update vlan offload");
> > +		em_dev_clear_queues(dev);
> > +		return ret;
> > +	}
> >
> >  	/* Set Interrupt Throttling Rate to maximum allowed value. */
> >  	E1000_WRITE_REG(hw, E1000_ITR, UINT16_MAX); @@ -1447,7 +1452,7 @@
> > static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
> >  	E1000_WRITE_REG(hw, E1000_CTRL, reg);  }
> >
> > -static void
> > +static int
> >  eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	if(mask & ETH_VLAN_STRIP_MASK){
> > @@ -1463,6 +1468,7 @@ static int eth_em_pci_remove(struct rte_pci_device
> *pci_dev)
> >  		else
> >  			em_vlan_hw_filter_disable(dev);
> >  	}
> > +	return 0;
> >  }
> >
> >  /*
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index e4f7a9f..fa15ee9 100644
> > --- a/drivers/net/e1000/igb_ethdev.c
> > +++ b/drivers/net/e1000/igb_ethdev.c
> > @@ -157,7 +157,7 @@ static int eth_igb_vlan_filter_set(struct
> > rte_eth_dev *dev,  static int eth_igb_vlan_tpid_set(struct rte_eth_dev
> *dev,
> >  				 enum rte_vlan_type vlan_type,
> >  				 uint16_t tpid_id);
> > -static void eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int
> > mask);
> > +static int eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int
> > +mask);
> >
> >  static void igb_vlan_hw_filter_enable(struct rte_eth_dev *dev);
> > static void igb_vlan_hw_filter_disable(struct rte_eth_dev *dev); @@
> > -1400,7 +1400,12 @@ static int eth_igbvf_pci_remove(struct
> rte_pci_device *pci_dev)
> >  	 */
> >  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
> >  			ETH_VLAN_EXTEND_MASK;
> > -	eth_igb_vlan_offload_set(dev, mask);
> > +	ret = eth_igb_vlan_offload_set(dev, mask);
> > +	if (ret) {
> > +		PMD_INIT_LOG(ERR, "Unable to set vlan offload");
> > +		igb_dev_clear_queues(dev);
> > +		return ret;
> > +	}
> >
> >  	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
> >  		/* Enable VLAN filter since VMDq always use VLAN filter */ @@
> > -2715,7 +2720,7 @@ static int eth_igbvf_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  						2 * VLAN_TAG_SIZE);
> >  }
> >
> > -static void
> > +static int
> >  eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	if(mask & ETH_VLAN_STRIP_MASK){
> > @@ -2738,6 +2743,7 @@ static int eth_igbvf_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  		else
> >  			igb_vlan_hw_extend_disable(dev);
> >  	}
> > +	return 0;
> >  }
> >
> >
> > diff --git a/drivers/net/enic/enic_ethdev.c
> > b/drivers/net/enic/enic_ethdev.c index da8fec2..fc1eac2 100644
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -347,7 +347,7 @@ static int enicpmd_vlan_filter_set(struct
> rte_eth_dev *eth_dev,
> >  	return err;
> >  }
> >
> > -static void enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int
> > mask)
> > +static int enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int
> > +mask)
> >  {
> >  	struct enic *enic = pmd_priv(eth_dev);
> >
> > @@ -371,6 +371,8 @@ static void enicpmd_vlan_offload_set(struct
> rte_eth_dev *eth_dev, int mask)
> >  		dev_warning(enic,
> >  			"Configuration of extended VLAN is not supported\n");
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev) @@
> > -392,9 +394,9 @@ static int enicpmd_dev_configure(struct rte_eth_dev
> *eth_dev)
> >  			eth_dev->data->dev_conf.rxmode.split_hdr_size);
> >  	}
> >
> > -	enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
> > +	ret = enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
> >  	enic->hw_ip_checksum = eth_dev->data-
> >dev_conf.rxmode.hw_ip_checksum;
> > -	return 0;
> > +	return ret;
> >  }
> >
> >  /* Start the device.
> > diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> > b/drivers/net/fm10k/fm10k_ethdev.c
> > index e60d3a3..f4626f7 100644
> > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > @@ -1590,7 +1590,7 @@ static int fm10k_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  	return 0;
> >  }
> >
> > -static void
> > +static int
> >  fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	if (mask & ETH_VLAN_STRIP_MASK) {
> > @@ -1609,6 +1609,7 @@ static int fm10k_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  		if (!dev->data->dev_conf.rxmode.hw_vlan_filter)
> >  			PMD_INIT_LOG(ERR, "VLAN filter is always on in fm10k");
> >  	}
> > +	return 0;
> >  }
> >
> >  /* Add/Remove a MAC address, and update filters to main VSI */ diff
> > --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 00b6082..d03a44b 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -278,7 +278,7 @@ static int i40e_vlan_filter_set(struct rte_eth_dev
> > *dev,  static int i40e_vlan_tpid_set(struct rte_eth_dev *dev,
> >  			      enum rte_vlan_type vlan_type,
> >  			      uint16_t tpid);
> > -static void i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> > +static int i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> >  static void i40e_vlan_strip_queue_set(struct rte_eth_dev *dev,
> >  				      uint16_t queue,
> >  				      int on);
> > @@ -3130,7 +3130,7 @@ static int i40e_dev_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  	return ret;
> >  }
> >
> > -static void
> > +static int
> >  i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > @@ -3163,6 +3163,7 @@ static int i40e_dev_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> >  		else
> >  			i40e_vsi_config_double_vlan(vsi, FALSE);
> >  	}
> > +	return 0;
> >  }
> >
> >  static void
> > @@ -5216,7 +5217,11 @@ struct i40e_vsi *
> >
> >  	/* Apply vlan offload setting */
> >  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK;
> > -	i40e_vlan_offload_set(dev, mask);
> > +	ret = i40e_vlan_offload_set(dev, mask);
> > +	if (ret) {
> > +		PMD_DRV_LOG(INFO, "Failed to update vlan offload");
> > +		return ret;
> > +	}
> >
> >  	/* Apply double-vlan setting, not implemented yet */
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index f6d8293..f7fffc2 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -118,7 +118,7 @@ static int i40evf_dev_xstats_get_names(struct
> > rte_eth_dev *dev,  static void i40evf_dev_xstats_reset(struct
> > rte_eth_dev *dev);  static int i40evf_vlan_filter_set(struct rte_eth_dev
> *dev,
> >  				  uint16_t vlan_id, int on);
> > -static void i40evf_vlan_offload_set(struct rte_eth_dev *dev, int
> > mask);
> > +static int i40evf_vlan_offload_set(struct rte_eth_dev *dev, int
> > +mask);
> >  static int i40evf_vlan_pvid_set(struct rte_eth_dev *dev, uint16_t pvid,
> >  				int on);
> >  static void i40evf_dev_close(struct rte_eth_dev *dev); @@ -1634,7
> > +1634,9 @@ static int eth_i40evf_pci_remove(struct rte_pci_device
> *pci_dev)
> >  	int ret;
> >
> >  	/* Apply vlan offload setting */
> > -	i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
> > +	ret = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
> > +	if (ret)
> > +		return ret;
> >
> >  	/* Apply pvid setting */
> >  	ret = i40evf_vlan_pvid_set(dev, data->dev_conf.txmode.pvid, @@
> > -1642,7 +1644,7 @@ static int eth_i40evf_pci_remove(struct
> rte_pci_device *pci_dev)
> >  	return ret;
> >  }
> >
> > -static void
> > +static int
> >  i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	struct rte_eth_conf *dev_conf = &dev->data->dev_conf; @@ -1655,6
> > +1657,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device
> *pci_dev)
> >  		else
> >  			i40evf_disable_vlan_strip(dev);
> >  	}
> > +	return 0;
> >  }
> >
> >  static int
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 22171d8..1ec5aaf 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -218,7 +218,7 @@ static void ixgbe_vlan_hw_strip_bitmap_set(struct
> rte_eth_dev *dev,
> >  		uint16_t queue, bool on);
> >  static void ixgbe_vlan_strip_queue_set(struct rte_eth_dev *dev,
> uint16_t queue,
> >  		int on);
> > -static void ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int
> > mask);
> > +static int ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> >  static void ixgbe_vlan_hw_strip_enable(struct rte_eth_dev *dev,
> > uint16_t queue);  static void ixgbe_vlan_hw_strip_disable(struct
> > rte_eth_dev *dev, uint16_t queue);  static void
> > ixgbe_vlan_hw_extend_enable(struct rte_eth_dev *dev); @@ -274,7 +274,7
> @@ static int ixgbevf_vlan_filter_set(struct rte_eth_dev *dev,
> >  		uint16_t vlan_id, int on);
> >  static void ixgbevf_vlan_strip_queue_set(struct rte_eth_dev *dev,
> >  		uint16_t queue, int on);
> > -static void ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int
> > mask);
> > +static int ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int
> > +mask);
> >  static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on);
> > static int ixgbevf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev,
> >  					    uint16_t queue_id);
> > @@ -2125,7 +2125,7 @@ static int eth_ixgbevf_pci_remove(struct
> rte_pci_device *pci_dev)
> >  	 */
> >  }
> >
> > -static void
> > +static int
> >  ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	if (mask & ETH_VLAN_STRIP_MASK) {
> > @@ -2148,6 +2148,7 @@ static int eth_ixgbevf_pci_remove(struct
> rte_pci_device *pci_dev)
> >  		else
> >  			ixgbe_vlan_hw_extend_disable(dev);
> >  	}
> > +	return 0;
> >  }
> >
> >  static void
> > @@ -2568,9 +2569,13 @@ static int eth_ixgbevf_pci_remove(struct
> rte_pci_device *pci_dev)
> >  		goto error;
> >  	}
> >
> > -    mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
> > +	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
> >  		ETH_VLAN_EXTEND_MASK;
> > -	ixgbe_vlan_offload_set(dev, mask);
> > +	err = ixgbe_vlan_offload_set(dev, mask);
> > +	if (err) {
> > +		PMD_INIT_LOG(ERR, "Unable to set VLAN offload");
> > +		goto error;
> > +	}
> >
> >  	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
> >  		/* Enable vlan filtering for VMDq */ @@ -4993,7 +4998,12 @@
> static
> > int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >  	/* Set HW strip */
> >  	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
> >  		ETH_VLAN_EXTEND_MASK;
> > -	ixgbevf_vlan_offload_set(dev, mask);
> > +	err = ixgbevf_vlan_offload_set(dev, mask);
> > +	if (err) {
> > +		PMD_INIT_LOG(ERR, "Unable to set VLAN offload (%d)", err);
> > +		ixgbe_dev_clear_queues(dev);
> > +		return err;
> > +	}
> >
> >  	ixgbevf_dev_rxtx_start(dev);
> >
> > @@ -5153,7 +5163,7 @@ static void ixgbevf_set_vfta_all(struct
> rte_eth_dev *dev, bool on)
> >  	ixgbe_vlan_hw_strip_bitmap_set(dev, queue, on);  }
> >
> > -static void
> > +static int
> >  ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	struct ixgbe_hw *hw =
> > @@ -5168,6 +5178,7 @@ static void ixgbevf_set_vfta_all(struct
> rte_eth_dev *dev, bool on)
> >  		for (i = 0; i < hw->mac.max_rx_queues; i++)
> >  			ixgbevf_vlan_strip_queue_set(dev, i, on);
> >  	}
> > +	return 0;
> >  }
> >
> >  int
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 43c5384..93e71be 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -283,7 +283,7 @@ int mlx5_xstats_get_names(struct rte_eth_dev *,
> >  /* mlx5_vlan.c */
> >
> >  int mlx5_vlan_filter_set(struct rte_eth_dev *, uint16_t, int); -void
> > mlx5_vlan_offload_set(struct rte_eth_dev *, int);
> > +int mlx5_vlan_offload_set(struct rte_eth_dev *, int);
> >  void mlx5_vlan_strip_queue_set(struct rte_eth_dev *, uint16_t, int);
> >
> >  /* mlx5_trigger.c */
> > diff --git a/drivers/net/mlx5/mlx5_vlan.c
> > b/drivers/net/mlx5/mlx5_vlan.c index 1b0fa40..7215909 100644
> > --- a/drivers/net/mlx5/mlx5_vlan.c
> > +++ b/drivers/net/mlx5/mlx5_vlan.c
> > @@ -210,7 +210,7 @@
> >   * @param mask
> >   *   VLAN offload bit mask.
> >   */
> > -void
> > +int
> >  mlx5_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	struct priv *priv = dev->data->dev_private; @@ -230,4 +230,5 @@
> >  			priv_vlan_strip_queue_set(priv, i, hw_vlan_strip);
> >  		priv_unlock(priv);
> >  	}
> > +	return 0;
> >  }
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index 92b03c4..6473edc 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -2149,11 +2149,12 @@ uint32_t nfp_net_txq_full(struct nfp_net_txq
> *txq)
> >  	return i;
> >  }
> >
> > -static void
> > +static int
> >  nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)  {
> >  	uint32_t new_ctrl, update;
> >  	struct nfp_net_hw *hw;
> > +	int ret;
> >
> >  	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >  	new_ctrl = 0;
> > @@ -2174,14 +2175,14 @@ uint32_t nfp_net_txq_full(struct nfp_net_txq
> *txq)
> >  		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> >
> >  	if (new_ctrl == 0)
> > -		return;
> > +		return 0;
> >
> >  	update = NFP_NET_CFG_UPDATE_GEN;
> >
> > -	if (nfp_net_reconfig(hw, new_ctrl, update) < 0)
> > -		return;
> > -
> > -	hw->ctrl = new_ctrl;
> > +	ret = nfp_net_reconfig(hw, new_ctrl, update);
> > +	if (!ret)
> > +		hw->ctrl = new_ctrl;
> > +	return ret;
> >  }
> >
> >  /* Update Redirection Table(RETA) of Receive Side Scaling of Ethernet
> device */
> > diff --git a/drivers/net/qede/qede_ethdev.c
> b/drivers/net/qede/qede_ethdev.c
> > index 0e05989..644f69d 100644
> > --- a/drivers/net/qede/qede_ethdev.c
> > +++ b/drivers/net/qede/qede_ethdev.c
> > @@ -975,7 +975,7 @@ static int qede_vlan_filter_set(struct rte_eth_dev
> *eth_dev,
> >  	return rc;
> >  }
> >
> > -static void qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int
> mask)
> > +static int qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
> >  {
> >  	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
> >  	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
> > @@ -1013,6 +1013,8 @@ static void qede_vlan_offload_set(struct
> rte_eth_dev *eth_dev, int mask)
> >
> >  	DP_INFO(edev, "vlan offload mask %d vlan-strip %d vlan-filter %d\n",
> >  		mask, rxmode->hw_vlan_strip, rxmode->hw_vlan_filter);
> > +
> > +	return 0;
> >  }
> >
> >  static void qede_prandom_bytes(uint32_t *buff)
> > @@ -1157,6 +1159,7 @@ static int qede_dev_configure(struct rte_eth_dev
> *eth_dev)
> >  	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
> >  	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
> >  	struct rte_eth_rxmode *rxmode = &eth_dev->data->dev_conf.rxmode;
> > +	int ret;
> >
> >  	PMD_INIT_FUNC_TRACE(edev);
> >
> > @@ -1237,9 +1240,11 @@ static int qede_dev_configure(struct rte_eth_dev
> *eth_dev)
> >  	qdev->enable_lro = rxmode->enable_lro;
> >
> >  	/* Enable VLAN offloads by default */
> > -	qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
> > +	ret = qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
> >  			ETH_VLAN_FILTER_MASK |
> >  			ETH_VLAN_EXTEND_MASK);
> > +	if (ret)
> > +		return ret;
> >
> >  	DP_INFO(edev, "Device configured with RSS=%d TSS=%d\n",
> >  			QEDE_RSS_COUNT(qdev), QEDE_TSS_COUNT(qdev));
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> > index e320811..72b4248 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -72,6 +72,7 @@ static void virtio_dev_info_get(struct rte_eth_dev
> *dev,
> >  				struct rte_eth_dev_info *dev_info);
> >  static int virtio_dev_link_update(struct rte_eth_dev *dev,
> >  	int wait_to_complete);
> > +static int virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int
> mask);
> >
> >  static void virtio_set_hwaddr(struct virtio_hw *hw);
> >  static void virtio_get_hwaddr(struct virtio_hw *hw);
> > @@ -779,6 +780,7 @@ struct rte_virtio_xstats_name_off {
> >  	.stats_reset             = virtio_dev_stats_reset,
> >  	.xstats_reset            = virtio_dev_stats_reset,
> >  	.link_update             = virtio_dev_link_update,
> > +	.vlan_offload_set        = virtio_dev_vlan_offload_set,
> >  	.rx_queue_setup          = virtio_dev_rx_queue_setup,
> >  	.rx_queue_intr_enable    = virtio_dev_rx_queue_intr_enable,
> >  	.rx_queue_intr_disable   = virtio_dev_rx_queue_intr_disable,
> > @@ -1875,6 +1877,25 @@ static void virtio_dev_free_mbufs(struct
> rte_eth_dev *dev)
> >  	return (old.link_status == link.link_status) ? -1 : 0;
> >  }
> >
> > +static int
> > +virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> > +{
> > +	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +
> > +	if (rxmode->hw_vlan_filter &&
> > +	    !vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VLAN)) {
> > +		PMD_DRV_LOG(NOTICE,
> > +			    "vlan filtering not available on this host");
> > +		return -ENOTSUP;
> > +	}
> > +
> > +	if (mask & ETH_VLAN_STRIP_MASK)
> > +		hw->vlan_strip = rxmode->hw_vlan_strip;
> > +
> > +	return 0;
> > +}
> > +
> >  static void
> >  virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info)
> >  {
> > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > index 3910991..06735dd 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > @@ -100,7 +100,7 @@ static void vmxnet3_dev_info_get(struct rte_eth_dev
> *dev,
> >  vmxnet3_dev_supported_ptypes_get(struct rte_eth_dev *dev);
> >  static int vmxnet3_dev_vlan_filter_set(struct rte_eth_dev *dev,
> >  				       uint16_t vid, int on);
> > -static void vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int
> mask);
> > +static int vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int
> mask);
> >  static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
> >  				 struct ether_addr *mac_addr);
> >  static void vmxnet3_interrupt_handler(void *param);
> > @@ -730,8 +730,10 @@ static int eth_vmxnet3_pci_remove(struct
> rte_pci_device *pci_dev)
> >  		devRead->rssConfDesc.confPA  = hw->rss_confPA;
> >  	}
> >
> > -	vmxnet3_dev_vlan_offload_set(dev,
> > +	ret = vmxnet3_dev_vlan_offload_set(dev,
> >  				     ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK);
> > +	if (ret)
> > +		return ret;
> >
> >  	vmxnet3_write_mac(hw, dev->data->mac_addrs->addr_bytes);
> >
> > @@ -1275,7 +1277,7 @@ static int eth_vmxnet3_pci_remove(struct
> rte_pci_device *pci_dev)
> >  	return 0;
> >  }
> >
> > -static void
> > +static int
> >  vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> >  {
> >  	struct vmxnet3_hw *hw = dev->data->dev_private;
> > @@ -1301,6 +1303,8 @@ static int eth_vmxnet3_pci_remove(struct
> rte_pci_device *pci_dev)
> >  		VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
> >  				       VMXNET3_CMD_UPDATE_VLAN_FILTERS);
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  static void
> > diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
> > index 0597641..05e52b8 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
> >  	struct rte_eth_dev *dev;
> >  	int ret = 0;
> >  	int mask = 0;
> > -	int cur, org = 0;
> > +	int cur, orig = 0;
> > +	uint8_t orig_strip, orig_filter, orig_extend;
> >
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >  	dev = &rte_eth_devices[port_id];
> >
> > +	/* save original values in case of failure */
> > +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> > +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> > +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
> > +
> >  	/*check which option changed by application*/
> >  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
> > -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> > -	if (cur != org) {
> > +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> > +	if (cur != orig) {
> >  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
> >  		mask |= ETH_VLAN_STRIP_MASK;
> >  	}
> >
> >  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
> > -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> > -	if (cur != org) {
> > +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> > +	if (cur != orig) {
> >  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
> >  		mask |= ETH_VLAN_FILTER_MASK;
> >  	}
> >
> >  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
> > -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> > -	if (cur != org) {
> > +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> > +	if (cur != orig) {
> >  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
> >  		mask |= ETH_VLAN_EXTEND_MASK;
> >  	}
> > @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
> >  		return ret;
> >
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
> > -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
> > +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> > +	if (ret) {
> > +		/* hit an error restore  original values */
> > +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
> > +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
> > +		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
> > +	}
> >
> Currently vlan offload is combining three functions:
> strip, filter and extend.
> 
> Not all PMDs in DPDK support all of three.
> Should not the error we extended to reflect, which of the VLAN offload
> is not supported?

There are many examples of APIs that not all PMDs support.
There are also other APIs that manipulate many attributes but do
not communicate which attribute fails on set.
Solving these issues I believe it outside the scope of this change
and it requires a larger community discussion.

This proposed change at least adheres to the existing paradigm.

> 
> >  	return ret;
> >  }
> > diff --git a/lib/librte_ether/rte_ethdev.h
> b/lib/librte_ether/rte_ethdev.h
> > index 0adf327..7254fd0 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -1245,7 +1245,7 @@ typedef int (*vlan_tpid_set_t)(struct rte_eth_dev
> *dev,
> >  			       enum rte_vlan_type type, uint16_t tpid);
> >  /**< @internal set the outer/inner VLAN-TPID by an Ethernet device. */
> >
> > -typedef void (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
> > +typedef int (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
> >  /**< @internal set VLAN offload function by an Ethernet device. */
> >
> >  typedef int (*vlan_pvid_set_t)(struct rte_eth_dev *dev,
> >
Hemant Agrawal Sept. 7, 2017, 9:37 a.m. UTC | #3 
On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
>
>
>> -----Original Message-----
>> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]
>> Sent: Friday, September 01, 2017 3:41 AM
>> Subject: Re: [PATCH v4] ethdev: allow returning error on VLAN offload
>> configuration
>>
>> On 9/1/2017 8:06 AM, David Harton wrote:
>>> Some devices may not support or fail setting VLAN offload
>>> configuration based on dynamic circurmstances so the
>>> vlan_offload_set_t vector is modified to return an int so the caller
>>> can determine success or not.
>>>
>>> rte_eth_dev_set_vlan_offload is updated to return the value provided
>>> by the vector when called along with restoring the original offload
>>> configs on failure.
>>>
>>> Existing vlan_offload_set_t vectors are modified to return an int.
>>> Majority of cases return 0 but a few that actually can fail now return
>>> their failure codes.
>>>
>>> Finally, a vlan_offload_set_t vector is added to virtio to facilitate
>>> dynamically turning VLAN strip on or off.
>>>
>>> Signed-off-by: David Harton <dharton@cisco.com>
>>> ---

<snip>....

>>> diff --git a/lib/librte_ether/rte_ethdev.c
>> b/lib/librte_ether/rte_ethdev.c
>>> index 0597641..05e52b8 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
>>>  	struct rte_eth_dev *dev;
>>>  	int ret = 0;
>>>  	int mask = 0;
>>> -	int cur, org = 0;
>>> +	int cur, orig = 0;
>>> +	uint8_t orig_strip, orig_filter, orig_extend;
>>>
>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>  	dev = &rte_eth_devices[port_id];
>>>
>>> +	/* save original values in case of failure */
>>> +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
>>> +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
>>> +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
>>> +
>>>  	/*check which option changed by application*/
>>>  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_STRIP_MASK;
>>>  	}
>>>
>>>  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_FILTER_MASK;
>>>  	}
>>>
>>>  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
>>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
>>> -	if (cur != org) {
>>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
>>> +	if (cur != orig) {
>>>  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
>>>  		mask |= ETH_VLAN_EXTEND_MASK;
>>>  	}
>>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
>>>  		return ret;
>>>
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
>>> -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
>>> +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
>>> +	if (ret) {
>>> +		/* hit an error restore  original values */
>>> +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
>>> +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
>>> +		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
>>> +	}
>>>
>> Currently vlan offload is combining three functions:
>> strip, filter and extend.
>>
>> Not all PMDs in DPDK support all of three.
>> Should not the error we extended to reflect, which of the VLAN offload
>> is not supported?
>
> There are many examples of APIs that not all PMDs support.
> There are also other APIs that manipulate many attributes but do
> not communicate which attribute fails on set.
> Solving these issues I believe it outside the scope of this change
> and it requires a larger community discussion.
>
> This proposed change at least adheres to the existing paradigm.

I agree that solving this issue is outside the scope of this patch.

However this patch may leave the drivers in incorrect state.

Earlier this API was not returning error or failing. With this change, 
the driver may want to implement the error handling if 2nd or 3rd 
attribute failed. Note that you are also assuming and reverting back to 
the original condition.

The change is fine w.r.t dpaa2 driver.
David Harton Sept. 7, 2017, 12:09 p.m. UTC | #4 
> -----Original Message-----
> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]
> 
> On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
> >
> >> -----Original Message-----
> >> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]
> >>
> >> On 9/1/2017 8:06 AM, David Harton wrote:
> >>> Some devices may not support or fail setting VLAN offload
> >>> configuration based on dynamic circurmstances so the
> >>> vlan_offload_set_t vector is modified to return an int so the caller
> >>> can determine success or not.
> >>>
> >>> rte_eth_dev_set_vlan_offload is updated to return the value provided
> >>> by the vector when called along with restoring the original offload
> >>> configs on failure.
> >>>
> >>> Existing vlan_offload_set_t vectors are modified to return an int.
> >>> Majority of cases return 0 but a few that actually can fail now
> >>> return their failure codes.
> >>>
> >>> Finally, a vlan_offload_set_t vector is added to virtio to
> >>> facilitate dynamically turning VLAN strip on or off.
> >>>
> >>> Signed-off-by: David Harton <dharton@cisco.com>
> >>> ---
> 
> <snip>....
> 
> >>> diff --git a/lib/librte_ether/rte_ethdev.c
> >> b/lib/librte_ether/rte_ethdev.c
> >>> index 0597641..05e52b8 100644
> >>> --- a/lib/librte_ether/rte_ethdev.c
> >>> +++ b/lib/librte_ether/rte_ethdev.c
> >>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
> >>>  	struct rte_eth_dev *dev;
> >>>  	int ret = 0;
> >>>  	int mask = 0;
> >>> -	int cur, org = 0;
> >>> +	int cur, orig = 0;
> >>> +	uint8_t orig_strip, orig_filter, orig_extend;
> >>>
> >>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >>>  	dev = &rte_eth_devices[port_id];
> >>>
> >>> +	/* save original values in case of failure */
> >>> +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> >>> +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> >>> +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
> >>> +
> >>>  	/*check which option changed by application*/
> >>>  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
> >>>  		mask |= ETH_VLAN_STRIP_MASK;
> >>>  	}
> >>>
> >>>  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
> >>>  		mask |= ETH_VLAN_FILTER_MASK;
> >>>  	}
> >>>
> >>>  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
> >>>  		mask |= ETH_VLAN_EXTEND_MASK;
> >>>  	}
> >>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
> >>>  		return ret;
> >>>
> >>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
> >>> -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
> >>> +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> >>> +	if (ret) {
> >>> +		/* hit an error restore  original values */
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
> >>> +	}
> >>>
> >> Currently vlan offload is combining three functions:
> >> strip, filter and extend.
> >>
> >> Not all PMDs in DPDK support all of three.
> >> Should not the error we extended to reflect, which of the VLAN
> >> offload is not supported?
> >
> > There are many examples of APIs that not all PMDs support.
> > There are also other APIs that manipulate many attributes but do not
> > communicate which attribute fails on set.
> > Solving these issues I believe it outside the scope of this change and
> > it requires a larger community discussion.
> >
> > This proposed change at least adheres to the existing paradigm.
> 
> I agree that solving this issue is outside the scope of this patch.
> 
> However this patch may leave the drivers in incorrect state.

I agree.  When failures happen in other APIs as well, whether reported 
to the caller or not, how the failure is handled is inconsistent.

Also, drivers could be in the incorrect state after this API call
with the current implementation.

> 
> Earlier this API was not returning error or failing. With this change, the
> driver may want to implement the error handling if 2nd or 3rd attribute
> failed. Note that you are also assuming and reverting back to the original
> condition.

The real thrust of this change is to report the failure so the
caller can attempt to take some corrective action.

Is there a policy one way or the other on whether DPDK restores original
state on failures or just leaves the system in a failed state and reports
the condition?

I've always come from the ilk that if an API call fails then the state of 
the system should, as much as possible, remain the same as prior to the 
API call which is why I restored the flags on failure.

With that, I can make ethdev call the device again with the "restored"
values and still return the first failure.

Or I could leave ethdev alone and leave the "bad state" as the previous 
implementation did?
 
> 
> The change is fine w.r.t dpaa2 driver.

Thanks.  And thanks for the good discussion.
Ferruh Yigit Oct. 10, 2017, 5:34 a.m. UTC | #5 
On 9/1/2017 3:36 AM, David Harton wrote:
> Some devices may not support or fail setting VLAN offload
> configuration based on dynamic circurmstances so the
> vlan_offload_set_t vector is modified to return an int so
> the caller can determine success or not.
> 
> rte_eth_dev_set_vlan_offload is updated to return the
> value provided by the vector when called along with restoring
> the original offload configs on failure.
> 
> Existing vlan_offload_set_t vectors are modified to return
> an int.  Majority of cases return 0 but a few that actually
> can fail now return their failure codes.
> 
> Finally, a vlan_offload_set_t vector is added to virtio
> to facilitate dynamically turning VLAN strip on or off.
> 
> Signed-off-by: David Harton <dharton@cisco.com>

Hi David,

This patch conflicts against latest version, would you mind rebasing it
on top of latest next-net?

Thanks,
ferruh
David Harton Oct. 10, 2017, 12:20 p.m. UTC | #6 
> -----Original Message-----

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]

> 

> On 9/1/2017 3:36 AM, David Harton wrote:

> > Some devices may not support or fail setting VLAN offload

> > configuration based on dynamic circurmstances so the

> > vlan_offload_set_t vector is modified to return an int so the caller

> > can determine success or not.

> >

> > rte_eth_dev_set_vlan_offload is updated to return the value provided

> > by the vector when called along with restoring the original offload

> > configs on failure.

> >

> > Existing vlan_offload_set_t vectors are modified to return an int.

> > Majority of cases return 0 but a few that actually can fail now return

> > their failure codes.

> >

> > Finally, a vlan_offload_set_t vector is added to virtio to facilitate

> > dynamically turning VLAN strip on or off.

> >

> > Signed-off-by: David Harton <dharton@cisco.com>

> 

> Hi David,

> 

> This patch conflicts against latest version, would you mind rebasing it on

> top of latest next-net?


Sure Ferruh, no problem.

Thanks,
Dave

> 

> Thanks,

> ferruh
diff mbox

Patch

diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 170f4f9..22df4fd 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -110,6 +110,13 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **Modified the vlan_offload_set_t function prototype in the ethdev library.**
+
+  Changed the function prototype of ``vlan_offload_set_t``.  The return value
+  has been changed from ``void`` to ``int`` so the caller to knows whether
+  the backing device supports the operation or if the operation was
+  successfully performed.
+
 
 ABI Changes
 -----------
@@ -125,7 +132,6 @@  ABI Changes
    =========================================================
 
 
-
 Shared Library Versions
 -----------------------
 
diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index c746a0e..4011dfa 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -70,7 +70,7 @@  static int avp_dev_create(struct rte_pci_device *pci_dev,
 static void avp_dev_close(struct rte_eth_dev *dev);
 static void avp_dev_info_get(struct rte_eth_dev *dev,
 			     struct rte_eth_dev_info *dev_info);
-static void avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int avp_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static int avp_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete);
 static void avp_dev_promiscuous_enable(struct rte_eth_dev *dev);
 static void avp_dev_promiscuous_disable(struct rte_eth_dev *dev);
@@ -2031,7 +2031,12 @@  struct avp_queue {
 	mask = (ETH_VLAN_STRIP_MASK |
 		ETH_VLAN_FILTER_MASK |
 		ETH_VLAN_EXTEND_MASK);
-	avp_vlan_offload_set(eth_dev, mask);
+	ret = avp_vlan_offload_set(eth_dev, mask);
+	if (ret < 0) {
+		PMD_DRV_LOG(ERR, "VLAN offload set failed by host, ret=%d\n",
+			    ret);
+		goto unlock;
+	}
 
 	/* update device config */
 	memset(&config, 0, sizeof(config));
@@ -2214,7 +2219,7 @@  struct avp_queue {
 	}
 }
 
-static void
+static int
 avp_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
 {
 	struct avp_dev *avp = AVP_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
@@ -2239,6 +2244,7 @@  struct avp_queue {
 		if (eth_dev->data->dev_conf.rxmode.hw_vlan_extend)
 			PMD_DRV_LOG(ERR, "VLAN extend offload not supported\n");
 	}
+	return 0;
 }
 
 static void
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index c9d1122..547bd55 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -144,7 +144,7 @@ 
 	ETH_RSS_NONFRAG_IPV6_TCP |	\
 	ETH_RSS_NONFRAG_IPV6_UDP)
 
-static void bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask);
+static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask);
 
 /***********************/
 
@@ -522,7 +522,9 @@  static int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
 		vlan_mask |= ETH_VLAN_FILTER_MASK;
 	if (eth_dev->data->dev_conf.rxmode.hw_vlan_strip)
 		vlan_mask |= ETH_VLAN_STRIP_MASK;
-	bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
+	rc = bnxt_vlan_offload_set_op(eth_dev, vlan_mask);
+	if (rc)
+		goto error;
 
 	return 0;
 
@@ -1275,7 +1277,7 @@  static int bnxt_vlan_filter_set_op(struct rte_eth_dev *eth_dev,
 		return bnxt_del_vlan_filter(bp, vlan_id);
 }
 
-static void
+static int
 bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask)
 {
 	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
@@ -1307,6 +1309,7 @@  static int bnxt_vlan_filter_set_op(struct rte_eth_dev *eth_dev,
 
 	if (mask & ETH_VLAN_EXTEND_MASK)
 		RTE_LOG(ERR, PMD, "Extend VLAN Not supported\n");
+	return 0;
 }
 
 static void
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 429b3a0..3390cb3 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -138,7 +138,7 @@ 
 	return ret;
 }
 
-static void
+static int
 dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct dpaa2_dev_priv *priv = dev->data->dev_private;
@@ -158,6 +158,7 @@ 
 			RTE_LOG(ERR, PMD, "Unable to set vlan filter = %d\n",
 				ret);
 	}
+	return 0;
 }
 
 static int
@@ -643,8 +644,14 @@ 
 		return ret;
 	}
 	/* VLAN Offload Settings */
-	if (priv->max_vlan_filters)
-		dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
+	if (priv->max_vlan_filters) {
+		ret = dpaa2_vlan_offload_set(dev, ETH_VLAN_FILTER_MASK);
+		if (ret) {
+			PMD_INIT_LOG(ERR, "Error to dpaa2_vlan_offload_set:"
+				     "code = %d\n", ret);
+			return ret;
+		}
+	}
 
 	return 0;
 }
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 3d4ab93..51f49d8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -99,7 +99,7 @@  static int eth_em_interrupt_action(struct rte_eth_dev *dev,
 
 static int eth_em_vlan_filter_set(struct rte_eth_dev *dev,
 		uint16_t vlan_id, int on);
-static void eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static void em_vlan_hw_filter_enable(struct rte_eth_dev *dev);
 static void em_vlan_hw_filter_disable(struct rte_eth_dev *dev);
 static void em_vlan_hw_strip_enable(struct rte_eth_dev *dev);
@@ -668,7 +668,12 @@  static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
 
 	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
 			ETH_VLAN_EXTEND_MASK;
-	eth_em_vlan_offload_set(dev, mask);
+	ret = eth_em_vlan_offload_set(dev, mask);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "Unable to update vlan offload");
+		em_dev_clear_queues(dev);
+		return ret;
+	}
 
 	/* Set Interrupt Throttling Rate to maximum allowed value. */
 	E1000_WRITE_REG(hw, E1000_ITR, UINT16_MAX);
@@ -1447,7 +1452,7 @@  static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
 	E1000_WRITE_REG(hw, E1000_CTRL, reg);
 }
 
-static void
+static int
 eth_em_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	if(mask & ETH_VLAN_STRIP_MASK){
@@ -1463,6 +1468,7 @@  static int eth_em_pci_remove(struct rte_pci_device *pci_dev)
 		else
 			em_vlan_hw_filter_disable(dev);
 	}
+	return 0;
 }
 
 /*
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index e4f7a9f..fa15ee9 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -157,7 +157,7 @@  static int eth_igb_vlan_filter_set(struct rte_eth_dev *dev,
 static int eth_igb_vlan_tpid_set(struct rte_eth_dev *dev,
 				 enum rte_vlan_type vlan_type,
 				 uint16_t tpid_id);
-static void eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 
 static void igb_vlan_hw_filter_enable(struct rte_eth_dev *dev);
 static void igb_vlan_hw_filter_disable(struct rte_eth_dev *dev);
@@ -1400,7 +1400,12 @@  static int eth_igbvf_pci_remove(struct rte_pci_device *pci_dev)
 	 */
 	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK | \
 			ETH_VLAN_EXTEND_MASK;
-	eth_igb_vlan_offload_set(dev, mask);
+	ret = eth_igb_vlan_offload_set(dev, mask);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "Unable to set vlan offload");
+		igb_dev_clear_queues(dev);
+		return ret;
+	}
 
 	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
 		/* Enable VLAN filter since VMDq always use VLAN filter */
@@ -2715,7 +2720,7 @@  static int eth_igbvf_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 						2 * VLAN_TAG_SIZE);
 }
 
-static void
+static int
 eth_igb_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	if(mask & ETH_VLAN_STRIP_MASK){
@@ -2738,6 +2743,7 @@  static int eth_igbvf_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 		else
 			igb_vlan_hw_extend_disable(dev);
 	}
+	return 0;
 }
 
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index da8fec2..fc1eac2 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -347,7 +347,7 @@  static int enicpmd_vlan_filter_set(struct rte_eth_dev *eth_dev,
 	return err;
 }
 
-static void enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
+static int enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
@@ -371,6 +371,8 @@  static void enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
 		dev_warning(enic,
 			"Configuration of extended VLAN is not supported\n");
 	}
+
+	return 0;
 }
 
 static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
@@ -392,9 +394,9 @@  static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
 			eth_dev->data->dev_conf.rxmode.split_hdr_size);
 	}
 
-	enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
+	ret = enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
 	enic->hw_ip_checksum = eth_dev->data->dev_conf.rxmode.hw_ip_checksum;
-	return 0;
+	return ret;
 }
 
 /* Start the device.
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index e60d3a3..f4626f7 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1590,7 +1590,7 @@  static int fm10k_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	return 0;
 }
 
-static void
+static int
 fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	if (mask & ETH_VLAN_STRIP_MASK) {
@@ -1609,6 +1609,7 @@  static int fm10k_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 		if (!dev->data->dev_conf.rxmode.hw_vlan_filter)
 			PMD_INIT_LOG(ERR, "VLAN filter is always on in fm10k");
 	}
+	return 0;
 }
 
 /* Add/Remove a MAC address, and update filters to main VSI */
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 00b6082..d03a44b 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -278,7 +278,7 @@  static int i40e_vlan_filter_set(struct rte_eth_dev *dev,
 static int i40e_vlan_tpid_set(struct rte_eth_dev *dev,
 			      enum rte_vlan_type vlan_type,
 			      uint16_t tpid);
-static void i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static void i40e_vlan_strip_queue_set(struct rte_eth_dev *dev,
 				      uint16_t queue,
 				      int on);
@@ -3130,7 +3130,7 @@  static int i40e_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	return ret;
 }
 
-static void
+static int
 i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
@@ -3163,6 +3163,7 @@  static int i40e_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 		else
 			i40e_vsi_config_double_vlan(vsi, FALSE);
 	}
+	return 0;
 }
 
 static void
@@ -5216,7 +5217,11 @@  struct i40e_vsi *
 
 	/* Apply vlan offload setting */
 	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK;
-	i40e_vlan_offload_set(dev, mask);
+	ret = i40e_vlan_offload_set(dev, mask);
+	if (ret) {
+		PMD_DRV_LOG(INFO, "Failed to update vlan offload");
+		return ret;
+	}
 
 	/* Apply double-vlan setting, not implemented yet */
 
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index f6d8293..f7fffc2 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -118,7 +118,7 @@  static int i40evf_dev_xstats_get_names(struct rte_eth_dev *dev,
 static void i40evf_dev_xstats_reset(struct rte_eth_dev *dev);
 static int i40evf_vlan_filter_set(struct rte_eth_dev *dev,
 				  uint16_t vlan_id, int on);
-static void i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static int i40evf_vlan_pvid_set(struct rte_eth_dev *dev, uint16_t pvid,
 				int on);
 static void i40evf_dev_close(struct rte_eth_dev *dev);
@@ -1634,7 +1634,9 @@  static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 	int ret;
 
 	/* Apply vlan offload setting */
-	i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+	ret = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+	if (ret)
+		return ret;
 
 	/* Apply pvid setting */
 	ret = i40evf_vlan_pvid_set(dev, data->dev_conf.txmode.pvid,
@@ -1642,7 +1644,7 @@  static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 	return ret;
 }
 
-static void
+static int
 i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
@@ -1655,6 +1657,7 @@  static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 		else
 			i40evf_disable_vlan_strip(dev);
 	}
+	return 0;
 }
 
 static int
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 22171d8..1ec5aaf 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -218,7 +218,7 @@  static void ixgbe_vlan_hw_strip_bitmap_set(struct rte_eth_dev *dev,
 		uint16_t queue, bool on);
 static void ixgbe_vlan_strip_queue_set(struct rte_eth_dev *dev, uint16_t queue,
 		int on);
-static void ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static void ixgbe_vlan_hw_strip_enable(struct rte_eth_dev *dev, uint16_t queue);
 static void ixgbe_vlan_hw_strip_disable(struct rte_eth_dev *dev, uint16_t queue);
 static void ixgbe_vlan_hw_extend_enable(struct rte_eth_dev *dev);
@@ -274,7 +274,7 @@  static int ixgbevf_vlan_filter_set(struct rte_eth_dev *dev,
 		uint16_t vlan_id, int on);
 static void ixgbevf_vlan_strip_queue_set(struct rte_eth_dev *dev,
 		uint16_t queue, int on);
-static void ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on);
 static int ixgbevf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev,
 					    uint16_t queue_id);
@@ -2125,7 +2125,7 @@  static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
 	 */
 }
 
-static void
+static int
 ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	if (mask & ETH_VLAN_STRIP_MASK) {
@@ -2148,6 +2148,7 @@  static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
 		else
 			ixgbe_vlan_hw_extend_disable(dev);
 	}
+	return 0;
 }
 
 static void
@@ -2568,9 +2569,13 @@  static int eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev)
 		goto error;
 	}
 
-    mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
+	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
 		ETH_VLAN_EXTEND_MASK;
-	ixgbe_vlan_offload_set(dev, mask);
+	err = ixgbe_vlan_offload_set(dev, mask);
+	if (err) {
+		PMD_INIT_LOG(ERR, "Unable to set VLAN offload");
+		goto error;
+	}
 
 	if (dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_ONLY) {
 		/* Enable vlan filtering for VMDq */
@@ -4993,7 +4998,12 @@  static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	/* Set HW strip */
 	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
 		ETH_VLAN_EXTEND_MASK;
-	ixgbevf_vlan_offload_set(dev, mask);
+	err = ixgbevf_vlan_offload_set(dev, mask);
+	if (err) {
+		PMD_INIT_LOG(ERR, "Unable to set VLAN offload (%d)", err);
+		ixgbe_dev_clear_queues(dev);
+		return err;
+	}
 
 	ixgbevf_dev_rxtx_start(dev);
 
@@ -5153,7 +5163,7 @@  static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	ixgbe_vlan_hw_strip_bitmap_set(dev, queue, on);
 }
 
-static void
+static int
 ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct ixgbe_hw *hw =
@@ -5168,6 +5178,7 @@  static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 		for (i = 0; i < hw->mac.max_rx_queues; i++)
 			ixgbevf_vlan_strip_queue_set(dev, i, on);
 	}
+	return 0;
 }
 
 int
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 43c5384..93e71be 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -283,7 +283,7 @@  int mlx5_xstats_get_names(struct rte_eth_dev *,
 /* mlx5_vlan.c */
 
 int mlx5_vlan_filter_set(struct rte_eth_dev *, uint16_t, int);
-void mlx5_vlan_offload_set(struct rte_eth_dev *, int);
+int mlx5_vlan_offload_set(struct rte_eth_dev *, int);
 void mlx5_vlan_strip_queue_set(struct rte_eth_dev *, uint16_t, int);
 
 /* mlx5_trigger.c */
diff --git a/drivers/net/mlx5/mlx5_vlan.c b/drivers/net/mlx5/mlx5_vlan.c
index 1b0fa40..7215909 100644
--- a/drivers/net/mlx5/mlx5_vlan.c
+++ b/drivers/net/mlx5/mlx5_vlan.c
@@ -210,7 +210,7 @@ 
  * @param mask
  *   VLAN offload bit mask.
  */
-void
+int
 mlx5_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct priv *priv = dev->data->dev_private;
@@ -230,4 +230,5 @@ 
 			priv_vlan_strip_queue_set(priv, i, hw_vlan_strip);
 		priv_unlock(priv);
 	}
+	return 0;
 }
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 92b03c4..6473edc 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -2149,11 +2149,12 @@  uint32_t nfp_net_txq_full(struct nfp_net_txq *txq)
 	return i;
 }
 
-static void
+static int
 nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	uint32_t new_ctrl, update;
 	struct nfp_net_hw *hw;
+	int ret;
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	new_ctrl = 0;
@@ -2174,14 +2175,14 @@  uint32_t nfp_net_txq_full(struct nfp_net_txq *txq)
 		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
 
 	if (new_ctrl == 0)
-		return;
+		return 0;
 
 	update = NFP_NET_CFG_UPDATE_GEN;
 
-	if (nfp_net_reconfig(hw, new_ctrl, update) < 0)
-		return;
-
-	hw->ctrl = new_ctrl;
+	ret = nfp_net_reconfig(hw, new_ctrl, update);
+	if (!ret)
+		hw->ctrl = new_ctrl;
+	return ret;
 }
 
 /* Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device */
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 0e05989..644f69d 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -975,7 +975,7 @@  static int qede_vlan_filter_set(struct rte_eth_dev *eth_dev,
 	return rc;
 }
 
-static void qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
+static int qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
 {
 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
 	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
@@ -1013,6 +1013,8 @@  static void qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask)
 
 	DP_INFO(edev, "vlan offload mask %d vlan-strip %d vlan-filter %d\n",
 		mask, rxmode->hw_vlan_strip, rxmode->hw_vlan_filter);
+
+	return 0;
 }
 
 static void qede_prandom_bytes(uint32_t *buff)
@@ -1157,6 +1159,7 @@  static int qede_dev_configure(struct rte_eth_dev *eth_dev)
 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
 	struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
 	struct rte_eth_rxmode *rxmode = &eth_dev->data->dev_conf.rxmode;
+	int ret;
 
 	PMD_INIT_FUNC_TRACE(edev);
 
@@ -1237,9 +1240,11 @@  static int qede_dev_configure(struct rte_eth_dev *eth_dev)
 	qdev->enable_lro = rxmode->enable_lro;
 
 	/* Enable VLAN offloads by default */
-	qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
+	ret = qede_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK  |
 			ETH_VLAN_FILTER_MASK |
 			ETH_VLAN_EXTEND_MASK);
+	if (ret)
+		return ret;
 
 	DP_INFO(edev, "Device configured with RSS=%d TSS=%d\n",
 			QEDE_RSS_COUNT(qdev), QEDE_TSS_COUNT(qdev));
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e320811..72b4248 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -72,6 +72,7 @@  static void virtio_dev_info_get(struct rte_eth_dev *dev,
 				struct rte_eth_dev_info *dev_info);
 static int virtio_dev_link_update(struct rte_eth_dev *dev,
 	int wait_to_complete);
+static int virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 
 static void virtio_set_hwaddr(struct virtio_hw *hw);
 static void virtio_get_hwaddr(struct virtio_hw *hw);
@@ -779,6 +780,7 @@  struct rte_virtio_xstats_name_off {
 	.stats_reset             = virtio_dev_stats_reset,
 	.xstats_reset            = virtio_dev_stats_reset,
 	.link_update             = virtio_dev_link_update,
+	.vlan_offload_set        = virtio_dev_vlan_offload_set,
 	.rx_queue_setup          = virtio_dev_rx_queue_setup,
 	.rx_queue_intr_enable    = virtio_dev_rx_queue_intr_enable,
 	.rx_queue_intr_disable   = virtio_dev_rx_queue_intr_disable,
@@ -1875,6 +1877,25 @@  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	return (old.link_status == link.link_status) ? -1 : 0;
 }
 
+static int
+virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
+{
+	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	struct virtio_hw *hw = dev->data->dev_private;
+
+	if (rxmode->hw_vlan_filter &&
+	    !vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VLAN)) {
+		PMD_DRV_LOG(NOTICE,
+			    "vlan filtering not available on this host");
+		return -ENOTSUP;
+	}
+
+	if (mask & ETH_VLAN_STRIP_MASK)
+		hw->vlan_strip = rxmode->hw_vlan_strip;
+
+	return 0;
+}
+
 static void
 virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 3910991..06735dd 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -100,7 +100,7 @@  static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
 vmxnet3_dev_supported_ptypes_get(struct rte_eth_dev *dev);
 static int vmxnet3_dev_vlan_filter_set(struct rte_eth_dev *dev,
 				       uint16_t vid, int on);
-static void vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
 static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
 				 struct ether_addr *mac_addr);
 static void vmxnet3_interrupt_handler(void *param);
@@ -730,8 +730,10 @@  static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
 		devRead->rssConfDesc.confPA  = hw->rss_confPA;
 	}
 
-	vmxnet3_dev_vlan_offload_set(dev,
+	ret = vmxnet3_dev_vlan_offload_set(dev,
 				     ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK);
+	if (ret)
+		return ret;
 
 	vmxnet3_write_mac(hw, dev->data->mac_addrs->addr_bytes);
 
@@ -1275,7 +1277,7 @@  static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
 	return 0;
 }
 
-static void
+static int
 vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 {
 	struct vmxnet3_hw *hw = dev->data->dev_private;
@@ -1301,6 +1303,8 @@  static int eth_vmxnet3_pci_remove(struct rte_pci_device *pci_dev)
 		VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
 				       VMXNET3_CMD_UPDATE_VLAN_FILTERS);
 	}
+
+	return 0;
 }
 
 static void
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0597641..05e52b8 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2048,29 +2048,35 @@  struct rte_eth_dev *
 	struct rte_eth_dev *dev;
 	int ret = 0;
 	int mask = 0;
-	int cur, org = 0;
+	int cur, orig = 0;
+	uint8_t orig_strip, orig_filter, orig_extend;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
+	/* save original values in case of failure */
+	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
+	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
+	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
+
 	/*check which option changed by application*/
 	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
-	if (cur != org) {
+	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
+	if (cur != orig) {
 		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
 		mask |= ETH_VLAN_STRIP_MASK;
 	}
 
 	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
-	if (cur != org) {
+	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
+	if (cur != orig) {
 		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
 		mask |= ETH_VLAN_FILTER_MASK;
 	}
 
 	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
-	if (cur != org) {
+	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
+	if (cur != orig) {
 		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
 		mask |= ETH_VLAN_EXTEND_MASK;
 	}
@@ -2080,7 +2086,13 @@  struct rte_eth_dev *
 		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
-	(*dev->dev_ops->vlan_offload_set)(dev, mask);
+	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
+	if (ret) {
+		/* hit an error restore  original values */
+		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
+		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
+		dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
+	}
 
 	return ret;
 }
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0adf327..7254fd0 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1245,7 +1245,7 @@  typedef int (*vlan_tpid_set_t)(struct rte_eth_dev *dev,
 			       enum rte_vlan_type type, uint16_t tpid);
 /**< @internal set the outer/inner VLAN-TPID by an Ethernet device. */
 
-typedef void (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
+typedef int (*vlan_offload_set_t)(struct rte_eth_dev *dev, int mask);
 /**< @internal set VLAN offload function by an Ethernet device. */
 
 typedef int (*vlan_pvid_set_t)(struct rte_eth_dev *dev,