[dpdk-dev,v2] e1000: configure VLAN TPID
Commit Message
This patch enables configuring the ether types of both inner and
outer VLANs. Note that outer TPID of single VLAN and inner TPID of
double VLAN are read only.
Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v2 changes:
Modify return value. Cause inner tpid is not supported by single vlan,
return -ENOTSUP.
Add return value. If want to set inner TPID of double vlan or set outer
tpid of single vlan, return -ENOTSUP.
drivers/net/e1000/igb_ethdev.c | 38 +++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
Comments
Hi,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> Sent: Friday, June 3, 2016 10:59 AM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
>
> This patch enables configuring the ether types of both inner and outer VLANs.
> Note that outer TPID of single VLAN and inner TPID of double VLAN are read
> only.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v2 changes:
> Modify return value. Cause inner tpid is not supported by single vlan, return -
> ENOTSUP.
> Add return value. If want to set inner TPID of double vlan or set outer tpid of
> single vlan, return -ENOTSUP.
>
Acked-by: Xiao Wang <xiao.w.wang@intel.com>
On Fri, Jun 03, 2016 at 10:59:15AM +0800, Beilei Xing wrote:
> This patch enables configuring the ether types of both inner and
> outer VLANs. Note that outer TPID of single VLAN and inner TPID of
> double VLAN are read only.
>
Commit message does not actually match the code, and I there is something
very strange about the code, see my comments below.
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v2 changes:
> Modify return value. Cause inner tpid is not supported by single vlan,
> return -ENOTSUP.
> Add return value. If want to set inner TPID of double vlan or set outer
> tpid of single vlan, return -ENOTSUP.
>
> drivers/net/e1000/igb_ethdev.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index f0921ee..5d37e2c 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -86,6 +86,14 @@
> #define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT)
> #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
>
> +/* CTRL_EXT bit mask*/
> +#define E1000_CTRL_EXT_EXT_VLAN (1 << 26)
> +
> +/* VLAN Ether Type bit mask */
> +#define E1000_VET_VET_EXT 0xFFFF0000
> +
No need for a blank line here. Update the comment above to refer to both
mask and shift values and put them on one line after the other.
> +#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 +2245,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;
Both branches of the if statment return -ENOTSUP and do not do anything to
set any VLAN parameters. This means that although we have the ability to
set the outer vlan, we can no longer set the inner type. This contradicts what
the commit message says.
> + 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");
> + }
> +
So according to the code, the only time you can change a vlan ether type is
to set the outer type for qinq? If this is the correct behaviour, please update
the commit title and message accordingly.
> break;
> default:
> ret = -EINVAL;
> --
> 2.5.0
>
Regards,
/Bruce
Hi Bruce,
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, June 15, 2016 7:04 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
>
> On Fri, Jun 03, 2016 at 10:59:15AM +0800, Beilei Xing wrote:
> > This patch enables configuring the ether types of both inner and outer
> > VLANs. Note that outer TPID of single VLAN and inner TPID of double
> > VLAN are read only.
> >
>
> Commit message does not actually match the code, and I there is something
> very strange about the code, see my comments below.
>
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> > v2 changes:
> > Modify return value. Cause inner tpid is not supported by single
> > vlan, return -ENOTSUP.
> > Add return value. If want to set inner TPID of double vlan or set
> > outer tpid of single vlan, return -ENOTSUP.
> >
> > drivers/net/e1000/igb_ethdev.c | 38
> > +++++++++++++++++++++++++++++++++++---
> > 1 file changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index f0921ee..5d37e2c 100644
> > --- a/drivers/net/e1000/igb_ethdev.c
> > +++ b/drivers/net/e1000/igb_ethdev.c
> > @@ -86,6 +86,14 @@
> > #define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT)
> > #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
> >
> > +/* CTRL_EXT bit mask*/
> > +#define E1000_CTRL_EXT_EXT_VLAN (1 << 26)
> > +
> > +/* VLAN Ether Type bit mask */
> > +#define E1000_VET_VET_EXT 0xFFFF0000
> > +
>
> No need for a blank line here. Update the comment above to refer to both
> mask and shift values and put them on one line after the other.
OK, I'll delete the blank line in next version.
>
> > +#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 +2245,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;
>
> Both branches of the if statment return -ENOTSUP and do not do anything to
> set any VLAN parameters. This means that although we have the ability to
> set the outer vlan, we can no longer set the inner type. This contradicts what
> the commit message says.
Yes, actually inner VLAN couldn't be changed.
>
> > + 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");
> > + }
> > +
>
> So according to the code, the only time you can change a vlan ether type is to
> set the outer type for qinq? If this is the correct behaviour, please update
> the commit title and message accordingly.
Yes, you're right, thanks for all your comments.
>
> > break;
> > default:
> > ret = -EINVAL;
> > --
> > 2.5.0
> >
>
> Regards,
> /Bruce
@@ -86,6 +86,14 @@
#define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT)
#define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
+/* CTRL_EXT bit mask*/
+#define E1000_CTRL_EXT_EXT_VLAN (1 << 26)
+
+/* VLAN Ether Type bit mask */
+#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 +2245,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;
+ 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;