Re: [dpdk-dev] [PATCH v3] e1000: configure VLAN TPID

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Jun 16, 2016 at 11:15:30AM +0800, Beilei Xing wrote:
> This patch enables configuring the outer TPID for double VLAN.
> Note that outer TPID of single VLAN and inner TPID of double VLAN
> are read only.

While correct, I think it would be clearer to actually state that all other
vlan TPID values are readonly. For single VLAN there is no outer and inner
VLAN tags as such.

> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v3 changes:
>  Update commit log and comments.
> 
>  drivers/net/e1000/igb_ethdev.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index f0921ee..1a5f1f1 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -86,6 +86,13 @@
>  #define E1000_INCVALUE_82576         (16 << IGB_82576_TSYNC_SHIFT)
>  #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
>  
> +/* External VLAN Enable bit mask */
> +#define E1000_CTRL_EXT_EXT_VLAN      (1 << 26)
> +
> +/* External VLAN Ether Type bit mask and shift */
> +#define E1000_VET_VET_EXT            0xFFFF0000
> +#define E1000_VET_VET_EXT_SHIFT      16
> +
>  static int  eth_igb_configure(struct rte_eth_dev *dev);
>  static int  eth_igb_start(struct rte_eth_dev *dev);
>  static void eth_igb_stop(struct rte_eth_dev *dev);
> @@ -2237,13 +2244,37 @@ eth_igb_vlan_tpid_set(struct rte_eth_dev *dev,
>  {
>  	struct e1000_hw *hw =
>  		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	uint32_t reg = ETHER_TYPE_VLAN;
> +	uint32_t reg;
> +	uint32_t qinq;
>  	int ret = 0;
>  
> +	qinq = E1000_READ_REG(hw, E1000_CTRL_EXT);
> +	qinq &= E1000_CTRL_EXT_EXT_VLAN;
> +
>  	switch (vlan_type) {
>  	case ETH_VLAN_TYPE_INNER:
> -		reg |= (tpid << 16);
> -		E1000_WRITE_REG(hw, E1000_VET, reg);
> +		if (qinq) {
> +			ret = -ENOTSUP;
> +			PMD_DRV_LOG(WARNING,
> +				    "Inner vlan ether type is read-only\n");
> +		} else {
> +			ret = -ENOTSUP;
> +			PMD_DRV_LOG(ERR, "Inner type is not supported"
> +				    " by single vlan\n");
> +		}
> +		break;

This would be clearer if you reworked it. The "ret = -ENOTSUP" should come
before the if statement (allowing you to remove the braces for the if).

In fact, for single vlan case, you could do the check without even going into
the switch statement, since you can never set a TPID if qinq is not enabled.
That would also make the code more readable.

> +	case ETH_VLAN_TYPE_OUTER:
> +		if (qinq) {
> +			reg = E1000_READ_REG(hw, E1000_VET);
> +			reg = (reg & (~E1000_VET_VET_EXT)) |
> +				((uint32_t)tpid << E1000_VET_VET_EXT_SHIFT);
> +			E1000_WRITE_REG(hw, E1000_VET, reg);
> +		} else {
> +			ret = -ENOTSUP;
> +			PMD_DRV_LOG(WARNING,
> +				    "Single vlan ether type is read-only\n");
> +		}
> +
>  		break;
>  	default:
>  		ret = -EINVAL;
> -- 

Two other issues I spot:

* Please also check the capitalization of VLAN - it should be all-caps since it's
an acronym.
* According to the macro for PMD_DRV_LOG in e1000_logs.h, a "\n" is automatically
appended to the log messages, so omit the "\n" at the end of the log messages
in this patch.

Regards,

/Bruce