From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4979FA04B5; Tue, 27 Oct 2020 12:15:20 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A9ADB2BE2; Tue, 27 Oct 2020 12:15:18 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id CE8EB2BAA for ; Tue, 27 Oct 2020 12:15:15 +0100 (CET) IronPort-SDR: 0E2Pa81405/yhbYvFsmxRknQsqsLxX9ZRM7k8r/TKBZiyq2/d1p2PXQk6VUOugGOlx78hlhs2A g0/r6L+B01eg== X-IronPort-AV: E=McAfee;i="6000,8403,9786"; a="147916344" X-IronPort-AV: E=Sophos;i="5.77,423,1596524400"; d="scan'208";a="147916344" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Oct 2020 04:15:12 -0700 IronPort-SDR: WoX0n/w1jXb4mUM0sJST40SNOOS90reNase9XTF+dVabrN6xgH7xReQ/bn7hYvtwkLH1tEAuss O5ecZGb915vQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,423,1596524400"; d="scan'208";a="350247735" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga008.jf.intel.com with ESMTP; 27 Oct 2020 04:15:06 -0700 Received: from sivswdev09.ir.intel.com (sivswdev09.ir.intel.com [10.237.217.48]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id 09RBF6Xi027699; Tue, 27 Oct 2020 11:15:06 GMT Received: from sivswdev09.ir.intel.com (localhost [127.0.0.1]) by sivswdev09.ir.intel.com with ESMTP id 09RBF5uC022722; Tue, 27 Oct 2020 11:15:05 GMT Received: (from lma25@localhost) by sivswdev09.ir.intel.com with LOCAL id 09RBF4Tf022718; Tue, 27 Oct 2020 11:15:04 GMT Date: Tue, 27 Oct 2020 11:15:04 +0000 From: "Liang, Ma" To: Thomas Monjalon Cc: dev@dpdk.org, anatoly.burakov@intel.com, viktorin@rehivetech.com, qi.z.zhang@intel.com, ruifeng.wang@arm.com, beilei.xing@intel.com, jia.guo@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, bruce.richardson@intel.com, konstantin.ananyev@intel.com, david.hunt@intel.com, jerinjacobk@gmail.com, nhorman@tuxdriver.com, timothy.mcdaniel@intel.com, gage.eads@intel.com, drc@linux.vnet.ibm.com, Andrew Rybchenko , ferruh.yigit@intel.com, jerinj@marvell.com, hemant.agrawal@nxp.com, viacheslavo@nvidia.com, matan@nvidia.com, ajit.khaparde@broadcom.com, rahul.lakkireddy@chelsio.com, johndale@cisco.com, xavier.huwei@huawei.com, shahafs@nvidia.com, sthemmin@microsoft.com, g.singh@nxp.com, rmody@marvell.com, maxime.coquelin@redhat.com, david.marchand@redhat.com Message-ID: <20201027111504.GB15973@sivswdev09.ir.intel.com> References: <1603494392-7181-5-git-send-email-liang.j.ma@intel.com> <2168123.9p5K5TOhLB@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2168123.9p5K5TOhLB@thomas> Subject: Re: [dpdk-dev] [PATCH v9 04/10] ethdev: add simple power management API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" thanks for your information. Sorry for that. All related maintainer(include other NIC PMD) will be Cced in v10. > You keep forgetting Cc ethdev maintainers > (it is automatic when using --cc-cmd devtools/get-maintainer.sh). > As a result we still don't have any feedback from Ferruh and Andrew. > And I believe such API requires having feedback from maintainers > of other NIC drivers. I tried to explain this concern already > in email and community meetings, but I see no progress. > > Below are my comments. > > > --- a/doc/guides/rel_notes/release_20_11.rst > > +++ b/doc/guides/rel_notes/release_20_11.rst > > @@ -139,6 +139,11 @@ New Features > > Hairpin Tx part flow rules can be inserted explicitly. > > New API is added to get the hairpin peer ports list. > > > > +* **ethdev: add 1 new EXPERIMENTAL API for PMD power management.** > > The guidelines at the top of the file say using past tense. > No need to mention it is experimental as any new API. agree > > > + > > + * ``rte_eth_get_wake_addr()`` > > + * add new eth_dev_ops ``get_wake_addr`` > > No need to mention the dev_ops in the release notes. > Better to explain what it brings from an user perspective. agree > > > --- a/lib/librte_ethdev/rte_ethdev.h > > +++ b/lib/librte_ethdev/rte_ethdev.h > > +/** > > + * Retrieve the wake up address for the receive queue. > > I guess how this function should be used, > but a bit more explanations would not hurt here. agree > > + * > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param queue_id > > + * The Rx queue on the Ethernet device for which information will be > > + * retrieved. > > + * @param wake_addr > > + * The pointer to the address which will be monitored. > > This function does not make the address monitored, right? This function only get the target wakeup address. that does not monitor this address. > > > + * @param expected > > + * The pointer to value to be expected when descriptor is set. > > Not sure we should restrict it to a "descriptor". actully that is not limited to a descriptor, any writeback content should work. > > Expecting a value or some bits looks too much restrictive. > I understand it probably fits well for Intel NICs, > but in the general case, we can imagine that any change > in a byte array could be a wake up signal. this parameter doesn not limited user how to use it. In fact, current design can support any bits change within 64 bits content. > > > + * @param mask > > + * The pointer to comparison bitmask for the expected value. > > + * @param data_sz > > + * The pointer to data size for the expected value and comparison bitmask. > > It is not clear that above 4 parameters are filled by the driver. > > > + * > > + * @return > > + * - 0: Success. > > + * -ENOTSUP: Operation not supported. > > + * -EINVAL: Invalid parameters. > > + * -ENODEV: Invalid port ID. > > + */ > > +__rte_experimental > > +int rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id, > > + volatile void **wake_addr, uint64_t *expected, uint64_t *mask, > > + uint8_t *data_sz); > [...] > > --- a/lib/librte_ethdev/version.map > > +++ b/lib/librte_ethdev/version.map > > @@ -244,6 +244,7 @@ EXPERIMENTAL { > > rte_flow_get_restore_info; > > rte_flow_tunnel_action_decap_release; > > rte_flow_tunnel_item_release; > > + rte_eth_get_wake_addr; > > }; > > Please sort in alphabetical order. agree will do > >