From: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>
To: "Iremonger, Bernard" <bernard.iremonger@intel.com>,
"E. Scott Daniels" <daniels@research.att.com>,
"Zhang, Helin" <helin.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "az5157@att.com" <az5157@att.com>
Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
Date: Thu, 20 Oct 2016 00:51:22 +0000 [thread overview]
Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09093933D3BE@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com>
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.
next prev parent reply other threads:[~2016-10-20 0:51 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
2016-10-19 13:56 ` Scott Daniels
2016-10-20 0:51 ` Lu, Wenzhuo [this message]
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=6A0DE07E22DDAD4C9103DF62FEBC09093933D3BE@shsmsx102.ccr.corp.intel.com \
--to=wenzhuo.lu@intel.com \
--cc=az5157@att.com \
--cc=bernard.iremonger@intel.com \
--cc=daniels@research.att.com \
--cc=dev@dpdk.org \
--cc=helin.zhang@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).