Re: [PATCH v7 1/4] ethdev: introduce protocol header API

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

On 10/4/22 05:21, Wang, YuanX wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, October 3, 2022 3:04 PM
>> To: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>;
>> Ray Kinsella <mdr@ashroe.eu>
>> Cc: ferruh.yigit@xilinx.com; Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Aman
>> Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>> <yuying.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Yang,
>> Qiming <qiming.yang@intel.com>; jerinjacobk@gmail.com;
>> viacheslavo@nvidia.com; stephen@networkplumber.org; Ding, Xuan
>> <xuan.ding@intel.com>; hpothula@marvell.com; Tang, Yaqi
>> <yaqi.tang@intel.com>; Wenxuan Wu <wenxuanx.wu@intel.com>
>> Subject: Re: [PATCH v7 1/4] ethdev: introduce protocol header API
>>
>> On 10/2/22 00:05, Yuan Wang wrote:
>>> Add a new ethdev API to retrieve supported protocol headers of a PMD,
>>> which helps to configure protocol header based buffer split.
>>>
>>> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
>>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
>>> Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> ---
>>>    doc/guides/rel_notes/release_22_11.rst |  5 ++++
>>>    lib/ethdev/ethdev_driver.h             | 15 ++++++++++++
>>>    lib/ethdev/rte_ethdev.c                | 33 ++++++++++++++++++++++++++
>>>    lib/ethdev/rte_ethdev.h                | 30 +++++++++++++++++++++++
>>>    lib/ethdev/version.map                 |  3 +++
>>>    5 files changed, 86 insertions(+)
>>>
>>> diff --git a/doc/guides/rel_notes/release_22_11.rst
>>> b/doc/guides/rel_notes/release_22_11.rst
>>> index 0231959874..6a7474a3d6 100644
>>> --- a/doc/guides/rel_notes/release_22_11.rst
>>> +++ b/doc/guides/rel_notes/release_22_11.rst
>>> @@ -96,6 +96,11 @@ New Features
>>>      * Added ``rte_event_eth_tx_adapter_queue_stop`` to stop the Tx
>> Adapter
>>>        from enqueueing any packets to the Tx queue.
>>>
>>> +* **Added new ethdev API for PMD to get buffer split supported
>>> +protocol types.**
>>> +
>>> +  * Added ``rte_eth_buffer_split_get_supported_hdr_ptypes()``, to get
>> supported
>>> +    header protocols of a PMD to split.
>>> +
>>
>> ethdev features should be grouped together in release notes.
>> I'll fix it on applying if a new version is not required.
> 
> We will send a new version. For the doc changes, I don't understand your point very well.
> Since will be no new changes to the code within this patch, could you help to adjust the doc?
> Thanks very much.

Please, read a comment just after 'New Features' section start.
Hopefully it will make my note clearer.
Anyway, don't worry about it a lot. I can easily fix it on
applying.

> 
>>
>> [snip]
>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
>>> 0c2c1088c0..1f0a7f8f3f 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -6002,6 +6002,39 @@ rte_eth_dev_priv_dump(uint16_t port_id, FILE
>> *file)
>>>    	return eth_err(port_id, (*dev->dev_ops->eth_dev_priv_dump)(dev,
>> file));
>>>    }
>>>
>>> +int
>>> +rte_eth_buffer_split_get_supported_hdr_ptypes(uint16_t port_id,
>>> +uint32_t *ptypes, int num) {
>>> +	int i, j;
>>> +	struct rte_eth_dev *dev;
>>> +	const uint32_t *all_types;
>>> +
>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> +	dev = &rte_eth_devices[port_id];
>>> +
>>> +	if (ptypes == NULL && num > 0) {
>>> +		RTE_ETHDEV_LOG(ERR,
>>> +			"Cannot get ethdev port %u supported header
>> protocol types to NULL when array size is non zero\n",
>>> +			port_id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (*dev->dev_ops->buffer_split_supported_hdr_ptypes_get == NULL)
>>> +		return -ENOTSUP;
>>> +	all_types =
>>> +(*dev->dev_ops->buffer_split_supported_hdr_ptypes_get)(dev);
>>> +
>>> +	if (!all_types)
>>
>> Should be compared with NULL explicitly as coding standard says. I can fix it
>> on applying as well.
> 
> Sure, I will fix in v8.
> 
>>
>> [snip]