Hi Andrew,
> On 1/20/22 19:26, Akhil Goyal wrote:
> > A new ethernet device op is added to give application control over
>
> ethernet -> Ethernet
Ok
>
> > 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.
>
Rte_eth_dev_info gives the maximum values/capabilities that a device can support
And also the default values set if user does not call set() API.
And get() op will give the currently set values.
> > 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 <gak...@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.
ok
>
> > +{
> > + 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?
Ok will correct it.
>
> > +
> > + 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
>
Ok
> > + * 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.
Yes.
>
> > 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.
Where should this be highlighted?
