From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id AD8DDC316 for ; Mon, 15 Jun 2015 20:23:22 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 15 Jun 2015 11:23:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,620,1427785200"; d="scan'208";a="727822947" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by fmsmga001.fm.intel.com with ESMTP; 15 Jun 2015 11:23:21 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.73]) by IRSMSX154.ger.corp.intel.com ([169.254.12.182]) with mapi id 14.03.0224.002; Mon, 15 Jun 2015 19:23:20 +0100 From: "Ananyev, Konstantin" To: "David Harton (dharton)" , "Wang, Liang-min" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info Thread-Index: AQHQo4/Uz84r4LZBA0eIhB09B1a4pJ2nKH/wgAAL/ACAABF3kIAAg7QAgAD8UZCABL/bgIAAEg9wgAAaYwCAABk6EA== Date: Mon, 15 Jun 2015 18:23:19 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A0A9E5@irsmsx105.ger.corp.intel.com> References: <1432946276-9424-1-git-send-email-liang-min.wang@intel.com> <1433948996-9716-1-git-send-email-liang-min.wang@intel.com> <1433948996-9716-2-git-send-email-liang-min.wang@intel.com> <2601191342CEEE43887BDE71AB97725836A08BA3@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A08BD4@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A08FD3@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A0A7FF@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jun 2015 18:23:23 -0000 > -----Original Message----- > From: David Harton (dharton) [mailto:dharton@cisco.com] > Sent: Monday, June 15, 2015 5:05 PM > To: Ananyev, Konstantin; Wang, Liang-min; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access= device info >=20 >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstanti= n > > Sent: Monday, June 15, 2015 9:46 AM > > To: Wang, Liang-min; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support acce= ss > > device info > > > > > > > > > -----Original Message----- > > > From: Wang, Liang-min > > > Sent: Monday, June 15, 2015 2:26 PM > > > To: Ananyev, Konstantin; dev@dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > access device info > > > > > > > > > > > > > -----Original Message----- > > > > From: Ananyev, Konstantin > > > > Sent: Friday, June 12, 2015 8:31 AM > > > > To: Wang, Liang-min; dev@dpdk.org > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > access device info > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Wang, Liang-min > > > > > Sent: Thursday, June 11, 2015 10:51 PM > > > > > To: Ananyev, Konstantin; dev@dpdk.org > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to suppor= t > > > > > access device info > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Ananyev, Konstantin > > > > > > Sent: Thursday, June 11, 2015 9:07 AM > > > > > > To: Wang, Liang-min; dev@dpdk.org > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > support access device info > > > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Wang, Liang-min > > > > > > > Sent: Thursday, June 11, 2015 1:58 PM > > > > > > > To: Ananyev, Konstantin; dev@dpdk.org > > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > support access device info > > > > > > > > > > > > > > Hi Konstantin, > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Ananyev, Konstantin > > > > > > > > Sent: Thursday, June 11, 2015 8:26 AM > > > > > > > > To: Wang, Liang-min; dev@dpdk.org > > > > > > > > Cc: Wang, Liang-min > > > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > > support access device info > > > > > > > > > > > > > > > > Hi Larry, > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of > > > > > > > > > Liang-Min Larry Wang > > > > > > > > > Sent: Wednesday, June 10, 2015 4:10 PM > > > > > > > > > To: dev@dpdk.org > > > > > > > > > Cc: Wang, Liang-min > > > > > > > > > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > > > support access device info > > > > > > > > > > > > > > > > > > add new apis: > > > > > > > > > - rte_eth_dev_default_mac_addr_set > > > > > > > > > - rte_eth_dev_reg_leng > > > > > > > > > - rte_eth_dev_reg_info > > > > > > > > > - rte_eth_dev_eeprom_leng > > > > > > > > > - rte_eth_dev_get_eeprom > > > > > > > > > - rte_eth_dev_set_eeprom > > > > > > > > > - rte_eth_dev_get_ringparam > > > > > > > > > - rte_eth_dev_set_ringparam > > > > > > > > > > > > > > > > > > to enable reading device parameters (mac-addr, register, > > > > > > > > > eeprom, > > > > > > > > > ring) based upon ethtool alike data parameter specificati= on. > > > > > > > > > > > > > > > > > > Signed-off-by: Liang-Min Larry Wang > > > > > > > > > > > > > > > > > > --- > > > > > > > > > lib/librte_ether/Makefile | 1 + > > > > > > > > > lib/librte_ether/rte_eth_dev_info.h | 80 > > +++++++++++++++++ > > > > > > > > > lib/librte_ether/rte_ethdev.c | 159 > > > > > > > > +++++++++++++++++++++++++++++++++ > > > > > > > > > lib/librte_ether/rte_ethdev.h | 158 > > > > > > > > ++++++++++++++++++++++++++++++++ > > > > > > > > > lib/librte_ether/rte_ether_version.map | 8 ++ > > > > > > > > > 5 files changed, 406 insertions(+) create mode 100644 > > > > > > > > > lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > > > > > > > > > > diff --git a/lib/librte_ether/Makefile > > > > > > > > > b/lib/librte_ether/Makefile index c0e5768..05209e9 100644 > > > > > > > > > --- a/lib/librte_ether/Makefile > > > > > > > > > +++ b/lib/librte_ether/Makefile > > > > > > > > > @@ -51,6 +51,7 @@ SRCS-y +=3D rte_ethdev.c > > > > > > > > > SYMLINK-y-include +=3D rte_ether.h SYMLINK-y-include += =3D > > > > > > > > > rte_ethdev.h SYMLINK-y-include > > > > > > > > > +=3D rte_eth_ctrl.h > > > > > > > > > +SYMLINK-y-include +=3D rte_eth_dev_info.h > > > > > > > > > > > > > > > > > > # this lib depends upon: > > > > > > > > > DEPDIRS-y +=3D lib/librte_eal lib/librte_mempool > > > > > > > > > lib/librte_ring lib/librte_mbuf diff --git > > > > > > > > > a/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > b/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > new file mode 100644 > > > > > > > > > index 0000000..002c4b5 > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > @@ -0,0 +1,80 @@ > > > > > > > > > +/*- > > > > > > > > > + * BSD LICENSE > > > > > > > > > + * > > > > > > > > > + * Copyright(c) 2015 Intel Corporation. All rights > > reserved. > > > > > > > > > + * All rights reserved. > > > > > > > > > + * > > > > > > > > > + * Redistribution and use in source and binary forms, > > with or > > > > without > > > > > > > > > + * modification, are permitted provided that the > > following > > > > conditions > > > > > > > > > + * are met: > > > > > > > > > + * > > > > > > > > > + * * Redistributions of source code must retain the > > above > > > > copyright > > > > > > > > > + * notice, this list of conditions and the followi= ng > > disclaimer. > > > > > > > > > + * * Redistributions in binary form must reproduce t= he > > above > > > > > > copyright > > > > > > > > > + * notice, this list of conditions and the followi= ng > > disclaimer in > > > > > > > > > + * the documentation and/or other materials provid= ed > > with the > > > > > > > > > + * distribution. > > > > > > > > > + * * Neither the name of Intel Corporation nor the > > names of its > > > > > > > > > + * contributors may be used to endorse or promote > > products > > > > > > derived > > > > > > > > > + * from this software without specific prior writt= en > > permission. > > > > > > > > > + * > > > > > > > > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS = AND > > > > > > > > CONTRIBUTORS > > > > > > > > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, > > > > INCLUDING, > > > > > > BUT > > > > > > > > NOT > > > > > > > > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILI= TY > > > > AND > > > > > > > > FITNESS FOR > > > > > > > > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SH= ALL > > > > THE > > > > > > > > COPYRIGHT > > > > > > > > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > > > > INDIRECT, > > > > > > > > INCIDENTAL, > > > > > > > > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > > > > > > (INCLUDING, > > > > > > > > BUT NOT > > > > > > > > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR > > > > SERVICES; > > > > > > > > LOSS OF USE, > > > > > > > > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > > > > > > CAUSED > > > > > > > > AND ON ANY > > > > > > > > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > > > > LIABILITY, > > > > > > OR > > > > > > > > TORT > > > > > > > > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY > > > > WAY > > > > > > OUT > > > > > > > > OF THE USE > > > > > > > > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILIT= Y > > OF > > > > SUCH > > > > > > > > DAMAGE. > > > > > > > > > + */ > > > > > > > > > + > > > > > > > > > +#ifndef _RTE_ETH_DEV_INFO_H_ #define _RTE_ETH_DEV_INFO_H= _ > > > > > > > > > + > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * Placeholder for accessing device registers */ struct > > > > > > > > > +rte_dev_reg_info { > > > > > > > > > + void *buf; /**< Buffer for register */ > > > > > > > > > + uint32_t offset; /**< Offset for 1st register to fetch > > */ > > > > > > > > > + uint32_t leng; /**< Number of registers to fetch */ > > > > > > > > > + uint32_t version; /**< Device version */ }; > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * Placeholder for accessing device eeprom */ struct > > > > > > > > > +rte_dev_eeprom_info { > > > > > > > > > + void *buf; /**< Buffer for eeprom */ > > > > > > > > > + uint32_t offset; /**< Offset for 1st eeprom location to > > > > > > > > > +access > > > > */ > > > > > > > > > + uint32_t leng; /**< Length of eeprom region to access *= / > > > > > > > > > + uint32_t magic; /**< Device ID */ }; > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * Placeholder for accessing device ring parameters */ > > > > > > > > > +struct rte_dev_ring_info { > > > > > > > > > + uint32_t rx_pending; /**< Number of outstanding Rx ring > > */ > > > > > > > > > + uint32_t tx_pending; /**< Number of outstanding Tx ring > > */ > > > > > > > > > + uint32_t rx_max_pending; /**< Maximum number of > > > > outstanding > > > > > > > > > +Rx > > > > > > > > ring */ > > > > > > > > > + uint32_t tx_max_pending; /**< Maximum number of > > > > outstanding > > > > > > > > > +Tx > > > > > > > > ring > > > > > > > > > +*/ }; > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * A data structure captures information as defined in > > > > > > > > > +struct ifla_vf_info > > > > > > > > > + * for user-space api > > > > > > > > > + */ > > > > > > > > > +struct rte_dev_vf_info { > > > > > > > > > + uint32_t vf; > > > > > > > > > + uint8_t mac[ETHER_ADDR_LEN]; > > > > > > > > > + uint32_t vlan; > > > > > > > > > + uint32_t tx_rate; > > > > > > > > > + uint32_t spoofchk; > > > > > > > > > +}; > > > > > > > > > > > > > > > > > > > > > > > > Wonder what that structure is for? > > > > > > > > I can't see it used in any function below? > > > > > > > > > > > > > > > > > > > > > > Good catch, this is designed for other ethtool ops that I did > > > > > > > not include in > > > > > > this release, I will remove this from next fix. > > > > > > > > > > > > > > > > + > > > > > > > > > +#endif /* _RTE_ETH_DEV_INFO_H_ */ > > > > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > > > > > > > > b/lib/librte_ether/rte_ethdev.c index 5a94654..186e85c > > > > > > > > > 100644 > > > > > > > > > --- a/lib/librte_ether/rte_ethdev.c > > > > > > > > > +++ b/lib/librte_ether/rte_ethdev.c > > > > > > > > > @@ -2751,6 +2751,32 @@ rte_eth_dev_mac_addr_remove(uint8_= t > > > > > > > > port_id, > > > > > > > > > struct ether_addr *addr) } > > > > > > > > > > > > > > > > > > int > > > > > > > > > +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct > > > > > > > > > +ether_addr > > > > > > > > > +*addr) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + const int index =3D 0; > > > > > > > > > + const uint32_t pool =3D 0; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + dev =3D &rte_eth_devices[port_id]; > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops- > > > > >mac_addr_remove, - > > > > > > > > ENOTSUP); > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, - > > > > > > > > ENOTSUP); > > > > > > > > > + > > > > > > > > > + /* Update NIC default MAC address*/ > > > > > > > > > + (*dev->dev_ops->mac_addr_remove)(dev, index); > > > > > > > > > + (*dev->dev_ops->mac_addr_add)(dev, addr, index, pool); > > > > > > > > > + > > > > > > > > > + /* Update default address in NIC data structure */ > > > > > > > > > + ether_addr_copy(addr, &dev->data->mac_addrs[index]); > > > > > > > > > + > > > > > > > > > + return 0; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id, uint16_t vf, > > > > > > > > > uint16_t rx_mode, uint8_t on) { @@ > > > > -3627,3 > > > > > > +3653,136 @@ > > > > > > > > > rte_eth_remove_tx_callback(uint8_t port_id, > > > > > > > > uint16_t queue_id, > > > > > > > > > /* Callback wasn't found. */ > > > > > > > > > return -EINVAL; > > > > > > > > > } > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_reg_leng(uint8_t port_id) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_length, > > > > - > > > > > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->get_reg_length)(dev); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_reg_info(uint8_t port_id, struct > > > > > > > > > +rte_dev_reg_info > > > > > > > > > +*info) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg, - > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->get_reg)(dev, info); } > > > > > > > > > > > > > > > > Seems that *get_reg* stuff, will be really good addition fo= r > > > > > > > > DPDK debugging abilities. > > > > > > > > Though, I'd suggest we change it a bit to make more generic > > > > > > > > and > > > > flexible: > > > > > > > > > > > > > > > > Introduce rte_eth_reg_read/rte_eth_reg_write(), > > > > > > > > or probably even better rte_pcidev_reg_read > > > > > > > > /rte_pcidev_reg_write at > > > > > > EAL. > > > > > > > > Something similar to what > > > > > > > > port_pci_reg_read/port_pci_reg_write() > > > > > > > > are doing now at testpmd.h. > > > > > > > > > > > > > > > > struct rte_pcidev_reg_info { > > > > > > > > const char *name; > > > > > > > > uint32_t endianes, bar, offset, size, count; }; > > > > > > > > > > > > > > > > int rte_pcidev_reg_read(const struct rte_pci_device *, cons= t > > > > > > > > struct rte_pcidev_reg_info *, uint64_t *reg_val); > > > > > > > > > > > > > > > > Then: > > > > > > > > int rte_eth_dev_get_reg_info(port_id, const struct > > > > > > > > rte_pcidev_reg_info **info); > > > > > > > > > > > > > > > > So each device would store in info a pointer to an array of > > > > > > > > it's register descriptions (finished by zero elem?). > > > > > > > > > > > > > > > > Then your ethtool (or any other upper layer) can do the > > > > > > > > following to read all device regs: > > > > > > > > > > > > > > > > > > > > > > The proposed reg info structure allows future improvement to > > > > > > > support > > > > > > individual register read/write. > > > > > > > Also, because each NIC device has a very distinguish register > > definition. > > > > > > > So, the plan is to have more comprehensive interface to > > > > > > > support query operation (for example, register name) before > > > > > > > introduce individual/group > > > > > > register access. > > > > > > > Points taken, the support will be in future release. > > > > > > > > > > > > Sorry, didn't get you. > > > > > > So you are ok to make these changes in next patch version? > > > > > > > > > > > I would like to get a consensus from dpdk community on how to > > > > > provide > > > > register information. > > > > > > > > Well, that' ok, but if it is just a trial patch that is not intende= d > > > > to be applied, then you should mark it as RFC. > > > > > > > > > Currently, it's designed for debug dumping. The register > > > > > information is very > > > > hardware dependent. > > > > > Need to consider current supported NIC device and future devices > > > > > for > > > > DPDK, so we won't make it a bulky interface. > > > > > > > > Ok, could you explain what exactly concerns you in the approach > > > > described above? > > > > What part you feel is bulky? > > > > > > > > > > > > > > > > > > > const struct rte_eth_dev_reg_info *reg_info; struct > > > > > > > > rte_eth_dev_info dev_info; > > > > > > > > > > > > > > > > rte_eth_dev_info_get(pid, &dev_info); > > > > > > > > rte_eth_dev_get_reg_info(port_id, ®_info); > > > > > > > > > > > > > > > > for (i =3D 0; reg_info[i].name !=3D NULL; i++) { > > > > > > > > ... > > > > > > > > rte_pcidev_read_reg(dev_info. pci_dev, reg_info[i], &v); > > > > > > > > .. > > > > > > > > } > > > > > > > > > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_eeprom_leng(uint8_t port_id) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops- > > > > >get_eeprom_length, - > > > > > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->get_eeprom_length)(dev); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_get_eeprom(uint8_t port_id, struct > > > > > > > > > +rte_dev_eeprom_info > > > > > > > > > +*info) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_eeprom, - > > > > > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->get_eeprom)(dev, info); } > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_set_eeprom(uint8_t port_id, struct > > > > > > > > > +rte_dev_eeprom_info > > > > > > > > > +*info) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_eeprom, - > > > > > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->set_eeprom)(dev, info); } > > > > > > > > > + > > > > > > > > > +int > > > > > > > > > +rte_eth_dev_get_ringparam(uint8_t port_id, struct > > > > > > > > > +rte_dev_ring_info > > > > > > > > > +*info) { > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > + > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > port_id); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > + return -ENODEV; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_ringparam, - > > > > > > > > ENOTSUP); > > > > > > > > > + return (*dev->dev_ops->get_ringparam)(dev, info); } > > > > > > > > > > > > > > > > I think it will be a useful addition to the ethdev API to > > > > > > > > have an ability to retrieve current RX/TX queue parameters. > > > > > > > > Though again, it need to be more generic, so it could be > > > > > > > > useful for > > > > > > > > non- ethtool upper layer too. > > > > > > > > So I suggest to modify it a bit. > > > > > > > > Something like that: > > > > > > > > > > > > > > > > struct rte_eth_tx_queue_info { > > > > > > > > struct rte_eth_txconf txconf; > > > > > > > > uint32_t nb_tx_desc; > > > > > > > > uint32_t nb_max_tx_desc; /*max allowable TXDs for that > > queue */ > > > > > > > > uint32_t nb_tx_free; /* number of free TXDs = at > > the moment > > > > of > > > > > > call. > > > > > > > > */ > > > > > > > > /* other tx queue data. */ }; > > > > > > > > > > > > > > > > int rte_etdev_get_tx_queue_info(portid, queue_id, struct > > > > > > > > rte_eth_tx_queue_info *qinfo) > > > > > > > > > > > > > > > > Then, your upper layer ethtool wrapper, can implement yours > > > > > > > > ethtool_get_ringparam() by: > > > > > > > > > > > > > > > > ... > > > > > > > > struct rte_eth_tx_queue_info qinfo; > > > > > > > > rte_ethdev_get_tx_queue_info(port, 0, &qinfo); > > > > > > > > ring_param->tx_pending =3D qinfo.nb_tx_desc - > > > > > > > > rte_eth_rx_queue_count(port, 0); > > > > > > > > > > > > > > > > Or probably even: > > > > > > > > ring_param->tx_pending =3D qinfo.nb_tx_desc - > > > > > > > > qinfo.nb_tx_free; > > > > > > > > > > > > > > > > Same for RX. > > > > > > > > > > > > > > > For now, this descriptor ring information is used by the etht= ool > > op. > > > > > > > To make this interface simple, i.e. caller doesn't need to > > > > > > > access other > > > > > > queue information. > > > > > > > > > > > > I just repeat what I said to you in off-line conversation: > > > > > > ethdev API is not equal ethtool API. > > > > > > It is ok to add a new function/structure to ethdev if it reall= y > > > > > > needed, but we should do mechanical one to one copy. > > > > > > It is much better to add a function/structure that would be > > > > > > more generic, and suit other users, not only ethtool. > > > > > > There is no point to have dozen functions in rte_ethdev API > > > > > > providing similar information. > > > > > > BTW, I don't see how API I proposed is much more complex, then > > > > > > yours > > > > one. > > > > > The ring parameter is a run-time information which is different > > > > > than data > > > > structure described in this discussion. > > > > > > > > I don't see how they are different. > > > > Looking at ixgbe_get_ringparam(), it returns: > > > > rx_max_pending - that's a static IXGBE PMD value (max possible > > > > number of RXDs per one queue). > > > > rx_pending - number of RXD currently in use by the HW for queue 0 > > > > (that information can be changed at each call). > > > > > > > > With the approach I suggesting - you can get same information for > > > > each RX queue by calling rte_ethdev_get_rx_queue_info() and > > > > rte_eth_rx_queue_count(). > > > > Plus you are getting other RX queue data. > > > > > > > > Another thing - what is practical usage of the information you > > > > retrieving now by get_ringparam()? > > > > Let say it returned to you: rx_max_pending=3D4096; rx_pending=3D128= ; How > > > > that information would help to understand what is going on with the > > device? > > > > Without knowing value of nb_tx_desc for the queue, you can't say i= s > > > > you queue full or not. > > > > Again, it could be that all your traffic going through some other > > > > queue (not 0). > > > > So from my point rte_eth_dev_get_ringparam() usage is very limited= , > > > > and doesn't provide enough information about current queue state. > > > > > > > > Same thing applies for TX. > > > > > > > > > > After careful review the suggestion in this comment, and review the > > existing dpdk source code. > > > I came to realize that neither rte_ethdev_get_rx_queue_info, > > > rte_ethdev_get_tx_queue_info, struct rte_eth_rx_queue_info and struct > > > rte_eth_tx_queue_info are available in the existing dpdk source code.= I > > could not make a patch based upon a set of non- existent API and data > > structure. > > > > Right now, in dpdk.org source code, struct rte_eth_dev_ring_info, stru= ct > > rte_dev_eeprom_info and struct rte_dev_reg_info don't exist also. > > Same as all these functions: > > > > rte_eth_dev_default_mac_addr_set > > rte_eth_dev_reg_length > > rte_eth_dev_reg_info > > rte_eth_dev_eeprom_length > > rte_eth_dev_get_eeprom > > rte_eth_dev_set_eeprom > > rte_eth_dev_get_ringparam > > > > All this is a new API that's you are trying to add. > > But, by some reason you consider it is ok to add 'struct > > rte_eth_dev_ring_info', but couldn't add struct > > 'rte_ethdev_get_tx_queue_info' > > That just doesn't make any sense to me. > > In fact, I think our conversation is going in cycles. > > If you are not happy with the suggested approach, please provide some > > meaningful reason why. > > Konstantin >=20 > It seems the new API aims at providing users a mechanism to quickly and > gracefully migrate from using ethtool/ioctl calls. I am fine with that goal in general. But it doesn't mean that all ethool API should be pushed into rte_ethdev la= yer. That's why a shim layer on top of rte_ethdev is created - it's goal is to provide for the upper layer an ethool-like API and hide actual implementation based on rte_ethdev API inside. > The provided get/set > ring param info is very similar to that of ethtool and facilitates the > ethtool needs. If rte_ethtool shim layer has to provide get/set ring_param API as it is - = that's ok with me. Though I don't see any good reason why rte_ethdev layer API should restrict= itself with exactly the same ethtool-like API. As I said before - current practical usage of rte_eth_dev_get_ringparam() l= ooks quite limited. Probably it just me, but I don't see how user can conclude what is the devi= ce state, using information provided by rte_eth_dev_get_ringparam(). My concern is - if we'll introduce rte_eth_dev_get_ringparam() as it sugges= ted by Larry now, one of two things would happen: - no one except ethtool shim layer would use it. - people will complain that it doesn't provide desired information.=20 So, in next release we'll have to either introduce a new function and suppo= rt 2 functions doing similar things (code duplication), or modify existing API (ABI breakage pain). It would be much better to introduce a new rte_ethdev API here that would b= e generic enough and fit common needs, not only one particular case (ethtool). After all, I don't think the changes I suggesting differ that much from cu= rrent approach. Konstantin > While additional enhancements to the API to provide additional details > such as those you have suggested are certainly possible, I believe Larry > is stating those ideas are outside the scope he has intended with the > API introduction and that they should be discussed further and delivered > in a future patch. >=20 > Does that seem reasonable? >=20 > Thanks, > Dave >=20 > > > > > > > > > > It's the desire of this patch to separate each data structure to > > > > > avoid cross > > > > dependency. > > > > > > > > That's too cryptic to me. > > > > Could you explain what cross dependency you are talking about? > > > > > > > > Konstantin