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.