Re: [dpdk-dev] [PATCH v3 1/6] ethdev: add hairpin bind and unbind APIs

[Thomas Monjalon <thomas@monjalon.net>]

Hi,
Cosmetic comments below:

08/10/2020 14:05, Bing Zhao:
> +int
> +rte_eth_hairpin_bind(uint16_t tx_port, uint16_t rx_port)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(tx_port, -EINVAL);

It should be -ENODEV

> +	dev = &rte_eth_devices[tx_port];
> +	if (!dev->data->dev_started) {
> +		RTE_ETHDEV_LOG(ERR, "TX port %d is not started", tx_port);
> +		return -EBUSY;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_bind, -ENOTSUP);
> +	ret = (*dev->dev_ops->hairpin_bind)(dev, rx_port);
> +	if (ret)
> +		RTE_ETHDEV_LOG(ERR, "Failed to bind hairpin TX %d "
> +			       "to RX %d (%d - all ports)", tx_port,
> +			       rx_port, RTE_MAX_ETHPORTS);

Looks like \n is missing.

> +
> +	return ret;
> +}
> +
> +int
> +rte_eth_hairpin_unbind(uint16_t tx_port, uint16_t rx_port)

Same comments as for bind function.

[...]
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Bind all hairpin TX queues of one port to the RX queues of the peer port.
> + * It is only allowed to call this API after all hairpin queues are configured
> + * properly and the devices of TX and peer RX are in started state.

"call this API" -> "call this function"

"devices of TX" is a strange wording.
I would make it simpler:
"the devices of TX and peer RX" -> "the devices"

> + *
> + * @param tx_port
> + *   The TX port identifier of the Ethernet device.

A device does not have a TX port.
I think you mean "The identifier of the TX port."

> + * @param rx_port
> + *   The peer RX port identifier of the Ethernet device.

Remove "of the Ethernet device"

> + *   RTE_MAX_ETHPORTS is allowed for the traversal of all devices.
> + *   RX port ID could have the same value with TX port ID.

"same value as"

> + *
> + * @return
> + *   - (0) if successful.
> + *   - (-EINVAL) if bad parameter.
> + *   - (-EBUSY) if device is not in started state.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - Others detailed errors from PMD drivers.

Please add ENODEV case, maybe instead of EINVAL.

> + */
> +__rte_experimental
> +int rte_eth_hairpin_bind(uint16_t tx_port, uint16_t rx_port);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Unbind all hairpin TX queues of one port from the RX queues of the peer port.
> + * This should be called before closing the TX or RX devices (optional). After

Split the line after the end of sentence and start next one on a fresh line.

> + * unbind the hairpin ports pair, it is allowed to bind them again.

"unbind" -> "unbinding"

> + * Changing queues configuration should be after stopping the device.
> + *
> + * @param tx_port
> + *   The TX port identifier of the Ethernet device.
> + * @param rx_port
> + *   The peer RX port identifier of the Ethernet device.
> + *   RTE_MAX_ETHPORTS is allowed for traversal of all devices.
> + *   RX port ID could have the same value with TX port ID.
> + *
> + * @return
> + *   - (0) if successful.
> + *   - (-EINVAL) if bad parameter.
> + *   - (-EBUSY) if device is in stopped state.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - Others detailed errors from PMD drivers.

Same comments as for the bind function.

> + */
> +__rte_experimental
> +int rte_eth_hairpin_unbind(uint16_t tx_port, uint16_t rx_port);