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

[Xueming(Steven) Li]

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 3:49 PM
>To: Xueming(Steven) Li <xuemingl at nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>;
>Olivier Matz <olivier.matz at 6wind.com>
>Cc: dev at dpdk.org; Slava Ovsiienko <viacheslavo at nvidia.com>; Asaf Penso
><asafp at nvidia.com>
>Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>
>On 1/19/21 10:37 AM, Xueming(Steven) Li wrote:
>> Hi Andrew,
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>>> Sent: Tuesday, January 19, 2021 3:25 PM
>>> To: Xueming(Steven) Li <xuemingl at nvidia.com>; NBU-Contact-Thomas
>>> Monjalon <thomas at monjalon.net>; Ferruh Yigit
>>> <ferruh.yigit at intel.com>; Olivier Matz <olivier.matz at 6wind.com>
>>> Cc: dev at dpdk.org; Slava Ovsiienko <viacheslavo at nvidia.com>; Asaf
>>> Penso <asafp at 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 at nvidia.com>
>>>> Acked-by: Viacheslav Ovsiienko <viacheslavo at nvidia.com>
>>>
>>> One nit below and a question below.
>>>
>>> In any case:
>>>
>>> Acked-by: Andrew Rybchenko <andrew.rybchenko at 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.

Good suggestion, will update
>
>>>
>>>> +};
>>>> +
>>>>  /** 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.
>>>
>>>>  };
>>>>
>>>>  /**
>>>>
>>