From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by dpdk.org (Postfix) with ESMTP id D2C304C8F for ; Tue, 9 Apr 2019 17:18:50 +0200 (CEST) Received: by mail-pg1-f193.google.com with SMTP id i2so9456495pgj.11 for ; Tue, 09 Apr 2019 08:18:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=c18V5oPtGjb/ZMxw0L1qdBQU+int2qOwm+USd4G6/So=; b=G2+dJBYiHqy/fUwb9bI1D2fPlWNLaysSa4kQAfgUEvq14hAWoZT7/ak/jpbuCMyeYi fVogg7lOjHn4JZQ/5tg5us1r4/6y0gAoaRG/gx0NMyM5as+pjQFMoo0U71gcf51R6odd Xln5u5/xm/BvKypUrWmqUUW643p9u8xYdgFn4luhhgNj/CsPNunC1Im4OKYZRnHg7iCF trF8wzPZi5MZ+C+ksE+AtlYpSgEn3ZIb7JWjYOPVLm2gEYFIKaLZW5dvj/2MZHZafmav D4TuhK1VnG9F4R87HYOx9A3MfZWXMNitMFmkK8o98+sZjD5jtHzGIKnchuMANRua/alG M2tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=c18V5oPtGjb/ZMxw0L1qdBQU+int2qOwm+USd4G6/So=; b=Cg9dHZt9z8pqzsbwLyWjPuLUutT0jMwf+JjjHu2qjGQTQ4+NzVkJat1kuanTN9qkk8 HQoXWWz2RWs7X0l4j9vYLUEmDiWkwdrrIx3iQPBJ0OcOYM99E9ywmReyTgmCb9R5ozRT tYhgPgkDtAzXtNv8qTRHPwbwUmhVTHNDugPA9UBRAdJkiS05FdCLQ3KymPLbRwEFiYtG 3pta8A8HYIwZsGwIRQKwQVcqvAa+Lhz87p26Eyo33qJS5GxsoUTV+ZX/RY8D1eWo4w/K CQzni0eoVu+SK8MdgsJv2vmUvi8EzdzL5173RlFXeMd0ccXqBaTkV7pYKi/a4DxMiuc3 AuSA== X-Gm-Message-State: APjAAAUKPlPd7BRP8woliAS72dhYJsdouSpUozoj6sYviB4BrCTlRm81 ndF7PwZ5IHN434iZflogM2Gd/A== X-Google-Smtp-Source: APXvYqyeMLXo0e/QyPHob5S0Rp2amMX09h4O1oWBMBLlueGdT3LJF14VXiFJP3r3AufO0npTic2GYA== X-Received: by 2002:a63:e845:: with SMTP id a5mr36107609pgk.246.1554823129802; Tue, 09 Apr 2019 08:18:49 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id l2sm34529968pgl.2.2019.04.09.08.18.49 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 09 Apr 2019 08:18:49 -0700 (PDT) Date: Tue, 9 Apr 2019 08:18:46 -0700 From: Stephen Hemminger To: Rosen Xu Cc: dev@dpdk.org, ferruh.yigit@intel.com, 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, david.lomartire@intel.com, jia.hu@intel.com Message-ID: <20190409081846.1e27f9cb@shemminger-XPS-13-9360> In-Reply-To: <1554813689-26834-4-git-send-email-rosen.xu@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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Tue, 09 Apr 2019 15:18:51 -0000 Minor nits. > + > + (*rd_data) = rte_le_to_cpu_32(read_data); > + return 0; > +} Paren around *rd_data are unnecessary > + indirect_addrs = (volatile void *)(hw->eth_group_bar[eth_group_sel] + > + 0x10); cast is to void * is unnecesary + return ipn3ke_indirect_read(hw, + rd_data, + addr, + dev_sel, + eth_group_sel); One parameter per line is not necessary here. +static int +ipn3ke_hw_cap_init(struct ipn3ke_hw *hw) +{ Always returns 0 make it void > + /* representor port net_bdf_port */ > + snprintf(name, sizeof(name), "net_%s_representor_%d", > + afu_dev->device.name, i); That seems like a longer than necessary eth_dev_name and doesn't seem to follow the convention of using PCI address. Why do only Intel drivers call rte_eth_dev_create, everyone else calls rte_eth_dev_allocate()? > + > + hw = (struct ipn3ke_hw *)afu_dev->shared.data; Cast of void * is unnecessary in C. > + for (i = 0; i < hw->port_num; i++) { > + snprintf(name, sizeof(name), "net_%s_representor_%d", You use this format twice, it should be a #define. > +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw, > + uint32_t addr) > +{ > + uint64_t word_offset = 0; > + uint64_t read_data = 0; > + uint64_t indirect_value = 0; > + volatile void *indirect_addrs = 0; 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 compiler and static checkers to find code paths where data is used uninitialized. > +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); Indentation issue here? > +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) 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 than always ipn3ke_afu: in the log message? 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 6D5DAA0096 for ; Tue, 9 Apr 2019 17:18:53 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2E3414D3A; Tue, 9 Apr 2019 17:18:53 +0200 (CEST) Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by dpdk.org (Postfix) with ESMTP id D2C304C8F for ; Tue, 9 Apr 2019 17:18:50 +0200 (CEST) Received: by mail-pg1-f193.google.com with SMTP id i2so9456495pgj.11 for ; Tue, 09 Apr 2019 08:18:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=c18V5oPtGjb/ZMxw0L1qdBQU+int2qOwm+USd4G6/So=; b=G2+dJBYiHqy/fUwb9bI1D2fPlWNLaysSa4kQAfgUEvq14hAWoZT7/ak/jpbuCMyeYi fVogg7lOjHn4JZQ/5tg5us1r4/6y0gAoaRG/gx0NMyM5as+pjQFMoo0U71gcf51R6odd Xln5u5/xm/BvKypUrWmqUUW643p9u8xYdgFn4luhhgNj/CsPNunC1Im4OKYZRnHg7iCF trF8wzPZi5MZ+C+ksE+AtlYpSgEn3ZIb7JWjYOPVLm2gEYFIKaLZW5dvj/2MZHZafmav D4TuhK1VnG9F4R87HYOx9A3MfZWXMNitMFmkK8o98+sZjD5jtHzGIKnchuMANRua/alG M2tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=c18V5oPtGjb/ZMxw0L1qdBQU+int2qOwm+USd4G6/So=; b=Cg9dHZt9z8pqzsbwLyWjPuLUutT0jMwf+JjjHu2qjGQTQ4+NzVkJat1kuanTN9qkk8 HQoXWWz2RWs7X0l4j9vYLUEmDiWkwdrrIx3iQPBJ0OcOYM99E9ywmReyTgmCb9R5ozRT tYhgPgkDtAzXtNv8qTRHPwbwUmhVTHNDugPA9UBRAdJkiS05FdCLQ3KymPLbRwEFiYtG 3pta8A8HYIwZsGwIRQKwQVcqvAa+Lhz87p26Eyo33qJS5GxsoUTV+ZX/RY8D1eWo4w/K CQzni0eoVu+SK8MdgsJv2vmUvi8EzdzL5173RlFXeMd0ccXqBaTkV7pYKi/a4DxMiuc3 AuSA== X-Gm-Message-State: APjAAAUKPlPd7BRP8woliAS72dhYJsdouSpUozoj6sYviB4BrCTlRm81 ndF7PwZ5IHN434iZflogM2Gd/A== X-Google-Smtp-Source: APXvYqyeMLXo0e/QyPHob5S0Rp2amMX09h4O1oWBMBLlueGdT3LJF14VXiFJP3r3AufO0npTic2GYA== X-Received: by 2002:a63:e845:: with SMTP id a5mr36107609pgk.246.1554823129802; Tue, 09 Apr 2019 08:18:49 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id l2sm34529968pgl.2.2019.04.09.08.18.49 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 09 Apr 2019 08:18:49 -0700 (PDT) Date: Tue, 9 Apr 2019 08:18:46 -0700 From: Stephen Hemminger To: Rosen Xu Cc: dev@dpdk.org, ferruh.yigit@intel.com, 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, david.lomartire@intel.com, jia.hu@intel.com Message-ID: <20190409081846.1e27f9cb@shemminger-XPS-13-9360> In-Reply-To: <1554813689-26834-4-git-send-email-rosen.xu@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> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit 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: <20190409151846.u1u1AtoBy8K92_jsXmUIFhWbrr2k4o-JHJxjJN4ZGRs@z> Minor nits. > + > + (*rd_data) = rte_le_to_cpu_32(read_data); > + return 0; > +} Paren around *rd_data are unnecessary > + indirect_addrs = (volatile void *)(hw->eth_group_bar[eth_group_sel] + > + 0x10); cast is to void * is unnecesary + return ipn3ke_indirect_read(hw, + rd_data, + addr, + dev_sel, + eth_group_sel); One parameter per line is not necessary here. +static int +ipn3ke_hw_cap_init(struct ipn3ke_hw *hw) +{ Always returns 0 make it void > + /* representor port net_bdf_port */ > + snprintf(name, sizeof(name), "net_%s_representor_%d", > + afu_dev->device.name, i); That seems like a longer than necessary eth_dev_name and doesn't seem to follow the convention of using PCI address. Why do only Intel drivers call rte_eth_dev_create, everyone else calls rte_eth_dev_allocate()? > + > + hw = (struct ipn3ke_hw *)afu_dev->shared.data; Cast of void * is unnecessary in C. > + for (i = 0; i < hw->port_num; i++) { > + snprintf(name, sizeof(name), "net_%s_representor_%d", You use this format twice, it should be a #define. > +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw, > + uint32_t addr) > +{ > + uint64_t word_offset = 0; > + uint64_t read_data = 0; > + uint64_t indirect_value = 0; > + volatile void *indirect_addrs = 0; 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 compiler and static checkers to find code paths where data is used uninitialized. > +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); Indentation issue here? > +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) 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 than always ipn3ke_afu: in the log message?