[ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.

[Weglicki, MichalX]

-----Original Message-----
From: Panu Matilainen [mailto:pmatilai at redhat.com] 
Sent: Wednesday, April 13, 2016 9:38 AM
To: Weglicki, MichalX <michalx.weglicki at intel.com>; dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.

On 04/13/2016 11:33 AM, Panu Matilainen wrote:
> On 04/13/2016 11:16 AM, Weglicki, MichalX wrote:
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai at redhat.com]
>> Sent: Wednesday, April 13, 2016 8:50 AM
>> To: Weglicki, MichalX <michalx.weglicki at intel.com>; dev at openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add
>> support for DPDK 16.04.
>>
>> On 04/12/2016 05:05 PM, mweglicx wrote:
>> [...]
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index e09b471..2295e53 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev
>>> *netdev_,
>>>        link = dev->link;
>>>        ovs_mutex_unlock(&dev->mutex);
>>>
>>> -    if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
>>> +    if (link.link_duplex == ETH_LINK_AUTONEG) {
>>
>> This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
>> ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
>> ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
>> autonegotiation end up with half-duplex these days isn't that likely.
>>
>> [MW] Thank you, your're right, just to be sure, you mean that I should
>> check autoneg through
>> link_autoneg flag, like this:
>>      if (link.link_autoneg) {
>>          *current = NETDEV_F_AUTONEG;
>>      } else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
>>          if (link.link_speed == ETH_SPEED_NUM_10M) {
>>              *current = NETDEV_F_10MB_HD;
>>          }
>>          if (link.link_speed == ETH_SPEED_NUM_100M) {
>>              *current = NETDEV_F_100MB_HD;
>>          }
>>          if (link.link_speed == ETH_SPEED_NUM_1G) {
>>              *current = NETDEV_F_1GB_HD;
>>          }
>>      } else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
>>          if (link.link_speed == ETH_SPEED_NUM_10M) {
>>              *current = NETDEV_F_10MB_FD;
>>          }
>>          if (link.link_speed == ETH_SPEED_NUM_100M) {
>>              *current = NETDEV_F_100MB_FD;
>>          }
>>          if (link.link_speed == ETH_SPEED_NUM_1G) {
>>              *current = NETDEV_F_1GB_FD;
>>          }
>>          if (link.link_speed == ETH_SPEED_NUM_10G) {
>>              *current = NETDEV_F_10GB_FD;
>>          }
>>      }
>>
>> Based on the comments in header files this should work. Thanks in
>> advance.
>
> Yup, that's what I meant. However turns out that isn't right either.
>
> Looking at lib/netdev-linux.c, *current is a bitfield with current speed
> set, AND if its autonegotiated then NETDEV_F_AUTONEG is *also* set. I
> think this code was never quite correct to begin with.

To clarify, I think the DPDK code for this was never quite correct to 
begin with.

	- Panu -
Yes, you are right, I double checked netdev-linux and this is exactly how it is implemented there. 

Ok, I will apply v2 shortly, thank you. 

Br, 
Michal. 
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.