DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: dev@dpdk.org, Ori Kam <orika@nvidia.com>,
	Ajit Khaparde <ajit.khaparde@broadcom.com>,
	Somnath Kotur <somnath.kotur@broadcom.com>,
	Chas Williams <chas3@att.com>,
	"Min Hu (Connor)" <humin29@huawei.com>,
	Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>,
	Sachin Saxena <sachin.saxena@oss.nxp.com>,
	Jeff Guo <jia.guo@intel.com>, Haiyue Wang <haiyue.wang@intel.com>,
	John Daley <johndale@cisco.com>,
	Hyong Youb Kim <hyonkim@cisco.com>, Gaetan Rivet <grive@u256.net>,
	Ziyang Xuan <xuanziyang2@huawei.com>,
	Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>,
	Guoyang Zhou <zhouguoyang@huawei.com>,
	Yisen Zhuang <yisen.zhuang@huawei.com>,
	Lijun Ou <oulijun@huawei.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Jingjing Wu <jingjing.wu@intel.com>,
	Qiming Yang <qiming.yang@intel.com>,
	Qi Zhang <qi.z.zhang@intel.com>, Rosen Xu <rosen.xu@intel.com>,
	Matan Azrad <matan@nvidia.com>,
	Shahaf Shuler <shahafs@nvidia.com>,
	Viacheslav Ovsiienko <viacheslavo@nvidia.com>,
	Liron Himi <lironh@marvell.com>, Jerin Jacob <jerinj@marvell.com>,
	Nithin Dabilpuram <ndabilpuram@marvell.com>,
	Kiran Kumar K <kirankumark@marvell.com>,
	Rasesh Mody <rmody@marvell.com>,
	Shahed Shaikh <shshaikh@marvell.com>,
	Jasvinder Singh <jasvinder.singh@intel.com>,
	Cristian Dumitrescu <cristian.dumitrescu@intel.com>,
	Keith Wiles <keith.wiles@intel.com>,
	Jiawen Wu <jiawenwu@trustnetic.com>,
	Jian Wang <jianwang@trustnetic.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: replace callback getting filter operations
Date: Fri, 19 Mar 2021 19:12:59 +0100	[thread overview]
Message-ID: <5237453.CUevj6q2m2@thomas> (raw)
In-Reply-To: <2840810.IV2kjoNJqS@thomas>

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.



  reply	other threads:[~2021-03-19 18:13 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11 22:17 [dpdk-dev] [PATCH 0/2] ethdev: remove some use of legacy filtering interface Thomas Monjalon
2021-03-11 22:17 ` [dpdk-dev] [PATCH 1/2] ethdev: replace callback getting filter operations Thomas Monjalon
2021-03-12  1:44   ` Wang, Haiyue
2021-03-12  8:22     ` Thomas Monjalon
2021-03-12  8:25       ` Andrew Rybchenko
2021-03-12  8:40         ` Wang, Haiyue
2021-03-12  7:09   ` Andrew Rybchenko
2021-03-12  8:26     ` Thomas Monjalon
2021-03-11 22:17 ` [dpdk-dev] [PATCH 2/2] drivers/net: remove explicit include of legacy filtering Thomas Monjalon
2021-03-12  1:21   ` Xu, Rosen
2021-03-12 17:46 ` [dpdk-dev] [PATCH v2 0/2] ethdev: remove some use of legacy filtering interface Thomas Monjalon
2021-03-12 17:46   ` [dpdk-dev] [PATCH v2 1/2] ethdev: replace callback getting filter operations Thomas Monjalon
2021-03-12 19:14     ` Ajit Khaparde
2021-03-13  4:16     ` Wang, Haiyue
2021-03-15  3:05     ` Xu, Rosen
2021-03-15  7:18     ` Andrew Rybchenko
2021-03-15  7:54       ` Thomas Monjalon
2021-03-15  8:43         ` Andrew Rybchenko
2021-03-15  8:55           ` Thomas Monjalon
2021-03-15  9:08             ` Andrew Rybchenko
2021-03-15  9:15               ` Thomas Monjalon
2021-03-19 18:12                 ` Thomas Monjalon [this message]
2021-03-20  7:54                   ` Andrew Rybchenko
2021-03-20 10:38                     ` Thomas Monjalon
2021-03-15  7:22     ` Hemant Agrawal
2021-03-12 17:46   ` [dpdk-dev] [PATCH v2 2/2] drivers/net: remove explicit include of legacy filtering Thomas Monjalon
2021-03-15  7:19     ` Hemant Agrawal
2021-03-21  8:59 ` [dpdk-dev] [PATCH v3 0/2] ethdev: remove some use " Thomas Monjalon
2021-03-21  9:00   ` [dpdk-dev] [PATCH v3 1/2] ethdev: replace callback getting filter operations Thomas Monjalon
2021-03-21  9:08     ` Andrew Rybchenko
2021-03-24 18:05     ` Ferruh Yigit
2021-03-26 17:41       ` Ferruh Yigit
2021-03-29 20:56     ` Matan Azrad
2021-03-21  9:00   ` [dpdk-dev] [PATCH v3 2/2] drivers/net: remove explicit include of legacy filtering Thomas Monjalon
2021-03-24 18:08     ` Ferruh Yigit
2021-03-24 20:00       ` Thomas Monjalon
2021-03-25  5:53         ` Andrew Rybchenko
2021-03-25 10:00           ` Ferruh Yigit
2021-03-25 10:20             ` Thomas Monjalon
2021-03-26 15:37               ` Ferruh Yigit
2021-03-26 21:45                 ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5237453.CUevj6q2m2@thomas \
    --to=thomas@monjalon.net \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=beilei.xing@intel.com \
    --cc=chas3@att.com \
    --cc=cloud.wangxiaoyun@huawei.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=grive@u256.net \
    --cc=haiyue.wang@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=humin29@huawei.com \
    --cc=hyonkim@cisco.com \
    --cc=jasvinder.singh@intel.com \
    --cc=jerinj@marvell.com \
    --cc=jia.guo@intel.com \
    --cc=jianwang@trustnetic.com \
    --cc=jiawenwu@trustnetic.com \
    --cc=jingjing.wu@intel.com \
    --cc=johndale@cisco.com \
    --cc=keith.wiles@intel.com \
    --cc=kirankumark@marvell.com \
    --cc=lironh@marvell.com \
    --cc=matan@nvidia.com \
    --cc=ndabilpuram@marvell.com \
    --cc=orika@nvidia.com \
    --cc=oulijun@huawei.com \
    --cc=qi.z.zhang@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=rmody@marvell.com \
    --cc=rosen.xu@intel.com \
    --cc=sachin.saxena@oss.nxp.com \
    --cc=shahafs@nvidia.com \
    --cc=shshaikh@marvell.com \
    --cc=somnath.kotur@broadcom.com \
    --cc=viacheslavo@nvidia.com \
    --cc=xuanziyang2@huawei.com \
    --cc=yisen.zhuang@huawei.com \
    --cc=zhouguoyang@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).