From mboxrd@z Thu Jan  1 00:00:00 1970
From: Remy Horton <remy.horton@intel.com>
Subject: Re: [PATCH v1 1/3] drivers/net/i40e: Add ethdev functions
Date: Wed, 24 Feb 2016 10:32:26 +0000
Message-ID: <56CD86BA.7040702@intel.com>
References: <1453970895-2639-1-git-send-email-remy.horton@intel.com>
 <1453970895-2639-2-git-send-email-remy.horton@intel.com>
 <F35DEAC7BCE34641BA9FAC6BCA4A12E70A9AE917@SHSMSX104.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: "Zhang, Helin" <helin.zhang@intel.com>,
 "Xie, Huawei" <huawei.xie@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
 by dpdk.org (Postfix) with ESMTP id 76D702BB0
 for <dev@dpdk.org>; Wed, 24 Feb 2016 11:32:29 +0100 (CET)
In-Reply-To: <F35DEAC7BCE34641BA9FAC6BCA4A12E70A9AE917@SHSMSX104.ccr.corp.intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Comments inline.

..Remy

On 23/02/2016 02:06, Zhang, Helin wrote:
 >
 >> +static inline int
 >> +i40e_read_regs(struct i40e_hw *hw, const struct reg_info *reg,
 >> +	       uint32_t *reg_buf)
 >> +{
 >> +	unsigned int i;
 >> +
 >> +	for (i = 0; i < reg->count; i++)
 >> +		reg_buf[i] = I40E_READ_REG(hw,
 >> +			reg->base_addr + i * reg->stride);
 >> +	return reg->count;
 >> +}
 >  From FVL5, some registers should be read by AQ commands, otherwise 
it may fail to
 > read without any warning.
 > Please see my patches of which registers should be read by AQ commands.
 > Please check i40e_osdep.h from below link. Thanks!
 > http://www.dpdk.org/dev/patchwork/patch/10654/

Ok - will change for v2.

I noticed that other patches in the same patchset expose extra registers 
- are these new or were they simply not exposed previously?


 >> +	/* Only support doing full dump */
 >> +	if (regs->offset != 0 && 0)
 > '&& 0' means it will never be false, right?
 > Anything wrong here?

Oops - some dead code that slipped through.. :)


 >> +		return -ENOTSUP;
 > A message before this return to tell the uers what happened would be 
better.

Will add these into v2.


 >> +static int i40e_get_eeprom_length(__rte_unused struct rte_eth_dev *dev)
 > Why needs __rte_unused?

Good point - surprised the compiler did not complain about them, as they 
are not supposed to be there..


 >> +static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 >> +				      struct ether_addr *mac_addr)
 >> +{
 >> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
 >>> dev_private);
 >> +
 >> +	/* Flags: 0x3 updates port address */
 >> +	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes,
 >> NULL); }
 > Checks are needed before writing the MAC address.

Will look into this.


 >> +struct reg_info {
 >> +	uint32_t base_addr;
 >> +	uint32_t count;
 >> +	uint32_t stride;
 >> +	const char *name;
 >> +} reg_info;
 > I think array definition shouldn't be added into a header file, 
otherwise any .c source
 > file which includes that header file will define that.

Since it is quite a large table I think this approach, which is also 
used in ixgbe, is the lesser of evils. i40e_ethdev.c itself is already 
pretty big, and would prefer to avoid giving a driver-specific table 
non-static visibility until it actually has to be used from other 
compilation units.