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 14C9FA0524; Sat, 20 Mar 2021 08:54:10 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B7A924067A; Sat, 20 Mar 2021 08:54:08 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id CCDF240395 for ; Sat, 20 Mar 2021 08:54:07 +0100 (CET) Received: from [192.168.1.192] (unknown [188.242.181.57]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 37ABF7F463; Sat, 20 Mar 2021 10:54:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 37ABF7F463 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1616226847; bh=JgCZpGtIdyvDTW1dktSUUm9LKJyOU4sabvtyGghqMcE=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=VrlmCqdN0plD0xBO5RMpdbKxRn4a+BAtTf3L0nesOKaWIxN3DtD1EJZiIAnSzeKZe jefsT4u7WRq7xXoi5AGpHJ43ry3ELTT6Hf4ClsJPu5sgYlnBQ94YwSNU/HvMo5JcS9 hiJytqRnUQEs/1SvrEOUsxKRjdVI4d/5DLYY7b7c= To: Thomas Monjalon 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 References: <20210311221742.3750589-1-thomas@monjalon.net> <2840810.IV2kjoNJqS@thomas> <5237453.CUevj6q2m2@thomas> From: Andrew Rybchenko Message-ID: <64fc7f0f-db64-3307-17aa-eeb42c07b099@oktetlabs.ru> Date: Sat, 20 Mar 2021 10:54:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <5237453.CUevj6q2m2@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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" 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).