------------------------------------------------------------------------ E. Scott Daniels PMTS - Cloud Software Research AT&T Labs - Research daniels at research.att.com 440.389.0011
On Wed, 19 Oct 2016, Lu, Wenzhuo wrote: > Hi Daniels, > > >> -----Original Message----- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of E. Scott Daniels >> Sent: Wednesday, October 19, 2016 3:13 AM >> To: Zhang, Helin; Iremonger, Bernard >> Cc: dev at dpdk.org; az5157 at 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 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. > 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 > >
