From: Bruce Richardson <bruce.richardson@intel.com>
To: Beilei Xing <beilei.xing@intel.com>
Cc: wenzhuo.lu@intel.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] e1000: configure VLAN TPID
Date: Thu, 16 Jun 2016 10:42:58 +0100 [thread overview]
Message-ID: <20160616094258.GA11016@bricha3-MOBL3> (raw)
In-Reply-To: <1466046930-11021-1-git-send-email-beilei.xing@intel.com>
On Thu, Jun 16, 2016 at 11:15:30AM +0800, Beilei Xing wrote:
> This patch enables configuring the outer TPID for double VLAN.
> Note that outer TPID of single VLAN and inner TPID of double VLAN
> are read only.
While correct, I think it would be clearer to actually state that all other
vlan TPID values are readonly. For single VLAN there is no outer and inner
VLAN tags as such.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v3 changes:
> Update commit log and comments.
>
> drivers/net/e1000/igb_ethdev.c | 37 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index f0921ee..1a5f1f1 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -86,6 +86,13 @@
> #define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT)
> #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
>
> +/* External VLAN Enable bit mask */
> +#define E1000_CTRL_EXT_EXT_VLAN (1 << 26)
> +
> +/* External VLAN Ether Type bit mask and shift */
> +#define E1000_VET_VET_EXT 0xFFFF0000
> +#define E1000_VET_VET_EXT_SHIFT 16
> +
> static int eth_igb_configure(struct rte_eth_dev *dev);
> static int eth_igb_start(struct rte_eth_dev *dev);
> static void eth_igb_stop(struct rte_eth_dev *dev);
> @@ -2237,13 +2244,37 @@ eth_igb_vlan_tpid_set(struct rte_eth_dev *dev,
> {
> struct e1000_hw *hw =
> E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> - uint32_t reg = ETHER_TYPE_VLAN;
> + uint32_t reg;
> + uint32_t qinq;
> int ret = 0;
>
> + qinq = E1000_READ_REG(hw, E1000_CTRL_EXT);
> + qinq &= E1000_CTRL_EXT_EXT_VLAN;
> +
> switch (vlan_type) {
> case ETH_VLAN_TYPE_INNER:
> - reg |= (tpid << 16);
> - E1000_WRITE_REG(hw, E1000_VET, reg);
> + if (qinq) {
> + ret = -ENOTSUP;
> + PMD_DRV_LOG(WARNING,
> + "Inner vlan ether type is read-only\n");
> + } else {
> + ret = -ENOTSUP;
> + PMD_DRV_LOG(ERR, "Inner type is not supported"
> + " by single vlan\n");
> + }
> + break;
This would be clearer if you reworked it. The "ret = -ENOTSUP" should come
before the if statement (allowing you to remove the braces for the if).
In fact, for single vlan case, you could do the check without even going into
the switch statement, since you can never set a TPID if qinq is not enabled.
That would also make the code more readable.
> + case ETH_VLAN_TYPE_OUTER:
> + if (qinq) {
> + reg = E1000_READ_REG(hw, E1000_VET);
> + reg = (reg & (~E1000_VET_VET_EXT)) |
> + ((uint32_t)tpid << E1000_VET_VET_EXT_SHIFT);
> + E1000_WRITE_REG(hw, E1000_VET, reg);
> + } else {
> + ret = -ENOTSUP;
> + PMD_DRV_LOG(WARNING,
> + "Single vlan ether type is read-only\n");
> + }
> +
> break;
> default:
> ret = -EINVAL;
> --
Two other issues I spot:
* Please also check the capitalization of VLAN - it should be all-caps since it's
an acronym.
* According to the macro for PMD_DRV_LOG in e1000_logs.h, a "\n" is automatically
appended to the log messages, so omit the "\n" at the end of the log messages
in this patch.
Regards,
/Bruce
next prev parent reply other threads:[~2016-06-16 9:43 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-21 8:55 [dpdk-dev] [PATCH] " Beilei Xing
2016-06-02 6:43 ` Wang, Xiao W
2016-06-03 3:10 ` Xing, Beilei
2016-06-03 2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
2016-06-03 3:59 ` Wang, Xiao W
2016-06-15 11:03 ` Bruce Richardson
2016-06-16 1:40 ` Xing, Beilei
2016-06-16 3:15 ` [dpdk-dev] [PATCH v3] " Beilei Xing
2016-06-16 6:57 ` Lu, Wenzhuo
2016-06-16 9:42 ` Bruce Richardson [this message]
2016-06-16 13:36 ` [dpdk-dev] [PATCH v4] " Beilei Xing
2016-06-16 13:59 ` Zhang, Helin
2016-06-24 9:59 ` Bruce Richardson
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=20160616094258.GA11016@bricha3-MOBL3 \
--to=bruce.richardson@intel.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--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).