diff mbox

[dpdk-dev] net/ixgbe: fix vlan insert parameter type and its use

Message ID 1476818007-13659-1-git-send-email-daniels@research.att.com (mailing list archive)
State Superseded, archived
Delegated to: Bruce Richardson
Headers

Commit Message

Scott Daniels Oct. 18, 2016, 7:13 p.m. UTC 
  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(-)

Comments

Wenzhuo Lu Oct. 19, 2016, 5:13 a.m. UTC | #1 
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
Iremonger, Bernard Oct. 19, 2016, 10:55 a.m. UTC | #2 
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.
Scott Daniels Oct. 19, 2016, 12:45 p.m. UTC | #3 
------------------------------------------------------------------------
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
>
>
Scott Daniels Oct. 19, 2016, 12:50 p.m. UTC | #4 
------------------------------------------------------------------------
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.
>
Iremonger, Bernard Oct. 19, 2016, 12:57 p.m. UTC | #5 
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.
Scott Daniels Oct. 19, 2016, 1:56 p.m. UTC | #6 
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.
>
>
Iremonger, Bernard Oct. 19, 2016, 2:47 p.m. UTC | #7 
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(-)
Wenzhuo Lu Oct. 20, 2016, 12:51 a.m. UTC | #8 
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.
Wenzhuo Lu Oct. 20, 2016, 12:54 a.m. UTC | #9 
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>
Bruce Richardson Oct. 24, 2016, 2:19 p.m. UTC | #10 
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
diff mbox

Patch

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