Re: [PATCH v2 2/4] ethdev: add dev op to set/get IP reassembly configuration

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 1/20/22 19:26, Akhil Goyal wrote:
> A new ethernet device op is added to give application control over

ethernet -> Ethernet

> the IP reassembly configuration. This operation is an optional
> call from the application, default values are set by PMD and
> exposed via rte_eth_dev_info.

Are defaults or maximum support values exposed via rte_eth_dev_info?
I guess it should be maximum. Defaults can be obtained using
get without set.

> Application should always first retrieve the capabilities from
> rte_eth_dev_info and then set the fields accordingly.
> User can get the currently set values using the get API.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>

[snip]


> +/**
> + * @internal
> + * Set configuration parameters for enabling IP reassembly offload in hardware.
> + *
> + * @param dev
> + *   Port (ethdev) handle
> + *
> + * @param[in] conf
> + *   Configuration parameters for IP reassembly.
> + *
> + * @return
> + *   Negative errno value on error, zero otherwise
> + */
> +typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
> +				       struct rte_eth_ip_reass_params *conf);

const

[snip]

> +int
> +rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> +			       struct rte_eth_ip_reass_params *conf)

Please, preserve order everywhere. If get comes first, it must be first
everywhere.

> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get reassembly info to NULL");
> +		return -EINVAL;
> +	}

Why is order of check different in set and get?

> +
> +	if (dev->data->dev_configured == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Device with port_id=%"PRIu16" is not configured.\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	if ((dev->data->dev_conf.rxmode.offloads &
> +			RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"The port (ID=%"PRIu16") is not configured for IP reassembly\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_conf_get,
> +				-ENOTSUP);
> +	memset(conf, 0, sizeof(struct rte_eth_ip_reass_params));
> +	return eth_err(port_id,
> +		       (*dev->dev_ops->ip_reassembly_conf_get)(dev, conf));
> +}
> +
>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>   
>   RTE_INIT(ethdev_init_telemetry)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 11427b2e4d..53af158bcb 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5218,6 +5218,57 @@ int rte_eth_representor_info_get(uint16_t port_id,
>   __rte_experimental
>   int rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get IP reassembly configuration parameters currently set in PMD,
> + * if device rx offload flag (RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) is

rx -> Rx

> + * enabled and the PMD supports IP reassembly offload.
> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param conf
> + *   A pointer to rte_eth_ip_reass_params structure.
> + * @return
> + *   - (-ENOTSUP) if offload configuration is not supported by device.
> + *   - (-EINVAL) if offload is not enabled in rte_eth_conf.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> +				   struct rte_eth_ip_reass_params *conf);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set IP reassembly configuration parameters if device rx offload

rx -> Rx

> + * flag (RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) is enabled and the PMD
> + * supports IP reassembly offload. User should first check the
> + * reass_capa in rte_eth_dev_info before setting the configuration.
> + * The values of configuration parameters must not exceed the device
> + * capabilities.

It sounds like set API should retrieve dev_info and check set
values vs maximums.

> The use of this API is optional and if called, it
> + * should be called before rte_eth_dev_start().

It should be highlighted that the device must be already configured.

> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param conf
> + *   A pointer to rte_eth_ip_reass_params structure.
> + * @return
> + *   - (-ENOTSUP) if offload configuration is not supported by device.
> + *   - (-EINVAL) if offload is not enabled in rte_eth_conf.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> +				   struct rte_eth_ip_reass_params *conf);
> +
> +
>   #include <rte_ethdev_core.h>
>   
>   /**
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index c2fb0669a4..ad829dd47e 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -256,6 +256,10 @@ EXPERIMENTAL {
>   	rte_flow_flex_item_create;
>   	rte_flow_flex_item_release;
>   	rte_flow_pick_transfer_proxy;
> +
> +	#added in 22.03
> +	rte_eth_ip_reassembly_conf_get;
> +	rte_eth_ip_reassembly_conf_set;
>   };
>   
>   INTERNAL {