[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
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 at dpdk.org; az5157 at att.com
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and
> its
> use
>
> Hi Wenzhuo, Scott,
>
>
>
> > > 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
> > > ---
> > > 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.
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
Hi Scott,
> >>> 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
> >>> ---
> >>> 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.
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
Hi Wenzhuo, Scott,
> > 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
> > ---
> > 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.
> > 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.
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
On Wed, 19 Oct 2016, Iremonger, Bernard wrote:
> Hi Scott,
>
>
>
>>>>> 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
>>>>> ---
>>>>> 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.
>
Will do, thanks. New to the patch world, so I appreciate your patience!
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
>
> Regards,
>
> Bernard.
>
>
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
E. Scott Daniels
PMTS - Cloud Software Research
AT Labs - Research
daniels at research.att.com
440.389.0011
On Wed, 19 Oct 2016, Iremonger, Bernard wrote:
> Hi Wenzhuo, Scott,
>
>
>
>>> 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
>>> ---
>>> 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
>
>>> 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.
>
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
E. Scott Daniels
PMTS - Cloud Software Research
AT 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
>> ---
>> 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
>
>
[dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
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
> ---
> 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.
> 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
[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
---
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;
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
