[dpdk-dev] [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 at dpdk.org; az5157 at 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 at 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.