RE: [PATCH v2 2/4] ethdev: add get restore flags driver callback

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> Before this patch, ethdev layer assumed that all drivers require that
> it has to forcefully restore:
> 
> - MAC addresses
> - promiscuous mode setting
> - all multicast mode setting
> 
> upon rte_eth_dev_start().
> 
> This patch introduces a new callback to eth_dev_ops -
> get_restore_flags().
> Drivers implementing this callback can explicitly enable/disable
> certain parts of config restore procedure.
> 
> In order to minimize the changes to all the drivers and
> preserve the current behavior, it is assumed that
> if this callback is not defined, all configuration should be restored.
> 
> Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>

LGTM, just few nits/suggestions, pls see below.
Series-Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

> ---
>  lib/ethdev/ethdev_driver.c | 11 +++++++
>  lib/ethdev/ethdev_driver.h | 64 ++++++++++++++++++++++++++++++++++++++
>  lib/ethdev/version.map     |  1 +
>  3 files changed, 76 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index c335a25a82..f78f9fb5c1 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -958,3 +958,14 @@ rte_eth_switch_domain_free(uint16_t domain_id)
> 
>  	return 0;
>  }
> +
> +void
> +rte_eth_get_restore_flags(struct rte_eth_dev *dev,
> +			  enum rte_eth_dev_operation op,
> +			  uint32_t *flags)

I would go with uint64_t for flags (to avoid limitations in future).
Also, If it is void anyway, might be just return flags?
i.e:
uint64_t  +rte_eth_get_restore_flags(struct rte_eth_dev *dev, enum rte_eth_dev_operation op);

Another thing - do we need to do update docs?
PMD or PG sections, release notes?

> +{
> +	if (dev->dev_ops->get_restore_flags != NULL)
> +		dev->dev_ops->get_restore_flags(dev, op, flags);
> +	else
> +		*flags = RTE_ETH_RESTORE_ALL;
> +}
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index ae00ead865..8ac5328521 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1235,6 +1235,47 @@ typedef int (*eth_count_aggr_ports_t)(struct rte_eth_dev *dev);
>  typedef int (*eth_map_aggr_tx_affinity_t)(struct rte_eth_dev *dev, uint16_t tx_queue_id,
>  					  uint8_t affinity);
> 
> +/**
> + * @internal
> + * Defines types of operations which can be executed by the application.
> + */
> +enum rte_eth_dev_operation {
> +	RTE_ETH_START,
> +};
> +
> +/**@{@name Restore flags
> + * Flags returned by get_restore_flags() callback.
> + * They indicate to ethdev layer which configuration is required to be restored.
> + */
> +/** If set, ethdev layer will forcefully restore default and any other added MAC addresses. */
> +#define RTE_ETH_RESTORE_MAC_ADDR RTE_BIT32(0)
> +/** If set, ethdev layer will forcefully restore current promiscuous mode setting. */
> +#define RTE_ETH_RESTORE_PROMISC  RTE_BIT32(1)
> +/** If set, ethdev layer will forcefully restore current all multicast mode setting. */
> +#define RTE_ETH_RESTORE_ALLMULTI RTE_BIT32(2)
> +/**@}*/
> +
> +/** All configuration which can be restored by ethdev layer. */
> +#define RTE_ETH_RESTORE_ALL (RTE_ETH_RESTORE_MAC_ADDR | \
> +			     RTE_ETH_RESTORE_PROMISC | \
> +			     RTE_ETH_RESTORE_ALLMULTI)
> +
> +/**
> + * @internal
> + * Fetch from the driver what kind of configuration must be restored by ethdev layer,
> + * after certain operations are performed by the application (such as rte_eth_dev_start()).
> + *
> + * @param dev
> + *   Port (ethdev) handle.
> + * @param op
> + *   Type of operation executed by the application.
> + * @param flags
> + *   Flags indicating what configuration must be restored by ethdev layer.
> + */
> +typedef void (*eth_get_restore_flags_t)(struct rte_eth_dev *dev,
> +					enum rte_eth_dev_operation op,
> +					uint32_t *flags);
> +
>  /**
>   * @internal A structure containing the functions exported by an Ethernet driver.
>   */
> @@ -1474,6 +1515,9 @@ struct eth_dev_ops {
>  	eth_count_aggr_ports_t count_aggr_ports;
>  	/** Map a Tx queue with an aggregated port of the DPDK port */
>  	eth_map_aggr_tx_affinity_t map_aggr_tx_affinity;
> +
> +	/** Get configuration which ethdev should restore */
> +	eth_get_restore_flags_t get_restore_flags;
>  };
> 
>  /**
> @@ -2131,6 +2175,26 @@ struct rte_eth_fdir_conf {
>  	struct rte_eth_fdir_flex_conf flex_conf;
>  };
> 
> +/**
> + * @internal
> + * Fetch from the driver what kind of configuration must be restored by ethdev layer,
> + * using get_restore_flags() callback.
> + *
> + * If callback is not defined, it is assumed that all supported configuration must be restored.
> + *
> + * @param dev
> + *   Port (ethdev) handle.
> + * @param op
> + *   Type of operation executed by the application.
> + * @param flags
> + *   Flags indicating what configuration must be restored by ethdev layer.
> + */
> +__rte_internal

For new function we probably need 'rte_experimental'...
But from other side - it is internal.
Not sure what are rules here for such case.

> +void
> +rte_eth_get_restore_flags(struct rte_eth_dev *dev,
> +			  enum rte_eth_dev_operation op,
> +			  uint32_t *flags);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 1669055ca5..fa0469e602 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -358,4 +358,5 @@ INTERNAL {
>  	rte_eth_switch_domain_alloc;
>  	rte_eth_switch_domain_free;
>  	rte_flow_fp_default_ops;
> +	rte_eth_get_restore_flags;
>  };
> --
> 2.39.5