Re: [dpdk-dev] [PATCH v2 1/2] ethdev: replace callback getting filter operations

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

On 3/19/21 9:12 PM, Thomas Monjalon wrote:
> 15/03/2021 10:15, Thomas Monjalon:
>> 15/03/2021 10:08, Andrew Rybchenko:
>>> On 3/15/21 11:55 AM, Thomas Monjalon wrote:
>>>> 15/03/2021 09:43, Andrew Rybchenko:
>>>>> On 3/15/21 10:54 AM, Thomas Monjalon wrote:
>>>>>> 15/03/2021 08:18, Andrew Rybchenko:
>>>>>>> On 3/12/21 8:46 PM, Thomas Monjalon wrote:
>>>>>>>> --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>> +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>> @@ -255,18 +255,19 @@ rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error)
>>>>>>>>   
>>>>>>>>   	if (unlikely(!rte_eth_dev_is_valid_port(port_id)))
>>>>>>>>   		code = ENODEV;
>>>>>>>> -	else if (unlikely(!dev->dev_ops->filter_ctrl ||
>>>>>>>> -			  dev->dev_ops->filter_ctrl(dev,
>>>>>>>> -						    RTE_ETH_FILTER_GENERIC,
>>>>>>>> -						    RTE_ETH_FILTER_GET,
>>>>>>>> -						    &ops) ||
>>>>>>>> -			  !ops))
>>>>>>>> -		code = ENOSYS;
>>>>>>>> +	else if (unlikely(dev->dev_ops->flow_ops_get == NULL))
>>>>>>>> +		code = ENOTSUP;
>>>
>>> It is described as:
>>>     -ENOTSUP: valid but unsupported rule specification (e.g.
>>>     partial bit-masks are unsupported).
>>> So, it looks different. May be it is really better to keep
>>> ENOSYS.
>>>
>>>>>>>>   	else
>>>>>>>> -		return ops;
>>>>>>>> -	rte_flow_error_set(error, code, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>>>>>> -			   NULL, rte_strerror(code));
>>>>>>>> -	return NULL;
>>>>>>>> +		code = dev->dev_ops->flow_ops_get(dev, &ops);
>>>>>>>> +	if (code == 0 && ops == NULL)
>>>>>>>> +		code = EACCES;
>>>>>>> It looks something new. I think it should be mentioned in flow_ops_get
>>>>>>> type documentation (similar to eth_promiscuous_enable_t) and
>>>>>>> rte_flow_validate() etc functions
>>>>>>> return values description.
>>>>>>
>>>>>> It is an internal function used only in rte_flow.c.
>>>>>> The real consequence is to set rte_errno in a lot of rte_flow API.
>>>>>> Not sure there is a good way to document the code details.
>>>>>> Other codes are not documented in rte_flow.h
>>>>>
>>>>> First of all it is a behaviour of the flow_ops_get callback and
>>>>> driver developers should know that it is a legal to return 0 and
>>>>> ops==NULL and know what it means.
>>>>
>>>> The combination code 0 and ops NULL is not new.
>>>> Previously, it was returning ENOSYS.
>>>> I've just given a more meaningful error code: EACCES,
>>>> while replacing ENOSYS with ENOTSUP for the other case.
>>>
>>> Yes, exactly. What I'm trying to say that it would be
>>> helpful to make it a bit more transparent to PMD developers.
>>> Yes, it was not documented before, I agree. I think it is
>>> a good time to improve documentation.
>>>
>>>>> Second, it is visible as rte_flow_validate() (and other functions
>>>>> which use rte_flow_ops_get()) return value value which has
>>>>> special meaning. So, should be documented.
>>>>
>>>> Yes, I should update the API doc where ENOSYS was mentioned.
>>>> Or probably better: I should keep the error code ENOSYS
>>>> and do not break API.
>>>> Preference?
>>>
>>> Good question. I think we should not distinguish NULL callback
>>> and NULL ops returned by not-NULL callback. So, I think
>>> keeping ENOSYS is the best option here.
>>
>> OK, thank you for the review.
>> So the conclusion is: keep ENOSYS and document NULL ops case.
> 
> After more thoughts, I don't think we need to insist on the NULL ops case.
> The function rte_flow_ops_get returns the ops,
> and it is documented that returning NULL is an error.
> So the function is just setting ENOSYS error code to have
> an error code associated with returning NULL.
> For the PMD, returning an ops NULL has no interest.
> 

May be I misunderstand above, but

One driver may support different NIC types. It could support flow API
for one NIC type and do not support for another. Sometimes it could
be easier to sort it out in flow ops get callback rather than
provide different set of ethdev ops callback. If so, returning NULL
ops makes sense and easier way to say that flow API is not supported
(vs, for example, providing dummy callbacks which return ENOSYS).