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 B957F559A for ; Wed, 10 Apr 2019 08:05:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Apr 2019 23:05:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,332,1549958400"; d="scan'208";a="335414198" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 09 Apr 2019 23:05:27 -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.408.0; Tue, 9 Apr 2019 23:05:27 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 9 Apr 2019 23:05:27 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.25]) with mapi id 14.03.0415.000; Wed, 10 Apr 2019 14:03:46 +0800 From: "Xu, Rosen" To: Stephen Hemminger CC: "dev@dpdk.org" , "Yigit, Ferruh" , "Zhang, Tianfei" , "Wei, Dan" , "Pei, Andy" , "Yang, Qiming" , "Wang, Haiyue" , "Chen, Santos" , "Zhang, Zhang" , "Lomartire, David" , "Hu, Jia" Thread-Topic: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev PMD driver Thread-Index: AQHU7tF7S60B8yYkIUWu2J31CLN0UqYza00AgAF6npA= Date: Wed, 10 Apr 2019 06:03:46 +0000 Message-ID: <0E78D399C70DA940A335608C6ED296D73A694C2F@SHSMSX104.ccr.corp.intel.com> References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1554813689-26834-1-git-send-email-rosen.xu@intel.com> <1554813689-26834-4-git-send-email-rosen.xu@intel.com> <20190409081846.1e27f9cb@shemminger-XPS-13-9360> In-Reply-To: <20190409081846.1e27f9cb@shemminger-XPS-13-9360> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjBjOWVkYWMtMDRlNy00YjY5LTljOWEtYzI1NzFlY2ZmMmZjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRkdRNHo4cm8waUdzNVl6dlFwbFhNZnd0ZnNmUEJ4emJNcXdENlwvejBUTkVQbmRqdmo1MTkxbFdnbjBSNDJoSEkifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 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 03/14] net/ipn3ke: add IPN3KE ethdev PMD 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: , X-List-Received-Date: Wed, 10 Apr 2019 06:05:29 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Tuesday, April 09, 2019 23:19 > To: Xu, Rosen > Cc: dev@dpdk.org; Yigit, Ferruh ; Zhang, Tianfei > ; Wei, Dan ; Pei, Andy > ; Yang, Qiming ; Wang, > Haiyue ; Chen, Santos ; > Zhang, Zhang ; Lomartire, David > ; Hu, Jia > Subject: Re: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev > PMD driver >=20 > Minor nits. My reply is online. > > + > > + (*rd_data) =3D rte_le_to_cpu_32(read_data); > > + return 0; > > +} >=20 > Paren around *rd_data are unnecessary Fixed in v7. > > + indirect_addrs =3D (volatile void *)(hw->eth_group_bar[eth_group_sel] > + > > + 0x10); >=20 > cast is to void * is unnecesary Fixed in v7. =20 > + return ipn3ke_indirect_read(hw, > + rd_data, > + addr, > + dev_sel, > + eth_group_sel); >=20 > One parameter per line is not necessary here. Fixed in v7. > +static int > +ipn3ke_hw_cap_init(struct ipn3ke_hw *hw) { >=20 > Always returns 0 make it void > > + /* representor port net_bdf_port */ > > + snprintf(name, sizeof(name), "net_%s_representor_%d", > > + afu_dev->device.name, i); >=20 > That seems like a longer than necessary eth_dev_name and doesn't seem to > follow the convention of using PCI address. I have checked, it follows eth_dev_name definition. =20 > Why do only Intel drivers call rte_eth_dev_create, everyone else calls > rte_eth_dev_allocate()? rte_eth_dev_create() is an API. All APIs may be called by other modules? Do you think so? > > + > > + hw =3D (struct ipn3ke_hw *)afu_dev->shared.data; >=20 > Cast of void * is unnecessary in C. Fixed in v7. > > + for (i =3D 0; i < hw->port_num; i++) { > > + snprintf(name, sizeof(name), "net_%s_representor_%d", >=20 > You use this format twice, it should be a #define. Any rules to define format used more than twice should be a #define? I have checked others' code, many usage as mine. > > +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw, > > + uint32_t addr) > > +{ > > + uint64_t word_offset =3D 0; > > + uint64_t read_data =3D 0; > > + uint64_t indirect_value =3D 0; > > + volatile void *indirect_addrs =3D 0; >=20 > You set all these values in next view lines so these initializations are = useless. > Doing extra initialization is bad since it overrides the ability of compi= ler and > static checkers to find code paths where data is used uninitialized. Fixed in v7. > > +static inline void ipn3ke_xmac_smac_ovd_dis(struct ipn3ke_hw *hw, > > + uint32_t mac_num, uint32_t eth_group_sel) { #define > > +IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE (0 & \ > > + (IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE_MASK)) > > + > > + (*hw->f_mac_write)(hw, > > + > IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE, > > + > IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE, > > + mac_num, > > + eth_group_sel); >=20 >=20 > Indentation issue here? No, I have checked in Vim of CentOS and Ubuntu. No indentation issue. Pls check your vim configuration. Thanks. =20 >=20 > > +extern int ipn3ke_afu_logtype; > > + > > +#define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \ > > + rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "ipn3ke_afu: " fmt, \ > > + ##args) >=20 > The common practice in Intel drivers is to put the newline in this macro = (and > remove it from the format strings). Also put the function name rather tha= n > always ipn3ke_afu: in the log message? Changed to: #define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \ rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "%s(): " fmt "\n", \ __func__, ##args) 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 45C9CA0096 for ; Wed, 10 Apr 2019 08:05:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BD0AA58EC; Wed, 10 Apr 2019 08:05:30 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id B957F559A for ; Wed, 10 Apr 2019 08:05:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Apr 2019 23:05:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,332,1549958400"; d="scan'208";a="335414198" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 09 Apr 2019 23:05:27 -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.408.0; Tue, 9 Apr 2019 23:05:27 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 9 Apr 2019 23:05:27 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.25]) with mapi id 14.03.0415.000; Wed, 10 Apr 2019 14:03:46 +0800 From: "Xu, Rosen" To: Stephen Hemminger CC: "dev@dpdk.org" , "Yigit, Ferruh" , "Zhang, Tianfei" , "Wei, Dan" , "Pei, Andy" , "Yang, Qiming" , "Wang, Haiyue" , "Chen, Santos" , "Zhang, Zhang" , "Lomartire, David" , "Hu, Jia" Thread-Topic: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev PMD driver Thread-Index: AQHU7tF7S60B8yYkIUWu2J31CLN0UqYza00AgAF6npA= Date: Wed, 10 Apr 2019 06:03:46 +0000 Message-ID: <0E78D399C70DA940A335608C6ED296D73A694C2F@SHSMSX104.ccr.corp.intel.com> References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1554813689-26834-1-git-send-email-rosen.xu@intel.com> <1554813689-26834-4-git-send-email-rosen.xu@intel.com> <20190409081846.1e27f9cb@shemminger-XPS-13-9360> In-Reply-To: <20190409081846.1e27f9cb@shemminger-XPS-13-9360> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjBjOWVkYWMtMDRlNy00YjY5LTljOWEtYzI1NzFlY2ZmMmZjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRkdRNHo4cm8waUdzNVl6dlFwbFhNZnd0ZnNmUEJ4emJNcXdENlwvejBUTkVQbmRqdmo1MTkxbFdnbjBSNDJoSEkifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev PMD 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" Message-ID: <20190410060346.nBcJI30-ga65a7yVVUuX-Kol97beD5UwZoh-ylx2KhA@z> > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Tuesday, April 09, 2019 23:19 > To: Xu, Rosen > Cc: dev@dpdk.org; Yigit, Ferruh ; Zhang, Tianfei > ; Wei, Dan ; Pei, Andy > ; Yang, Qiming ; Wang, > Haiyue ; Chen, Santos ; > Zhang, Zhang ; Lomartire, David > ; Hu, Jia > Subject: Re: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev > PMD driver >=20 > Minor nits. My reply is online. > > + > > + (*rd_data) =3D rte_le_to_cpu_32(read_data); > > + return 0; > > +} >=20 > Paren around *rd_data are unnecessary Fixed in v7. > > + indirect_addrs =3D (volatile void *)(hw->eth_group_bar[eth_group_sel] > + > > + 0x10); >=20 > cast is to void * is unnecesary Fixed in v7. =20 > + return ipn3ke_indirect_read(hw, > + rd_data, > + addr, > + dev_sel, > + eth_group_sel); >=20 > One parameter per line is not necessary here. Fixed in v7. > +static int > +ipn3ke_hw_cap_init(struct ipn3ke_hw *hw) { >=20 > Always returns 0 make it void > > + /* representor port net_bdf_port */ > > + snprintf(name, sizeof(name), "net_%s_representor_%d", > > + afu_dev->device.name, i); >=20 > That seems like a longer than necessary eth_dev_name and doesn't seem to > follow the convention of using PCI address. I have checked, it follows eth_dev_name definition. =20 > Why do only Intel drivers call rte_eth_dev_create, everyone else calls > rte_eth_dev_allocate()? rte_eth_dev_create() is an API. All APIs may be called by other modules? Do you think so? > > + > > + hw =3D (struct ipn3ke_hw *)afu_dev->shared.data; >=20 > Cast of void * is unnecessary in C. Fixed in v7. > > + for (i =3D 0; i < hw->port_num; i++) { > > + snprintf(name, sizeof(name), "net_%s_representor_%d", >=20 > You use this format twice, it should be a #define. Any rules to define format used more than twice should be a #define? I have checked others' code, many usage as mine. > > +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw, > > + uint32_t addr) > > +{ > > + uint64_t word_offset =3D 0; > > + uint64_t read_data =3D 0; > > + uint64_t indirect_value =3D 0; > > + volatile void *indirect_addrs =3D 0; >=20 > You set all these values in next view lines so these initializations are = useless. > Doing extra initialization is bad since it overrides the ability of compi= ler and > static checkers to find code paths where data is used uninitialized. Fixed in v7. > > +static inline void ipn3ke_xmac_smac_ovd_dis(struct ipn3ke_hw *hw, > > + uint32_t mac_num, uint32_t eth_group_sel) { #define > > +IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE (0 & \ > > + (IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE_MASK)) > > + > > + (*hw->f_mac_write)(hw, > > + > IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE, > > + > IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE, > > + mac_num, > > + eth_group_sel); >=20 >=20 > Indentation issue here? No, I have checked in Vim of CentOS and Ubuntu. No indentation issue. Pls check your vim configuration. Thanks. =20 >=20 > > +extern int ipn3ke_afu_logtype; > > + > > +#define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \ > > + rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "ipn3ke_afu: " fmt, \ > > + ##args) >=20 > The common practice in Intel drivers is to put the newline in this macro = (and > remove it from the format strings). Also put the function name rather tha= n > always ipn3ke_afu: in the log message? Changed to: #define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \ rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "%s(): " fmt "\n", \ __func__, ##args)