From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 7BCA1A0471 for ; Tue, 18 Jun 2019 18:07:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4198B1C1A8; Tue, 18 Jun 2019 18:07:40 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id EB7FD1C0D4 for ; Tue, 18 Jun 2019 18:07:38 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jun 2019 09:07:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,389,1557212400"; d="scan'208";a="334899766" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 18 Jun 2019 09:07:37 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 18 Jun 2019 09:07:37 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 18 Jun 2019 09:07:36 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.83]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.173]) with mapi id 14.03.0439.000; Wed, 19 Jun 2019 00:07:34 +0800 From: "Wu, Jingjing" To: "Li, Xiaoyun" , "Wiles, Keith" , "Liang, Cunming" , "Maslekar, Omkar" CC: "dev@dpdk.org" Thread-Topic: [PATCH v6 1/6] raw/ntb: introduce ntb rawdev driver Thread-Index: AQHVJXsyzeJckvwvJ0mpktUJCsAjM6ahVIRg Date: Tue, 18 Jun 2019 16:07:34 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F81145FE71@SHSMSX103.ccr.corp.intel.com> References: <20190614021940.78631-1-xiaoyun.li@intel.com> <20190618021055.12709-1-xiaoyun.li@intel.com> <20190618021055.12709-2-xiaoyun.li@intel.com> In-Reply-To: <20190618021055.12709-2-xiaoyun.li@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjk5NzFhOTYtODZmNS00ZjJlLWFkODMtMzQyMzE5OWUyNTA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWU5FdHByaDhRb3Z2WGE1Q3k5Q0o2ZFF0RUw0bGFwVE9cL2tDQjhJdFAra25GaVdnWUgwcURyaFwvb3RUY1M4ak9YIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action 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 v6 1/6] raw/ntb: introduce ntb rawdev driver 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" > + > +static void > +ntb_dev_info_get(struct rte_rawdev *dev, rte_rawdev_obj_t dev_info) > +{ > + struct ntb_hw *hw =3D dev->dev_private; > + struct ntb_attr *ntb_attrs =3D dev_info; > + > + strncpy(ntb_attrs[NTB_TOPO_ID].name, NTB_TOPO_NAME, > NTB_ATTR_NAME_LEN); > + switch (hw->topo) { > + case NTB_TOPO_B2B_DSD: > + strncpy(ntb_attrs[NTB_TOPO_ID].value, "B2B DSD", > + NTB_ATTR_NAME_LEN); > + break; > + case NTB_TOPO_B2B_USD: > + strncpy(ntb_attrs[NTB_TOPO_ID].value, "B2B USD", > + NTB_ATTR_NAME_LEN); > + break; > + default: > + strncpy(ntb_attrs[NTB_TOPO_ID].value, "Unsupported", > + NTB_ATTR_NAME_LEN); > + } > + > + strncpy(ntb_attrs[NTB_LINK_STATUS_ID].name, NTB_LINK_STATUS_NAME, > + NTB_ATTR_NAME_LEN); > + snprintf(ntb_attrs[NTB_LINK_STATUS_ID].value, NTB_ATTR_NAME_LEN, > + "%d", hw->link_status); > + You are sharing NTB_ATTR_NAME_LEN for both name and value? How about NTB_AT= TR_NUM_LEN/NTB_ATTR_VALUE_LEN? [...] > + if (!strncmp(attr_name, NTB_PEER_SPAD_14, NTB_ATTR_NAME_LEN)) { > + if (hw->ntb_ops->spad_write =3D=3D NULL) > + return -ENOTSUP; > + (*hw->ntb_ops->spad_write)(dev, 14, 1, attr_value); > + NTB_LOG(INFO, "Set attribute (%s) Value (%" PRIu64 ")", > + attr_name, attr_value); > + return 0; > + } > + > + if (!strncmp(attr_name, NTB_PEER_SPAD_15, NTB_ATTR_NAME_LEN)) { > + if (hw->ntb_ops->spad_write =3D=3D NULL) > + return -ENOTSUP; > + (*hw->ntb_ops->spad_write)(dev, 15, 1, attr_value); > + NTB_LOG(INFO, "Set attribute (%s) Value (%" PRIu64 ")", > + attr_name, attr_value); > + return 0; > + } > + What are 14 and 15 for? Is that generic? > +static int > +ntb_init_hw(struct rte_rawdev *dev, struct rte_pci_device *pci_dev) > +{ > + struct ntb_hw *hw =3D dev->dev_private; > + int ret; > + > + hw->pci_dev =3D pci_dev; > + hw->peer_dev_up =3D 0; > + hw->link_status =3D 0; > + hw->link_speed =3D NTB_SPEED_NONE; > + hw->link_width =3D NTB_WIDTH_NONE; > + > + switch (pci_dev->id.device_id) { > + default: > + NTB_LOG(ERR, "Not supported device."); > + return -EINVAL; > + } > + > + if (hw->ntb_ops->ntb_dev_init =3D=3D NULL) > + return -ENOTSUP; > + ret =3D (*hw->ntb_ops->ntb_dev_init)(dev); > + if (ret) { > + NTB_LOG(ERR, "Unanle to init ntb dev."); Typo: unanle -> unable [...] > +static int > +ntb_rawdev_create(struct rte_pci_device *pci_dev, int socket_id) > +{ > + char name[RTE_RAWDEV_NAME_MAX_LEN]; > + struct rte_rawdev *rawdev =3D NULL; > + int ret; > + > + if (pci_dev =3D=3D NULL) { > + NTB_LOG(ERR, "Invalid pci_dev."); > + ret =3D -EINVAL; > + goto fail; Is there possibility to release rawdev at fail? Return might be enough. > + } > + > + memset(name, 0, sizeof(name)); > + snprintf(name, RTE_RAWDEV_NAME_MAX_LEN, "NTB:%x:%02x.%x", > + pci_dev->addr.bus, pci_dev->addr.devid, > + pci_dev->addr.function); > + > + NTB_LOG(INFO, "Init %s on NUMA node %d", name, rte_socket_id()); Why not use the parameter "socket_id"? > + > + /* Allocate device structure. */ > + rawdev =3D rte_rawdev_pmd_allocate(name, sizeof(struct ntb_hw), > + socket_id); > + if (rawdev =3D=3D NULL) { > + NTB_LOG(ERR, "Unable to allocate rawdev."); > + ret =3D -EINVAL; > + goto fail; No need to goto fail as rawdev in NULL. [...] > +enum ntb_link { > + NTB_LINK_DOWN =3D 0, > + NTB_LINK_UP, > +}; > + You defined the enum, but you used 0 and 1 above. Thanks Jingjing