On 27 March 2016 at 11:53, Thomas Monjalon [off-list ref]
wrote:

2016-03-26 11:24, Marc:

quoted

On 26 March 2016 at 09:08, Thomas Monjalon [off-list ref]
wrote:

quoted

2016-03-25 22:30, Marc:

quoted

From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and

ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work:

quoted

marc@Beluga:~/personal/dpdk/tools$ git diff

diff --git a/lib/librte_ether/rte_ethdev.h

b/lib/librte_ether/rte_ethdev.h

quoted

index ef2502a..fb247a7 100644

--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h

@@ -244,8 +244,8 @@ struct rte_eth_stats {
 /**
  * Device supported speeds bitmap flags
  */
-#define ETH_LINK_SPEED_FIXED    (0 <<  0)  /**< Disable autoneg

(fixed

quoted

quoted

speed) */

quoted

-#define ETH_LINK_SPEED_AUTONEG  (1 <<  0)  /**< Autonegotiate (all

speeds) */

quoted

+#define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all

speeds) */

quoted

+#define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg

(fixed

quoted

quoted

speed) */

quoted

 #define ETH_LINK_SPEED_10M_HD   (1 <<  1)  /**<  10 Mbps

half-duplex */

quoted

quoted

quoted

 #define ETH_LINK_SPEED_10M      (1 <<  2)  /**<  10 Mbps

full-duplex */

quoted

quoted

quoted

 #define ETH_LINK_SPEED_100M_HD  (1 <<  3)  /**< 100 Mbps

half-duplex */

quoted

quoted

quoted

I think having autoneg == 0 is better. Do you agree Thomas?

No I do not agree, because this flag is now used also for
rte_eth_link.link_autoneg.
So it is more logic to set it to 1.

Having to explicitly specify ETH_LINK_SPEED_AUTONEG in link_speeds during
port configuration for achieving auto-negociation, which is what 98% of
applications will use, seems anti-natural to me and breaks existing
applications.

Yes, you're right. We have to avoid apps modifications.
By keeping autoneg the default behaviour (value 0), we avoid issues.

quoted

The only benefit of your approach is not to have another macro #define
ETH_LINK_AUTONEG 1, which is marginal IMHO.

Yes, you're right.
I suggest adding 2 macros for rte_eth_link.link_autoneg:
#define ETH_LINK_FIXED      0 /**< No autonegotiation. */
#define ETH_LINK_AUTONEG    1 /**< Autonegotiated. */

and keep these ones to use with rte_eth_conf.link_speeds and
rte_eth_dev_info.speed_capa:
#define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds)
*/
#define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed
speed) */

Opinions?

Agree on all of them. I can add them in the (hopefully) final v14, once all
or main drivers have been tested.

Regards
marc