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 <tho...@monjalon.net> > > A couple of minor notes below. Other than that: > > Reviewed-by: Andrew Rybchenko <andrew.rybche...@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.
