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 79746A0524; Fri, 19 Mar 2021 19:13:12 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E1E8B14101E; Fri, 19 Mar 2021 19:13:11 +0100 (CET) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by mails.dpdk.org (Postfix) with ESMTP id 5218740143 for ; Fri, 19 Mar 2021 19:13:10 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id A003F580787; Fri, 19 Mar 2021 14:13:08 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Fri, 19 Mar 2021 14:13:08 -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= 7JOQUYxSTbamyA8zyuVVXUnyCZcLUzjvDpeLiyWMx68=; b=n8DKwYXhnKNh5KER cu1OU/18T6aQSVi0rZOooy+OqyW7bZoymrv/ImSY6wqgiLGK/ZDRd7JXk4bGvC1+ defjz0i+VoXENKOQl0mv5OjJ8nzQmjt13O4i7hZHmN0xbO7WM/fcFtTlHWADznR7 2Z4a2sxP7U4BF+TT7jBgzQKFaPjiw9N8a4PmpVS7jWnG7zCy4AJtvkmjEJvEwbxE Dr9VPTQEkPWY7V6olmjgUkqEs4lnAAulEKZslDsMpZYBrg+jL6zYDWcV7jXKNYWn yV6riNHj258huZiGgkzi+Tdu6WRlAzYnwdqGFhKPm9+kcwMRMyoyhDV0ScZG7lmR yGn/8Q== 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=7JOQUYxSTbamyA8zyuVVXUnyCZcLUzjvDpeLiyWMx 68=; b=nB+0To2mt3FTkvJsY60STxDp5IiLCQL15j0lCvQ0ksU+U/jbFaGGaIsMN QE0ffQ+CX6WxnG+TCMRhh1xoSvBi/T7SA88vVcVRCtTPxd+w411IgR589SWS6jJi D8IkQgWGydoACD4PBuNv7+90bamqCQLBmK9VCT9ybKVzBAiI/ECScyVd2c8kItsU 1EKAj+hixDIJm0MhjETCBNXi9qaSzPRJipeBFKzyQDpZhz3hYCxOXKCDW/NNm6dq xzGZsL9CGGrwKy044ck7N1ELp+QakkWGNgt7DLRlakNVAZbLD0a1q2pdMMjJhuRV mb/rQWkOzHv+CPQ0ZmRAnXsK4Kt/Q== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrudefkedgudduudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdej ueeiiedvffegheenucfkphepjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrh fuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgr lhhonhdrnhgvth 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 A60DA240067; Fri, 19 Mar 2021 14:13:01 -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 , Thomas Monjalon Date: Fri, 19 Mar 2021 19:12:59 +0100 Message-ID: <5237453.CUevj6q2m2@thomas> In-Reply-To: <2840810.IV2kjoNJqS@thomas> References: <20210311221742.3750589-1-thomas@monjalon.net> <2840810.IV2kjoNJqS@thomas> 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" 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.