From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
To: Scott Daniels <daniels@research.att.com>
Cc: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
"Zhang, Helin" <helin.zhang@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>, "ZELEZNIAK, ALEX" <az5157@att.com>
Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
Date: Wed, 19 Oct 2016 12:57:40 +0000 [thread overview]
Message-ID: <8CEF83825BEC744B83065625E567D7C21A093DA3@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <alpine.LSU.2.20.12.1610190848440.24883@localhost>
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.
next prev parent reply other threads:[~2016-10-19 12:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-18 19:13 E. Scott Daniels
2016-10-19 5:13 ` Lu, Wenzhuo
2016-10-19 10:55 ` Iremonger, Bernard
2016-10-19 12:50 ` Scott Daniels
2016-10-19 12:57 ` Iremonger, Bernard [this message]
2016-10-19 13:56 ` Scott Daniels
2016-10-20 0:51 ` Lu, Wenzhuo
2016-10-19 12:45 ` Scott Daniels
2016-10-19 14:47 ` [dpdk-dev] [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert Bernard Iremonger
2016-10-20 0:54 ` Lu, Wenzhuo
2016-10-24 14:19 ` Bruce Richardson
2016-10-19 14:47 ` [dpdk-dev] [PATCH v2 1/2] net/ixgbe: fix VLAN insert parameter type and its use Bernard Iremonger
2016-10-19 14:47 ` [dpdk-dev] [PATCH v2 2/2] app/test_pmd: change to the VF VLAN insert command Bernard Iremonger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8CEF83825BEC744B83065625E567D7C21A093DA3@IRSMSX108.ger.corp.intel.com \
--to=bernard.iremonger@intel.com \
--cc=az5157@att.com \
--cc=daniels@research.att.com \
--cc=dev@dpdk.org \
--cc=helin.zhang@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).