| Message ID | CAExC=0TPWX6v2pk6hO+vARS0_nH1FE3iSaiD0wVnnJ8Nm2J3fw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 991D75902; Fri, 25 Mar 2016 22:31:13 +0100 (CET) Received: from mail-lf0-f67.google.com (mail-lf0-f67.google.com [209.85.215.67]) by dpdk.org (Postfix) with ESMTP id 4B42C568E for <dev@dpdk.org>; Fri, 25 Mar 2016 22:31:12 +0100 (CET) Received: by mail-lf0-f67.google.com with SMTP id i75so6718863lfb.1 for <dev@dpdk.org>; Fri, 25 Mar 2016 14:31:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Or0IRq3AhB1cj9gAUIpX+8gIBRkP1+XoHNntcbY6h2E=; b=Rj4IWX7pVSo3H7CwWYdxJcObcTuopoOjhu+ctySmYrfze+qhmew6mh59APX+Eao3H9 2r9nAsmDHwMRcpBixLwla8d1xWi1R5X7b7yF4vzXermTcgxMvkmM1onPZYBzgpCSfitk GsrpVdazFJMskg5d/UAnbY2h4OzdlM6ay7f/8Y9rG4OVVOxogafNsZZD/cMvjIxFK8EO bTTCsgDCRvxc+BUPlklvB5laTMgROjfYIJh8AwqZwzn2LMsuszxLJWKJPmRUYYRl5YBe Fo1YphR5sjkULu5z8MoQ+Dheg6fH2VbBfv8eqYpIZ4JsXdqTs7ymcgFjSqOwaTEIQkRO pFgA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=Or0IRq3AhB1cj9gAUIpX+8gIBRkP1+XoHNntcbY6h2E=; b=UYK9MZfPmdJVkwmzxC5EWhgRejF6I+3W6rOi+r9FLUzwsrx4Idxv7b++HMmjxNZzFO njihOdENK9F8dRh0NS4wLLRKR57sk/5GpM72ycVpnpPpxdFZoe808fR+g5sPCBHLwHW8 Nk7+U/MyKwDdYy7W4ifjz/8tbE9xGolZMn3UEhRjrENXOFL5eYCxz2rerhWqC/G7/itc rA5p9jwcX6TI5MFWYqHRKdcdPsH+5EyMTzG3B6SmgUObVk8zOa8liCuxbbeaCk14hjNL pLc8HArG1gEN5MCChCEPG3ZJ+XgKEFHUcXOlrNmXAi0lk2Rs+0K5HuNCFMwfSNKOpKs/ O3Eg== X-Gm-Message-State: AD7BkJJ+kQVPCCuQS2FRW6U5QtsaF8qHwi8ya2CWqRhR6kKrU8zcjOQaiKk1ahwm9mrdqrLITDu7VGJPSWWT6w== X-Received: by 10.25.152.205 with SMTP id a196mr6659213lfe.85.1458941471681; Fri, 25 Mar 2016 14:31:11 -0700 (PDT) MIME-Version: 1.0 Received: by 10.112.155.196 with HTTP; Fri, 25 Mar 2016 14:30:51 -0700 (PDT) In-Reply-To: <CAExC=0RTdzC5pzuumn3-zEYFopb2pE2oaL3p39sz7HRnhcZuEg@mail.gmail.com> References: <1457992546-32230-1-git-send-email-thomas.monjalon@6wind.com> <CAExC=0RfZmggUor3fU_HD8kjsmGW-XUY0=NMv3cVkO5c9BxNLw@mail.gmail.com> <82F45D86ADE5454A95A89742C8D1410E032113DB@shsmsx102.ccr.corp.intel.com> <3250488.B81B9g3x6N@xps13> <F35DEAC7BCE34641BA9FAC6BCA4A12E70A9D1A28@SHSMSX104.ccr.corp.intel.com> <CAExC=0RTdzC5pzuumn3-zEYFopb2pE2oaL3p39sz7HRnhcZuEg@mail.gmail.com> From: Marc <marcdevel@gmail.com> Date: Fri, 25 Mar 2016 22:30:51 +0100 X-Google-Sender-Auth: MYzXpiuV6Nq3C6NumLcKA3RRFoY Message-ID: <CAExC=0TPWX6v2pk6hO+vARS0_nH1FE3iSaiD0wVnnJ8Nm2J3fw@mail.gmail.com> To: "Zhang, Helin" <helin.zhang@intel.com> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>, "Xu, Qian Q" <qian.q.xu@intel.com>, "Xing, Beilei" <beilei.xing@intel.com>, "dev@dpdk.org" <dev@dpdk.org>, "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Richardson, Bruce" <bruce.richardson@intel.com>, "Glynn, Michael J" <michael.j.glynn@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed API refactoring X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Marc Sune
March 25, 2016, 9:30 p.m. UTC
On 25 March 2016 at 21:41, Marc <marcdevel@gmail.com> wrote: > > On 25 March 2016 at 16:07, Zhang, Helin <helin.zhang@intel.com> wrote: > >> Hi Thomas >> >> Beilei is investigating that, she will give her findings soon later, and >> possibly a fix after validating that. >> Thanks! >> >> > I will try to reproduce this on my side too with the latest v12. I could > not try latest patchsets, but i40 (XL710) and igb (82540EM) were working on > my side for previous versions. Which exact NICs were used to test the > patchset for igb? > I am able to reproduce it straight away by applying v12. The problem is testpmd and in general existing applications have the default value of 0 as link_speeds for autoneg. From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: marc@Beluga:~/personal/dpdk/tools$ git diff 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 */ I think having autoneg == 0 is better. Do you agree Thomas? With this change my current NIC (I218-LM) is able to initialize: Option: 27 Enter hex bitmask of cores to execute testpmd app on Example: to execute app on cores 0 to 7, enter 0xff bitmask: 0x3 Launching app EAL: Detected lcore 0 as core 0 on socket 0 EAL: Detected lcore 1 as core 0 on socket 0 EAL: Detected lcore 2 as core 1 on socket 0 EAL: Detected lcore 3 as core 1 on socket 0 EAL: Support maximum 128 logical core(s) by configuration. EAL: Detected 4 lcore(s) EAL: Probing VFIO support... EAL: Module /sys/module/vfio_pci not found! error 2 (No such file or directory) EAL: VFIO modules not loaded, skipping VFIO support... EAL: Setting up physically contiguous memory... EAL: Ask a virtual area of 0x26800000 bytes EAL: Virtual area found at 0x7f33ef800000 (size = 0x26800000) EAL: Ask a virtual area of 0x6e00000 bytes EAL: Virtual area found at 0x7f33e8800000 (size = 0x6e00000) EAL: Ask a virtual area of 0x800000 bytes EAL: Virtual area found at 0x7f33e7e00000 (size = 0x800000) EAL: Ask a virtual area of 0x4400000 bytes EAL: Virtual area found at 0x7f33e3800000 (size = 0x4400000) EAL: Ask a virtual area of 0xe00000 bytes EAL: Virtual area found at 0x7f33e2800000 (size = 0xe00000) EAL: Ask a virtual area of 0x600000 bytes EAL: Virtual area found at 0x7f33e2000000 (size = 0x600000) EAL: Ask a virtual area of 0x200000 bytes EAL: Virtual area found at 0x7f33e1c00000 (size = 0x200000) EAL: Ask a virtual area of 0x43600000 bytes EAL: Virtual area found at 0x7f339e400000 (size = 0x43600000) EAL: Ask a virtual area of 0x8e00000 bytes EAL: Virtual area found at 0x7f3395400000 (size = 0x8e00000) EAL: Ask a virtual area of 0x200000 bytes EAL: Virtual area found at 0x7f3395000000 (size = 0x200000) EAL: Ask a virtual area of 0x200000 bytes EAL: Virtual area found at 0x7f3394c00000 (size = 0x200000) EAL: Requesting 1024 pages of size 2MB from socket 0 EAL: TSC frequency is ~2593996 KHz EAL: Master lcore 0 is ready (tid=180078c0;cpuset=[0]) EAL: lcore 1 is ready (tid=94bff700;cpuset=[1]) EAL: PCI device 0000:00:19.0 on NUMA socket -1 EAL: probe driver: 8086:15a2 rte_em_pmd EAL: PCI memory mapped at 0x7f3416000000 EAL: PCI memory mapped at 0x7f3416020000 PMD: eth_em_dev_init(): port_id 0 vendorID=0x8086 deviceID=0x15a2 Interactive-mode selected Configuring Port 0 (socket 0) PMD: eth_em_tx_queue_setup(): sw_ring=0x7f33e210efc0 hw_ring=0x7f33e21110c0 dma_addr=0x745110c0 PMD: eth_em_rx_queue_setup(): sw_ring=0x7f33e20fea80 hw_ring=0x7f33e20fef80 dma_addr=0x744fef80 PMD: eth_em_start(): << I am troubleshooting link status reporting, which seems not correct with l2fwd. I will also double check that fixed speed and autoneg with subset of speeds work. @Thomas: once I've fixed this shall I submit v13 or should we wait for more feedback from the rest of untested NICs? This patchset needs to be tested by all drivers, at least. marc > Marc > > >> Regards, >> Helin >> >> > -----Original Message----- >> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] >> > Sent: Friday, March 25, 2016 5:36 PM >> > To: Xu, Qian Q <qian.q.xu@intel.com> >> > Cc: dev@dpdk.org; Marc <marcdevel@gmail.com>; Ananyev, Konstantin >> > <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; >> > Zhang, Helin <helin.zhang@intel.com>; Richardson, Bruce >> > <bruce.richardson@intel.com>; Glynn, Michael J < >> michael.j.glynn@intel.com> >> > Subject: Re: [dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed API >> > refactoring >> > >> > Is there someone investigating the issue? >> > I think it should be simple to fix for someone mastering these Intel >> drivers. >> > >> > 2016-03-25 01:02, Xu, Qian Q: >> > > Marc >> > > #Test1 is just a simple test. Just launch testpmd with these nic port. >> > > ./testpmd –c 0x3 –n 4 -- -i >> > > >> > > Thanks >> > > Qian >> > > >> > > From: marc.sune@gmail.com [mailto:marc.sune@gmail.com] On Behalf Of >> > > Marc >> > > Sent: Thursday, March 24, 2016 3:48 PM >> > > To: Xu, Qian Q >> > > Cc: Thomas Monjalon; Ananyev, Konstantin; Lu, Wenzhuo; Zhang, Helin; >> > > Richardson, Bruce; dev@dpdk.org >> > > Subject: Re: [dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed >> > > API refactoring >> > > >> > > >> > > >> > > On 24 March 2016 at 07:21, Xu, Qian Q >> > <qian.q.xu@intel.com<mailto:qian.q.xu@intel.com>> wrote: >> > > Marc >> > > I didn’t quite get your points, I observed that after applying this >> patchset, all >> > intel nic can’t be started, maybe something wrong happened when you >> check >> > the duplex/autoneg value for different NICs. If we want to merge the >> patchset in >> > RC2, we need fix them. Maybe not an easy job in several days. >> > > >> > > Is this test#1 one of the tests contained in the DPDK repository or >> is it an >> > internal test? >> > > >> > > Marc >> > > >> > > >> > > >> > > Thanks >> > > Qian >> > > >> > > From: marc.sune@gmail.com<mailto:marc.sune@gmail.com> >> > > [mailto:marc.sune@gmail.com<mailto:marc.sune@gmail.com>] On Behalf Of >> > > Marc >> > > Sent: Thursday, March 24, 2016 4:54 AM >> > > To: Xu, Qian Q >> > > Cc: Thomas Monjalon; Ananyev, Konstantin; Lu, Wenzhuo; Zhang, Helin; >> > > Richardson, Bruce; dev@dpdk.org<mailto:dev@dpdk.org> >> > > >> > > Subject: Re: [dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed >> > > API refactoring >> > > >> > > Qian, >> > > >> > > On 23 March 2016 at 02:18, Xu, Qian Q >> > <qian.q.xu@intel.com<mailto:qian.q.xu@intel.com>> wrote: >> > > We have tested with intel nic and found port can't be started for all >> > nics:ixgbe/i40e/igb/bonding, see attached mail for more details. Please >> check >> > and fix it. >> > > >> > > >> > > Thanks >> > > Qian >> > > >> > > -----Original Message----- >> > > From: dev [mailto:dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>] >> > > On Behalf Of Thomas Monjalon >> > > Sent: Wednesday, March 23, 2016 3:59 AM >> > > To: Ananyev, Konstantin; Lu, Wenzhuo; Zhang, Helin >> > > Cc: marcdevel@gmail.com<mailto:marcdevel@gmail.com>; Richardson, >> > > Bruce; dev@dpdk.org<mailto:dev@dpdk.org> >> > > Subject: Re: [dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed >> > > API refactoring >> > > >> > > 2016-03-17 19:08, Thomas Monjalon: >> > > > There are still too few tests and reviews, especially for >> > > > autonegotiation with Intel devices (patch #6). >> > > > I would not be surprised to see some bugs in this rework. >> > > >> > > Any feedback about autoneg in e1000/ixgbe/i40e? >> > > Has it been tested before its integration in RC2? >> > > >> > > > The capabilities must be adapted per device. It can be improved in a >> > > > separate patch. >> > > > >> > > > It will be integrated in 16.04-rc2. >> > > > Please test and review shortly, thanks! >> > > >> > > >> > > ---------- Forwarded message ---------- >> > > From: "Xu, Qian Q" <qian.q.xu@intel.com<mailto:qian.q.xu@intel.com>> >> > > To: "Cao, Waterman" >> > > <waterman.cao@intel.com<mailto:waterman.cao@intel.com>>, "Glynn, >> > > Michael J" >> > > <michael.j.glynn@intel.com<mailto:michael.j.glynn@intel.com>> >> > > Cc: "Richardson, Bruce" >> > > <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>, >> "Zhu, >> > > Heqing" <heqing.zhu@intel.com<mailto:heqing.zhu@intel.com>>, >> > > "O'Driscoll, Tim" >> > > <tim.odriscoll@intel.com<mailto:tim.odriscoll@intel.com>>, "Mcnamara, >> > > John" <john.mcnamara@intel.com<mailto:john.mcnamara@intel.com>>, "Xu, >> > > HuilongX" <huilongx.xu@intel.com<mailto:huilongx.xu@intel.com>>, "Fu, >> > > JingguoX" <jingguox.fu@intel.com<mailto:jingguox.fu@intel.com>>, "Xu, >> > > Qian Q" <qian.q.xu@intel.com<mailto:qian.q.xu@intel.com>>, "Zhang, >> > > Helin" <helin.zhang@intel.com<mailto:helin.zhang@intel.com>> >> > > Date: Tue, 22 Mar 2016 06:41:37 +0000 >> > > Subject: RE: DPDK link speed with Intel devices Hi, all We have worked >> > > out the basic test cases for the patchset. >> > > 1. Test the link speed on major Intel NICs to see if the speed is >> right. >> > > 2. Test the auto-negoation on major Intel NICs to ensure it's working. >> > > Nic covered: ixgbe, igb, i40e, fm10k, bonding(SW), virtio(SW) >> > > >> > > When we run the Test#1 for all major NICs. We found that all these >> NIC port(igb, >> > ixgbe, i40e, fm10k) can't be started. Pls check, if the patch is >> applied, all INTEL >> > port can't be start, terrible things! >> > > >> > > Interactive-mode selected >> > > Configuring Port 0 (socket 0) >> > > PMD: ixgbe_dev_tx_queue_setup(): sw_ring=0x7f13e99e3440 >> > > hw_ring=0x7f13e99e5480 dma_addr=0x8299e5480 >> > > PMD: ixgbe_set_tx_function(): Using simple tx code path >> > > PMD: ixgbe_set_tx_function(): Vector tx enabled. >> > > PMD: ixgbe_dev_rx_queue_setup(): sw_ring=0x7f13ffcb8080 >> > > sw_sc_ring=0x7f13ffcbaac0 hw_ring=0x7f13e99d3380 dma_addr=0x8299d3380 >> > > PMD: ixgbe_dev_start(): Invalid link_speeds for port 0; >> > > autonegotiation disabled Fail to start port 0 Configuring Port 1 >> > > (socket 0) >> > > PMD: i40e_set_tx_function_flag(): Vector tx can be enabled on this >> txq. >> > > PMD: i40e_dev_rx_queue_setup(): Rx Burst Bulk Alloc Preconditions are >> > satisfied. Rx Burst Bulk Alloc function will be used on port=1, queue=0. >> > > PMD: i40e_dev_start(): Invalid link_speeds for port 1; autonegotiation >> > > disabled >> > > >> > > >> > > Just to double-check; is the test#1 adapted to the _new_ API that >> ethdev has >> > to set link speeds? For the output it seems autoneg is disabled => >> fixed speed, >> > hence the new bitmaps have to be used. >> > > >> > > (I am not claiming patchset is bug free; there might be issues still) >> > > >> > > Regards >> > > marc >> > > >> > > Fail to start port 1 >> > > Please stop the ports first >> > > Done >> > > >> > > Thanks >> > > Qian >> > > >> > > >> > > -----Original Message----- >> > > From: Cao, Waterman >> > > Sent: Tuesday, March 22, 2016 11:06 AM >> > > To: Glynn, Michael J >> > > Cc: Richardson, Bruce; Zhu, Heqing; O'Driscoll, Tim; Mcnamara, John; >> > > Xu, Qian Q; Cao, Waterman >> > > Subject: RE: DPDK link speed with Intel devices >> > > >> > > Hi Mike, >> > > >> > > We just knew this patch set last week. >> > > Since this patch set is required to test with a lot of NIC, >> we need >> > more document from Dev about this patch. >> > > Currently, Qian is working on with Wenzhuo on it now. >> > > >> > > Waterman >> > > >> > > >> > > -----Original Message----- >> > > From: Glynn, Michael J >> > > Sent: Tuesday, March 22, 2016 1:31 AM >> > > To: Cao, Waterman >> > > <waterman.cao@intel.com<mailto:waterman.cao@intel.com>> >> > > Cc: Richardson, Bruce >> > > <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>; Zhu, >> > > Heqing <heqing.zhu@intel.com<mailto:heqing.zhu@intel.com>>; >> > > O'Driscoll, Tim >> > > <tim.odriscoll@intel.com<mailto:tim.odriscoll@intel.com>>; Mcnamara, >> > > John <john.mcnamara@intel.com<mailto:john.mcnamara@intel.com>> >> > > Subject: FW: DPDK link speed with Intel devices >> > > Importance: High >> > > >> > > Hi Waterman, all >> > > >> > > See below - are you aware? And if so where are we with >> testing/resolution? >> > > >> > > Regards >> > > Mike >> > > >> > > >> > > >> > > -----Original Message----- >> > > From: Thomas Monjalon >> > > >> > [mailto:thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>] >> > > Sent: Monday, March 21, 2016 2:19 PM >> > > To: O'Driscoll, Tim >> > > <tim.odriscoll@intel.com<mailto:tim.odriscoll@intel.com>>; Glynn, >> > > Michael J >> > > <michael.j.glynn@intel.com<mailto:michael.j.glynn@intel.com>>; Zhu, >> > > Heqing <heqing.zhu@intel.com<mailto:heqing.zhu@intel.com>> >> > > Cc: vincent.jardin@6wind.com<mailto:vincent.jardin@6wind.com> >> > > Subject: DPDK link speed with Intel devices >> > > >> > > Hi, >> > > >> > > We are still waiting for test feedbacks for this important patchset: >> > > ethdev: 100G and link speed API refactoring It is possible >> that it >> > breaks the autonegotiation in e1000/ixgbe/i40e. >> > > >> > > Thanks for taking care. >> > > >> > > >> > >> >> >
Comments
Hi Marc, Thanks for finding time to help. 2016-03-25 22:30, Marc: > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: > > marc@Beluga:~/personal/dpdk/tools$ git diff > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > 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 speed) */ > -#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (all speeds) */ > +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */ > +#define ETH_LINK_SPEED_FIXED (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 */ > > 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. Would it be possible to fix without reverting?
On 26 March 2016 at 09:08, Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > Hi Marc, > > Thanks for finding time to help. > > 2016-03-25 22:30, Marc: > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: > > > > marc@Beluga:~/personal/dpdk/tools$ git diff > > diff --git a/lib/librte_ether/rte_ethdev.h > b/lib/librte_ether/rte_ethdev.h > > 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 > speed) */ > > -#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (all > speeds) */ > > +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all > speeds) */ > > +#define ETH_LINK_SPEED_FIXED (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 */ > > > > 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
2016-03-26 11:24, Marc: > On 26 March 2016 at 09:08, Thomas Monjalon <thomas.monjalon@6wind.com> > wrote: > > 2016-03-25 22:30, Marc: > > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and > > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: > > > > > > marc@Beluga:~/personal/dpdk/tools$ git diff > > > diff --git a/lib/librte_ether/rte_ethdev.h > > b/lib/librte_ether/rte_ethdev.h > > > 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 > > speed) */ > > > -#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (all > > speeds) */ > > > +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all > > speeds) */ > > > +#define ETH_LINK_SPEED_FIXED (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 */ > > > > > > 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. > 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?
On 27 March 2016 at 11:53, Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > 2016-03-26 11:24, Marc: > > On 26 March 2016 at 09:08, Thomas Monjalon <thomas.monjalon@6wind.com> > > wrote: > > > 2016-03-25 22:30, Marc: > > > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and > > > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: > > > > > > > > marc@Beluga:~/personal/dpdk/tools$ git diff > > > > diff --git a/lib/librte_ether/rte_ethdev.h > > > b/lib/librte_ether/rte_ethdev.h > > > > 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 > > > speed) */ > > > > -#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (all > > > speeds) */ > > > > +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all > > > speeds) */ > > > > +#define ETH_LINK_SPEED_FIXED (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 */ > > > > > > > > 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. > > > 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
Hi Marc, 2016-03-27 21:39, Marc: > On 27 March 2016 at 11:53, Thomas Monjalon <thomas.monjalon@6wind.com> > wrote: > > 2016-03-26 11:24, Marc: > > > On 26 March 2016 at 09:08, Thomas Monjalon <thomas.monjalon@6wind.com> > > > wrote: > > > > 2016-03-25 22:30, Marc: > > > > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and > > > > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: [...] > > > > > 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. > > > > > 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. It would be better to make the v14 as soon as possible to let others test the latest revision. Thanks
On 28 March 2016 at 15:42, Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > Hi Marc, > > 2016-03-27 21:39, Marc: > > On 27 March 2016 at 11:53, Thomas Monjalon <thomas.monjalon@6wind.com> > > wrote: > > > 2016-03-26 11:24, Marc: > > > > On 26 March 2016 at 09:08, Thomas Monjalon < > thomas.monjalon@6wind.com> > > > > wrote: > > > > > 2016-03-25 22:30, Marc: > > > > > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and > > > > > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work: > [...] > > > > > > 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. > > > > > > > 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. > > It would be better to make the v14 as soon as possible to let others test > the latest revision. > Thanks > Adding these MACROs (ETH_LINK_FIXED, ETH_LINK_AUTONEG) are the only changes to be done for v14 so far. The rest were there already (v13). There is no point on doing this re-spin now, because these MACROs are anyway not used by applications yet (autoneg flag in rte_link is an addition to this patch set). So I would go for testing drivers on this v13 and collect any changes, eventually, for v14. marc
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h 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 speed) */ -#define ETH_LINK_SPEED_AUTONEG (1 << 0) /**< Autonegotiate (all speeds) */ +#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */ +#define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed