From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
Subject: Re: [PATCH 2/2] net/i40e: configurable PTYPE mapping
Date: Tue, 7 Mar 2017 02:37:02 +0000
Message-ID: <039ED4275CED7440929022BC67E7061153063970@SHSMSX103.ccr.corp.intel.com>
References: <20170227045612.48030-1-qi.z.zhang@intel.com>
 <20170227045612.48030-3-qi.z.zhang@intel.com>
 <45a02a73-102d-129a-43da-605d8bdc271e@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "Wu, Jingjing"
 <jingjing.wu@intel.com>, "Zhang, Helin" <helin.zhang@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id D9153108A
 for <dev@dpdk.org>; Tue,  7 Mar 2017 03:37:08 +0100 (CET)
In-Reply-To: <45a02a73-102d-129a-43da-605d8bdc271e@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <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>

Hi Ferruh:
=09
	Thanks all the good suggestion and help.

=09
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 6, 2017 11:32 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] net/i40e: configurable PTYPE mapping
>=20
> On 2/27/2017 4:56 AM, Qi Zhang wrote:
> > The patch adds 4 APIs to support configurable PTYPE mapping for i40e
> > device.
> > rte_pmd_i40e_update_ptype_mapping.
> > rte_pmd_i40e_reset_ptype_mapping.
> > rte_pmd_i40e_get_ptype_mapping.
> > rte_pmd_i40e_replace_ptype_mapping.
>=20
> Hi Qi,
>=20
> These are added as PMD specific APIs, but not used anywhere, how did you
> test them? Or how others can test it?
>=20
> Does it make sense to add them into testpmd?
>=20
Yes, I plan to add=20
>=20
>=20
> And related to API naming, what do you think about following syntax:
> <name_space>_<object>_<action> ?
>=20
> This helps finding APIs for same object (ptype_mapping for this case):
>=20
> rte_pmd_i40e_ptype_mapping_update()
> rte_pmd_i40e_ptype_mapping_reset()
> rte_pmd_i40e_ptype_mapping_get()
> rte_pmd_i40e_ptype_mapping_replace()

Agree, that's good.
>=20
>=20
> And not directly related to this patch, but it is good idea to extract PM=
D
> specific APIs into separate file, would you mind taking that task before =
this
> patch? And add these new APIs to that new file?
>=20
I like this idea, since we have many changes on this recently, I'd better d=
iscuss with=20
other team member to decide if do the separation before merge or after the =
merge.
>=20
> > The mapping from hardware defined packet type to software defined
> > packet type can be updated/reset/read out with these APIs.
> > Also a software ptype with the most significent bit set will be
> > regarded as a custom defined ptype
> > (RTE_PMD_I40E_PTYPE_USER_DEFINE_MASK) so application can use it to
> defined its own PTYPE naming system.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c  | 190
> ++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/i40e_rxtx.c    |   1 -
> >  drivers/net/i40e/i40e_rxtx.h    |   2 +
> >  drivers/net/i40e/rte_pmd_i40e.h |  81 +++++++++++++++++
>=20
> Need to update *version.map file too, for shared library build. It is har=
d to
> catch these issues since APIs are not used.
>=20
Will fix.
> <...>
>=20
> > +
> > +int rte_pmd_i40e_update_ptype_mapping(
>=20
> DPDK coding convention suggest having return type in separate line:
> int
> rte_pmd_i40e_update_ptype_mapping(...
>=20
> > +			uint8_t port,
> > +			struct rte_pmd_i40e_ptype_mapping *mapping_items,
> > +			uint16_t count,
> > +			uint8_t exclusive)
> > +{
> > +	struct rte_eth_dev *dev;
> > +	struct i40e_adapter *ad;
> > +	int i;
>=20
> For PMD specific APIs, port_id needs to be verified if it is i40e port or=
 not.
> There is already is_device_supported() function in i40e for this.

Thanks
>=20
> CC'ed Wenzhuo, he already did this a few times, and may help.
>=20
> <...>
>=20
> > +/**
> > + * Update hardware defined ptype to software defined packet type
> > + * mapping table.
> > + *
> > + * @param port
> > + *    pointer to port identifier of the device.
> > + * @param mapping_items
> > + *    the base address of the mapping items array.
> > + * @param count
> > + *    number of mapping items.
> > + * @param exclusive
> > + *    the flag indicate different ptype mapping update method.
> > + *    -(0) only overwrite refferred PTYPE mapping,
>=20
> referred
>=20
> > + *	keep other PTYPEs mapping unchanged.
> > + *    -(!0) overwrite referred PTYPE mapping,
> > + *	set other PTYPEs maps to PTYPE_UNKNOWN.
> > + */
> > +int rte_pmd_i40e_update_ptype_mapping(
> > +			uint8_t port,
> > +			struct rte_pmd_i40e_ptype_mapping *mapping_items,
> > +			uint16_t count,
> > +			uint8_t exclusive);
> > +
> > +/**
> > + * Reset hardware defined ptype to software defined ptype
> > + * mapping table to default.
> > + *
> > + * @param port
> > + *    pointer to port identifiier of the device
>=20
> s/identifiier/identifier
>=20
> <...>
>=20
> > +/**
> > + * Replace a specific or a group of software defined ptypes
> > + * with a new one
> > + *
> > + * @param port
> > + *    pointer to port identifier of the device
> > + * @param target
> > + *    the packet type to be replaced
> > + * @param mask
> > + *    -(0) target represent a specific software defined ptype.
> > + *    -(!0) target is a mask to represent a group of software defined
> ptypes.
> > + * @param pkt_type
> > + *    the new packet type to overwrite
> > + */
> > +int rte_pmd_i40e_replace_pkt_type(uint8_t port,
> > +				  uint32_t target,
> > +				  uint8_t mask,
> > +				  uint32_t pkt_type);
>=20
> API names does not match with one in commit log, and "pkt_type" usage is
> not consistent with other APIs.
Will fix.
>=20
> > +
> >  #endif /* _PMD_I40E_H_ */
> >