[dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev PMD driver

[Xu, Rosen]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Tuesday, April 09, 2019 23:19
> To: Xu, Rosen <rosen.xu at intel.com>
> Cc: dev at dpdk.org; Yigit, Ferruh <ferruh.yigit at intel.com>; Zhang, Tianfei
> <tianfei.zhang at intel.com>; Wei, Dan <dan.wei at intel.com>; Pei, Andy
> <andy.pei at intel.com>; Yang, Qiming <qiming.yang at intel.com>; Wang,
> Haiyue <haiyue.wang at intel.com>; Chen, Santos <santos.chen at intel.com>;
> Zhang, Zhang <zhang.zhang at intel.com>; Lomartire, David
> <david.lomartire at intel.com>; Hu, Jia <jia.hu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev
> PMD driver
> 
> Minor nits.

My reply is online.

> > +
> > +	(*rd_data) = rte_le_to_cpu_32(read_data);
> > +	return 0;
> > +}
> 
> Paren around *rd_data are unnecessary

Fixed in v7.

> > +	indirect_addrs = (volatile void *)(hw->eth_group_bar[eth_group_sel]
> +
> > +		0x10);
> 
> cast is to void * is unnecesary

Fixed in v7.
 
> +	return ipn3ke_indirect_read(hw,
> +				rd_data,
> +				addr,
> +				dev_sel,
> +				eth_group_sel);
> 
> One parameter per line is not necessary here.

Fixed in v7.

> +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.

I have checked, it follows eth_dev_name definition.
 
> 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 = (struct ipn3ke_hw *)afu_dev->shared.data;
> 
> Cast of void * is unnecessary in C.

Fixed in v7.

> > +	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.

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 = 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.

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);
> 
> 
> Indentation issue here?

No, I have checked in Vim of CentOS and Ubuntu. No indentation issue.
Pls check your vim configuration. Thanks.
 
> 
> > +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?

Changed to:
#define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \
	rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "%s(): " fmt "\n", \
		__func__, ##args)