> -----Original Message-----
> From: Stephen Hemminger [mailto:[email protected]]
> Sent: Tuesday, April 09, 2019 23:19
> To: Xu, Rosen <[email protected]>
> Cc: [email protected]; Yigit, Ferruh <[email protected]>; Zhang, Tianfei
> <[email protected]>; Wei, Dan <[email protected]>; Pei, Andy
> <[email protected]>; Yang, Qiming <[email protected]>; Wang,
> Haiyue <[email protected]>; Chen, Santos <[email protected]>;
> Zhang, Zhang <[email protected]>; Lomartire, David
> <[email protected]>; Hu, Jia <[email protected]>
> 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)
