Hi Ferruh:
Thanks all the good suggestion and help.
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 6, 2017 11:32 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Wu, Jingjing
> <jingjing...@intel.com>; Zhang, Helin <helin.zh...@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] net/i40e: configurable PTYPE mapping
>
> 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.
>
> Hi Qi,
>
> These are added as PMD specific APIs, but not used anywhere, how did you
> test them? Or how others can test it?
>
> Does it make sense to add them into testpmd?
>
Yes, I plan to add
>
>
> And related to API naming, what do you think about following syntax:
> <name_space>_<object>_<action> ?
>
> This helps finding APIs for same object (ptype_mapping for this case):
>
> 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.
>
>
> And not directly related to this patch, but it is good idea to extract PMD
> specific APIs into separate file, would you mind taking that task before this
> patch? And add these new APIs to that new file?
>
I like this idea, since we have many changes on this recently, I'd better
discuss with
other team member to decide if do the separation before merge or after the
merge.
>
> > 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.zh...@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 +++++++++++++++++
>
> Need to update *version.map file too, for shared library build. It is hard to
> catch these issues since APIs are not used.
>
Will fix.
> <...>
>
> > +
> > +int rte_pmd_i40e_update_ptype_mapping(
>
> DPDK coding convention suggest having return type in separate line:
> int
> rte_pmd_i40e_update_ptype_mapping(...
>
> > + 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;
>
> 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
>
> CC'ed Wenzhuo, he already did this a few times, and may help.
>
> <...>
>
> > +/**
> > + * 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,
>
> referred
>
> > + * 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
>
> s/identifiier/identifier
>
> <...>
>
> > +/**
> > + * 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);
>
> API names does not match with one in commit log, and "pkt_type" usage is
> not consistent with other APIs.
Will fix.
>
> > +
> > #endif /* _PMD_I40E_H_ */
> >
