From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f173.google.com (mail-wi0-f173.google.com [209.85.212.173]) by dpdk.org (Postfix) with ESMTP id DFD795958 for ; Wed, 9 Sep 2015 11:29:27 +0200 (CEST) Received: by wiclk2 with SMTP id lk2so149646324wic.0 for ; Wed, 09 Sep 2015 02:29:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-type:content-disposition:content-transfer-encoding :user-agent; bh=moOLcBI1vvNydiJTOD+S1uJJIzZD/eh1lNqjgQD485Y=; b=YdPJ5SAI6vtxHZczyVlDRBQAGX2pzbAVH9DuzcZ+7DIQJ10/vfH8UnFgEzJKE/Sorj 9cyzzXqTjrSOvtIMB0aPZDaQfVsXI65J+wBX2bkLJHU/zjb8UjZy2V1oPyR2C6kCMw2r hE8vZk294Fq9NWKENEq1+bz/AHdY9OY1DGhg+5aRF6hC1/vsOgH+nTn51/L6QcvNXJ1/ iASdA44NxZjBRTWQOA1IRxebIsgSmhFje4koRanY7dSQ0Ri3yKGiXEFWMghvyg64WxOR nNoj9vVU55E8G31KPDX7H4MWNBPwm0pBtwvDEPv2YVCsSqeqEKDRlDDPSjKPuSFOU+bw KfgQ== X-Gm-Message-State: ALoCoQnq1PgSjmdQXO3mFJXguuuJ1++l3WXswTPcdvPmVxzmRTEE/KpI4PGKiAgvLVVh7rjHLcf1 X-Received: by 10.180.8.68 with SMTP id p4mr55072864wia.27.1441790967718; Wed, 09 Sep 2015 02:29:27 -0700 (PDT) Received: from autoinstall.dev.6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id h8sm2818861wib.21.2015.09.09.02.29.26 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 09 Sep 2015 02:29:27 -0700 (PDT) Date: Wed, 9 Sep 2015 11:29:13 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Marc Sune Message-ID: <20150909092913.GD17463@autoinstall.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability 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, 09 Sep 2015 09:29:28 -0000 bitmap Reply-To: Shern , Adrien Mazarguil Bcc: Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap Reply-To: In-Reply-To: <20150909090855.GC17463@autoinstall.dev.6wind.com> Marc, On Tue, Sep 08, 2015 at 10:24:36PM +0200, Marc Sune wrote: > Neilo, > > 2015-09-08 12:03 GMT+02:00 Nélio Laranjeiro : > > On Mon, Sep 07, 2015 at 10:52:53PM +0200, Marc Sune wrote: > > 2015-08-29 2:16 GMT+02:00 Marc Sune : > > > > > The current rte_eth_dev_info abstraction does not provide any mechanism > to > > > get the supported speed(s) of an ethdev. > > > > > > For some drivers (e.g. ixgbe), an educated guess can be done based on > the > > > driver's name (driver_name in rte_eth_dev_info), see: > > > > > > http://dpdk.org/ml/archives/dev/2013-August/000412.html > > > > > > However, i) doing string comparisons is annoying, and can silently > > > break existing applications if PMDs change their names ii) it does not > > > provide all the supported capabilities of the ethdev iii) for some > drivers > > > it > > > is impossible determine correctly the (max) speed by the application > > > (e.g. in i40, distinguish between XL710 and X710). > > > > > > This small patch adds speed_capa bitmap in rte_eth_dev_info, which is > > > filled > > > by the PMDs according to the physical device capabilities. > > > > > > v2: rebase, converted speed_capa into 32 bits bitmap, fixed alignment > > > (checkpatch). > > > > > > v3: rebase to v2.1. unified ETH_LINK_SPEED and ETH_SPEED_CAP into > > > ETH_SPEED. > > >     Converted field speed in struct rte_eth_conf to speeds, to allow a > > > bitmap > > >     for defining the announced speeds, as suggested by M. Brorup. Fixed > > >     spelling issues. > > > > > > v4: fixed errata in the documentation of field speeds of rte_eth_conf, > and > > >     commit 1/2 message. rebased to v2.1.0. v3 was incorrectly based on > > >     ~2.1.0-rc1. > > > > > > > Thomas, > > > > Since mostly you were commenting for v1 and v2; any opinion on this one? > > > > Regards > > marc > > Hi Marc, > > I have read your patches, and there are a few mistakes, for instance mlx4 > (ConnectX-3 devices) does not support 100Gbps. > > > When I circulated v1 and v2 I was kindly asking maintainers and reviewers of > the drivers to fix any mistakes in SPEED capabilities, since I was taking the > speeds from the online websites&catalogues. Some were fixed, but apparently > some were still missing. I will remove 100Gbps. Please circulate any other > error you have spotted. >>From Mellanox website: - ConnectX-3 EN: 10/40/56Gb/s - ConnectX-3 Pro EN 10GBASE-T: 10G/s - ConnectX-3 Pro: EN 10/40/56GbE - ConnectX-3 Pro Programmable: 10/40Gb/s This PMD works with any of the ConnectX-3 adapters, so the announce speed should be 10/40/56Gb/s. > In addition, it seems your new bitmap does not support all kind of > speeds, take a look at the header of Ethtool, in the Linux kernel > (include/uapi/linux/ethtool.h) which already consumes 30bits without even > managing speeds above 56Gbps. > > > The bitmaps you are referring is SUPPORTED_ and ADVERTISED_. These bitmaps not > only contain the speeds but PHY properties (e.g. BASE for ETH). > > The intention of this patch was to expose speed capabilities, similar to the > bitmap SPEED_ in include/uapi/linux/ethtool.h, which as you see maps closely to > ETH_SPEED_ proposed in this patch. > > I think the encoding of other things, like the exact model of the interface and > its PHY details should go somewhere else. But I might be wrong here, so open to > hear opinions. I understand the need to have capability fields, but I don't understand why you want to mix speeds and duplex mode in something which was previously only handling speeds. We now have redundant information in struct rte_eth_conf, whereas that structure has a speed field which embeds the duplex mode and a duplex field which does the same, which one should be used? > It would be nice to keep the field to represent the real speed of the > link, in case it is not represented by the bitmap, it could be also > useful for aggregated links (bonding for instance).  The current API > already works this way, it just needs to be extended from 16 to 32 bit > to manage speed above 64Gbps. > > > This patch does not remove rte_eth_link_get() API. It just changes the encoding > of speed in struct rte_eth_link, to have an homogeneous set of constants with > the speed capabilities bitmap, as discussed previously in the thread (see > Thomas comments). IOW, it returns now a single SPEED_ value in the struct > rte_eth_link's link_speed field. You change the coding of the speed field, but applications still expect an integer, see port_infos_display function in app/test-pmd/config.c which directly uses printf on rte_eth_link.speed field, there are other places as well in PMDs (bn2x, bond, ...). This patch currently expects that everything uses a bitmap but it is not the case. I don't understand the need to change the rte_eth_link.speed field behavior to have the informations about the capability of the PHY, for this are two distinct things: - capability - speed and duplex negotiated (or not). I suggest to drop the part of the patch which changes the behavior of link_speed in struct rte_eth_link. PS: Sorry I did not sent the email as reply all. -- Nélio Laranjeiro 6WIND