Re: [dpdk-dev] [PATCH v4 01/15] net/avf/base: add base code for avf PMD

["Lu, Wenzhuo" <wenzhuo.lu@intel.com>]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, January 6, 2018 4:26 AM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 01/15] net/avf/base: add base code for avf
> PMD
> 
> O
> > diff --git a/drivers/net/avf/base/avf_adminq.c
> > b/drivers/net/avf/base/avf_adminq.c
> > new file mode 100644
> > index 0000000..616e2a9
> > --- /dev/null
> > +++ b/drivers/net/avf/base/avf_adminq.c
> > @@ -0,0 +1,1010 @@
> >
> +/**************************************************************
> ******
> > +***********
> > +
> > +Copyright (c) 2013 - 2015, Intel Corporation All rights reserved.
> 
> SPDX instead of more boilerplate.
> Copyright 2018?
> 
> > +STATIC void avf_adminq_init_regs(struct avf_hw *hw)
> 
> Why is there a STATIC macro??
> 
> ...
> 
> > +/**
> > + *  avf_config_asq_regs - configure ASQ registers
> > + *  @hw: pointer to the hardware structure
> > + *
> > + *  Configure base address and length registers for the transmit
> > +queue  **/ STATIC enum avf_status_code avf_config_asq_regs(struct
> > +avf_hw *hw) {
> > +	enum avf_status_code ret_code = AVF_SUCCESS;
> > +	u32 reg = 0;
> > +
> > +	/* Clear Head and Tail */
> > +	wr32(hw, hw->aq.asq.head, 0);
> > +	wr32(hw, hw->aq.asq.tail, 0);
> > +
> > +	/* set starting point */
> > +#ifdef INTEGRATED_VF
> > +	if (avf_is_vf(hw))
> > +		wr32(hw, hw->aq.asq.len, (hw->aq.num_asq_entries |
> > +					  AVF_ATQLEN1_ATQENABLE_MASK));
> > +#else
> > +	wr32(hw, hw->aq.asq.len, (hw->aq.num_asq_entries |
> > +				  AVF_ATQLEN1_ATQENABLE_MASK));
> > +#endif /* INTEGRATED_VF */
> 
> No ifdef please? do it in header file if you have to.
> as in:
> #ifdef INTERGRATED_VF
> #define avf_is_vf(hw)	(1)
> 
> ...
> 
> 
> > +/* internal (0x00XX) commands */
> > +
> > +/* Get version (direct 0x0001) */
> > +struct avf_aqc_get_version {
> > +	__le32 rom_ver;
> > +	__le32 fw_build;
> > +	__le16 fw_major;
> > +	__le16 fw_minor;
> > +	__le16 api_major;
> > +	__le16 api_minor;
> > +};
> 
> The use of __le16 and __le32 is a Linux kernel code style, typically not used
> in DPDK userland.
> 
> Are you trying to share code here?
> 
> ...
> 
> > +/**
> > + * virtchnl_vc_validate_vf_msg
> > + * @ver: Virtchnl version info
> > + * @v_opcode: Opcode for the message
> > + * @msg: pointer to the msg buffer
> > + * @msglen: msg length
> > + *
> > + * validate msg format against struct for each opcode  */ static
> > +inline int virtchnl_vc_validate_vf_msg(struct virtchnl_version_info
> > +*ver, u32 v_opcode,
> > +			    u8 *msg, u16 msglen)
> > +{
> >
> 
> This function is way to big to be an inline.
Thanks for your comments. Let me explain. This is the base code, like what's in ixgbe, i40e ... We have to let it be so it's much easier for us to update it the next time. That's why the code style is a little different. And also some checkpatch problem not handled. 
We have had some discussion about the copyright license here and internally. But unfortunately we don't achieve a conclusion internally so we have to keep the long license now and may change it later.