20/03/2021 08:54, Andrew Rybchenko: > 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).
Yes you're right, returning NULL ops may have an interest. I will document it in eth_flow_ops_get_t.