From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1E90BA0524; Sat, 20 Mar 2021 11:38:55 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 85A894067A; Sat, 20 Mar 2021 11:38:54 +0100 (CET) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by mails.dpdk.org (Postfix) with ESMTP id 6A08840395 for ; Sat, 20 Mar 2021 11:38:53 +0100 (CET) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id 6924A5807C9; Sat, 20 Mar 2021 06:38:52 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Sat, 20 Mar 2021 06:38:52 -0400 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=fm3; bh= iQQJrlGkQhdzB+VtAJE1Ppm7ZEC+cnSab5qjFvJT2V8=; b=kczAzQuE0LJCnJTr GDC4nXxO87GVSXAs7sT7wbw/dDJHvHB2OndqbWJIWghXqfRDyOsVQhxL1zq66Mm1 2WuuKbymEPtOcfsRGKvsRcAlLfqk7YSHoQQWKEm+m01Nb/H1+Yc2CFB8Q9+li1Tn ZYsY6hb/ROldxYxSjNoRb7Vvcd17iYTE+fkvOFu21ru1GsS0ZnYJ7F2DoXgqaUk9 OL0aDYGkrAdBGFcSvvnmUhbVtio2Q07/0Vs2IC7lc0kpqMZTR90DzWo09r2BxYXQ EZSPh0wVXjQoGJ0CfcygMFCwoCggGfR5L5HeJH8pkfPWXObffZN3tmo2PAi7m6t+ xtt5ZA== 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=fm2; bh=iQQJrlGkQhdzB+VtAJE1Ppm7ZEC+cnSab5qjFvJT2 V8=; b=WOmYpnZVscX6isvzNEG6opdmUrP0OvmrpK4XWT0lCxhxnTawti4QEPr0h UBngZG2UP+GmDdTPPJtI2vFRcpyIbuzYDFN9eQZI7I85vgvHYY8Cr07jmue3nd1y WUkup0dA+gdyFV5HvjwMHD2IGiRvKpZ1xhgfsmvPCMVo6GRq4aFJN/fL/8PwW6ap tOoQfbp8/PTx9w/DuPuVnYCjsJ0pz91fdFrL8WRkARv5EeI9S+1dYPVY+Vd3O+fj vslYYEnFva18bo8iY9LHhObUty72uDI0PawEa2ODk6A69cZd3oC8ZE0DVs4rMvDJ O6aza6HZzCWKT/KpOqsMHo9gw7GAA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrudegtddguddvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecukfhppeejjedrudefgedrvddtfedrudekgeenucevlhhushhtvghruf hiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghl ohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 805421080054; Sat, 20 Mar 2021 06:38:46 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: dev@dpdk.org, Ori Kam , Ajit Khaparde , Somnath Kotur , Chas Williams , "Min Hu (Connor)" , Rahul Lakkireddy , Hemant Agrawal , Sachin Saxena , Jeff Guo , Haiyue Wang , John Daley , Hyong Youb Kim , Gaetan Rivet , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , Yisen Zhuang , Lijun Ou , Beilei Xing , Jingjing Wu , Qiming Yang , Qi Zhang , Rosen Xu , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Liron Himi , Jerin Jacob , Nithin Dabilpuram , Kiran Kumar K , Rasesh Mody , Shahed Shaikh , Jasvinder Singh , Cristian Dumitrescu , Keith Wiles , Jiawen Wu , Jian Wang , Ferruh Yigit Date: Sat, 20 Mar 2021 11:38:44 +0100 Message-ID: <5523516.UNfRd8LfVT@thomas> In-Reply-To: <64fc7f0f-db64-3307-17aa-eeb42c07b099@oktetlabs.ru> References: <20210311221742.3750589-1-thomas@monjalon.net> <5237453.CUevj6q2m2@thomas> <64fc7f0f-db64-3307-17aa-eeb42c07b099@oktetlabs.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: replace callback getting filter operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 20/03/2021 08:54, Andrew Rybchenko: > On 3/19/21 9:12 PM, Thomas Monjalon wrote: > > 15/03/2021 10:15, Thomas Monjalon: > >> 15/03/2021 10:08, Andrew Rybchenko: > >>> On 3/15/21 11:55 AM, Thomas Monjalon wrote: > >>>> 15/03/2021 09:43, Andrew Rybchenko: > >>>>> On 3/15/21 10:54 AM, Thomas Monjalon wrote: > >>>>>> 15/03/2021 08:18, Andrew Rybchenko: > >>>>>>> On 3/12/21 8:46 PM, Thomas Monjalon wrote: > >>>>>>>> --- a/lib/librte_ethdev/rte_flow.c > >>>>>>>> +++ b/lib/librte_ethdev/rte_flow.c > >>>>>>>> @@ -255,18 +255,19 @@ rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error) > >>>>>>>> > >>>>>>>> if (unlikely(!rte_eth_dev_is_valid_port(port_id))) > >>>>>>>> code = ENODEV; > >>>>>>>> - else if (unlikely(!dev->dev_ops->filter_ctrl || > >>>>>>>> - dev->dev_ops->filter_ctrl(dev, > >>>>>>>> - RTE_ETH_FILTER_GENERIC, > >>>>>>>> - RTE_ETH_FILTER_GET, > >>>>>>>> - &ops) || > >>>>>>>> - !ops)) > >>>>>>>> - code = ENOSYS; > >>>>>>>> + else if (unlikely(dev->dev_ops->flow_ops_get == NULL)) > >>>>>>>> + code = ENOTSUP; > >>> > >>> It is described as: > >>> -ENOTSUP: valid but unsupported rule specification (e.g. > >>> partial bit-masks are unsupported). > >>> So, it looks different. May be it is really better to keep > >>> ENOSYS. > >>> > >>>>>>>> else > >>>>>>>> - return ops; > >>>>>>>> - rte_flow_error_set(error, code, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >>>>>>>> - NULL, rte_strerror(code)); > >>>>>>>> - return NULL; > >>>>>>>> + code = dev->dev_ops->flow_ops_get(dev, &ops); > >>>>>>>> + if (code == 0 && ops == NULL) > >>>>>>>> + code = EACCES; > >>>>>>> It looks something new. I think it should be mentioned in flow_ops_get > >>>>>>> type documentation (similar to eth_promiscuous_enable_t) and > >>>>>>> rte_flow_validate() etc functions > >>>>>>> return values description. > >>>>>> > >>>>>> It is an internal function used only in rte_flow.c. > >>>>>> The real consequence is to set rte_errno in a lot of rte_flow API. > >>>>>> Not sure there is a good way to document the code details. > >>>>>> Other codes are not documented in rte_flow.h > >>>>> > >>>>> First of all it is a behaviour of the flow_ops_get callback and > >>>>> driver developers should know that it is a legal to return 0 and > >>>>> ops==NULL and know what it means. > >>>> > >>>> The combination code 0 and ops NULL is not new. > >>>> Previously, it was returning ENOSYS. > >>>> I've just given a more meaningful error code: EACCES, > >>>> while replacing ENOSYS with ENOTSUP for the other case. > >>> > >>> Yes, exactly. What I'm trying to say that it would be > >>> helpful to make it a bit more transparent to PMD developers. > >>> Yes, it was not documented before, I agree. I think it is > >>> a good time to improve documentation. > >>> > >>>>> Second, it is visible as rte_flow_validate() (and other functions > >>>>> which use rte_flow_ops_get()) return value value which has > >>>>> special meaning. So, should be documented. > >>>> > >>>> Yes, I should update the API doc where ENOSYS was mentioned. > >>>> Or probably better: I should keep the error code ENOSYS > >>>> and do not break API. > >>>> Preference? > >>> > >>> Good question. I think we should not distinguish NULL callback > >>> and NULL ops returned by not-NULL callback. So, I think > >>> keeping ENOSYS is the best option here. > >> > >> OK, thank you for the review. > >> So the conclusion is: keep ENOSYS and document NULL ops case. > > > > After more thoughts, I don't think we need to insist on the NULL ops case. > > The function rte_flow_ops_get returns the ops, > > and it is documented that returning NULL is an error. > > So the function is just setting ENOSYS error code to have > > an error code associated with returning NULL. > > For the PMD, returning an ops NULL has no interest. > > > > May be I misunderstand above, but > > One driver may support different NIC types. It could support flow API > for one NIC type and do not support for another. Sometimes it could > be easier to sort it out in flow ops get callback rather than > provide different set of ethdev ops callback. If so, returning NULL > ops makes sense and easier way to say that flow API is not supported > (vs, for example, providing dummy callbacks which return ENOSYS). Yes you're right, returning NULL ops may have an interest. I will document it in eth_flow_ops_get_t.