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

[Panu Matilainen]

On 04/13/2016 11:38 AM, Panu Matilainen wrote:
> 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.

So I had a quick look whether 2.5 and earlier need some kind of fix in 
this area. AFAICS the deal is that the autoneg case is a "can't happen" 
situation here: when initializing a port, value of zero indicates 
"autonegotiate this, please" but the driver then sets the fields to the 
negotiated value and there's no way to tell whether it was 
autonegotiated or not after the fact.

In other words, this could be applied to OVS 2.5 (and older) and nothing 
would actually change:

--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1736,11 +1736,7 @@ 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_speed == ETH_LINK_SPEED_AUTONEG) {
-            *current = NETDEV_F_AUTONEG;
-        }
-    } else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
+    if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
          if (link.link_speed == ETH_LINK_SPEED_10) {
              *current = NETDEV_F_10MB_HD;
          }

But since its just a NOP and now fixed for newer versions... probably 
not worth the trouble.

	- Panu -