Re: [PATCH v11 0/8] ethdev: 100G and link speed API refactoring
From: Marc <hidden>
Date: 2016-03-26 10:24:44
On 26 March 2016 at 09:08, Thomas Monjalon [off-list ref] wrote:
Hi Marc, Thanks for finding time to help. 2016-03-25 22:30, Marc:quoted
From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG andETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work:quoted
marc@Beluga:~/personal/dpdk/tools$ git diffdiff --git a/lib/librte_ether/rte_ethdev.hb/lib/librte_ether/rte_ethdev.hquoted
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 (fixedspeed) */quoted
-#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (allspeeds) */quoted
+#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (allspeeds) */quoted
+#define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixedspeed) */quoted
#define ETH_LINK_SPEED_10M_HD (1 << 1) /**< 10 Mbps half-duplex */ #define ETH_LINK_SPEED_10M (1 << 2) /**< 10 Mbps full-duplex */ #define ETH_LINK_SPEED_100M_HD (1 << 3) /**< 100 Mbps half-duplex */ 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. The only benefit of your approach is not to have another macro #define ETH_LINK_AUTONEG 1, which is marginal IMHO.
Would it be possible to fix without reverting?
At least, all existing applications will have to be modified. I would have to go through v12 again to see if there are other issues still to be fixed, and also apply the 2 fixes I found for e1000. Marc