From: Ferruh Yigit <ferruh.yigit@intel.com> To: Andrew Rybchenko <arybchenko@solarflare.com>, "John W. Linville" <linville@tuxdriver.com>, Xiaolong Ye <xiaolong.ye@intel.com>, Qi Zhang <qi.z.zhang@intel.com>, Igor Russkikh <igor.russkikh@aquantia.com>, Pavel Belous <pavel.belous@aquantia.com>, Allain Legacy <allain.legacy@windriver.com>, Matt Peters <matt.peters@windriver.com>, Ravi Kumar <ravi1.kumar@amd.com>, Rasesh Mody <rmody@marvell.com>, Shahed Shaikh <shshaikh@marvell.com>, Ajit Khaparde <ajit.khaparde@broadcom.com>, Somnath Kotur <somnath.kotur@broadcom.com>, Chas Williams <chas3@att.com>, Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>, Hemant Agrawal <hemant.agrawal@nxp.com>, Sachin Saxena <sachin.saxena@nxp.com>, Wenzhuo Lu <wenzhuo.lu@intel.com>, Gagandeep Singh <g.singh@nxp.com>, John Daley <johndale@cisco.com>, Hyong Youb Kim <hyonkim@cisco.com>, Gaetan Rivet <gaetan.rivet@6wind.com>, Xiao Wang <xiao.w.wang@intel.com>, Ziyang Xuan <xuanziyang2@huawei.com>, Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>, Guoyang Zhou <zhouguoyang@huawei.com>, Beilei Xing <beilei.xing@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Qiming Yang <qiming.yang@intel.com>, Rosen Xu <rosen.xu@intel.com>, Konstantin Ananyev <konstantin.ananyev@intel.com>, Shijith Thotton <sthotton@marvell.com>, Srisivasubramanian Srinivasan <srinivasan@marvell.com>, Matan Azrad <matan@mellanox.com>, Shahaf Shuler <shahafs@mellanox.com>, Yongseok Koh <yskoh@mellanox.com>, Viacheslav Ovsiienko <viacheslavo@mellanox.com>, Zyta Szpak <zr@semihalf.com>, Liron Himi <lironh@marvell.com>, Tomasz Duszynski <tdu@semihalf.com>, Stephen Hemminger <sthemmin@microsoft.com>, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, Rastislav Cernay <cernay@netcope.com>, Jan Remes <remes@netcope.com>, Alejandro Lucero <alejandro.lucero@netronome.com>, Jerin Jacob <jerinj@marvell.com>, Nithin Dabilpuram <ndabilpuram@marvell.com>, Kiran Kumar K <kirankumark@marvell.com>, Keith Wiles <keith.wiles@intel.com>, Maciej Czekaj <mczekaj@marvell.com>, Maxime Coquelin <maxime.coquelin@redhat.com>, Tiwei Bie <tiwei.bie@intel.com>, Zhihong Wang <zhihong.wang@intel.com>, Yong Wang <yongwang@vmware.com>, Thomas Monjalon <thomas@monjalon.net> Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status Date: Fri, 13 Sep 2019 17:33:11 +0100 Message-ID: <2ad9fc47-b55b-5bfe-9667-c3e2dd4b5c63@intel.com> (raw) In-Reply-To: <d4a4698e-5f42-2a9f-82ea-7d5eaa128ebd@solarflare.com> On 9/13/2019 4:54 PM, Andrew Rybchenko wrote: > On 9/13/19 6:34 PM, Ferruh Yigit wrote: >> On 9/9/2019 12:58 PM, Andrew Rybchenko wrote: >>> Enabling/disabling of promiscuous mode is not always successful and >>> it should be taken into account to be able to handle it properly. >>> >>> When correct return status is unclear from driver code, -EAGAIN is used. >>> >>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> >> <...> >> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >>> index b97dd8aa85..f2e6b4c83b 100644 >>> --- a/lib/librte_ethdev/rte_ethdev.c >>> +++ b/lib/librte_ethdev/rte_ethdev.c >>> @@ -1892,30 +1892,38 @@ int >>> rte_eth_promiscuous_enable(uint16_t port_id) >>> { >>> struct rte_eth_dev *dev; >>> + uint8_t old_promiscuous; >>> + int diag; >>> >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>> dev = &rte_eth_devices[port_id]; >>> >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_enable, -ENOTSUP); >>> - (*dev->dev_ops->promiscuous_enable)(dev); >>> - dev->data->promiscuous = 1; >>> + old_promiscuous = dev->data->promiscuous; >>> + diag = (*dev->dev_ops->promiscuous_enable)(dev); >>> + dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous; >>> >>> - return 0; >>> + return eth_err(port_id, diag); >>> } >>> >>> int >>> rte_eth_promiscuous_disable(uint16_t port_id) >>> { >>> struct rte_eth_dev *dev; >>> + uint8_t old_promiscuous; >>> + int diag; >>> >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>> dev = &rte_eth_devices[port_id]; >>> >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_disable, -ENOTSUP); >>> + old_promiscuous = dev->data->promiscuous; >>> dev->data->promiscuous = 0; >>> - (*dev->dev_ops->promiscuous_disable)(dev); >>> + diag = (*dev->dev_ops->promiscuous_disable)(dev); >>> + if (diag != 0) >>> + dev->data->promiscuous = old_promiscuous; >> Not a real issue, but the enable/disable does exact same thing, slightly >> different way, it makes double check if logic is different, >> can you adapt one of them for both please. >> >> " >> diag = foo(); >> dev->data->promiscuous = (diag == 0) ? 1 : old_promiscuous; >> " >> vs >> >> " >> dev->data->promiscuous = 0; >> diag = bar(); >> if (diag != 0) >> dev->data->promiscuous = old_promiscuous; >> " > > I simply did not want to touch the logic in big patch series. > Don't know why, but enable sets promiscuous=1 after callback, > but disable does it before callback. That's why the difference. Ahh, so there is a difference indeed. > Logically it is a separate change and I definitely don't want to > mix it. Ok, fair enough. > >>> >>> - return 0; >>> + return eth_err(port_id, diag); >>> } >>> >>> int >>> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h >>> index 2394b32c83..6322348d17 100644 >>> --- a/lib/librte_ethdev/rte_ethdev_core.h >>> +++ b/lib/librte_ethdev/rte_ethdev_core.h >>> @@ -52,10 +52,10 @@ typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev); >>> typedef int (*eth_is_removed_t)(struct rte_eth_dev *dev); >>> /**< @internal Function used to detect an Ethernet device removal. */ >>> >>> -typedef void (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev); >>> +typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev); >>> /**< @internal Function used to enable the RX promiscuous mode of an Ethernet device. */ >>> >>> -typedef void (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev); >>> +typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev); >>> /**< @internal Function used to disable the RX promiscuous mode of an Ethernet device. */ >> Should we add a note what is expected return value? This information is not >> available anyplace, it may help driver developers. > > Makes sense. > >
next prev parent reply index Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-05 16:10 [dpdk-dev] [PATCH 00/13] ethdev: change promiscuous mode functions " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko 2019-09-05 16:25 ` Gaëtan Rivet 2019-09-05 16:32 ` Andrew Rybchenko 2019-09-05 16:36 ` Stephen Hemminger 2019-09-05 16:38 ` Andrew Rybchenko 2019-09-06 9:24 ` Gaëtan Rivet 2019-09-06 10:08 ` Andrew Rybchenko 2019-09-05 16:40 ` Stephen Hemminger 2019-09-05 16:49 ` Andrew Rybchenko 2019-09-05 17:13 ` Stephen Hemminger 2019-09-05 16:10 ` [dpdk-dev] [PATCH 03/13] net/bonding: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 07/13] app/testpmd: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 08/13] app/eventdev: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 09/13] app/pdump: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 10/13] app/test: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 11/13] kni: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 12/13] test/bonding: " Andrew Rybchenko 2019-09-05 16:10 ` [dpdk-dev] [PATCH 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 00/13] ethdev: change promiscuous mode functions to return status Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko 2019-09-13 15:35 ` Ferruh Yigit 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko 2019-09-09 12:48 ` Gaëtan Rivet 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 03/13] net/bonding: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko 2019-09-10 7:53 ` Matan Azrad 2019-09-13 21:05 ` Andrew Rybchenko 2019-09-11 8:46 ` Hyong Youb Kim (hyonkim) 2019-09-13 21:06 ` Andrew Rybchenko 2019-09-16 6:48 ` Hyong Youb Kim (hyonkim) 2019-09-13 15:34 ` Ferruh Yigit 2019-09-13 15:54 ` Andrew Rybchenko 2019-09-13 16:33 ` Ferruh Yigit [this message] 2019-09-13 15:39 ` Ferruh Yigit 2019-09-13 16:05 ` Andrew Rybchenko 2019-09-13 16:34 ` Ferruh Yigit 2019-09-13 19:57 ` Andrew Rybchenko 2019-09-16 13:22 ` Ferruh Yigit 2019-09-16 15:45 ` Andrew Rybchenko 2019-09-16 15:58 ` Ferruh Yigit 2019-09-13 16:43 ` Ferruh Yigit 2019-09-13 20:33 ` Andrew Rybchenko 2019-09-13 16:56 ` Ferruh Yigit 2019-09-13 20:38 ` Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 07/13] app/testpmd: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 08/13] app/eventdev: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 09/13] app/pdump: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 10/13] app/test: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 11/13] kni: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 12/13] test/bonding: " Andrew Rybchenko 2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko 2019-09-13 16:40 ` Ferruh Yigit 2019-09-13 18:30 ` Andrew Rybchenko 2019-09-13 16:37 ` [dpdk-dev] [PATCH v2 00/13] ethdev: change promiscuous mode functions to return status Ferruh Yigit 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 03/13] net/bonding: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 07/13] app/testpmd: " Andrew Rybchenko 2019-09-16 13:18 ` Iremonger, Bernard 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 08/13] app/eventdev: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 09/13] app/pdump: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 10/13] app/test: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 11/13] kni: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 12/13] test/bonding: " Andrew Rybchenko 2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko 2019-09-24 7:31 ` [dpdk-dev] [PATCH v3 00/13] ethdev: change promiscuous mode functions to return status Ferruh Yigit
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=2ad9fc47-b55b-5bfe-9667-c3e2dd4b5c63@intel.com \ --to=ferruh.yigit@intel.com \ --cc=ajit.khaparde@broadcom.com \ --cc=alejandro.lucero@netronome.com \ --cc=allain.legacy@windriver.com \ --cc=arybchenko@solarflare.com \ --cc=beilei.xing@intel.com \ --cc=cernay@netcope.com \ --cc=chas3@att.com \ --cc=cloud.wangxiaoyun@huawei.com \ --cc=dev@dpdk.org \ --cc=g.singh@nxp.com \ --cc=gaetan.rivet@6wind.com \ --cc=haiyangz@microsoft.com \ --cc=hemant.agrawal@nxp.com \ --cc=hyonkim@cisco.com \ --cc=igor.russkikh@aquantia.com \ --cc=jerinj@marvell.com \ --cc=jingjing.wu@intel.com \ --cc=johndale@cisco.com \ --cc=keith.wiles@intel.com \ --cc=kirankumark@marvell.com \ --cc=konstantin.ananyev@intel.com \ --cc=kys@microsoft.com \ --cc=linville@tuxdriver.com \ --cc=lironh@marvell.com \ --cc=matan@mellanox.com \ --cc=matt.peters@windriver.com \ --cc=maxime.coquelin@redhat.com \ --cc=mczekaj@marvell.com \ --cc=ndabilpuram@marvell.com \ --cc=pavel.belous@aquantia.com \ --cc=qi.z.zhang@intel.com \ --cc=qiming.yang@intel.com \ --cc=rahul.lakkireddy@chelsio.com \ --cc=ravi1.kumar@amd.com \ --cc=remes@netcope.com \ --cc=rmody@marvell.com \ --cc=rosen.xu@intel.com \ --cc=sachin.saxena@nxp.com \ --cc=shahafs@mellanox.com \ --cc=shshaikh@marvell.com \ --cc=somnath.kotur@broadcom.com \ --cc=srinivasan@marvell.com \ --cc=sthemmin@microsoft.com \ --cc=sthotton@marvell.com \ --cc=tdu@semihalf.com \ --cc=thomas@monjalon.net \ --cc=tiwei.bie@intel.com \ --cc=viacheslavo@mellanox.com \ --cc=wenzhuo.lu@intel.com \ --cc=xiao.w.wang@intel.com \ --cc=xiaolong.ye@intel.com \ --cc=xuanziyang2@huawei.com \ --cc=yongwang@vmware.com \ --cc=yskoh@mellanox.com \ --cc=zhihong.wang@intel.com \ --cc=zhouguoyang@huawei.com \ --cc=zr@semihalf.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
DPDK-dev Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \ dev@dpdk.org public-inbox-index dpdk-dev Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git