[dpdk-dev] [PATCH 1/2] ethdev: replace callback getting filter operations
Thomas Monjalon
thomas at monjalon.net
Fri Mar 12 09:26:34 CET 2021
12/03/2021 08:09, Andrew Rybchenko:
> On 3/12/21 1:17 AM, Thomas Monjalon wrote:
> > Since rte_flow is the only API for filtering operations,
> > the legacy driver interface filter_ctrl was too much complicated
> > for the simple task of getting the struct rte_flow_ops.
> >
> > The filter type RTE_ETH_FILTER_GENERIC and
> > the filter operarion RTE_ETH_FILTER_GET are removed.
> > The new driver callback flow_ops_get replaces filter_ctrl.
> >
> > Signed-off-by: Thomas Monjalon <thomas at monjalon.net>
>
> A couple of minor notes below. Other than that:
>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>
> [snip]
>
> > diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> > index 7405a9864f..1260539a21 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -571,7 +571,7 @@ API Changes
> > a TC is greater than 256.
> >
> > * ethdev: Removed the legacy filter API, including
> > - ``rte_eth_dev_filter_supported()`` and ``rte_eth_dev_filter_ctrl()``.
> > + ``rte_eth_dev_filter_supported()`` and ``rte_eth_dev_flow_ops_get()``.
>
> Mass substitution is dangerous. I guess this one is not
> intended.
>
> >
> > * ethdev: Removed the legacy L2 tunnel configuration API, including
> > ``rte_eth_dev_l2_tunnel_eth_type_conf()`` and
> > diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
> > index cea5c8746d..f7deeac34b 100644
> > --- a/doc/guides/rel_notes/release_2_2.rst
> > +++ b/doc/guides/rel_notes/release_2_2.rst
> > @@ -501,7 +501,7 @@ API Changes
> > -----------
> >
> > * The deprecated flow director API is removed.
> > - It was replaced by ``rte_eth_dev_filter_ctrl()``.
> > + It was replaced by ``rte_eth_dev_flow_ops_get()``.
>
> As well as this one.
Oops, good catch, sorry I did not double check this part.
[...]
> > -typedef int (*eth_filter_ctrl_t)(struct rte_eth_dev *dev,
> > - enum rte_filter_type filter_type,
> > - enum rte_filter_op filter_op,
> > - void *arg);
> > -/**< @internal Take operations to assigned filter type on an Ethernet device */
> > +struct rte_flow_ops;
> > +typedef int (*eth_flow_ops_get_t)(struct rte_eth_dev *dev,
> > + const struct rte_flow_ops **ops);
> > +/**< @internal Get flow operations */
>
> I'm OK with the callback prototype. I think there is no point
> to optimize it and make ops a return value. It is called in
> just one place. Nothing to optimize.
OK
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -255,12 +255,9 @@ 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))
> > + else if (unlikely(!dev->dev_ops->flow_ops_get ||
> > + dev->dev_ops->flow_ops_get(dev, &ops) ||
>
> If the callback return error code, may be it should be used
> and forwarded here?
>
> > + ops == NULL))
> > code = ENOSYS;
Yes you are right.
Strange that it was not already forwarded.
More information about the dev
mailing list