From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v1 04/11] drivers/raw/ifpga_rawdev: add IPN3KE support for IFPGA Rawdev Date: Wed, 6 Mar 2019 12:31:14 +0000 Message-ID: References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1551338000-120348-5-git-send-email-rosen.xu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: tianfei.zhang@intel.com, dan.wei@intel.com, andy.pei@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, santos.chen@intel.com, zhang.zhang@intel.com, Hemant Agrawal , Shreyansh Jain To: Rosen Xu , dev@dpdk.org Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3A69854AE for ; Wed, 6 Mar 2019 13:31:19 +0100 (CET) In-Reply-To: <1551338000-120348-5-git-send-email-rosen.xu@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2/28/2019 7:13 AM, Rosen Xu wrote: > Add Intel FPGA Acceleration NIC IPN3KE support for IFPGA Rawdev. > > Signed-off-by: Rosen Xu > Signed-off-by: Tianfei Zhang > Signed-off-by: Andy Pei <...> > +static int > +ifgpa_rawdev_get_attr(struct rte_rawdev *dev, > + const char *attr_name, > + uint64_t *attr_value) > +{ > + struct ifpga_rawdev_mac_info *mac_info; > + struct ifpga_rawdevg_retimer_info *retimer_info; > + struct opae_retimer_info or_info; > + struct opae_adapter *adapter; > + struct opae_manager *mgr; > + struct ifpga_rawdevg_link_info *linfo; > + struct opae_retimer_status rstatus; > + > + IFPGA_RAWDEV_PMD_FUNC_TRACE(); > + > + if (!dev || !attr_name || !attr_value) { > + IFPGA_BUS_ERR("Invalid arguments for getting attributes"); > + return -1; > + } > + > + adapter = ifpga_rawdev_get_priv(dev); > + if (!adapter) > + return -1; > + > + mgr = opae_adapter_get_mgr(adapter); > + if (!mgr) > + return -1; > + > + if (!strcmp(attr_name, "retimer_info")) { > + retimer_info = (struct ifpga_rawdevg_retimer_info *)attr_value; How you are using the 'get_attr' & 'set_attr' is a little odd. I think the intention with these APIs to provide a map/dictionary like data structure, save/get 'attr_values' by 'attr_name'. How you are using is, instead of getting 'attr_value', you are passing valid pointers via 'attr_value' and make this function to fill that struct. Why you don't have a specific function for this purpose instead of using 'get_attr' & 'set_attr' ops? I believe this will be source of confusion in long term. > + if (opae_manager_get_retimer_info(mgr, &or_info)) > + return -1; > + > + retimer_info->retimer_num = or_info.num_retimer; > + retimer_info->port_num = or_info.num_port; > + switch (or_info.support_speed) { > + case MXD_10GB: > + retimer_info->mac_type = > + IFPGA_RAWDEV_RETIMER_MAC_TYPE_10GE_XFI; > + break; > + case MXD_25GB: > + retimer_info->mac_type = > + IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI; > + break; > + case MXD_40GB: > + retimer_info->mac_type = > + IFPGA_RAWDEVG_RETIMER_MAC_TYPE_40GE_XLAUI; > + break; > + case MXD_100GB: > + retimer_info->mac_type = > + IFPGA_RAWDEV_RETIMER_MAC_TYPE_100GE_CAUI; > + break; > + default: > + retimer_info->mac_type = > + IFPGA_RAWDEV_RETIMER_MAC_TYPE_UNKNOWN; > + break; > + } > + return 0; > + } else if (!strcmp(attr_name, "default_mac")) { > + /* not implement by MAX */ > + mac_info = (struct ifpga_rawdev_mac_info *)attr_value; > + mac_info->addr.addr_bytes[0] = 0; > + mac_info->addr.addr_bytes[1] = 0; > + mac_info->addr.addr_bytes[2] = 0; > + mac_info->addr.addr_bytes[3] = 0; > + mac_info->addr.addr_bytes[4] = 0; > + mac_info->addr.addr_bytes[5] = 0xA + mac_info->port_id; > + > + return 0; > + } else if (!strcmp(attr_name, "retimer_linkstatus")) { > + linfo = (struct ifpga_rawdevg_link_info *)attr_value; > + linfo->link_up = 0; > + linfo->link_speed = IFPGA_RAWDEV_LINK_SPEED_UNKNOWN; > + > + if (opae_manager_get_retimer_status(mgr, linfo->port, &rstatus)) > + return -1; > + > + linfo->link_up = rstatus.line_link; > + switch (rstatus.speed) { > + case MXD_10GB: > + linfo->link_speed = > + IFPGA_RAWDEV_LINK_SPEED_10GB; > + break; > + case MXD_25GB: > + linfo->link_speed = > + IFPGA_RAWDEV_LINK_SPEED_25GB; > + break; > + case MXD_40GB: > + linfo->link_speed = > + IFPGA_RAWDEV_LINK_SPEED_40GB; > + break; > + default: > + linfo->link_speed = > + IFPGA_RAWDEV_LINK_SPEED_UNKNOWN; > + break; > + } > + > + return 0; > + } else > + return -1; > + > + /* Attribute not found */ > + return -1; > +} > + > +static int ifgpa_rawdev_set_attr(struct rte_rawdev *dev, > + const char *attr_name, > + const uint64_t attr_value) > +{ > + struct opae_adapter *adapter; > + struct opae_manager *mgr; > + /*struct ifpga_rawdevg_link_info *linfo;*/ > + /*struct opae_retimer_status rstatus;*/ Please remove commented out code. > + > + IFPGA_RAWDEV_PMD_FUNC_TRACE(); > + > + if (!dev || !attr_name) { > + IFPGA_BUS_ERR("Invalid arguments for setting attributes"); > + return -1; > + } > + > + adapter = ifpga_rawdev_get_priv(dev); > + if (!adapter) > + return -1; > + > + mgr = opae_adapter_get_mgr(adapter); > + if (!mgr) > + return -1; nothing done with 'adapter' & 'mgr' ? Why getting them? > + > + if (!strcmp(attr_name, "retimer_linkstatus")) > + printf("ifgpa_rawdev_set_attr_func %lx\n", attr_value); %lx usage is wrong for 32bits, it is giving a build warning [1], also please don't use 'printf' for logging. And why there is a specific check for a value and log, in a generic function like set_attr? [1] .../drivers/raw/ifpga_rawdev/ifpga_rawdev.c: In function ‘ifgpa_rawdev_set_attr’: .../drivers/raw/ifpga_rawdev/ifpga_rawdev.c:465:40: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint64_t’ {aka ‘const long long unsigned int’} [-We$ ror=format=] printf("ifgpa_rawdev_set_attr_func %lx\n", attr_value); ~~^ ~~~~~~~~~~ %llx > + > + return -1; Will function always fail? And are you aware the you did not set anything in this function? Just ignored the 'attr_value'? <...> > @@ -419,7 +559,7 @@ > > rawdev->dev_ops = &ifpga_rawdev_ops; > rawdev->device = &pci_dev->device; > - rawdev->driver_name = pci_dev->device.driver->name; > + rawdev->driver_name = pci_dev->driver->driver.name; They are both same right? <...> > + > +#include > + > +#ifndef ETH_ALEN > +#define ETH_ALEN 6 > +#endif You can use "ETHER_ADDR_LEN" instead, it is defined in above included 'rte_ether.h' header.