[PATCH] net/ixgbe: fix vlan insert parameter type and its use

[PATCH] net/ixgbe: fix vlan insert parameter type and its use

[E. Scott Daniels]

The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
and treated as a binary flag when it needs to be a uint16_t
and treated as a VLAN id.  The data sheet (sect 8.2.3.27.13) describes
the right most 16 bits as the VLAN id that is to be inserted; the
16.11  code is accepting only a 1 or 0 thus effectively only
allowing the VLAN id 1 to be inserted (0 disables the insertion
setting).

This patch changes the final parm name to represent the data that
is being accepted (vlan_id), changes the type to permit all valid
VLAN ids, and validates the parameter based on the range of 0 to
4095. Corresponding changes to prototype and documentation in the
.h file.

Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")

Signed-off-by: E. Scott Daniels <daniels@research.att.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
 drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 4ca5747..316af73 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on)
 }
 
 int
-rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
+rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t vlan_id)
 {
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
@@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
 	if (vf >= dev_info.max_vfs)
 		return -EINVAL;
 
-	if (on > 1)
+	if (vlan_id > 4095)
 		return -EINVAL;
 
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
-	if (on) {
-		ctrl = on;
+	if (vlan_id) {
+		ctrl = vlan_id;
 		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
 	} else {
 		ctrl = 0;
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h b/drivers/net/ixgbe/rte_pmd_ixgbe.h
index 2fdf530..c2fb826 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
@@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on);
  *    The port identifier of the Ethernet device.
  * @param vf
  *    ID specifying VF.
- * @param on
- *    1 - Enable VF's vlan insert.
- *    0 - Disable VF's vlan insert
+ * @param vlan_id
+ *    0 - Disable VF's vlan insert.
+ *    n - Enable; n is inserted as the vlan id.
  *
  * @return
  *   - (0) if successful.
  *   - (-ENODEV) if *port* invalid.
  *   - (-EINVAL) if bad parameter.
  */
-int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
+int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
+		uint16_t vlan_id);
 
 /**
  * Enable/Disable tx loopback
-- 
1.9.1

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Lu, Wenzhuo]

Hi Daniels,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of E. Scott Daniels
> Sent: Wednesday, October 19, 2016 3:13 AM
> To: Zhang, Helin; Iremonger, Bernard
> Cc: dev@dpdk.org; az5157@att.com; E. Scott Daniels
> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
> 
> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and treated
> as a binary flag when it needs to be a uint16_t and treated as a VLAN id.  The
> data sheet (sect 8.2.3.27.13) describes the right most 16 bits as the VLAN id that
> is to be inserted; the
> 16.11  code is accepting only a 1 or 0 thus effectively only allowing the VLAN id 1
> to be inserted (0 disables the insertion setting).
> 
> This patch changes the final parm name to represent the data that is being
> accepted (vlan_id), changes the type to permit all valid VLAN ids, and validates
> the parameter based on the range of 0 to 4095. Corresponding changes to
> prototype and documentation in the .h file.
> 
> Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
> 
> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 4ca5747..316af73 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
> uint16_t vf, uint8_t on)  }
> 
>  int
> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
> +vlan_id)
>  {
>  	struct ixgbe_hw *hw;
>  	uint32_t ctrl;
> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port,
> uint16_t vf, uint8_t on)
>  	if (vf >= dev_info.max_vfs)
>  		return -EINVAL;
> 
> -	if (on > 1)
> +	if (vlan_id > 4095)
>  		return -EINVAL;
> 
>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
> -	if (on) {
> -		ctrl = on;
> +	if (vlan_id) {
> +		ctrl = vlan_id;
I believe this patch is a good idea of an enhancement. But you cannot leverage the existing code like this.
The register IXGBE_VMVIR is only for enable/disable. We cannot write the VLAN ID into it.
If you want to merge the 2 things, enabling/disabling and setting VLAN ID together. I think we need a totally new implementation. So NACK.

>  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>  	} else {
>  		ctrl = 0;
> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> index 2fdf530..c2fb826 100644
> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
> uint16_t vf, uint8_t on);
>   *    The port identifier of the Ethernet device.
>   * @param vf
>   *    ID specifying VF.
> - * @param on
> - *    1 - Enable VF's vlan insert.
> - *    0 - Disable VF's vlan insert
> + * @param vlan_id
> + *    0 - Disable VF's vlan insert.
> + *    n - Enable; n is inserted as the vlan id.
>   *
>   * @return
>   *   - (0) if successful.
>   *   - (-ENODEV) if *port* invalid.
>   *   - (-EINVAL) if bad parameter.
>   */
> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> +		uint16_t vlan_id);
> 
>  /**
>   * Enable/Disable tx loopback
> --
> 1.9.1

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Iremonger, Bernard]

Hi Wenzhuo, Scott,

<snip>

> > Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type
> > and its use
> >
> > The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and
> > treated as a binary flag when it needs to be a uint16_t and treated as
> > a VLAN id.  The data sheet (sect 8.2.3.27.13) describes the right most
> > 16 bits as the VLAN id that is to be inserted; the
> > 16.11  code is accepting only a 1 or 0 thus effectively only allowing
> > the VLAN id 1 to be inserted (0 disables the insertion setting).
> >
> > This patch changes the final parm name to represent the data that is
> > being accepted (vlan_id), changes the type to permit all valid VLAN
> > ids, and validates the parameter based on the range of 0 to 4095.
> > Corresponding changes to prototype and documentation in the .h file.
> >
> > Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
> >
> > Signed-off-by: E. Scott Daniels <daniels@research.att.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
> > drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 4ca5747..316af73 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> > port, uint16_t vf, uint8_t on)  }
> >
> >  int
> > -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
> > on)
> > +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
> > +vlan_id)
> >  {
> >  	struct ixgbe_hw *hw;
> >  	uint32_t ctrl;
> > @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
> port,
> > uint16_t vf, uint8_t on)
> >  	if (vf >= dev_info.max_vfs)
> >  		return -EINVAL;
> >
> > -	if (on > 1)
> > +	if (vlan_id > 4095)
> >  		return -EINVAL;
> >
> >  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
> > -	if (on) {
> > -		ctrl = on;
> > +	if (vlan_id) {
> > +		ctrl = vlan_id;
> I believe this patch is a good idea of an enhancement. But you cannot
> leverage the existing code like this.
> The register IXGBE_VMVIR is only for enable/disable. We cannot write the
> VLAN ID into it.
> If you want to merge the 2 things, enabling/disabling and setting VLAN ID
> together. I think we need a totally new implementation. So NACK.

Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is correct.
The NACK is not correct.

However changing the API means that where it is called needs to be changed too.
It is called at present from app/testpmd/cmdline.c  line 4731.


 
> >  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
> >  	} else {
> >  		ctrl = 0;
> > diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > index 2fdf530..c2fb826 100644
> > --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> > port, uint16_t vf, uint8_t on);
> >   *    The port identifier of the Ethernet device.
> >   * @param vf
> >   *    ID specifying VF.
> > - * @param on
> > - *    1 - Enable VF's vlan insert.
> > - *    0 - Disable VF's vlan insert
> > + * @param vlan_id
> > + *    0 - Disable VF's vlan insert.
> > + *    n - Enable; n is inserted as the vlan id.
> >   *
> >   * @return
> >   *   - (0) if successful.
> >   *   - (-ENODEV) if *port* invalid.
> >   *   - (-EINVAL) if bad parameter.
> >   */
> > -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> > uint8_t on);
> > +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> > +		uint16_t vlan_id);
> >
> >  /**
> >   * Enable/Disable tx loopback
> > --
> > 1.9.1

Regards,

Bernard.

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Scott Daniels]

------------------------------------------------------------------------
E. Scott Daniels
PMTS - Cloud Software Research
AT&T Labs - Research
daniels@research.att.com
440.389.0011


On Wed, 19 Oct 2016, Lu, Wenzhuo wrote:

> Hi Daniels,
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of E. Scott Daniels
>> Sent: Wednesday, October 19, 2016 3:13 AM
>> To: Zhang, Helin; Iremonger, Bernard
>> Cc: dev@dpdk.org; az5157@att.com; E. Scott Daniels
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
>>
>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and treated
>> as a binary flag when it needs to be a uint16_t and treated as a VLAN id.  The
>> data sheet (sect 8.2.3.27.13) describes the right most 16 bits as the VLAN id that
>> is to be inserted; the
>> 16.11  code is accepting only a 1 or 0 thus effectively only allowing the VLAN id 1
>> to be inserted (0 disables the insertion setting).
>>
>> This patch changes the final parm name to represent the data that is being
>> accepted (vlan_id), changes the type to permit all valid VLAN ids, and validates
>> the parameter based on the range of 0 to 4095. Corresponding changes to
>> prototype and documentation in the .h file.
>>
>> Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
>>
>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 4ca5747..316af73 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
>> uint16_t vf, uint8_t on)  }
>>
>>  int
>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
>> +vlan_id)
>>  {
>>  	struct ixgbe_hw *hw;
>>  	uint32_t ctrl;
>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port,
>> uint16_t vf, uint8_t on)
>>  	if (vf >= dev_info.max_vfs)
>>  		return -EINVAL;
>>
>> -	if (on > 1)
>> +	if (vlan_id > 4095)
>>  		return -EINVAL;
>>
>>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>> -	if (on) {
>> -		ctrl = on;
>> +	if (vlan_id) {
>> +		ctrl = vlan_id;
> I believe this patch is a good idea of an enhancement. But you cannot leverage the existing code like this.
> The register IXGBE_VMVIR is only for enable/disable. We cannot write the VLAN ID into it.
> If you want to merge the 2 things, enabling/disabling and setting VLAN ID together. I think we need a totally new implementation. So NACK.
>

I'm a bit confused.  According to the data sheet (section 8.2.3.27.13) 
this register accepts the "port VLAN tag to insert if the VLANA field = 
01b" in the right most 16 bits.  This allows the given tag to be inserted 
in outgoing packets.  The current implementation always sets this bit (via 
the IXGBE_VMVIR_VLANA_DEFAULT constant) which causes the tag in the right 
most bits to be inserted.

The current code, which I belive is broken, only ever inserts a 1 or 0 
into the right most bits as it takes the value passed in and ORs it with 
the constant, effectively only allowing the VLAN tag of 1 to be 
inserted.  The value passed in is not being used to set/reset the 
always/never insert VLAN action bit.

The only way to insert VLAN tags 2 through 4095 is to modify the code to 
accept the tag and insert it as described in the patch.

Scott


>>  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>>  	} else {
>>  		ctrl = 0;
>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> index 2fdf530..c2fb826 100644
>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
>> uint16_t vf, uint8_t on);
>>   *    The port identifier of the Ethernet device.
>>   * @param vf
>>   *    ID specifying VF.
>> - * @param on
>> - *    1 - Enable VF's vlan insert.
>> - *    0 - Disable VF's vlan insert
>> + * @param vlan_id
>> + *    0 - Disable VF's vlan insert.
>> + *    n - Enable; n is inserted as the vlan id.
>>   *
>>   * @return
>>   *   - (0) if successful.
>>   *   - (-ENODEV) if *port* invalid.
>>   *   - (-EINVAL) if bad parameter.
>>   */
>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>> +		uint16_t vlan_id);
>>
>>  /**
>>   * Enable/Disable tx loopback
>> --
>> 1.9.1
>
>

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Scott Daniels]

------------------------------------------------------------------------
E. Scott Daniels
PMTS - Cloud Software Research
AT&T Labs - Research
daniels@research.att.com
440.389.0011


On Wed, 19 Oct 2016, Iremonger, Bernard wrote:

> Hi Wenzhuo, Scott,
>
> <snip>
>
>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type
>>> and its use
>>>
>>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and
>>> treated as a binary flag when it needs to be a uint16_t and treated as
>>> a VLAN id.  The data sheet (sect 8.2.3.27.13) describes the right most
>>> 16 bits as the VLAN id that is to be inserted; the
>>> 16.11  code is accepting only a 1 or 0 thus effectively only allowing
>>> the VLAN id 1 to be inserted (0 disables the insertion setting).
>>>
>>> This patch changes the final parm name to represent the data that is
>>> being accepted (vlan_id), changes the type to permit all valid VLAN
>>> ids, and validates the parameter based on the range of 0 to 4095.
>>> Corresponding changes to prototype and documentation in the .h file.
>>>
>>> Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
>>>
>>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>>> ---
>>>  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
>>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> index 4ca5747..316af73 100644
>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>> port, uint16_t vf, uint8_t on)  }
>>>
>>>  int
>>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
>>> on)
>>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
>>> +vlan_id)
>>>  {
>>>  	struct ixgbe_hw *hw;
>>>  	uint32_t ctrl;
>>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
>> port,
>>> uint16_t vf, uint8_t on)
>>>  	if (vf >= dev_info.max_vfs)
>>>  		return -EINVAL;
>>>
>>> -	if (on > 1)
>>> +	if (vlan_id > 4095)
>>>  		return -EINVAL;
>>>
>>>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>>  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>>> -	if (on) {
>>> -		ctrl = on;
>>> +	if (vlan_id) {
>>> +		ctrl = vlan_id;
>> I believe this patch is a good idea of an enhancement. But you cannot
>> leverage the existing code like this.
>> The register IXGBE_VMVIR is only for enable/disable. We cannot write the
>> VLAN ID into it.
>> If you want to merge the 2 things, enabling/disabling and setting VLAN ID
>> together. I think we need a totally new implementation. So NACK.
>
> Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is correct.
> The NACK is not correct.
>
> However changing the API means that where it is called needs to be changed too.
> It is called at present from app/testpmd/cmdline.c  line 4731.
>
>
Bernard,
Should I add this change to the patch? (Didn't occur to me to look for 
use).

Scott

>
>>>  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>>>  	} else {
>>>  		ctrl = 0;
>>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> index 2fdf530..c2fb826 100644
>>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>> port, uint16_t vf, uint8_t on);
>>>   *    The port identifier of the Ethernet device.
>>>   * @param vf
>>>   *    ID specifying VF.
>>> - * @param on
>>> - *    1 - Enable VF's vlan insert.
>>> - *    0 - Disable VF's vlan insert
>>> + * @param vlan_id
>>> + *    0 - Disable VF's vlan insert.
>>> + *    n - Enable; n is inserted as the vlan id.
>>>   *
>>>   * @return
>>>   *   - (0) if successful.
>>>   *   - (-ENODEV) if *port* invalid.
>>>   *   - (-EINVAL) if bad parameter.
>>>   */
>>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>> uint8_t on);
>>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>> +		uint16_t vlan_id);
>>>
>>>  /**
>>>   * Enable/Disable tx loopback
>>> --
>>> 1.9.1
>
> Regards,
>
> Bernard.
>

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Iremonger, Bernard]

Hi Scott,

<snip>

> >>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter
> >>> type and its use
> >>>
> >>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
> >>> and treated as a binary flag when it needs to be a uint16_t and
> >>> treated as a VLAN id.  The data sheet (sect 8.2.3.27.13) describes
> >>> the right most
> >>> 16 bits as the VLAN id that is to be inserted; the
> >>> 16.11  code is accepting only a 1 or 0 thus effectively only
> >>> allowing the VLAN id 1 to be inserted (0 disables the insertion setting).
> >>>
> >>> This patch changes the final parm name to represent the data that is
> >>> being accepted (vlan_id), changes the type to permit all valid VLAN
> >>> ids, and validates the parameter based on the range of 0 to 4095.
> >>> Corresponding changes to prototype and documentation in the .h file.
> >>>
> >>> Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
> >>>
> >>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
> >>> ---
> >>>  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
> >>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
> >>>  2 files changed, 9 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> index 4ca5747..316af73 100644
> >>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> @@ -4727,7 +4727,7 @@
> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> >>> port, uint16_t vf, uint8_t on)  }
> >>>
> >>>  int
> >>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
> >>> on)
> >>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> +uint16_t
> >>> +vlan_id)
> >>>  {
> >>>  	struct ixgbe_hw *hw;
> >>>  	uint32_t ctrl;
> >>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
> >> port,
> >>> uint16_t vf, uint8_t on)
> >>>  	if (vf >= dev_info.max_vfs)
> >>>  		return -EINVAL;
> >>>
> >>> -	if (on > 1)
> >>> +	if (vlan_id > 4095)
> >>>  		return -EINVAL;
> >>>
> >>>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >>>  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
> >>> -	if (on) {
> >>> -		ctrl = on;
> >>> +	if (vlan_id) {
> >>> +		ctrl = vlan_id;
> >> I believe this patch is a good idea of an enhancement. But you cannot
> >> leverage the existing code like this.
> >> The register IXGBE_VMVIR is only for enable/disable. We cannot write
> >> the VLAN ID into it.
> >> If you want to merge the 2 things, enabling/disabling and setting
> >> VLAN ID together. I think we need a totally new implementation. So
> NACK.
> >
> > Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is
> correct.
> > The NACK is not correct.
> >
> > However changing the API means that where it is called needs to be
> changed too.
> > It is called at present from app/testpmd/cmdline.c  line 4731.
> >
> >
> Bernard,
> Should I add this change to the patch? (Didn't occur to me to look for use).
> 
> Scott

No, it would be better to make the testpmd change in a separate patch, and send a v2 patchset with two patches.


> >>>  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
> >>>  	} else {
> >>>  		ctrl = 0;
> >>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> index 2fdf530..c2fb826 100644
> >>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> @@ -99,16 +99,17 @@ int
> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> >>> port, uint16_t vf, uint8_t on);
> >>>   *    The port identifier of the Ethernet device.
> >>>   * @param vf
> >>>   *    ID specifying VF.
> >>> - * @param on
> >>> - *    1 - Enable VF's vlan insert.
> >>> - *    0 - Disable VF's vlan insert
> >>> + * @param vlan_id
> >>> + *    0 - Disable VF's vlan insert.
> >>> + *    n - Enable; n is inserted as the vlan id.
> >>>   *
> >>>   * @return
> >>>   *   - (0) if successful.
> >>>   *   - (-ENODEV) if *port* invalid.
> >>>   *   - (-EINVAL) if bad parameter.
> >>>   */
> >>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> uint8_t on);
> >>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> +		uint16_t vlan_id);
> >>>
> >>>  /**
> >>>   * Enable/Disable tx loopback
> >>> --
> >>> 1.9.1

Regards,

Bernard.

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Scott Daniels]

On Wed, 19 Oct 2016, Iremonger, Bernard wrote:

> Hi Scott,
>
> <snip>
>
>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter
>>>>> type and its use
>>>>>
>>>>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
>>>>> and treated as a binary flag when it needs to be a uint16_t and
>>>>> treated as a VLAN id.  The data sheet (sect 8.2.3.27.13) describes
>>>>> the right most
>>>>> 16 bits as the VLAN id that is to be inserted; the
>>>>> 16.11  code is accepting only a 1 or 0 thus effectively only
>>>>> allowing the VLAN id 1 to be inserted (0 disables the insertion setting).
>>>>>
>>>>> This patch changes the final parm name to represent the data that is
>>>>> being accepted (vlan_id), changes the type to permit all valid VLAN
>>>>> ids, and validates the parameter based on the range of 0 to 4095.
>>>>> Corresponding changes to prototype and documentation in the .h file.
>>>>>
>>>>> Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
>>>>>
>>>>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>>>>> ---
>>>>>  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
>>>>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>>>>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> index 4ca5747..316af73 100644
>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> @@ -4727,7 +4727,7 @@
>> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>>>> port, uint16_t vf, uint8_t on)  }
>>>>>
>>>>>  int
>>>>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
>>>>> on)
>>>>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> +uint16_t
>>>>> +vlan_id)
>>>>>  {
>>>>>  	struct ixgbe_hw *hw;
>>>>>  	uint32_t ctrl;
>>>>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
>>>> port,
>>>>> uint16_t vf, uint8_t on)
>>>>>  	if (vf >= dev_info.max_vfs)
>>>>>  		return -EINVAL;
>>>>>
>>>>> -	if (on > 1)
>>>>> +	if (vlan_id > 4095)
>>>>>  		return -EINVAL;
>>>>>
>>>>>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>>>>  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>>>>> -	if (on) {
>>>>> -		ctrl = on;
>>>>> +	if (vlan_id) {
>>>>> +		ctrl = vlan_id;
>>>> I believe this patch is a good idea of an enhancement. But you cannot
>>>> leverage the existing code like this.
>>>> The register IXGBE_VMVIR is only for enable/disable. We cannot write
>>>> the VLAN ID into it.
>>>> If you want to merge the 2 things, enabling/disabling and setting
>>>> VLAN ID together. I think we need a totally new implementation. So
>> NACK.
>>>
>>> Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is
>> correct.
>>> The NACK is not correct.
>>>
>>> However changing the API means that where it is called needs to be
>> changed too.
>>> It is called at present from app/testpmd/cmdline.c  line 4731.
>>>
>>>
>> Bernard,
>> Should I add this change to the patch? (Didn't occur to me to look for use).
>>
>> Scott
>
> No, it would be better to make the testpmd change in a separate patch, and send a v2 patchset with two patches.
>
Will do, thanks. New to the patch world, so I appreciate your patience!

Scott

>
>>>>>  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>>>>>  	} else {
>>>>>  		ctrl = 0;
>>>>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> index 2fdf530..c2fb826 100644
>>>>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> @@ -99,16 +99,17 @@ int
>> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>>>> port, uint16_t vf, uint8_t on);
>>>>>   *    The port identifier of the Ethernet device.
>>>>>   * @param vf
>>>>>   *    ID specifying VF.
>>>>> - * @param on
>>>>> - *    1 - Enable VF's vlan insert.
>>>>> - *    0 - Disable VF's vlan insert
>>>>> + * @param vlan_id
>>>>> + *    0 - Disable VF's vlan insert.
>>>>> + *    n - Enable; n is inserted as the vlan id.
>>>>>   *
>>>>>   * @return
>>>>>   *   - (0) if successful.
>>>>>   *   - (-ENODEV) if *port* invalid.
>>>>>   *   - (-EINVAL) if bad parameter.
>>>>>   */
>>>>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> uint8_t on);
>>>>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> +		uint16_t vlan_id);
>>>>>
>>>>>  /**
>>>>>   * Enable/Disable tx loopback
>>>>> --
>>>>> 1.9.1
>
> Regards,
>
> Bernard.
>
>

[PATCH v2 0/2] net/ixgbe: fix VF VLAN insert

[Bernard Iremonger]

Changes in v2:
Add testpmd patch.
Update testpmd for change to rte_pmd_ixgbe_set_vf_vlan_insert function.

Bernard Iremonger (1):
  app/test_pmd: change to the VF VLAN insert command

E. Scott Daniels (1):
  net/ixgbe: fix VLAN insert parameter type and its use

 app/test-pmd/cmdline.c                      | 19 +++++++++----------
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c            |  8 ++++----
 drivers/net/ixgbe/rte_pmd_ixgbe.h           |  9 +++++----
 4 files changed, 19 insertions(+), 19 deletions(-)

-- 
2.10.1

[PATCH v2 1/2] net/ixgbe: fix VLAN insert parameter type and its use

[Bernard Iremonger]

From: "E. Scott Daniels" <daniels@research.att.com>

The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
and treated as a binary flag when it needs to be a uint16_t
and treated as a VLAN id.  The data sheet (sect 8.2.3.27.13) describes
the right most 16 bits as the VLAN id that is to be inserted; the
16.11  code is accepting only a 1 or 0 thus effectively only
allowing the VLAN id 1 to be inserted (0 disables the insertion
setting).

This patch changes the final parm name to represent the data that
is being accepted (vlan_id), changes the type to permit all valid
VLAN ids, and validates the parameter based on the range of 0 to
4095. Corresponding changes to prototype and documentation in the
.h file.

Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")

Signed-off-by: E. Scott Daniels <daniels@research.att.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
 drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 4ca5747..316af73 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on)
 }
 
 int
-rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
+rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t vlan_id)
 {
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
@@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
 	if (vf >= dev_info.max_vfs)
 		return -EINVAL;
 
-	if (on > 1)
+	if (vlan_id > 4095)
 		return -EINVAL;
 
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
-	if (on) {
-		ctrl = on;
+	if (vlan_id) {
+		ctrl = vlan_id;
 		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
 	} else {
 		ctrl = 0;
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h b/drivers/net/ixgbe/rte_pmd_ixgbe.h
index 2fdf530..c2fb826 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
@@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on);
  *    The port identifier of the Ethernet device.
  * @param vf
  *    ID specifying VF.
- * @param on
- *    1 - Enable VF's vlan insert.
- *    0 - Disable VF's vlan insert
+ * @param vlan_id
+ *    0 - Disable VF's vlan insert.
+ *    n - Enable; n is inserted as the vlan id.
  *
  * @return
  *   - (0) if successful.
  *   - (-ENODEV) if *port* invalid.
  *   - (-EINVAL) if bad parameter.
  */
-int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
+int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
+		uint16_t vlan_id);
 
 /**
  * Enable/Disable tx loopback
-- 
2.10.1

[PATCH v2 2/2] app/test_pmd: change to the VF VLAN insert command

[Bernard Iremonger]

The third parameter to the function rte_pmd_ixgbe_set_vf_vlan_insert
has changed to vlan_id from on|off.
The testpmd doc file has been changed to reflect this change.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/cmdline.c                      | 19 +++++++++----------
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  2 +-
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index e0e4fe4..15dbd2c 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -286,7 +286,7 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set vf vlan stripq (port_id) (vf_id) (on|off)\n"
 			"    Set the VLAN strip for all queues in a pool for a VF from the PF.\n\n"
 
-			"set vf vlan insert (port_id) (vf_id) (on|off)\n"
+			"set vf vlan insert (port_id) (vf_id) (vlan_id)\n"
 			"    Set VLAN insert for a VF from the PF.\n\n"
 
 			"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
@@ -11017,7 +11017,7 @@ struct cmd_vf_vlan_insert_result {
 	cmdline_fixed_string_t insert;
 	uint8_t port_id;
 	uint16_t vf_id;
-	cmdline_fixed_string_t on_off;
+	uint16_t vlan_id;
 };
 
 /* Common CLI fields for vf vlan insert enable disable */
@@ -11045,10 +11045,10 @@ cmdline_parse_token_num_t cmd_vf_vlan_insert_vf_id =
 	TOKEN_NUM_INITIALIZER
 		(struct cmd_vf_vlan_insert_result,
 		 vf_id, UINT16);
-cmdline_parse_token_string_t cmd_vf_vlan_insert_on_off =
-	TOKEN_STRING_INITIALIZER
+cmdline_parse_token_num_t cmd_vf_vlan_insert_vlan_id =
+	TOKEN_NUM_INITIALIZER
 		(struct cmd_vf_vlan_insert_result,
-		 on_off, "on#off");
+		 vlan_id, UINT16);
 
 static void
 cmd_set_vf_vlan_insert_parsed(
@@ -11058,14 +11058,13 @@ cmd_set_vf_vlan_insert_parsed(
 {
 	struct cmd_vf_vlan_insert_result *res = parsed_result;
 	int ret;
-	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
 
-	ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id, is_on);
+	ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id, res->vlan_id);
 	switch (ret) {
 	case 0:
 		break;
 	case -EINVAL:
-		printf("invalid vf_id %d or is_on %d\n", res->vf_id, is_on);
+		printf("invalid vf_id %d or vlan_id %d\n", res->vf_id, res->vlan_id);
 		break;
 	case -ENODEV:
 		printf("invalid port_id %d\n", res->port_id);
@@ -11078,7 +11077,7 @@ cmd_set_vf_vlan_insert_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
 	.f = cmd_set_vf_vlan_insert_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan insert port_id vf_id on|off",
+	.help_str = "set vf vlan insert port_id vf_id vlan_id",
 	.tokens = {
 		(void *)&cmd_vf_vlan_insert_set,
 		(void *)&cmd_vf_vlan_insert_vf,
@@ -11086,7 +11085,7 @@ cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
 		(void *)&cmd_vf_vlan_insert_insert,
 		(void *)&cmd_vf_vlan_insert_port_id,
 		(void *)&cmd_vf_vlan_insert_vf_id,
-		(void *)&cmd_vf_vlan_insert_on_off,
+		(void *)&cmd_vf_vlan_insert_vlan_id,
 		NULL,
 	},
 };
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index c04805b..9a45932 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -533,7 +533,7 @@ vlan set insert (for VF)
 
 Set VLAN insert for a VF from the PF::
 
-   testpmd> set vf vlan insert (port_id) (vf_id) (on|off)
+   testpmd> set vf vlan insert (port_id) (vf_id) (vlan_id)
 
 vlan set antispoof (for VF)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.10.1

Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use

[Lu, Wenzhuo]

Hi Bernard,


> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, October 19, 2016 6:56 PM
> To: Lu, Wenzhuo; E. Scott Daniels; Zhang, Helin
> Cc: dev@dpdk.org; az5157@att.com
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its
> use
> 
> Hi Wenzhuo, Scott,
> 
> <snip>
> 
> > > Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter
> > > type and its use
> > >
> > > The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
> > > and treated as a binary flag when it needs to be a uint16_t and
> > > treated as a VLAN id.  The data sheet (sect 8.2.3.27.13) describes
> > > the right most
> > > 16 bits as the VLAN id that is to be inserted; the
> > > 16.11  code is accepting only a 1 or 0 thus effectively only
> > > allowing the VLAN id 1 to be inserted (0 disables the insertion setting).
> > >
> > > This patch changes the final parm name to represent the data that is
> > > being accepted (vlan_id), changes the type to permit all valid VLAN
> > > ids, and validates the parameter based on the range of 0 to 4095.
> > > Corresponding changes to prototype and documentation in the .h file.
> > >
> > > Fixes:  49e248223e9f71 ("net/ixgbe: add API for VF management")
> > >
> > > Signed-off-by: E. Scott Daniels <daniels@research.att.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_ethdev.c  | 8 ++++----
> > > drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
> > >  2 files changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 4ca5747..316af73 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> > > port, uint16_t vf, uint8_t on)  }
> > >
> > >  int
> > > -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
> > > on)
> > > +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> > > +uint16_t
> > > +vlan_id)
> > >  {
> > >  	struct ixgbe_hw *hw;
> > >  	uint32_t ctrl;
> > > @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
> > port,
> > > uint16_t vf, uint8_t on)
> > >  	if (vf >= dev_info.max_vfs)
> > >  		return -EINVAL;
> > >
> > > -	if (on > 1)
> > > +	if (vlan_id > 4095)
> > >  		return -EINVAL;
> > >
> > >  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > >  	ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
> > > -	if (on) {
> > > -		ctrl = on;
> > > +	if (vlan_id) {
> > > +		ctrl = vlan_id;
> > I believe this patch is a good idea of an enhancement. But you cannot
> > leverage the existing code like this.
> > The register IXGBE_VMVIR is only for enable/disable. We cannot write
> > the VLAN ID into it.
> > If you want to merge the 2 things, enabling/disabling and setting VLAN
> > ID together. I think we need a totally new implementation. So NACK.
> 
> Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is
> correct.
> The NACK is not correct.
You're right. I made a mistake about the words 'default vlan'. It's not fixed but the value of 'Port VLAN ID'.
So the previous patch is not good, we need to fix it. Thanks for correcting me.

> 
> However changing the API means that where it is called needs to be changed
> too.
> It is called at present from app/testpmd/cmdline.c  line 4731.
> 
> 
> 
> > >  		ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
> > >  	} else {
> > >  		ctrl = 0;
> > > diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > > b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > > index 2fdf530..c2fb826 100644
> > > --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > > +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> > > @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> > > port, uint16_t vf, uint8_t on);
> > >   *    The port identifier of the Ethernet device.
> > >   * @param vf
> > >   *    ID specifying VF.
> > > - * @param on
> > > - *    1 - Enable VF's vlan insert.
> > > - *    0 - Disable VF's vlan insert
> > > + * @param vlan_id
> > > + *    0 - Disable VF's vlan insert.
> > > + *    n - Enable; n is inserted as the vlan id.
> > >   *
> > >   * @return
> > >   *   - (0) if successful.
> > >   *   - (-ENODEV) if *port* invalid.
> > >   *   - (-EINVAL) if bad parameter.
> > >   */
> > > -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> > > uint8_t on);
> > > +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> > > +		uint16_t vlan_id);
> > >
> > >  /**
> > >   * Enable/Disable tx loopback
> > > --
> > > 1.9.1
> 
> Regards,
> 
> Bernard.

Re: [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert

[Lu, Wenzhuo]

Hi,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, October 19, 2016 10:48 PM
> To: dev@dpdk.org; daniels@research.att.com; Lu, Wenzhuo; az5157@att.com
> Cc: Iremonger, Bernard
> Subject: [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert
> 
> Changes in v2:
> Add testpmd patch.
> Update testpmd for change to rte_pmd_ixgbe_set_vf_vlan_insert function.
> 
> Bernard Iremonger (1):
>   app/test_pmd: change to the VF VLAN insert command
> 
> E. Scott Daniels (1):
>   net/ixgbe: fix VLAN insert parameter type and its use
> 
>  app/test-pmd/cmdline.c                      | 19 +++++++++----------
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c            |  8 ++++----
>  drivers/net/ixgbe/rte_pmd_ixgbe.h           |  9 +++++----
>  4 files changed, 19 insertions(+), 19 deletions(-)
> 
> --
> 2.10.1
Series-Acked-by: Wenzhuo Lu <Wenzhuo.lu@intel.com>

Re: [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert

[Bruce Richardson]

On Thu, Oct 20, 2016 at 12:54:52AM +0000, Lu, Wenzhuo wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Wednesday, October 19, 2016 10:48 PM
> > To: dev@dpdk.org; daniels@research.att.com; Lu, Wenzhuo; az5157@att.com
> > Cc: Iremonger, Bernard
> > Subject: [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert
> > 
> > Changes in v2:
> > Add testpmd patch.
> > Update testpmd for change to rte_pmd_ixgbe_set_vf_vlan_insert function.
> > 
> > Bernard Iremonger (1):
> >   app/test_pmd: change to the VF VLAN insert command
> > 
> > E. Scott Daniels (1):
> >   net/ixgbe: fix VLAN insert parameter type and its use
> > 
> >  app/test-pmd/cmdline.c                      | 19 +++++++++----------
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  2 +-
> >  drivers/net/ixgbe/ixgbe_ethdev.c            |  8 ++++----
> >  drivers/net/ixgbe/rte_pmd_ixgbe.h           |  9 +++++----
> >  4 files changed, 19 insertions(+), 19 deletions(-)
> > 
> > --
> > 2.10.1
> Series-Acked-by: Wenzhuo Lu <Wenzhuo.lu@intel.com>
> 
Applied to dpdk-next-net/rel_16_11

/Bruce