From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id B8A911094 for ; Mon, 16 Jan 2017 09:51:55 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP; 16 Jan 2017 00:51:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="scan'208";a="213828408" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 16 Jan 2017 00:51:54 -0800 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 16 Jan 2017 00:51:54 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 16 Jan 2017 00:51:54 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.177]) by shsmsx102.ccr.corp.intel.com ([169.254.2.88]) with mapi id 14.03.0248.002; Mon, 16 Jan 2017 16:51:50 +0800 From: "Yang, Qiming" To: Andrew Rybchenko , "dev@dpdk.org" CC: "thomas.monjalon@6wind.com" , "Yigit, Ferruh" Thread-Topic: [dpdk-dev] [PATCH v9 1/5] ethdev: add firmware version get Thread-Index: AQHSb7yGHQSIDi+vykumHvwAPsc6H6E6KCKAgACenDA= Date: Mon, 16 Jan 2017 08:51:50 +0000 Message-ID: References: <1484202716-41669-2-git-send-email-qiming.yang@intel.com> <1484545498-33882-1-git-send-email-qiming.yang@intel.com> <1484545498-33882-2-git-send-email-qiming.yang@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmYzZGFmMTgtZThlZC00NWE0LWI0NGQtM2I2ZWQ4YzFjNjg0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkllbG1lZGN2eWE4Zm4xTFFrUDMwSEFua0JlYmRRWmJ0czlXSlBkV2t0aDA9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v9 1/5] ethdev: add firmware version get 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: , X-List-Received-Date: Mon, 16 Jan 2017 08:51:56 -0000 Hi, Andrew=20 > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index 917557a..89cffcf 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1588,6 +1588,18 @@ > rte_eth_dev_set_rx_queue_stats_mapping(uint8_t port_id, uint16_t > rx_queue_id, > > STAT_QMAP_RX); > > } > > > > +int > > +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, size_t > > +fw_size) { > > + struct rte_eth_dev *dev; > > + > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > + dev =3D &rte_eth_devices[port_id]; > > + > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, - > ENOTSUP); > > + return (*dev->dev_ops->fw_version_get)(dev, fw_version, fw_size); >=20 > I think it would be good to handle difference from snprintf() behaviour h= ere > and specify that fw_version_get callback has exactly snprintf()-like retu= rn > value. > It would allow to avoid > duplicated code in all drivers (adding 1 for terminating null, conversion= of > success value to 0). But I think it would be better to just keep the way it is. If I handle snpr= itf() behavior here, may the function rte_eth_dev_fw_version_get and ops fw_version_get will hav= e different=20 definition of return value. > Also I think warning about insufficient space is not required. It could b= e > intentional to call the first time with 0 (or some small) space to get re= quired > space to be (re)allocated. > May be debug level message would be useful. Good advice, when call this function with 0, this function will become a AP= I to get firmware version size. I think we can remove the warning message here. >=20 > > +} > > + > > void > > rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info > *dev_info) > > { > > diff --git a/lib/librte_ether/rte_ethdev.h > > b/lib/librte_ether/rte_ethdev.h index ded43d7..37a55ef 100644 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -1177,6 +1177,10 @@ typedef uint32_t > (*eth_rx_queue_count_t)(struct rte_eth_dev *dev, > > typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset); > > /**< @internal Check DD bit of specific RX descriptor */ > > > > +typedef int (*eth_fw_version_get_t)(struct rte_eth_dev *dev, > > + char *fw_version, size_t fw_size); /**< > @internal Get > > +firmware information of an Ethernet device. */ > > + >=20 > If we finally have different return value for > rte_eth_dev_fw_version_get() and here, > it would be useful to highlight it here. If I keep the handle of snprintf() return value in drivers, they will not h= ave different return here, so the highlight is not necessary. > > typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev, > > uint16_t rx_queue_id, struct rte_eth_rxq_info *qinfo); > > > > @@ -1459,6 +1463,7 @@ struct eth_dev_ops { > > eth_dev_infos_get_t dev_infos_get; /**< Get device info. */ > > eth_rxq_info_get_t rxq_info_get; /**< retrieve RX queue > information. */ > > eth_txq_info_get_t txq_info_get; /**< retrieve TX queue > information. */ > > + eth_fw_version_get_t fw_version_get; /**< Get firmware > version. */ > > eth_dev_supported_ptypes_get_t dev_supported_ptypes_get; > > /**< Get packet types supported and identified by device. */ > > > > @@ -2396,6 +2401,27 @@ void rte_eth_macaddr_get(uint8_t port_id, > struct ether_addr *mac_addr); > > void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info > > *dev_info); > > > > /** > > + * Retrieve the firmware version of a device. > > + * > > + * @param port_id > > + * The port identifier of the device. > > + * @param fw_version > > + * A array pointer to store the firmware version of a device, > > + * allocated by caller. > > + * @param fw_size > > + * The size of the array pointed by fw_version, which should be > > + * large enough to store firmware version of the device. > > + * @return > > + * - (0) if successful. > > + * - (-ENOTSUP) if operation is not supported. > > + * - (-ENODEV) if *port_id* invalid. > > + * - (>0) if *fw_size* is not enough to store firmware version, retu= rn > > + * the size of the non truncated string. >=20 > It is OK for me to keep 0 for success here and it is right in this case t= o include > terminating null iin the return value if size is insufficient (to cover c= orner case > with empty FW version). > Please, highlight that terminating null is included here. It is the diffe= rence > from snprintf() and it should be 100% clear. >=20 You are right, I'll highlight that the firmware version stored here is incl= ude '\0'. > > + */ > > +int rte_eth_dev_fw_version_get(uint8_t port_id, > > + char *fw_version, size_t fw_size); > > + > > +/** > > * Retrieve the supported packet types of an Ethernet device. > > * > > * When a packet type is announced as supported, it *must* be > > recognized by diff --git a/lib/librte_ether/rte_ether_version.map > > b/lib/librte_ether/rte_ether_version.map > > index 0c2859e..c6c9d0d 100644 > > --- a/lib/librte_ether/rte_ether_version.map > > +++ b/lib/librte_ether/rte_ether_version.map > > @@ -146,6 +146,7 @@ DPDK_17.02 { > > global: > > > > _rte_eth_dev_reset; > > + rte_eth_dev_fw_version_get; > > rte_flow_create; > > rte_flow_destroy; > > rte_flow_flush; >=20