Re: [dpdk-dev] [PATCH v4 1/9] ethdev: introduce representor type

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 1/19/21 10:37 AM, Xueming(Steven) Li wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, January 19, 2021 3:25 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>> Olivier Matz <olivier.matz@6wind.com>
>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
>> <asafp@nvidia.com>
>> Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>>
>> On 1/18/21 2:16 PM, Xueming Li wrote:
>>> To support more representor type, this patch introduces representor
>>> type enum. The enum is subject to extend for new types upcoming.
>>>
>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>
>> One nit below and a question below.
>>
>> In any case:
>>
>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>> [snip]
>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>>> b/lib/librte_ethdev/rte_ethdev_driver.h
>>> index 0eacfd8425..3bc5c5bbbb 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>> @@ -1193,6 +1193,14 @@ __rte_internal
>>>  int
>>>  rte_eth_switch_domain_free(uint16_t domain_id);
>>>
>>> +/** Ethernet device representor type */ enum rte_eth_representor_type
>>> +{
>>> +	RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
>>> +	RTE_ETH_REPRESENTOR_VF,   /**< representor of VF. */
>>> +	RTE_ETH_REPRESENTOR_SF,   /**< representor of SF. */
>>> +	RTE_ETH_REPRESENTOR_PF,   /**< representor of host PF. */
>>
>> RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
>> IMHO, addition of these members here make future patches which add
>> support inconsistent.
> 
> Yes, later patch in this patchset will support it.


I know. The question is why it is not added in the
later patches when these types are actually supported.

>>
>>> +};
>>> +
>>>  /** Generic Ethernet device arguments  */  struct rte_eth_devargs {
>>>  	uint16_t ports[RTE_MAX_ETHPORTS];
>>> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
>>>  	/** representor port/s identifier to enable on device */
>>>  	uint16_t nb_representor_ports;
>>>  	/** number of ports in representor port field */
>>> +	enum rte_eth_representor_type type; /* type of representor */
>>
>> Is it intended and documented limitation that we can't add different type
>> representors in one request? Or am I missing something and it is possible?
> 
> Correct, current devargs structure can't support mix of different types.
> I'll update in next version if any.
>>
>>>  };
>>>
>>>  /**
>>>
>