From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E9239A04FA; Wed, 8 Jan 2020 14:06:23 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5670F1D71C; Wed, 8 Jan 2020 14:06:23 +0100 (CET) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id 61EC91D6DF for <dev@dpdk.org>; Wed, 8 Jan 2020 14:06:22 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id E6B1A217FC; Wed, 8 Jan 2020 08:06:21 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 08 Jan 2020 08:06:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=xe0xq5jOB50Dt9K6mSjNyB0pfBqmyWzyycOlNzn5TlM=; b=Lv2fmGzVaGBC A4xPxYNPgYrJesUqb5hmh+BvjbJNVO8XD2AUtU/xaY8njH+z2/0XCzLUGjKK5WPI DTBHeoIZ1ebFhIiiUfEo7nRIJVP+MzXt3qLubiDCihojnFsQALhc/BHv6ML+B0er FA+v7yTHTbGwq8dKj9RAxDlQXiX+r28= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=xe0xq5jOB50Dt9K6mSjNyB0pfBqmyWzyycOlNzn5T lM=; b=wWAluvs6/c0Qy1L0tA1DAipm/OIpIwkCvK6PbCWgfXXW/ie++z22TvUTp JpwrdaEcdBf416p903df/qw8SJQhTWIkfF+Sg/jbkZDMI8LU/bRNP5Mx0WLJSYSK x9U8GOa5XGvbJnHOQN3sHplF3kK8ve6H1sMM3E6ohuOZz5iEFkKksQ5jt55s/HcJ 5cixpKIGAuLS3B7A3Kd0pQ/fHCtvbdVPZ4rQtrkTcU2Hr1c8iuyvlSsWOkagaAlo pVRbWwr2zMW+aR4IvA96a7rLCQYCcNo26d+n4oXs8VgUUaaTLOOgl4VghfRsqJTL Vdi1gX7uwb1iLYvmWD/ZiPw6crdWw== X-ME-Sender: <xms:zdMVXloIu6s45fVQxdqZBktoPrGeMsFoyaOGvyrJZJec-l6s2mJs1A> X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrvdehkedggeejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtqhertddttddunecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecukf hppeejjedrudefgedrvddtfedrudekgeenucfrrghrrghmpehmrghilhhfrhhomhepthhh ohhmrghssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: <xmx:zdMVXnMBw4bq-sTlxJFjcxxzuaA1OHxULVYcDO_-g4OqzfhU4XhR1w> <xmx:zdMVXopA2VM2AqTXtOvZl24lVMHqeZedsvnd9rEe5qrXcA4dDdHBoQ> <xmx:zdMVXgdFgJDuejtQPya1Xo7su9a8_0JDtJoQ6_NFHwLyaVi2WKIzLg> <xmx:zdMVXqORpKS8t-ow5EoUR4wwu9fMvbDp1y1O3MRGETC_EQmgQabuFA> Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 9B68A8005B; Wed, 8 Jan 2020 08:06:20 -0500 (EST) From: Thomas Monjalon <thomas@monjalon.net> To: Laurent Hardy <laurent.hardy@6wind.com>, David Marchand <david.marchand@redhat.com>, Ferruh Yigit <ferruh.yigit@intel.com> Cc: dev <dev@dpdk.org>, Olivier Matz <olivier.matz@6wind.com>, Andrew Rybchenko <arybchenko@solarflare.com> Date: Wed, 08 Jan 2020 14:06:19 +0100 Message-ID: <1825898.usQuhbGJ8B@xps> In-Reply-To: <779e74a2-7816-216b-fdc2-282bab822af4@intel.com> References: <20200107145637.8922-1-laurent.hardy@6wind.com> <4b86e1d8-9c25-ce7c-f5f4-124c63a7c8b0@6wind.com> <779e74a2-7816-216b-fdc2-282bab822af4@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH] librte_ethdev: extend dpdk api led control to query capability X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> 08/01/2020 13:59, Ferruh Yigit: > On 1/8/2020 10:31 AM, Laurent Hardy wrote: > > Hi all, > >=20 > > On 1/8/20 10:55 AM, David Marchand wrote: > >> On Wed, Jan 8, 2020 at 10:09 AM Ferruh Yigit <ferruh.yigit@intel.com> = wrote: > >>> On 1/8/2020 8:56 AM, David Marchand wrote: > >>>> Hello Laurent, > >>>> > >>>> Bonne ann=E9e. > >>>> > >>>> Cc: maintainers. > >>>> > >>>> On Tue, Jan 7, 2020 at 3:57 PM Laurent Hardy <laurent.hardy@6wind.co= m> wrote: > >>>>> In current led control API we have no way to know if a device is ab= le > >>>>> to handle on/off requests coming from the application. > >>>>> Knowing if the device is led control capable could be useful to avo= id > >>>>> exchanges between application and kernel. > >>>>> Using the on/off requests to flag if the device is led control capa= ble > >>>>> (based on the ENOSUP returned error) is not convenient as such requ= est > >>>>> can change the led state on device. > >>>>> > >>>>> This patch adds a new function rte_eth_led_ctrl_capable() that will= look > >>>>> for led_off/on dev ops availability on the related pmd, to know if = the > >>>>> device is able to handle such led control requests (on/off). > >>>> This patch breaks the ABI, which is BAD :-). > >>> Why it is an ABI break, dev_ops should be between library and drivers= , so it > >>> should be out of the ABI concern, isn't it. > >> You are right. > >> So in our context, this is not an ABI breakage. > >> But abidiff still reports it, so maybe some filtering is required to > >> avoid this false positive. > >> > >> Note that if we insert an ops before rx_queue_count, we would have a > >> real ABI breakage, as this ops is accessed via an inline wrapper by > >> applications. > >> > >> > >>>> This new api only needs to look at the existing ops, so you can remo= ve > >>>> the (unused in your patch) dev_led_ctrl_capable ops. > >>>> > >>>> OTOH, would it make sense to expose this capability in dev_flags? > >>>> > >>> 'rte_eth_led_on()' & 'rte_eth_led_off()' APIs returns '-ENOTSUP' when= the not > >>> supported, can that help application to understand? > >> You might want to know it is supported without changing the state. > >> Laurent? > >=20 > > First, happy new year :) > >=20 > > Yes exactly, the purpose of this patch is to query if the device is led= =20 > > control capable or not without changing the led state. > >=20 > > About exposing the capability through a dev_flags, means to make some=20 > > modification in each pmds. It looks more easy in term of pmds=20 > > maintenance to relying on the rte_eth_led_off()/on() dev ops=20 > > availability at rte_ethdev level, right ? > >=20 >=20 > 'dev_flag' definition is not clear, right now it holds the combination of= status > and capability. And we have 'rte_eth_dev_info' struct, which is again > combination of device capability and status. I agree capabilities in ethdev are a bit of a mess. I would appreciate someone makes a complete audit of it so we can discuss how to improve the situation. > Perhaps we should have explicit capabilities and status fields, even in t= he > rte_device level which inherited by net/crypto devices etc.. No, ethdev capabilities should stay in ethdev. > But for dev_ops, instead of having another capabilities indicator, which > requires PMDs to keep this synchronized, I think it is better if we can s= elf > contain this information within dev_ops, like not implementing dev_ops wo= uld > mean it is not supported, this way it is easier to maintain and less erro= r prone. It means the dev_ops is resetted at init if a device does not support the f= eature. It is against having const dev_ops. > Only we should have it without side effect, >=20 > 1- adding an additional 'dry-run' parameter can work, but this means brea= king > ABI and updating majority of the ethdev APIs :) > 2- Adding 'is_supported' versions of the APIs as we need can be an option= , like > 'rte_eth_led_on_is_supported()' > 3- Olivier's suggestion to add a new API to get the led status, so that t= his > information can be used select led API which won't cause side affect and = let us > learn if it is supported. >=20 > Any other alternatives? >=20 > I would prefer the 2) in above ones, which is very similar to the origina= l patch. The other alternatives are in rte_eth_dev_info and dev_flags.