From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4C4F137B4 for ; Thu, 9 Mar 2017 04:30:45 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Mar 2017 19:30:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,266,1486454400"; d="scan'208";a="73965594" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga005.fm.intel.com with ESMTP; 08 Mar 2017 19:30:44 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 8 Mar 2017 19:30:44 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 8 Mar 2017 19:30:44 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.177]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Thu, 9 Mar 2017 11:30:42 +0800 From: "Xing, Beilei" To: "Yigit, Ferruh" , "Wu, Jingjing" CC: "Zhang, Helin" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 1/5] net/i40e: support pipeline personalization profile Thread-Index: AQHSmASOGCN3G/Tjtke3NFSFHBtp2qGL1mjg Date: Thu, 9 Mar 2017 03:30:42 +0000 Message-ID: <94479800C636CB44BD422CB454846E01315A7BA5@SHSMSX101.ccr.corp.intel.com> References: <1488525977-15321-7-git-send-email-beilei.xing@intel.com> <1488526784-16621-1-git-send-email-beilei.xing@intel.com> <1488526784-16621-2-git-send-email-beilei.xing@intel.com> <51f0202e-844b-a8be-ced9-a253b2fc8717@intel.com> In-Reply-To: <51f0202e-844b-a8be-ced9-a253b2fc8717@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 1/5] net/i40e: support pipeline personalization profile 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: Thu, 09 Mar 2017 03:30:46 -0000 > -----Original Message----- > From: Yigit, Ferruh > Sent: Wednesday, March 8, 2017 8:07 PM > To: Xing, Beilei ; Wu, Jingjing > Cc: Zhang, Helin ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/5] net/i40e: support pipeline > personalization profile >=20 > On 3/3/2017 7:39 AM, Beilei Xing wrote: > > Add admin queue functions for Pipeline Personalization Profile AQ > > commands defined in DCR 287: >=20 > You can drop DCR reference here. Sorry as the DCR in my hand is also draft and intel confidential. I will sh= are it if it's released. >=20 > > - Write Recipe Command buffer (Opcode: 0x0270) > > - Get Applied Profiles list (Opcode: 0x0271) This patch will be moved > > to base driver in future. > > > > Signed-off-by: Beilei Xing >=20 > <...> >=20 > > + > > +/** > > + * i40e_aq_write_ppp - Write pipeline personalization profile (ppp) > > + * @hw: pointer to the hw struct > > + * @buff: command buffer (size in bytes =3D buff_size) > > + * @buff_size: buffer size in bytes > > + * @track_id: package tracking id > > + * @error_offset: returns error offset > > + * @error_info: returns error information > > + * @cmd_details: pointer to command details structure or NULL **/ > > +enum i40e_status_code i40e_aq_write_ppp(struct i40e_hw *hw, void > > +*buff, > > + uint16_t buff_size, uint32_t track_id, > > + uint32_t *error_offset, uint32_t > *error_info, > > + struct i40e_asq_cmd_details *cmd_details) >=20 > Is there a reason to not make these functions "static" ? No. In fact, I design the patch in base driver first, and follow the base = code style. Agree to use "static" if the function is in PMD. >=20 > What do you think making function calls more consistent, below is > "i40e_aq_get_ppp_list", so thi can be "i40e_aq_ppp_write" >=20 > And since this is NIC driver, ppp cane be confused with "Point-to-Point > Protocol", is there any possible abbreviation or capitalization to preven= t > confusion? Yes, will consider about it. >=20 > <...> >=20 > > +/** > > + * i40e_find_segment_in_package > > + * @segment_type: the segment type to search for (i.e., > > +SEGMENT_TYPE_I40E) > > + * @pkg_hdr: pointer to the package header to be searched > > + * > > + * This function searches a package file for a particular segment > > +type. On > > + * success it returns a pointer to the segment header, otherwise it > > +will > > + * return NULL. > > + */ > > +struct i40e_generic_seg_header * > > +i40e_find_segment_in_package(uint32_t segment_type, > > + struct i40e_package_header *pkg_hdr) >=20 > This function name is also generic, what segment, what package. if this i= s > related to the ppp, please use relevant function name. OK. >=20 > <...> >=20 > > + > > +/** > > + * i40e_write_profile > > + * @hw: pointer to the hardware structure > > + * @profile: pointer to the profile segment of the package to be > > +downloaded > > + * @track_id: package tracking id > > + * > > + * Handles the download of a complete package. > > + */ > > +enum i40e_status_code > > +i40e_write_profile(struct i40e_hw *hw, struct i40e_profile_segment > *profile, > > + uint32_t track_id) >=20 > What about having "ppp" in API, like i40e_ppp_write()? Seems good. >=20 > <...> >=20 > > +struct i40e_profile_section_header { > > + uint16_t tbl_size; > > + uint16_t data_end; > > + struct { > > +#define SECTION_TYPE_INFO 0x0000010 > > +#define SECTION_TYPE_MMIO 0x0000800 > > +#define SECTION_TYPE_AQ 0x0000801 >=20 > unaligned >=20 > > +#define SECTION_TYPE_NOTE 0x80000000 > > +#define SECTION_TYPE_NAME 0x80000001 > > + uint32_t type; > > + uint32_t offset; > > + uint32_t size; > > + } section; > > +}; > > + >=20 > <...> >=20 > > @@ -805,6 +915,23 @@ int i40e_dev_tunnel_filter_set(struct i40e_pf *pf, > > struct rte_eth_tunnel_filter_conf *tunnel_filter, > > uint8_t add); > > int i40e_fdir_flush(struct rte_eth_dev *dev); > > +enum i40e_status_code i40e_aq_write_ppp(struct i40e_hw *hw, void > *buff, > > + uint16_t buff_size, uint32_t track_id, > > + uint32_t *error_offset, uint32_t *error_info, > > + struct i40e_asq_cmd_details *cmd_details); > enum i40e_status_code > > +i40e_aq_get_ppp_list(struct i40e_hw *hw, void *buff, > > + uint16_t buff_size, uint8_t flags, > > + struct i40e_asq_cmd_details *cmd_details); > struct > > +i40e_generic_seg_header * i40e_find_segment_in_package(uint32_t > > +segment_type, > > + struct i40e_package_header *pkg_header); enum > > +i40e_status_code i40e_write_profile(struct i40e_hw *hw, struct > > +i40e_profile_segment *i40e_seg, > > + uint32_t track_id); > > +enum i40e_status_code > > +i40e_add_pinfo_to_list(struct i40e_hw *hw, > > + struct i40e_profile_segment *profile, > > + uint8_t *profile_info_sec, uint32_t track_id); >=20 > These declarations can go away if you make functions static. Yes. >=20 > > > > #define I40E_DEV_TO_PCI(eth_dev) \ > > RTE_DEV_TO_PCI((eth_dev)->device) > >