From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id CA81B7CC4 for ; Thu, 11 Jan 2018 18:01:42 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2018 09:01:42 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,345,1511856000"; d="scan'208";a="9386999" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by fmsmga002.fm.intel.com with ESMTP; 11 Jan 2018 09:01:40 -0800 To: Stephen Hemminger , Andrew Rybchenko Cc: dev@dpdk.org References: <20180108174514.14688-1-stephen@networkplumber.org> <20180108174514.14688-3-stephen@networkplumber.org> <20180109082902.475cb461@xeon-e3> From: Ferruh Yigit Message-ID: <77fb3f8f-ab2f-a038-69e1-032878cdf782@intel.com> Date: Thu, 11 Jan 2018 17:01:40 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180109082902.475cb461@xeon-e3> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 02/15] ethdev: add linkstatus get/set helper 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: Thu, 11 Jan 2018 17:01:43 -0000 On 1/9/2018 4:29 PM, Stephen Hemminger wrote: > On Tue, 9 Jan 2018 13:30:46 +0300 > Andrew Rybchenko wrote: > >> On 01/08/2018 08:45 PM, Stephen Hemminger wrote: >>> Many drivers are all doing copy/paste of the same code to atomically >>> update the link status. Reduce duplication, and allow for future >>> changes by having common function for this. >>> >>> Signed-off-by: Stephen Hemminger <...> >>> +int >>> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev, >>> + const struct rte_eth_link *new_link) I see "_rte" prefix is used to say this is internal only but this is not the syntax for other internal APIs, only there is one using this syntax, so may be confusing to have a mix of them. "@internal" marker in function comment already saying that this is internal. Also I send a patch to separate public APIs and PMD APIs to different files, hopefully that will also help clarifying target of the APIs. And although these two new APIs are internal, they need to be in .map file for shared library case. >>> +{ >>> + volatile uint64_t *dev_link >>> + = (volatile uint64_t *)&(dev->data->dev_link); >>> + union { >>> + uint64_t val64; >>> + struct rte_eth_link link; >>> + } orig; >>> + >>> + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t)); >>> + >>> + orig.val64 = rte_atomic64_exchange(dev_link, >>> + *(const uint64_t *)new_link); >>> + >>> + return (orig.link.link_status == new_link->link_status) ? -1 : 0; >> >> It still contradicts to: -1 if link state has changed, 0 if the same. +1, function documentation in header file and this conflicts, one needs fixing. >> BTW, it looks like the return value of the link_update callback is not >> specified (described) and not used. It explains why different drivers >> behave differently and nobody notices it. > > It looks like link_status callback could be void. > The only places that use return value from set are code that wants > to log something if status changed. When a set function returns -1/0, it looks like it returns set status. Returning if link status changes seems odd to me. What about returning original link struct, so PMD may get this information if they want.