diff mbox series

[v2,3/3] ethdev: add the check for the valitity of timestamp offload

Message ID 20220702081730.1168-4-liudongdong3@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Andrew Rybchenko
Headers
Series some bugfixes for PTP |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Dongdong Liu July 2, 2022, 8:17 a.m. UTC 
  From: Huisong Li <lihuisong@huawei.com>

This patch adds the check for the valitity of timestamp offload.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

Comments

Andrew Rybchenko July 6, 2022, 2:57 p.m. UTC | #1 
On 7/2/22 11:17, Dongdong Liu wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> This patch adds the check for the valitity of timestamp offload.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 1979dc0850..9b8ba3a348 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5167,15 +5167,48 @@ rte_eth_dev_set_mc_addr_list(uint16_t port_id,
>   						mc_addr_set, nb_mc_addr));
>   }
>   
> +static int
> +rte_eth_timestamp_offload_valid(struct rte_eth_dev *dev)
> +{
> +	struct rte_eth_dev_info dev_info;
> +	struct rte_eth_rxmode *rxmode;
> +	int ret;
> +
> +	ret = rte_eth_dev_info_get(dev->data->port_id, &dev_info);
> +	if (ret != 0) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get port (%u) device information.\n",
> +			       dev->data->port_id);
> +		return ret;
> +	}
> +
> +	if ((dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {

As I understand strictly speaking the Rx offload is not the same as PTP
support. May be it is a corner case, but I can imagine case when HW
cannot provide timestamp for each packet (lack of space in extra
information associated with a packet), but can return timestamps
out-of-band using timestamp get API.

I have no strong opinion. May be we are not interested in the corner
case, but I think it requires acks from maintainers of all drivers
which support PTP.

> +		RTE_ETHDEV_LOG(ERR, "Driver does not support PTP.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	rxmode = &dev->data->dev_conf.rxmode;
> +	if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {
> +		RTE_ETHDEV_LOG(ERR, "Please enable 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' offload.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   int
>   rte_eth_timesync_enable(uint16_t port_id)
>   {
>   	struct rte_eth_dev *dev;
> +	int ret;
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>   	dev = &rte_eth_devices[port_id];
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_enable, -ENOTSUP);
> +	ret = rte_eth_timestamp_offload_valid(dev);
> +	if (ret != 0)
> +		return ret;
> +

Typically ops pointer check is done just before usage. So, it is
better to avoid adding code between check and usage.
Same in all cases below.
if there is a good reason to do so, please, explain it.

>   	return eth_err(port_id, (*dev->dev_ops->timesync_enable)(dev));
>   }
>   

[snip]
lihuisong (C) July 7, 2022, 2:05 a.m. UTC | #2 
在 2022/7/6 22:57, Andrew Rybchenko 写道:
> On 7/2/22 11:17, Dongdong Liu wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch adds the check for the valitity of timestamp offload.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>   lib/ethdev/rte_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 1979dc0850..9b8ba3a348 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5167,15 +5167,48 @@ rte_eth_dev_set_mc_addr_list(uint16_t port_id,
>>                           mc_addr_set, nb_mc_addr));
>>   }
>>   +static int
>> +rte_eth_timestamp_offload_valid(struct rte_eth_dev *dev)
>> +{
>> +    struct rte_eth_dev_info dev_info;
>> +    struct rte_eth_rxmode *rxmode;
>> +    int ret;
>> +
>> +    ret = rte_eth_dev_info_get(dev->data->port_id, &dev_info);
>> +    if (ret != 0) {
>> +        RTE_ETHDEV_LOG(ERR, "Cannot get port (%u) device 
>> information.\n",
>> +                   dev->data->port_id);
>> +        return ret;
>> +    }
>> +
>> +    if ((dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 
>> 0) {
>
> As I understand strictly speaking the Rx offload is not the same as PTP
> support. May be it is a corner case, but I can imagine case when HW
> cannot provide timestamp for each packet (lack of space in extra
> information associated with a packet), but can return timestamps
> out-of-band using timestamp get API.
>
Generally speaking, Rx offload is a data plane thing and done in hardware.
If hardware cannot provide timestamp for each packet, and can implement PTP
only by using timestamp APIs. Can this corner case still be classified as
Rx offload? If so, It is more of a control plane thing, like MTU.

On the other hand, this offload is a capability obtained from driver.
If driver doesn't support PTP, the ethdev layer should block them in
timestamp APIs.
> I have no strong opinion. May be we are not interested in the corner
> case, but I think it requires acks from maintainers of all drivers
> which support PTP.
>
>> +        RTE_ETHDEV_LOG(ERR, "Driver does not support PTP.\n");
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    rxmode = &dev->data->dev_conf.rxmode;
>> +    if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {
>> +        RTE_ETHDEV_LOG(ERR, "Please enable 
>> 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' offload.\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int
>>   rte_eth_timesync_enable(uint16_t port_id)
>>   {
>>       struct rte_eth_dev *dev;
>> +    int ret;
>>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>       dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_enable, -ENOTSUP);
>> +    ret = rte_eth_timestamp_offload_valid(dev);
>> +    if (ret != 0)
>> +        return ret;
>> +
>
> Typically ops pointer check is done just before usage. So, it is
> better to avoid adding code between check and usage.
> Same in all cases below.
> if there is a good reason to do so, please, explain it.
A coin has two sides.
Both styles exist in the ethdev layer API. There is no need to check input
configuration if driver doesn't support the API. I am ok for them.
>
>>       return eth_err(port_id, (*dev->dev_ops->timesync_enable)(dev));
>>   }
>
> [snip]
> .
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 1979dc0850..9b8ba3a348 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5167,15 +5167,48 @@  rte_eth_dev_set_mc_addr_list(uint16_t port_id,
 						mc_addr_set, nb_mc_addr));
 }
 
+static int
+rte_eth_timestamp_offload_valid(struct rte_eth_dev *dev)
+{
+	struct rte_eth_dev_info dev_info;
+	struct rte_eth_rxmode *rxmode;
+	int ret;
+
+	ret = rte_eth_dev_info_get(dev->data->port_id, &dev_info);
+	if (ret != 0) {
+		RTE_ETHDEV_LOG(ERR, "Cannot get port (%u) device information.\n",
+			       dev->data->port_id);
+		return ret;
+	}
+
+	if ((dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {
+		RTE_ETHDEV_LOG(ERR, "Driver does not support PTP.\n");
+		return -ENOTSUP;
+	}
+
+	rxmode = &dev->data->dev_conf.rxmode;
+	if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {
+		RTE_ETHDEV_LOG(ERR, "Please enable 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' offload.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_timesync_enable(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_enable, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_enable)(dev));
 }
 
@@ -5183,11 +5216,15 @@  int
 rte_eth_timesync_disable(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_disable, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
 	return eth_err(port_id, (*dev->dev_ops->timesync_disable)(dev));
 }
 
@@ -5196,6 +5233,7 @@  rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
 				   uint32_t flags)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -5207,7 +5245,12 @@  rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
 		return -EINVAL;
 	}
 
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp, -ENOTSUP);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp,
+				-ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_read_rx_timestamp)
 				(dev, timestamp, flags));
 }
@@ -5217,6 +5260,7 @@  rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 				   struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -5229,6 +5273,10 @@  rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 	}
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_tx_timestamp, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_read_tx_timestamp)
 				(dev, timestamp));
 }
@@ -5237,11 +5285,16 @@  int
 rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_adjust_time, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_adjust_time)(dev, delta));
 }
 
@@ -5249,6 +5302,7 @@  int
 rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -5261,6 +5315,10 @@  rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 	}
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_time, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_read_time)(dev,
 								timestamp));
 }
@@ -5269,6 +5327,7 @@  int
 rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -5281,6 +5340,10 @@  rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
 	}
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_write_time, -ENOTSUP);
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	return eth_err(port_id, (*dev->dev_ops->timesync_write_time)(dev,
 								timestamp));
 }