[dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop

[Iremonger, Bernard]

Hi Ferruh,
<snip>
> >>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> >>>>>
> >>>>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>>>>> Add a new private API to support the untag drop enable/disable
> >>>>>> for specific VF.
> >>>>>>
> >>>>>> Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>
> >>>>>> ---
> >>>>>>  drivers/net/i40e/i40e_ethdev.c  | 49
> >>>>>> +++++++++++++++++++++++++++++++++++++++++
> >>>>>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> >>>>>
> >>>>> Shared library is giving build error because of API is missing in
> >>>>> *version.map file
> >>>>>
> >>>>>>  2 files changed, 67 insertions(+)
> >>>>>>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> >>>>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t
> port,
> >>>>>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >>>>>>  				uint16_t vf_id);
> >>>>>>
> >>>>>> +/**
> >>>>>> + * Enable/Disable VF untag drop
> >>>>>> + *
> >>>>>> + * @param port
> >>>>>> + *    The port identifier of the Ethernet device.
> >>>>>> + * @param vf_id
> >>>>>> + *    VF on witch to enable/disable
> >>>>>> + * @param on
> >>>>>> + *    Enable or Disable
> >>>>>> + * @retura
> >>>>>
> >>>>> @return
> >>>>>
> >>>>>> + *  - (0) if successful.
> >>>>>> + *  -(-ENODEVE) if *port* invalid
> >>>>>> + *  -(-EINVAL) if bad parameter.
> >>>>>> + */
> >>>>>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> >>>>>> +					uint16_t vf_id,
> >>>>>> +					uint8_t on);
> >>>>>
> >>>>> As discussed previously, I believe it is good to keep following
> >>>>> syntax in
> >>> API:
> >>>>> <name_space>_<object>_<action>, for this API it becomes:
> >>>> I think, current naming rule is <name_space>_<action>_<object>
> right?
> >
> >  This seems to be the existing naming convention.
> >
> >>>
> >>> Overall, I am not aware of any defined naming rule, I am for defining
> one.
> >>>
> >>>> See below
> >>>> 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
> >>>>         rte_pmd_i40e_set_vf_vlan_filter;
> >>>>         rte_pmd_i40e_set_vf_vlan_insert;
> >>>>         rte_pmd_i40e_set_vf_vlan_stripq;
> >>>>         rte_pmd_i40e_set_vf_vlan_tag; so what's wrong with this
> >>>
> >>> This breaks hierarchical approach, if you think API name as tree.
> >>> Easier to see this when you sort the APIs, ns_set_x, ns_reset_x,
> >>> ns_del_x will spread to different locations.
> >> I agree with your point, I had thought your concern is only about
> >> this patch, but actually it's not.
> >>>
> >>> This looks OK when you work on one type of object already, but with
> >>> all APIs in concern, I believe object based grouping is better than
> >>> action based grouping.
> >>
> >>>
> >>> And why do you think above one is better? Again, as long as one is
> >>> agreed on,
> >> I don't, sorry for make you misunderstand
> >
> > I don't think changing the name convention at this point is a good idea.
> 
> I am not suggesting changing existing ones, for the sake compatibility.
> But that can also be an option, since these are PMD specific API, I expect
> usage will be limited and these does not carry as high standard as library APIs.

These API's are already in use with users. 

> > It would be better to remain consistent with the existing naming
> convention.
> 
> Existing i40e ones added this way to be compatible with existing ixgbe ones.
> But I don't think we need to follow old usage with new APIs.
> 
> > Otherwise both naming conventions will exist for the rte_pmd_i40e_*
> API's.
> 
> It can be for a while, later all can be fixed. If you think proposed convention is
> not better, that is something else.

I don't see anything to be gained by using two naming conventions side by side.
I don't have a strong view on which name convention is better, however the existing naming convention is what is in use with users at present. It is also the naming convention used librte_ether.

Changing API naming conventions is something that should probably be discussed in a separate thread, as it can have a wider impact than the i40e and ixgbe PMD's.
 
> >
> >
> >>>>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> >>> removed?
> >>>>>
> >>>>>> +
> >>>>>>  #endif /* _PMD_I40E_H_ */
> >>>>>>
> >>>>

Regards,

Bernard.