From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 7E6631B1A4 for ; Wed, 17 Jan 2018 08:56:08 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 3BA618005C; Wed, 17 Jan 2018 07:56:07 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Tue, 16 Jan 2018 23:56:03 -0800 To: Stephen Hemminger , References: <20180116183755.24542-1-stephen@networkplumber.org> From: Andrew Rybchenko Message-ID: <42f4f5a2-008d-c3e8-4c00-ed9ef59065c8@solarflare.com> Date: Wed, 17 Jan 2018 10:56:02 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180116183755.24542-1-stephen@networkplumber.org> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ocex03.SolarFlarecom.com (10.20.40.36) X-MDID: 1516175768-R1761KhS8vnz Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v5 00/15] common ethdev linkstatus functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jan 2018 07:56:08 -0000 On 01/16/2018 09:37 PM, Stephen Hemminger wrote: > While reviewing drivers, noticed a lot of unnecessary > duplication of code in drivers for handling the eth_dev link status > information. While consolidating this, it also became obvious that > some drivers behave differently for no good reason. > > It also was a good chance to introduce atomic exchange primitives > in EAL because there are other places using cmpset where not > necessary (such as bonding). > > Mostly only compile tested only, don't have all of the hardware > available (except ixgbe and virtio) to test. > > Note: the eth_dev_link_update function return value is inconsistent > across drivers. Should be changed to be void. I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now. > > v5 > - checkpatch whitespace cleanup > > v4 > - incorporate review feedback > - rename _rte_linkstatus to rte_linkstatus > - change return value of _rte_linkstatus > - optimize get on 64bit platforms > - change return value of rte_linkstatus_set > > v3 > - align rte_linkstatus_get with rte_atomic64_read > - virtio use ETH_SPEED_NUM_10G > - add net/ > > v2 > - function names changed > - rebased to current master > > Stephen Hemminger (15): > eal: introduce atomic exchange operation > ethdev: add linkstatus get/set helper functions > net/virtio: use eth_linkstatus_set > net/vmxnet3: use rte_eth_linkstatus_set > net/dpaa2: use rte_eth_linkstatus_set > net/nfp: use rte_eth_linkstatus functions > net/e1000: use rte_eth_linkstatus helpers > net/ixgbe: use rte_eth_linkstatus functions > net/sfc: use new rte_eth_linkstatus functions > net/i40e: use rte_eth_linkstatus functions > net/liquidio: use rte_eth_linkstatus_set > net/thunderx: use rte_eth_linkstatus_set > net/szedata: use _rte_eth_linkstatus_set > net/octeontx: use rte_eth_linkstatus_set > net/enic: use rte_eth_linkstatus_set > > drivers/net/dpaa2/dpaa2_ethdev.c | 75 ++--------------- > drivers/net/e1000/em_ethdev.c | 69 ++-------------- > drivers/net/e1000/igb_ethdev.c | 70 ++-------------- > drivers/net/enic/enic_ethdev.c | 5 +- > drivers/net/enic/enic_main.c | 17 ++-- > drivers/net/i40e/i40e_ethdev.c | 43 ++-------- > drivers/net/i40e/i40e_ethdev_vf.c | 18 +--- > drivers/net/ixgbe/ixgbe_ethdev.c | 96 ++++------------------ > drivers/net/liquidio/lio_ethdev.c | 53 ++---------- > drivers/net/nfp/nfp_net.c | 77 ++--------------- > drivers/net/octeontx/octeontx_ethdev.c | 17 +--- > drivers/net/sfc/sfc_ethdev.c | 21 +---- > drivers/net/sfc/sfc_ev.c | 20 +---- > drivers/net/szedata2/rte_eth_szedata2.c | 19 ++--- > drivers/net/thunderx/nicvf_ethdev.c | 18 +--- > drivers/net/virtio/virtio_ethdev.c | 65 +++------------ > drivers/net/vmxnet3/vmxnet3_ethdev.c | 86 ++++--------------- > .../common/include/arch/x86/rte_atomic.h | 24 ++++++ > .../common/include/arch/x86/rte_atomic_32.h | 12 +++ > .../common/include/arch/x86/rte_atomic_64.h | 12 +++ > lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++ > lib/librte_ether/rte_ethdev.c | 22 +---- > lib/librte_ether/rte_ethdev.h | 62 ++++++++++++++ > 23 files changed, 302 insertions(+), 677 deletions(-) >