Re: [dpdk-dev] [PATCH 1/2] ethdev: replace callback getting filter operations
From: Andrew Rybchenko <hidden>
Date: 2021-03-12 07:09:58
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 <redacted>
A couple of minor notes below. Other than that: Reviewed-by: Andrew Rybchenko <redacted> [snip]
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
* ethdev: Removed the legacy L2 tunnel configuration API, including ``rte_eth_dev_l2_tunnel_eth_type_conf()`` anddiff --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.
* The ``dcb_queue`` is renamed to ``dcb_tc`` in following dcb configuration structures: ``rte_eth_dcb_rx_conf``, ``rte_eth_dcb_tx_conf``,
[snip]
quoted hunk ↗ jump to hunk
diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h index 57fdedaa1a..1c6592ec23 100644 --- a/lib/librte_ethdev/ethdev_driver.h +++ b/lib/librte_ethdev/ethdev_driver.h@@ -465,34 +465,10 @@ typedef int (*eth_get_module_eeprom_t)(struct rte_eth_dev *dev, struct rte_dev_eeprom_info *info); /**< @internal Retrieve plugin module eeprom data */ -/** - * Feature filter types - */ -enum rte_filter_type { - RTE_ETH_FILTER_NONE = 0, - RTE_ETH_FILTER_ETHERTYPE, - RTE_ETH_FILTER_FLEXIBLE, - RTE_ETH_FILTER_SYN, - RTE_ETH_FILTER_NTUPLE, - RTE_ETH_FILTER_TUNNEL, - RTE_ETH_FILTER_FDIR, - RTE_ETH_FILTER_HASH, - RTE_ETH_FILTER_L2_TUNNEL, - RTE_ETH_FILTER_GENERIC, -}; - -/** - * Generic operations on filters - */ -enum rte_filter_op { - RTE_ETH_FILTER_GET, /**< get flow API ops */ -}; - -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.
quoted hunk ↗ jump to hunk
typedef int (*eth_tm_ops_get_t)(struct rte_eth_dev *dev, void *ops); /**< @internal Get Traffic Management (TM) operations on an Ethernet device */@@ -880,7 +856,7 @@ struct eth_dev_ops { eth_get_module_eeprom_t get_module_eeprom; /** Get plugin module eeprom data. */ - eth_filter_ctrl_t filter_ctrl; /**< common filter control. */ + eth_flow_ops_get_t flow_ops_get; /**< Get flow operations. */ eth_get_dcb_info get_dcb_info; /** Get DCB information. */@@ -1377,6 +1353,18 @@ rte_eth_hairpin_queue_peer_unbind(uint16_t cur_port, uint16_t cur_queue, * Legacy ethdev API used internally by drivers. */ +enum rte_filter_type { + RTE_ETH_FILTER_NONE = 0, + RTE_ETH_FILTER_ETHERTYPE, + RTE_ETH_FILTER_FLEXIBLE, + RTE_ETH_FILTER_SYN, + RTE_ETH_FILTER_NTUPLE, + RTE_ETH_FILTER_TUNNEL, + RTE_ETH_FILTER_FDIR, + RTE_ETH_FILTER_HASH, + RTE_ETH_FILTER_L2_TUNNEL, +}; + /** * Define all structures for Ethertype Filter type. */diff --git a/lib/librte_ethdev/rte_eth_ctrl.h b/lib/librte_ethdev/rte_eth_ctrl.h index 8a50dbfef9..42652f9cce 100644 --- a/lib/librte_ethdev/rte_eth_ctrl.h +++ b/lib/librte_ethdev/rte_eth_ctrl.h@@ -339,7 +339,7 @@ struct rte_eth_fdir_action { }; /** - * A structure used to define the flow director filter entry by filter_ctrl API. + * A structure used to define the flow director filter entry. */ struct rte_eth_fdir_filter { uint32_t soft_id;diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c index 241af6c4ca..ed9f826b85 100644 --- 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; else return ops;
[snip]