From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f180.google.com (mail-io0-f180.google.com [209.85.223.180]) by dpdk.org (Postfix) with ESMTP id 2BAE769C8 for ; Wed, 7 Oct 2015 15:31:39 +0200 (CEST) Received: by ioii196 with SMTP id i196so21913357ioi.3 for ; Wed, 07 Oct 2015 06:31:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=uk4j/j3lYdVc3u2bt8idrVINjPxB0+qTkiFhT7pgtsA=; b=eTLm+G2dL9p7iht+vkIva1PnGPE0I9ugjiHD7o5M1J7ppO04FtDhTZwiusNI24iMz3 d/qM2BXoKbmhnRkhN3hm9hIaZEUSsnxg7aLfHEzvbCMzblJysFSQPXk9iYUdqWB8CrSO zDE9ci1hWvpMuF0usjirj6/N3DQusz1KpPj67iE+GvRFY7mTcy8OijK2lB5QKhCEMayg Q4wD4TkdUdb37ltIzA8wclVKDt344Nfo5admGhxuzmnenv1tzXAYhNNrOKH2MMHzivs3 kDKbaLJazJZVEk0wrNKVHwPcXTXg32byxKacyGfF9Bv8qPm3//Wdbs0ZChZO/BNJ1UwX hOJA== MIME-Version: 1.0 X-Received: by 10.107.10.11 with SMTP id u11mr1967161ioi.36.1444224698247; Wed, 07 Oct 2015 06:31:38 -0700 (PDT) Received: by 10.79.109.22 with HTTP; Wed, 7 Oct 2015 06:31:38 -0700 (PDT) In-Reply-To: <20151005105910.GA3457@hmsreliant.think-freely.org> References: <1440807373-24770-1-git-send-email-marc.sune@bisdn.de> <1443993167-1150-1-git-send-email-marcdevel@gmail.com> <1443993167-1150-4-git-send-email-marcdevel@gmail.com> <20151005105910.GA3457@hmsreliant.think-freely.org> Date: Wed, 7 Oct 2015 15:31:38 +0200 Message-ID: From: Marc Sune To: Neil Horman Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v5 3/4] ethdev: redesign link speed config API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Oct 2015 13:31:39 -0000 2015-10-05 12:59 GMT+02:00 Neil Horman : > On Sun, Oct 04, 2015 at 11:12:46PM +0200, Marc Sune wrote: > > This patch redesigns the API to set the link speed/s configure > > for an ethernet port. Specifically: > > > > - it allows to define a set of advertised speeds for > > auto-negociation. > > - it allows to disable link auto-negociation (single fixed speed). > > - default: auto-negociate all supported speeds. > > > > Other changes: > > > > * Added utility MACROs ETH_SPEED_NUM_XXX with the numeric > > values of all supported link speeds, in Mbps. > > * Converted link_speed to uint32_t to accomodate 100G speeds > > (bug). > > * Added autoneg flag in struct rte_eth_link to indicate if > > link speed was a result of auto-negociation or was fixed > > by configuration. > > * Added utility function to convert numeric speeds to bitmap > > fields. > > * Adapted testpmd to the new link API. > > > > Signed-off-by: Marc Sune > > > > diff --git a/lib/librte_ether/rte_ethdev.c > b/lib/librte_ether/rte_ethdev.c > > index f593f6e..29b2960 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1072,6 +1072,55 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, > uint16_t nb_rx_q, uint16_t nb_tx_q, > > } > > > > int > > +rte_eth_speed_to_bm_flag(uint32_t speed, int duplex, uint32_t *flag) > > +{ > > + switch (speed) { > > + case ETH_SPEED_NUM_10M: > > + *flag = (duplex) ? ETH_LINK_SPEED_10M : > > + > ETH_LINK_SPEED_10M_HD; > > + break; > > + case ETH_SPEED_NUM_100M: > > + *flag = (duplex) ? ETH_LINK_SPEED_100M : > > + > ETH_LINK_SPEED_100M_HD; > > + break; > > + case ETH_SPEED_NUM_1G: > > + *flag = ETH_LINK_SPEED_1G; > > + break; > > + case ETH_SPEED_NUM_2_5G: > > + *flag = ETH_LINK_SPEED_2_5G; > > + break; > > + case ETH_SPEED_NUM_5G: > > + *flag = ETH_LINK_SPEED_5G; > > + break; > > + case ETH_SPEED_NUM_10G: > > + *flag = ETH_LINK_SPEED_10G; > > + break; > > + case ETH_SPEED_NUM_20G: > > + *flag = ETH_LINK_SPEED_20G; > > + break; > > + case ETH_SPEED_NUM_25G: > > + *flag = ETH_LINK_SPEED_25G; > > + break; > > + case ETH_SPEED_NUM_40G: > > + *flag = ETH_LINK_SPEED_40G; > > + break; > > + case ETH_SPEED_NUM_50G: > > + *flag = ETH_LINK_SPEED_50G; > > + break; > > + case ETH_SPEED_NUM_56G: > > + *flag = ETH_LINK_SPEED_56G; > > + break; > > + case ETH_SPEED_NUM_100G: > > + *flag = ETH_LINK_SPEED_100G; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > This nees to go in the appropriate version.map file for shared library > building. > > > +int > > rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t > nb_tx_q, > > const struct rte_eth_conf *dev_conf) > > { > > diff --git a/lib/librte_ether/rte_ethdev.h > b/lib/librte_ether/rte_ethdev.h > > index 951a423..bd333e4 100644 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -238,26 +238,59 @@ struct rte_eth_stats { > > }; > > > > /** > > + * Device supported speeds bitmap flags > > + */ > > +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /*< > Autonegociate (all speeds) */ > > +#define ETH_LINK_SPEED_NO_AUTONEG (1 << 0) /*< Disable autoneg > (fixed speed) */ > > +#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 */ > > +#define ETH_LINK_SPEED_100M (1 << 4) /*< 100 Mbps full-duplex > */ > > +#define ETH_LINK_SPEED_1G (1 << 5) /*< 1 Gbps */ > > +#define ETH_LINK_SPEED_2_5G (1 << 6) /*< 2.5 Gbps */ > > +#define ETH_LINK_SPEED_5G (1 << 7) /*< 5 Gbps */ > > +#define ETH_LINK_SPEED_10G (1 << 8) /*< 10 Mbps */ > > +#define ETH_LINK_SPEED_20G (1 << 9) /*< 20 Gbps */ > > +#define ETH_LINK_SPEED_25G (1 << 10) /*< 25 Gbps */ > > +#define ETH_LINK_SPEED_40G (1 << 11) /*< 40 Gbps */ > > +#define ETH_LINK_SPEED_50G (1 << 12) /*< 50 Gbps */ > > +#define ETH_LINK_SPEED_56G (1 << 13) /*< 56 Gbps */ > > +#define ETH_LINK_SPEED_100G (1 << 14) /*< 100 Gbps */ > > + > > +/** > > + * Ethernet numeric link speeds in Mbps > > + */ > > +#define ETH_SPEED_NUM_NONE 0 /*< Not defined */ > > +#define ETH_SPEED_NUM_10M 10 /*< 10 Mbps */ > > +#define ETH_SPEED_NUM_100M 100 /*< 100 Mbps */ > > +#define ETH_SPEED_NUM_1G 1000 /*< 1 Gbps */ > > +#define ETH_SPEED_NUM_2_5G 2500 /*< 2.5 Gbps */ > > +#define ETH_SPEED_NUM_5G 5000 /*< 5 Gbps */ > > +#define ETH_SPEED_NUM_10G 10000 /*< 10 Mbps */ > > +#define ETH_SPEED_NUM_20G 20000 /*< 20 Gbps */ > > +#define ETH_SPEED_NUM_25G 25000 /*< 25 Gbps */ > > +#define ETH_SPEED_NUM_40G 40000 /*< 40 Gbps */ > > +#define ETH_SPEED_NUM_50G 50000 /*< 50 Gbps */ > > +#define ETH_SPEED_NUM_56G 56000 /*< 56 Gbps */ > > +#define ETH_SPEED_NUM_100G 100000 /*< 100 Gbps */ > > + > > +/** > > * A structure used to retrieve link-level information of an Ethernet > port. > > */ > > struct rte_eth_link { > > - uint16_t link_speed; /**< ETH_LINK_SPEED_[10, 100, 1000, > 10000] */ > > - uint16_t link_duplex; /**< ETH_LINK_[HALF_DUPLEX, FULL_DUPLEX] > */ > > - uint8_t link_status : 1; /**< 1 -> link up, 0 -> link down */ > > -}__attribute__((aligned(8))); /**< aligned for atomic64 read/write > */ > > - > > -#define ETH_LINK_SPEED_AUTONEG 0 /**< Auto-negotiate link speed. > */ > > -#define ETH_LINK_SPEED_10 10 /**< 10 megabits/second. */ > > -#define ETH_LINK_SPEED_100 100 /**< 100 megabits/second. */ > > -#define ETH_LINK_SPEED_1000 1000 /**< 1 gigabits/second. */ > > -#define ETH_LINK_SPEED_10000 10000 /**< 10 gigabits/second. */ > > -#define ETH_LINK_SPEED_10G 10000 /**< alias of 10 > gigabits/second. */ > > -#define ETH_LINK_SPEED_20G 20000 /**< 20 gigabits/second. */ > > -#define ETH_LINK_SPEED_40G 40000 /**< 40 gigabits/second. */ > > + uint32_t link_speed; /**< Link speed (ETH_SPEED_NUM_) */ > > + uint16_t link_duplex; /**< 1 -> full duplex, 0 -> half duplex > */ > > + uint8_t link_autoneg : 1; /**< 1 -> link speed has been autoneg */ > > + uint8_t link_status : 1; /**< 1 -> link up, 0 -> link down */ > > +} __attribute__((aligned(8))); /**< aligned for atomic64 > read/write */ > > > > -#define ETH_LINK_AUTONEG_DUPLEX 0 /**< Auto-negotiate duplex. */ > > -#define ETH_LINK_HALF_DUPLEX 1 /**< Half-duplex connection. */ > > -#define ETH_LINK_FULL_DUPLEX 2 /**< Full-duplex connection. */ > > +/* Utility constants */ > > +#define ETH_LINK_HALF_DUPLEX 0 /**< Half-duplex connection. */ > > +#define ETH_LINK_FULL_DUPLEX 1 /**< Full-duplex connection. */ > > +#define ETH_LINK_SPEED_FIXED 0 /**< Link speed was not > autonegociated. */ > > +#define ETH_LINK_SPEED_NEG 1 /**< Link speed was > autonegociated. */ > > +#define ETH_LINK_DOWN 0 /**< Link is down. */ > > +#define ETH_LINK_UP 1 /**< Link is up. */ > > > > /** > > * A structure used to configure the ring threshold registers of an > RX/TX > > @@ -747,10 +780,14 @@ struct rte_intr_conf { > > * configuration settings may be needed. > > */ > > struct rte_eth_conf { > > - uint16_t link_speed; > > - /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */ > > - uint16_t link_duplex; > > - /**< ETH_LINK_[HALF_DUPLEX|FULL_DUPLEX], or 0 for autonegotation */ > > + uint32_t link_speeds; /**< bitmap of ETH_LINK_SPEED_XXX of speeds > to be > > + used. ETH_LINK_SPEED_NO_AUTONEG disables > link > > + autonegociation, and a unique speed shall > be > > + set. Otherwise, the bitmap defines the set > of > > + speeds to be advertised. If the special > value > > + ETH_LINK_SPEED_AUTONEG (0) is used, all > speeds > > + supported are advertised. > > + */ > This structure is allocated by applications, and the link_speed/duplex > field may > be accessed by them. As such this is an ABI change and should go through > the > ABI process. Arguably, while its not really supposed to be done, given > that > there are so many changes comming to rte_eth_dev for 2.2, I could see an > argument for sliding this in with those changes, so we didn't have to wait > for > an additional relelase cycle. > If I understand you correctly you are arguing that this patch should go through the process of NEXT_ABI -> unique implementation. I keep saying I do not understand what is the benefit of doing so when the ABI for 2.2 is going to change anyway (even other patches will modify ethdev ABI). Having a strict rule for ABI changes between minor releases (e.g. 2.1.0 and 2.1.1) is a must or if the next release is announced to be ABI compatible. Other than that I don't see it, specially since users have to recompile anyway their binaries due to (other) ABI changes. But maybe I am missing something? > The removal of the Speed/Duplex macros should also be noted. > Agree. I will add it into the doc Marc