[PATCH 0/3] some bugfixes for PTP

[PATCH 0/3] some bugfixes for PTP

[Dongdong Liu]

The patchset include some bugfixes for PTP.

Huisong Li (3):
  examples/ptpclient: add the check for PTP capability
  net/hns3: fix fail to receive PTP packet
  ethdev: add the check for the valitity of timestamp offload

 drivers/net/hns3/hns3_ptp.c      |  1 -
 drivers/net/hns3/hns3_rxtx_vec.c | 22 +++++------
 examples/ptpclient/ptpclient.c   |  5 +++
 lib/ethdev/rte_ethdev.c          | 65 +++++++++++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 14 deletions(-)

--
2.22.0

[PATCH 1/3] examples/ptpclient: add the check for PTP capability

[Dongdong Liu]

From: Huisong Li <lihuisong@huawei.com>

If a port doesn't support PTP, there is no need to keep running
app. So this patch adds the check for PTP capability.

Signe-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 examples/ptpclient/ptpclient.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index 1f1c9c9c52..9689f42f86 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -195,6 +195,11 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
 
 	if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
 		port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+	else {
+		printf("port(%u) doesn't support PTP: %s\n", port,
+		       strerror(-retval));
+		return -ENOTSUP;
+	}
 
 	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
 		port_conf.txmode.offloads |=
-- 
2.22.0

[PATCH 2/3] net/hns3: fix fail to receive PTP packet

[Dongdong Liu]

From: Huisong Li <lihuisong@huawei.com>

The Rx and Tx vector algorithm of hns3 PMD don't support PTP
function. Currently, hns3 driver uses 'pf->ptp_enable' to check
whether PTP is enabled so as to not select Rx and Tx vector
algorithm. And the variable is set when call rte_eth_timesync_enable().
Namely, it may not be set before selecting Rx/Tx function, let's say
the case: set PTP offload in dev_configure(), do dev_start() and then
call rte_eth_timesync_enable(). In this case, all PTP packets can not
be received to application. So this patch fixes the check based on the
RTE_ETH_RX_OFFLOAD_TIMESTAMP flag.

Fixes: 71f4d1aae11f ("net/hns3: fix vector burst unsupported when PTP capability set")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 drivers/net/hns3/hns3_ptp.c      |  1 -
 drivers/net/hns3/hns3_rxtx_vec.c | 22 ++++++++++------------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/hns3/hns3_ptp.c b/drivers/net/hns3/hns3_ptp.c
index 0b0061bba5..6bbd85ba23 100644
--- a/drivers/net/hns3/hns3_ptp.c
+++ b/drivers/net/hns3/hns3_ptp.c
@@ -125,7 +125,6 @@ hns3_timesync_enable(struct rte_eth_dev *dev)
 
 	if (pf->ptp_enable)
 		return 0;
-	hns3_warn(hw, "note: please ensure Rx/Tx burst mode is simple or common when enabling PTP!");
 
 	rte_spinlock_lock(&hw->lock);
 	ret = hns3_timesync_configure(hns, true);
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 73f0ab6bc8..22c087b9a7 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -17,15 +17,18 @@ int
 hns3_tx_check_vec_support(struct rte_eth_dev *dev)
 {
 	struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_pf *pf = &hns->pf;
+	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 
 	/* Only support RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE */
 	if (txmode->offloads != RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
 		return -ENOTSUP;
 
-	/* Vec is not supported when PTP enabled */
-	if (pf->ptp_enable)
+	/*
+	 * PTP function requires the cooperation of Rx and Tx.
+	 * Tx vector isn't supported if RTE_ETH_RX_OFFLOAD_TIMESTAMP is set
+	 * in Rx offloads.
+	 */
+	if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
 		return -ENOTSUP;
 
 	return 0;
@@ -232,10 +235,9 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 {
 	struct rte_eth_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
-				 RTE_ETH_RX_OFFLOAD_VLAN;
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_pf *pf = &hns->pf;
+	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO | \
+				 RTE_ETH_RX_OFFLOAD_VLAN | \
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
@@ -249,9 +251,5 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	if (hns3_rxq_iterate(dev, hns3_rxq_vec_check, NULL) != 0)
 		return -ENOTSUP;
 
-	/* Vec is not supported when PTP enabled */
-	if (pf->ptp_enable)
-		return -ENOTSUP;
-
 	return 0;
 }
-- 
2.22.0

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

[Dongdong Liu]

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) {
+		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));
 }
-- 
2.22.0

[PATCH v2 0/3] some bugfixes for PTP

[Dongdong Liu]

The patchset include some bugfixes for PTP.

v1->v2:
- fix the checkpatch warning.

Huisong Li (3):
  examples/ptpclient: add the check for PTP capability
  net/hns3: fix fail to receive PTP packet
  ethdev: add the check for the valitity of timestamp offload

 drivers/net/hns3/hns3_ptp.c      |  1 -
 drivers/net/hns3/hns3_rxtx_vec.c | 20 +++++-----
 examples/ptpclient/ptpclient.c   |  5 +++
 lib/ethdev/rte_ethdev.c          | 65 +++++++++++++++++++++++++++++++-
 4 files changed, 78 insertions(+), 13 deletions(-)

--
2.22.0

[PATCH v2 1/3] examples/ptpclient: add the check for PTP capability

[Dongdong Liu]

From: Huisong Li <lihuisong@huawei.com>

If a port doesn't support PTP, there is no need to keep running
app. So this patch adds the check for PTP capability.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 examples/ptpclient/ptpclient.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index 1f1c9c9c52..9689f42f86 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -195,6 +195,11 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
 
 	if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
 		port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+	else {
+		printf("port(%u) doesn't support PTP: %s\n", port,
+		       strerror(-retval));
+		return -ENOTSUP;
+	}
 
 	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
 		port_conf.txmode.offloads |=
-- 
2.22.0

[PATCH v2 2/3] net/hns3: fix fail to receive PTP packet

[Dongdong Liu]

From: Huisong Li <lihuisong@huawei.com>

The Rx and Tx vector algorithm of hns3 PMD don't support PTP
function. Currently, hns3 driver uses 'pf->ptp_enable' to check
whether PTP is enabled so as to not select Rx and Tx vector
algorithm. And the variable is set when call rte_eth_timesync_enable().
Namely, it may not be set before selecting Rx/Tx function, let's say
the case: set PTP offload in dev_configure(), do dev_start() and then
call rte_eth_timesync_enable(). In this case, all PTP packets can not
be received to application. So this patch fixes the check based on the
RTE_ETH_RX_OFFLOAD_TIMESTAMP flag.

Fixes: 71f4d1aae11f ("net/hns3: fix vector burst unsupported when PTP capability set")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 drivers/net/hns3/hns3_ptp.c      |  1 -
 drivers/net/hns3/hns3_rxtx_vec.c | 20 +++++++++-----------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/hns3/hns3_ptp.c b/drivers/net/hns3/hns3_ptp.c
index 0b0061bba5..6bbd85ba23 100644
--- a/drivers/net/hns3/hns3_ptp.c
+++ b/drivers/net/hns3/hns3_ptp.c
@@ -125,7 +125,6 @@ hns3_timesync_enable(struct rte_eth_dev *dev)
 
 	if (pf->ptp_enable)
 		return 0;
-	hns3_warn(hw, "note: please ensure Rx/Tx burst mode is simple or common when enabling PTP!");
 
 	rte_spinlock_lock(&hw->lock);
 	ret = hns3_timesync_configure(hns, true);
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 73f0ab6bc8..153866cf03 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -17,15 +17,18 @@ int
 hns3_tx_check_vec_support(struct rte_eth_dev *dev)
 {
 	struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_pf *pf = &hns->pf;
+	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 
 	/* Only support RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE */
 	if (txmode->offloads != RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
 		return -ENOTSUP;
 
-	/* Vec is not supported when PTP enabled */
-	if (pf->ptp_enable)
+	/*
+	 * PTP function requires the cooperation of Rx and Tx.
+	 * Tx vector isn't supported if RTE_ETH_RX_OFFLOAD_TIMESTAMP is set
+	 * in Rx offloads.
+	 */
+	if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
 		return -ENOTSUP;
 
 	return 0;
@@ -233,9 +236,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	struct rte_eth_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
-				 RTE_ETH_RX_OFFLOAD_VLAN;
-	struct hns3_adapter *hns = dev->data->dev_private;
-	struct hns3_pf *pf = &hns->pf;
+				 RTE_ETH_RX_OFFLOAD_VLAN |
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
@@ -249,9 +251,5 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	if (hns3_rxq_iterate(dev, hns3_rxq_vec_check, NULL) != 0)
 		return -ENOTSUP;
 
-	/* Vec is not supported when PTP enabled */
-	if (pf->ptp_enable)
-		return -ENOTSUP;
-
 	return 0;
 }
-- 
2.22.0

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

[Dongdong Liu]

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) {
+		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));
 }
-- 
2.22.0

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

[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.

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]

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

[lihuisong (C)]

在 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]
> .

[PATCH v3 0/2] ethdev: add the check for PTP capability

[Huisong Li]

From the first version of ptpclient, it seems that this example assume that
the PMDs support the PTP feature and enable PTP by default. Please see
commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
which are introduced in 2015.

And two years later, Rx HW timestamp offload was introduced to enable or
disable PTP feature in HW via rte_eth_rxmode. Please see
commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability"). 

And then about four years later, ptpclient enable Rx timestamp offload
because some PMDs require this offload to enable. Please see
commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload").

By all the records, this is more like a process of perfecting PTP feature.
Not all network adaptors support PTP feature. So adding the check for PTP
capability in ethdev layer is necessary.

---
v3:
 - patch [2/3] for hns3 has been applied and so remove it.
 - ops pointer check is closer to usage.

Huisong Li (2):
  examples/ptpclient: add the check for PTP capability
  ethdev: add the check for the valitity of timestamp offload

 examples/ptpclient/ptpclient.c |  5 +++
 lib/ethdev/rte_ethdev.c        | 57 +++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[Huisong Li]

If a port doesn't support PTP, there is no need to keep running
app. So this patch adds the check for PTP capability.

Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 examples/ptpclient/ptpclient.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index cdf2da64df..181d8fb357 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
 
 	if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
 		port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+	else {
+		printf("port(%u) doesn't support PTP: %s\n", port,
+		       strerror(-retval));
+		return -ENOTSUP;
+	}
 
 	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
 		port_conf.txmode.offloads |=
-- 
2.33.0

Re: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[Ferruh Yigit]

On 8/17/2023 9:42 AM, Huisong Li wrote:
> If a port doesn't support PTP, there is no need to keep running
> app. So this patch adds the check for PTP capability.
> 
> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
>  examples/ptpclient/ptpclient.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
> index cdf2da64df..181d8fb357 100644
> --- a/examples/ptpclient/ptpclient.c
> +++ b/examples/ptpclient/ptpclient.c
> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
>  
>  	if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>  		port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
> +	else {
> +		printf("port(%u) doesn't support PTP: %s\n", port,
> +		       strerror(-retval));
> +		return -ENOTSUP;
> +	}
>  

I am not sure why TIMESTAMP offload is required for PTP, I think there
is a confusion.


Gagandeep, Hemant,
Can you please clarify why TIMESTAMP offload is enabled?

Re: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[lihuisong (C)]

在 2023/9/16 1:29, Ferruh Yigit 写道:
> On 8/17/2023 9:42 AM, Huisong Li wrote:
>> If a port doesn't support PTP, there is no need to keep running
>> app. So this patch adds the check for PTP capability.
>>
>> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>   examples/ptpclient/ptpclient.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
>> index cdf2da64df..181d8fb357 100644
>> --- a/examples/ptpclient/ptpclient.c
>> +++ b/examples/ptpclient/ptpclient.c
>> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool *mbuf_pool)
>>   
>>   	if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>>   		port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
>> +	else {
>> +		printf("port(%u) doesn't support PTP: %s\n", port,
>> +		       strerror(-retval));
>> +		return -ENOTSUP;
>> +	}
>>   
> I am not sure why TIMESTAMP offload is required for PTP, I think there
> is a confusion.
If TIMESTAMP offload is not required for PTP, there isn't PTP offload in 
ethdev lib.
>
>
> Gagandeep, Hemant,
> Can you please clarify why TIMESTAMP offload is enabled?
looking forward to your reply.
>
> .

Re: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[Ferruh Yigit]

On 9/21/2023 10:18 AM, lihuisong (C) wrote:
> 
> 在 2023/9/16 1:29, Ferruh Yigit 写道:
>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>> If a port doesn't support PTP, there is no need to keep running
>>> app. So this patch adds the check for PTP capability.
>>>
>>> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>> ---
>>>   examples/ptpclient/ptpclient.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/examples/ptpclient/ptpclient.c
>>> b/examples/ptpclient/ptpclient.c
>>> index cdf2da64df..181d8fb357 100644
>>> --- a/examples/ptpclient/ptpclient.c
>>> +++ b/examples/ptpclient/ptpclient.c
>>> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool
>>> *mbuf_pool)
>>>         if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>>>           port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
>>> +    else {
>>> +        printf("port(%u) doesn't support PTP: %s\n", port,
>>> +               strerror(-retval));
>>> +        return -ENOTSUP;
>>> +    }
>>>   
>> I am not sure why TIMESTAMP offload is required for PTP, I think there
>> is a confusion.
> If TIMESTAMP offload is not required for PTP, there isn't PTP offload in
> ethdev lib.
>

What do you mean with "PTP offload"?

If you check the ptpclient sample app, it parses ptp packets in the
application.


>>
>>
>> Gagandeep, Hemant,
>> Can you please clarify why TIMESTAMP offload is enabled?
> looking forward to your reply.
>>
>> .

RE: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[Hemant Agrawal]

> 
> On 9/21/2023 10:18 AM, lihuisong (C) wrote:
> >
> > 在 2023/9/16 1:29, Ferruh Yigit 写道:
> >> On 8/17/2023 9:42 AM, Huisong Li wrote:
> >>> If a port doesn't support PTP, there is no need to keep running app.
> >>> So this patch adds the check for PTP capability.
> >>>
> >>> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
> >>> offload")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >>> ---
> >>>   examples/ptpclient/ptpclient.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/examples/ptpclient/ptpclient.c
> >>> b/examples/ptpclient/ptpclient.c index cdf2da64df..181d8fb357 100644
> >>> --- a/examples/ptpclient/ptpclient.c
> >>> +++ b/examples/ptpclient/ptpclient.c
> >>> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool
> >>> *mbuf_pool)
> >>>         if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
> >>>           port_conf.rxmode.offloads |=
> RTE_ETH_RX_OFFLOAD_TIMESTAMP;
> >>> +    else {
> >>> +        printf("port(%u) doesn't support PTP: %s\n", port,
> >>> +               strerror(-retval));
> >>> +        return -ENOTSUP;
> >>> +    }
> >>>
> >> I am not sure why TIMESTAMP offload is required for PTP, I think
> >> there is a confusion.
> > If TIMESTAMP offload is not required for PTP, there isn't PTP offload
> > in ethdev lib.
> >
> 
> What do you mean with "PTP offload"?
> 
> If you check the ptpclient sample app, it parses ptp packets in the application.

> 
> 
> >>
> >>
> >> Gagandeep, Hemant,
> >> Can you please clarify why TIMESTAMP offload is enabled?
> > looking forward to your reply.

[Hemant] as explained in other mail, it is a requirement for dpaa2. So, we are just passing the offload argument.

Well, currently there is no such offload to know HW PTP support in DPDK. It can be introduced. 

And I agree the above else should not be there atleast w.r.t TIMESTAMP OFFLOAD.

> >> .

Re: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[lihuisong (C)]

Hi Hemant and Ferruh,

在 2023/9/21 19:22, Hemant Agrawal 写道:
>> On 9/21/2023 10:18 AM, lihuisong (C) wrote:
>>> 在 2023/9/16 1:29, Ferruh Yigit 写道:
>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>> If a port doesn't support PTP, there is no need to keep running app.
>>>>> So this patch adds the check for PTP capability.
>>>>>
>>>>> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>>>> offload")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>> ---
>>>>>    examples/ptpclient/ptpclient.c | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/examples/ptpclient/ptpclient.c
>>>>> b/examples/ptpclient/ptpclient.c index cdf2da64df..181d8fb357 100644
>>>>> --- a/examples/ptpclient/ptpclient.c
>>>>> +++ b/examples/ptpclient/ptpclient.c
>>>>> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool
>>>>> *mbuf_pool)
>>>>>          if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>>>>>            port_conf.rxmode.offloads |=
>> RTE_ETH_RX_OFFLOAD_TIMESTAMP;
>>>>> +    else {
>>>>> +        printf("port(%u) doesn't support PTP: %s\n", port,
>>>>> +               strerror(-retval));
>>>>> +        return -ENOTSUP;
>>>>> +    }
>>>>>
>>>> I am not sure why TIMESTAMP offload is required for PTP, I think
>>>> there is a confusion.
>>> If TIMESTAMP offload is not required for PTP, there isn't PTP offload
>>> in ethdev lib.
>>>
>> What do you mean with "PTP offload"?
>>
>> If you check the ptpclient sample app, it parses ptp packets in the application.
>>
>>>>
>>>> Gagandeep, Hemant,
>>>> Can you please clarify why TIMESTAMP offload is enabled?
>>> looking forward to your reply.
> [Hemant] as explained in other mail, it is a requirement for dpaa2. So, we are just passing the offload argument.
>
> Well, currently there is no such offload to know HW PTP support in DPDK. It can be introduced.
I agree with you, Heman.
>
> And I agree the above else should not be there atleast w.r.t TIMESTAMP OFFLOAD.

Ack.

@Ferruh, I wonder what you think. Looking forward to your reply.

>
>>>> .

Re: [PATCH v3 1/2] examples/ptpclient: add the check for PTP capability

[lihuisong (C)]

在 2023/9/21 19:02, Ferruh Yigit 写道:
> On 9/21/2023 10:18 AM, lihuisong (C) wrote:
>> 在 2023/9/16 1:29, Ferruh Yigit 写道:
>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>> If a port doesn't support PTP, there is no need to keep running
>>>> app. So this patch adds the check for PTP capability.
>>>>
>>>> Fixes: 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>> ---
>>>>    examples/ptpclient/ptpclient.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/examples/ptpclient/ptpclient.c
>>>> b/examples/ptpclient/ptpclient.c
>>>> index cdf2da64df..181d8fb357 100644
>>>> --- a/examples/ptpclient/ptpclient.c
>>>> +++ b/examples/ptpclient/ptpclient.c
>>>> @@ -196,6 +196,11 @@ port_init(uint16_t port, struct rte_mempool
>>>> *mbuf_pool)
>>>>          if (dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>>>>            port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
>>>> +    else {
>>>> +        printf("port(%u) doesn't support PTP: %s\n", port,
>>>> +               strerror(-retval));
>>>> +        return -ENOTSUP;
>>>> +    }
>>>>    
>>> I am not sure why TIMESTAMP offload is required for PTP, I think there
>>> is a confusion.
>> If TIMESTAMP offload is not required for PTP, there isn't PTP offload in
>> ethdev lib.
>>
> What do you mean with "PTP offload"?
Yes, I mean we need like a PTP offload.
>
> If you check the ptpclient sample app, it parses ptp packets in the
> application.
App did parse PTP packet already.
What is the relationship between this and the verification in the app?
>
>
>>>
>>> Gagandeep, Hemant,
>>> Can you please clarify why TIMESTAMP offload is enabled?
>> looking forward to your reply.
>>> .
> .

[PATCH v3 2/2] ethdev: add the check for the valitity of timestamp offload

[Huisong Li]

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 | 57 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0840d2b594..77fa71047a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5996,6 +5996,34 @@ rte_eth_dev_set_mc_addr_list(uint16_t port_id,
 	return ret;
 }
 
+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)
 {
@@ -6005,6 +6033,10 @@ rte_eth_timesync_enable(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_enable == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_enable)(dev));
@@ -6023,6 +6055,10 @@ rte_eth_timesync_disable(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_disable == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_disable)(dev));
@@ -6049,6 +6085,10 @@ rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
 		return -EINVAL;
 	}
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_read_rx_timestamp == NULL)
 		return -ENOTSUP;
 
@@ -6078,9 +6118,12 @@ rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 		return -EINVAL;
 	}
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_read_tx_timestamp == NULL)
 		return -ENOTSUP;
-
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_read_tx_timestamp)
 			       (dev, timestamp));
 
@@ -6099,6 +6142,10 @@ rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_adjust_time == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_adjust_time)(dev, delta));
@@ -6124,6 +6171,10 @@ rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 		return -EINVAL;
 	}
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_read_time == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_read_time)(dev,
@@ -6150,6 +6201,10 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
 		return -EINVAL;
 	}
 
+	ret = rte_eth_timestamp_offload_valid(dev);
+	if (ret != 0)
+		return ret;
+
 	if (*dev->dev_ops->timesync_write_time == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->timesync_write_time)(dev,
-- 
2.33.0

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 8/17/2023 9:42 AM, Huisong Li wrote:
> From the first version of ptpclient, it seems that this example assume that
> the PMDs support the PTP feature and enable PTP by default. Please see
> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
> which are introduced in 2015.
> 
> And two years later, Rx HW timestamp offload was introduced to enable or
> disable PTP feature in HW via rte_eth_rxmode. Please see
> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability"). 
> 

Hi Huisong,

As far as I know this offload is not for PTP.
PTP and TIMESTAMP are different.

PTP is a protocol for time sync.
Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.

> And then about four years later, ptpclient enable Rx timestamp offload
> because some PMDs require this offload to enable. Please see
> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload").
> 

dpaa2 seems using TIMESTAMP offload and PTP together, hence they updated
ptpclient sample to set TIMESTAMP offload.

We need to clarify dpaa2 usage.

> By all the records, this is more like a process of perfecting PTP feature.
> Not all network adaptors support PTP feature. So adding the check for PTP
> capability in ethdev layer is necessary.
> 

Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
checked, so no additional check is needed.

We just need to clarify TIMESTAMP offload and PTP usage and find out
what is causing confusion.
I would be great if you can help on clarification, and update
documentation or API comments, or what ever required, for this.

> ---
> v3:
>  - patch [2/3] for hns3 has been applied and so remove it.
>  - ops pointer check is closer to usage.
> 
> Huisong Li (2):
>   examples/ptpclient: add the check for PTP capability
>   ethdev: add the check for the valitity of timestamp offload
> 
>  examples/ptpclient/ptpclient.c |  5 +++
>  lib/ethdev/rte_ethdev.c        | 57 +++++++++++++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 1 deletion(-)
> 

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

Hi Ferruh,

Sorry for my delay reply because of taking a look at all PMDs 
implementation.


在 2023/9/16 1:46, Ferruh Yigit 写道:
> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>  From the first version of ptpclient, it seems that this example assume that
>> the PMDs support the PTP feature and enable PTP by default. Please see
>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>> which are introduced in 2015.
>>
>> And two years later, Rx HW timestamp offload was introduced to enable or
>> disable PTP feature in HW via rte_eth_rxmode. Please see
>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>
> Hi Huisong,
>
> As far as I know this offload is not for PTP.
> PTP and TIMESTAMP are different.
If TIMESTAMP offload cannot stand for PTP, we may need to add one new 
offlaod for PTP.
>
> PTP is a protocol for time sync.
> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
Yes.
But a lot of PMDs actually depand on HW to report Rx timestamp releated 
information
because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp 
API.
>
>> And then about four years later, ptpclient enable Rx timestamp offload
>> because some PMDs require this offload to enable. Please see
>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload").
>>
> dpaa2 seems using TIMESTAMP offload and PTP together, hence they updated
> ptpclient sample to set TIMESTAMP offload.
There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3 
and so on.
>
> We need to clarify dpaa2 usage.
>
>> By all the records, this is more like a process of perfecting PTP feature.
>> Not all network adaptors support PTP feature. So adding the check for PTP
>> capability in ethdev layer is necessary.
>>
> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
> checked, so no additional check is needed.
But only having dev_ops about PTP doesn't satisfy the use of this feature.
For example,
there are serveal network ports belonged to a driver on one OS, and only 
one port support PTP function.
So driver needs one *PTP* offload.
>
> We just need to clarify TIMESTAMP offload and PTP usage and find out
> what is causing confusion.
Yes it is a little bit confusion.
There are two kinds of implementation:
A： ixgbe and txgbe (it seems that their HW is similar) don't need 
TIMESTAMP offload，and only use dev_ops to finish PTP feature.
B:  saving "Rx timestamp related information" from Rx description when 
receive PTP SYNC packet and
     report it in read_rx_timestamp API.
For case B, most of driver use TIMESTAMP offload to decide if driver 
save "Rx timestamp related information.
What do you think about this, Ferruh?
> I would be great if you can help on clarification, and update
> documentation or API comments, or what ever required, for this.
ok
>
>> ---
>> v3:
>>   - patch [2/3] for hns3 has been applied and so remove it.
>>   - ops pointer check is closer to usage.
>>
>> Huisong Li (2):
>>    examples/ptpclient: add the check for PTP capability
>>    ethdev: add the check for the valitity of timestamp offload
>>
>>   examples/ptpclient/ptpclient.c |  5 +++
>>   lib/ethdev/rte_ethdev.c        | 57 +++++++++++++++++++++++++++++++++-
>>   2 files changed, 61 insertions(+), 1 deletion(-)
>>
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 9/21/2023 11:02 AM, lihuisong (C) wrote:
> Hi Ferruh,
> 
> Sorry for my delay reply because of taking a look at all PMDs
> implementation.
> 
> 
> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>  From the first version of ptpclient, it seems that this example
>>> assume that
>>> the PMDs support the PTP feature and enable PTP by default. Please see
>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>> which are introduced in 2015.
>>>
>>> And two years later, Rx HW timestamp offload was introduced to enable or
>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>
>> Hi Huisong,
>>
>> As far as I know this offload is not for PTP.
>> PTP and TIMESTAMP are different.
> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
> offlaod for PTP.
>

Can you please detail what is "PTP offload"?

>>
>> PTP is a protocol for time sync.
>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
> Yes.
> But a lot of PMDs actually depand on HW to report Rx timestamp releated
> information
> because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp
> API.
>

HW support may be required for PTP but this doesn't mean timestamp
offload is used.

>>
>>> And then about four years later, ptpclient enable Rx timestamp offload
>>> because some PMDs require this offload to enable. Please see
>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload").
>>>
>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they updated
>> ptpclient sample to set TIMESTAMP offload.
> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3
> and so on.
>

Can you please point the ice & igc code, cc'ing their maintainers, we
can look together?


>>
>> We need to clarify dpaa2 usage.
>>
>>> By all the records, this is more like a process of perfecting PTP
>>> feature.
>>> Not all network adaptors support PTP feature. So adding the check for
>>> PTP
>>> capability in ethdev layer is necessary.
>>>
>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
>> checked, so no additional check is needed.
> But only having dev_ops about PTP doesn't satisfy the use of this feature.
> For example,
> there are serveal network ports belonged to a driver on one OS, and only
> one port support PTP function.
> So driver needs one *PTP* offload.
>>
>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>> what is causing confusion.
> Yes it is a little bit confusion.
> There are two kinds of implementation:
> A： ixgbe and txgbe (it seems that their HW is similar) don't need
> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
> B:  saving "Rx timestamp related information" from Rx description when
> receive PTP SYNC packet and
>     report it in read_rx_timestamp API.
> For case B, most of driver use TIMESTAMP offload to decide if driver
> save "Rx timestamp related information.
> What do you think about this, Ferruh?
>> I would be great if you can help on clarification, and update
>> documentation or API comments, or what ever required, for this.
> ok
>>
>>> ---
>>> v3:
>>>   - patch [2/3] for hns3 has been applied and so remove it.
>>>   - ops pointer check is closer to usage.
>>>
>>> Huisong Li (2):
>>>    examples/ptpclient: add the check for PTP capability
>>>    ethdev: add the check for the valitity of timestamp offload
>>>
>>>   examples/ptpclient/ptpclient.c |  5 +++
>>>   lib/ethdev/rte_ethdev.c        | 57 +++++++++++++++++++++++++++++++++-
>>>   2 files changed, 61 insertions(+), 1 deletion(-)
>>>
>> .

RE: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Hemant Agrawal]

HI Ferruh,

> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
> > Hi Ferruh,
> >
> > Sorry for my delay reply because of taking a look at all PMDs
> > implementation.
> >
> >
> > 在 2023/9/16 1:46, Ferruh Yigit 写道:
> >> On 8/17/2023 9:42 AM, Huisong Li wrote:
> >>>  From the first version of ptpclient, it seems that this example
> >>> assume that the PMDs support the PTP feature and enable PTP by
> >>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
> >>> minimal PTP client") which are introduced in 2015.
> >>>
> >>> And two years later, Rx HW timestamp offload was introduced to
> >>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
> >>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
> >>>
> >> Hi Huisong,
> >>
> >> As far as I know this offload is not for PTP.
> >> PTP and TIMESTAMP are different.
> > If TIMESTAMP offload cannot stand for PTP, we may need to add one new
> > offlaod for PTP.
> >
> 
> Can you please detail what is "PTP offload"?
> 
> >>
> >> PTP is a protocol for time sync.
> >> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
> > Yes.
> > But a lot of PMDs actually depand on HW to report Rx timestamp
> > releated information because of reading Rx timestamp of PTP SYNC
> > packet in read_rx_timestamp API.
> >
> 
> HW support may be required for PTP but this doesn't mean timestamp
> offload is used.

> 
> >>
> >>> And then about four years later, ptpclient enable Rx timestamp
> >>> offload because some PMDs require this offload to enable. Please see
> >>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
> offload").
> >>>
> >> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
> >> updated ptpclient sample to set TIMESTAMP offload.

[Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In the current dpaa2 driver
If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the HW timestamp
Otherwise, we are only enabling it when the TIMESTAMP offload is selected.  

We added patch in ptpclient earlier to pass the timestamp offload, however later we also updated the driver to do it by default. 


> > There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
> > hns3 and so on.
> >
> 
> Can you please point the ice & igc code, cc'ing their maintainers, we can look
> together?
> 
> 
> >>
> >> We need to clarify dpaa2 usage.
> >>
> >>> By all the records, this is more like a process of perfecting PTP
> >>> feature.
> >>> Not all network adaptors support PTP feature. So adding the check
> >>> for PTP capability in ethdev layer is necessary.
> >>>
> >> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
> >> already checked, so no additional check is needed.
> > But only having dev_ops about PTP doesn't satisfy the use of this feature.
> > For example,
> > there are serveal network ports belonged to a driver on one OS, and
> > only one port support PTP function.
> > So driver needs one *PTP* offload.
> >>
> >> We just need to clarify TIMESTAMP offload and PTP usage and find out
> >> what is causing confusion.
> > Yes it is a little bit confusion.
> > There are two kinds of implementation:
> > A： ixgbe and txgbe (it seems that their HW is similar) don't need
> > TIMESTAMP offload，and only use dev_ops to finish PTP feature.
> > B:  saving "Rx timestamp related information" from Rx description when
> > receive PTP SYNC packet and
> >     report it in read_rx_timestamp API.
> > For case B, most of driver use TIMESTAMP offload to decide if driver
> > save "Rx timestamp related information.
> > What do you think about this, Ferruh?
> >> I would be great if you can help on clarification, and update
> >> documentation or API comments, or what ever required, for this.
> > ok
> >>
> >>> ---
> >>> v3:
> >>>   - patch [2/3] for hns3 has been applied and so remove it.
> >>>   - ops pointer check is closer to usage.
> >>>
> >>> Huisong Li (2):
> >>>    examples/ptpclient: add the check for PTP capability
> >>>    ethdev: add the check for the valitity of timestamp offload
> >>>
> >>>   examples/ptpclient/ptpclient.c |  5 +++
> >>>   lib/ethdev/rte_ethdev.c        | 57
> >>> +++++++++++++++++++++++++++++++++-
> >>>   2 files changed, 61 insertions(+), 1 deletion(-)
> >>>
> >> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

在 2023/9/21 19:17, Hemant Agrawal 写道:
> HI Ferruh,
>
>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> Sorry for my delay reply because of taking a look at all PMDs
>>> implementation.
>>>
>>>
>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>   From the first version of ptpclient, it seems that this example
>>>>> assume that the PMDs support the PTP feature and enable PTP by
>>>>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
>>>>> minimal PTP client") which are introduced in 2015.
>>>>>
>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>
>>>> Hi Huisong,
>>>>
>>>> As far as I know this offload is not for PTP.
>>>> PTP and TIMESTAMP are different.
>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>> offlaod for PTP.
>>>
>> Can you please detail what is "PTP offload"?
>>
>>>> PTP is a protocol for time sync.
>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>> Yes.
>>> But a lot of PMDs actually depand on HW to report Rx timestamp
>>> releated information because of reading Rx timestamp of PTP SYNC
>>> packet in read_rx_timestamp API.
>>>
>> HW support may be required for PTP but this doesn't mean timestamp
>> offload is used.
>>>>> And then about four years later, ptpclient enable Rx timestamp
>>>>> offload because some PMDs require this offload to enable. Please see
>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>> offload").
>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>> updated ptpclient sample to set TIMESTAMP offload.
> [Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In the current dpaa2 driver
> If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the HW timestamp
> Otherwise, we are only enabling it when the TIMESTAMP offload is selected.
>
> We added patch in ptpclient earlier to pass the timestamp offload, however later we also updated the driver to do it by default.
>
It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
Actually, whether PTP code is compiled should not depended on this macro 
RTE_LIBRTE_IEEE1588.
If there is a capability, it will be perfect, no matter whether it is 
TIMESTAMP offload.
What do you think, Ferruh?
>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
>>> hns3 and so on.
>>>
>> Can you please point the ice & igc code, cc'ing their maintainers, we can look
>> together?
>>
>>
>>>> We need to clarify dpaa2 usage.
>>>>
>>>>> By all the records, this is more like a process of perfecting PTP
>>>>> feature.
>>>>> Not all network adaptors support PTP feature. So adding the check
>>>>> for PTP capability in ethdev layer is necessary.
>>>>>
>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
>>>> already checked, so no additional check is needed.
>>> But only having dev_ops about PTP doesn't satisfy the use of this feature.
>>> For example,
>>> there are serveal network ports belonged to a driver on one OS, and
>>> only one port support PTP function.
>>> So driver needs one *PTP* offload.
>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>> what is causing confusion.
>>> Yes it is a little bit confusion.
>>> There are two kinds of implementation:
>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>> B:  saving "Rx timestamp related information" from Rx description when
>>> receive PTP SYNC packet and
>>>      report it in read_rx_timestamp API.
>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>> save "Rx timestamp related information.
>>> What do you think about this, Ferruh?
>>>> I would be great if you can help on clarification, and update
>>>> documentation or API comments, or what ever required, for this.
>>> ok
>>>>> ---
>>>>> v3:
>>>>>    - patch [2/3] for hns3 has been applied and so remove it.
>>>>>    - ops pointer check is closer to usage.
>>>>>
>>>>> Huisong Li (2):
>>>>>     examples/ptpclient: add the check for PTP capability
>>>>>     ethdev: add the check for the valitity of timestamp offload
>>>>>
>>>>>    examples/ptpclient/ptpclient.c |  5 +++
>>>>>    lib/ethdev/rte_ethdev.c        | 57
>>>>> +++++++++++++++++++++++++++++++++-
>>>>>    2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 10/20/2023 4:58 AM, lihuisong (C) wrote:
> 
> 在 2023/9/21 19:17, Hemant Agrawal 写道:
>> HI Ferruh,
>>
>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>> implementation.
>>>>
>>>>
>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>   From the first version of ptpclient, it seems that this example
>>>>>> assume that the PMDs support the PTP feature and enable PTP by
>>>>>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
>>>>>> minimal PTP client") which are introduced in 2015.
>>>>>>
>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>
>>>>> Hi Huisong,
>>>>>
>>>>> As far as I know this offload is not for PTP.
>>>>> PTP and TIMESTAMP are different.
>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>> offlaod for PTP.
>>>>
>>> Can you please detail what is "PTP offload"?
>>>
>>>>> PTP is a protocol for time sync.
>>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>>> Yes.
>>>> But a lot of PMDs actually depand on HW to report Rx timestamp
>>>> releated information because of reading Rx timestamp of PTP SYNC
>>>> packet in read_rx_timestamp API.
>>>>
>>> HW support may be required for PTP but this doesn't mean timestamp
>>> offload is used.
>>>>>> And then about four years later, ptpclient enable Rx timestamp
>>>>>> offload because some PMDs require this offload to enable. Please see
>>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>> offload").
>>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>>> updated ptpclient sample to set TIMESTAMP offload.
>> [Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In
>> the current dpaa2 driver
>> If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the
>> HW timestamp
>> Otherwise, we are only enabling it when the TIMESTAMP offload is
>> selected.
>>
>> We added patch in ptpclient earlier to pass the timestamp offload,
>> however later we also updated the driver to do it by default.
>>
>>
> It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
> Actually, whether PTP code is compiled should not depended on this macro
> RTE_LIBRTE_IEEE1588.
>

There is already a patch by Thomas to remove RTE_LIBRTE_IEEE1588 [1],
agree that this functionality needs some attention.

Removing RTE_LIBRTE_IEEE1588 impact drivers, that is what holding us back.


[1]
https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/

> If there is a capability, it will be perfect, no matter whether it is
> TIMESTAMP offload.
> What do you think, Ferruh?
>

Difficulty is to know when to enable HW timestamp, and for some drivers
this may change the descriptor format (to include timestamp), so driver
should set correct datapath functions for this case.

We know when a HW timer is required, it is required for PTP protocol and
required for TIMESTAMP offload.

What do you think to dynamically enable it for PTP when
'rte_eth_timesync_enable()' API called, and for TIMESTAMP offload when
the offload is enabled.
If this works, now new configuration item or offload is required, what
do you think?


>>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
>>>> hns3 and so on.
>>>>
>>> Can you please point the ice & igc code, cc'ing their maintainers, we
>>> can look
>>> together?
>>>
>>>
>>>>> We need to clarify dpaa2 usage.
>>>>>
>>>>>> By all the records, this is more like a process of perfecting PTP
>>>>>> feature.
>>>>>> Not all network adaptors support PTP feature. So adding the check
>>>>>> for PTP capability in ethdev layer is necessary.
>>>>>>
>>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
>>>>> already checked, so no additional check is needed.
>>>> But only having dev_ops about PTP doesn't satisfy the use of this
>>>> feature.
>>>> For example,
>>>> there are serveal network ports belonged to a driver on one OS, and
>>>> only one port support PTP function.
>>>> So driver needs one *PTP* offload.
>>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>>> what is causing confusion.
>>>> Yes it is a little bit confusion.
>>>> There are two kinds of implementation:
>>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>>> B:  saving "Rx timestamp related information" from Rx description when
>>>> receive PTP SYNC packet and
>>>>      report it in read_rx_timestamp API.
>>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>>> save "Rx timestamp related information.
>>>> What do you think about this, Ferruh?
>>>>> I would be great if you can help on clarification, and update
>>>>> documentation or API comments, or what ever required, for this.
>>>> ok
>>>>>> ---
>>>>>> v3:
>>>>>>    - patch [2/3] for hns3 has been applied and so remove it.
>>>>>>    - ops pointer check is closer to usage.
>>>>>>
>>>>>> Huisong Li (2):
>>>>>>     examples/ptpclient: add the check for PTP capability
>>>>>>     ethdev: add the check for the valitity of timestamp offload
>>>>>>
>>>>>>    examples/ptpclient/ptpclient.c |  5 +++
>>>>>>    lib/ethdev/rte_ethdev.c        | 57
>>>>>> +++++++++++++++++++++++++++++++++-
>>>>>>    2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

在 2023/11/2 7:39, Ferruh Yigit 写道:
> On 10/20/2023 4:58 AM, lihuisong (C) wrote:
>> 在 2023/9/21 19:17, Hemant Agrawal 写道:
>>> HI Ferruh,
>>>
>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>> implementation.
>>>>>
>>>>>
>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>    From the first version of ptpclient, it seems that this example
>>>>>>> assume that the PMDs support the PTP feature and enable PTP by
>>>>>>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
>>>>>>> minimal PTP client") which are introduced in 2015.
>>>>>>>
>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>
>>>>>> Hi Huisong,
>>>>>>
>>>>>> As far as I know this offload is not for PTP.
>>>>>> PTP and TIMESTAMP are different.
>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>>> offlaod for PTP.
>>>>>
>>>> Can you please detail what is "PTP offload"?
>>>>
>>>>>> PTP is a protocol for time sync.
>>>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>>>> Yes.
>>>>> But a lot of PMDs actually depand on HW to report Rx timestamp
>>>>> releated information because of reading Rx timestamp of PTP SYNC
>>>>> packet in read_rx_timestamp API.
>>>>>
>>>> HW support may be required for PTP but this doesn't mean timestamp
>>>> offload is used.
>>>>>>> And then about four years later, ptpclient enable Rx timestamp
>>>>>>> offload because some PMDs require this offload to enable. Please see
>>>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>>> offload").
>>>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>>>> updated ptpclient sample to set TIMESTAMP offload.
>>> [Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In
>>> the current dpaa2 driver
>>> If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the
>>> HW timestamp
>>> Otherwise, we are only enabling it when the TIMESTAMP offload is
>>> selected.
>>>
>>> We added patch in ptpclient earlier to pass the timestamp offload,
>>> however later we also updated the driver to do it by default.
>>>
>>>
>> It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
>> Actually, whether PTP code is compiled should not depended on this macro
>> RTE_LIBRTE_IEEE1588.
>>
> There is already a patch by Thomas to remove RTE_LIBRTE_IEEE1588 [1],
> agree that this functionality needs some attention.
>
> Removing RTE_LIBRTE_IEEE1588 impact drivers, that is what holding us back.
+1 remove the compile macro RTE_LIBRTE_IEEE1588.
And hns3 had beed removed it.
>
>
> [1]
> https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/
>
>> If there is a capability, it will be perfect, no matter whether it is
>> TIMESTAMP offload.
>> What do you think, Ferruh?
>>
> Difficulty is to know when to enable HW timestamp, and for some drivers
> this may change the descriptor format (to include timestamp), so driver
> should set correct datapath functions for this case.
Yes, to get Rx timestamp of PTP packet from descriptor for many NIC.
>
> We know when a HW timer is required, it is required for PTP protocol and
> required for TIMESTAMP offload.
TIMESTAMP offload may be unnecessary for some NIC which don't get Rx 
timestamp from descriptor(But, IMO, like this hardware is very rare.).
>
> What do you think to dynamically enable it for PTP when
> 'rte_eth_timesync_enable()' API called, and for TIMESTAMP offload when
> the offload is enabled.
Agree above.
At least, this can make sure all NIC can enable PTP feature.
> If this works, now new configuration item or offload is required, what
> do you think?
The new capability item is required to know if the port support PTP feature.
so application can enable/disable PTP based on this capability.
>
>>>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
>>>>> hns3 and so on.
>>>>>
>>>> Can you please point the ice & igc code, cc'ing their maintainers, we
>>>> can look
>>>> together?
>>>>
>>>>
>>>>>> We need to clarify dpaa2 usage.
>>>>>>
>>>>>>> By all the records, this is more like a process of perfecting PTP
>>>>>>> feature.
>>>>>>> Not all network adaptors support PTP feature. So adding the check
>>>>>>> for PTP capability in ethdev layer is necessary.
>>>>>>>
>>>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
>>>>>> already checked, so no additional check is needed.
>>>>> But only having dev_ops about PTP doesn't satisfy the use of this
>>>>> feature.
>>>>> For example,
>>>>> there are serveal network ports belonged to a driver on one OS, and
>>>>> only one port support PTP function.
>>>>> So driver needs one *PTP* offload.
>>>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>>>> what is causing confusion.
>>>>> Yes it is a little bit confusion.
>>>>> There are two kinds of implementation:
>>>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>>>> B:  saving "Rx timestamp related information" from Rx description when
>>>>> receive PTP SYNC packet and
>>>>>       report it in read_rx_timestamp API.
>>>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>>>> save "Rx timestamp related information.
>>>>> What do you think about this, Ferruh?
>>>>>> I would be great if you can help on clarification, and update
>>>>>> documentation or API comments, or what ever required, for this.
>>>>> ok
>>>>>>> ---
>>>>>>> v3:
>>>>>>>     - patch [2/3] for hns3 has been applied and so remove it.
>>>>>>>     - ops pointer check is closer to usage.
>>>>>>>
>>>>>>> Huisong Li (2):
>>>>>>>      examples/ptpclient: add the check for PTP capability
>>>>>>>      ethdev: add the check for the valitity of timestamp offload
>>>>>>>
>>>>>>>     examples/ptpclient/ptpclient.c |  5 +++
>>>>>>>     lib/ethdev/rte_ethdev.c        | 57
>>>>>>> +++++++++++++++++++++++++++++++++-
>>>>>>>     2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>> .
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 9/21/2023 12:17 PM, Hemant Agrawal wrote:
> HI Ferruh,
> 
>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> Sorry for my delay reply because of taking a look at all PMDs
>>> implementation.
>>>
>>>
>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>  From the first version of ptpclient, it seems that this example
>>>>> assume that the PMDs support the PTP feature and enable PTP by
>>>>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
>>>>> minimal PTP client") which are introduced in 2015.
>>>>>
>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>
>>>> Hi Huisong,
>>>>
>>>> As far as I know this offload is not for PTP.
>>>> PTP and TIMESTAMP are different.
>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>> offlaod for PTP.
>>>
>>
>> Can you please detail what is "PTP offload"?
>>
>>>>
>>>> PTP is a protocol for time sync.
>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>> Yes.
>>> But a lot of PMDs actually depand on HW to report Rx timestamp
>>> releated information because of reading Rx timestamp of PTP SYNC
>>> packet in read_rx_timestamp API.
>>>
>>
>> HW support may be required for PTP but this doesn't mean timestamp
>> offload is used.
> 
>>
>>>>
>>>>> And then about four years later, ptpclient enable Rx timestamp
>>>>> offload because some PMDs require this offload to enable. Please see
>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>> offload").
>>>>>
>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>> updated ptpclient sample to set TIMESTAMP offload.
> 
> [Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In the current dpaa2 driver
> If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the HW timestamp
> Otherwise, we are only enabling it when the TIMESTAMP offload is selected.  
> 

I think this is reasonable, HW timestamp enabled only when required.


> We added patch in ptpclient earlier to pass the timestamp offload, however later we also updated the driver to do it by default. 
> 

This part I am not sure,
so application request TIMESTAMP offload enable HW timestamp to use it
for PTP.

There are already 'rte_eth_timesync_enable()' and
'rte_eth_timesync_disable()' functions, and ptpclient sample already
uses them, why now utilize these APIs to enable HW timestamp, or other
related configuration?


> 
>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
>>> hns3 and so on.
>>>
>>
>> Can you please point the ice & igc code, cc'ing their maintainers, we can look
>> together?
>>
>>
>>>>
>>>> We need to clarify dpaa2 usage.
>>>>
>>>>> By all the records, this is more like a process of perfecting PTP
>>>>> feature.
>>>>> Not all network adaptors support PTP feature. So adding the check
>>>>> for PTP capability in ethdev layer is necessary.
>>>>>
>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
>>>> already checked, so no additional check is needed.
>>> But only having dev_ops about PTP doesn't satisfy the use of this feature.
>>> For example,
>>> there are serveal network ports belonged to a driver on one OS, and
>>> only one port support PTP function.
>>> So driver needs one *PTP* offload.
>>>>
>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>> what is causing confusion.
>>> Yes it is a little bit confusion.
>>> There are two kinds of implementation:
>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>> B:  saving "Rx timestamp related information" from Rx description when
>>> receive PTP SYNC packet and
>>>     report it in read_rx_timestamp API.
>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>> save "Rx timestamp related information.
>>> What do you think about this, Ferruh?
>>>> I would be great if you can help on clarification, and update
>>>> documentation or API comments, or what ever required, for this.
>>> ok
>>>>
>>>>> ---
>>>>> v3:
>>>>>   - patch [2/3] for hns3 has been applied and so remove it.
>>>>>   - ops pointer check is closer to usage.
>>>>>
>>>>> Huisong Li (2):
>>>>>    examples/ptpclient: add the check for PTP capability
>>>>>    ethdev: add the check for the valitity of timestamp offload
>>>>>
>>>>>   examples/ptpclient/ptpclient.c |  5 +++
>>>>>   lib/ethdev/rte_ethdev.c        | 57
>>>>> +++++++++++++++++++++++++++++++++-
>>>>>   2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>> .
> 

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

add ice & igc maintainers

在 2023/9/21 19:06, Ferruh Yigit 写道:
> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> Sorry for my delay reply because of taking a look at all PMDs
>> implementation.
>>
>>
>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>   From the first version of ptpclient, it seems that this example
>>>> assume that
>>>> the PMDs support the PTP feature and enable PTP by default. Please see
>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>> which are introduced in 2015.
>>>>
>>>> And two years later, Rx HW timestamp offload was introduced to enable or
>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>
>>> Hi Huisong,
>>>
>>> As far as I know this offload is not for PTP.
>>> PTP and TIMESTAMP are different.
>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>> offlaod for PTP.
>>
> Can you please detail what is "PTP offload"?
It indicates whether the device supports PTP or enable  PTP feature.
If TIMESTAMP offload is not for PTP, I don't know what the point of this 
offload independent existence is.
>
>>> PTP is a protocol for time sync.
>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>> Yes.
>> But a lot of PMDs actually depand on HW to report Rx timestamp releated
>> information
>> because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp
>> API.
>>
> HW support may be required for PTP but this doesn't mean timestamp
> offload is used.
understand.
>
>>>> And then about four years later, ptpclient enable Rx timestamp offload
>>>> because some PMDs require this offload to enable. Please see
>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp offload").
>>>>
>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they updated
>>> ptpclient sample to set TIMESTAMP offload.
>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3
>> and so on.
>>
> Can you please point the ice & igc code, cc'ing their maintainers, we
> can look together?

*-->igc code:*

Having following codes in igc_recv_scattered_pkts():

         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
                     uint32_t *, -IGC_TS_HDR_LEN);
             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
                     ts[2];
             rxm->timesync = rxq->queue_id;
         }
Note：this rxm->timesync will be used in timesync_read_rx_timestamp()

*-->ice code:*

#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
         if (ice_timestamp_dynflag > 0 &&
             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             if (unlikely(is_tsinit)) {
                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1, 
rxq->time_high);
                 rxq->hw_time_low = (uint32_t)ts_ns;
                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
                 is_tsinit = false;
             } else {
                 if (rxq->time_high < rxq->hw_time_low)
                     rxq->hw_time_high += 1;
                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
                 rxq->hw_time_low = rxq->time_high;
             }
             rxq->hw_time_update = rte_get_timer_cycles() /
                          (rte_get_timer_hz() / 1000);
             *RTE_MBUF_DYNFIELD(rxm,
                        (ice_timestamp_dynfield_offset),
                        rte_mbuf_timestamp_t *) = ts_ns;
             pkt_flags |= ice_timestamp_dynflag;
         }

         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
             RTE_PTYPE_L2_ETHER_TIMESYNC)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             rxm->timesync = rxq->queue_id;
             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
         }
#endif

Note: rxm->timesync and rxq->time_high will be used in 
timesync_read_rx_timestamp()

>
>
>>> We need to clarify dpaa2 usage.
>>>
>>>> By all the records, this is more like a process of perfecting PTP
>>>> feature.
>>>> Not all network adaptors support PTP feature. So adding the check for
>>>> PTP
>>>> capability in ethdev layer is necessary.
>>>>
>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
>>> checked, so no additional check is needed.
>> But only having dev_ops about PTP doesn't satisfy the use of this feature.
>> For example,
>> there are serveal network ports belonged to a driver on one OS, and only
>> one port support PTP function.
>> So driver needs one *PTP* offload.
>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>> what is causing confusion.
>> Yes it is a little bit confusion.
>> There are two kinds of implementation:
>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>> B:  saving "Rx timestamp related information" from Rx description when
>> receive PTP SYNC packet and
>>      report it in read_rx_timestamp API.
>> For case B, most of driver use TIMESTAMP offload to decide if driver
>> save "Rx timestamp related information.
>> What do you think about this, Ferruh?
>>> I would be great if you can help on clarification, and update
>>> documentation or API comments, or what ever required, for this.
>> ok
>>>> ---
>>>> v3:
>>>>    - patch [2/3] for hns3 has been applied and so remove it.
>>>>    - ops pointer check is closer to usage.
>>>>
>>>> Huisong Li (2):
>>>>     examples/ptpclient: add the check for PTP capability
>>>>     ethdev: add the check for the valitity of timestamp offload
>>>>
>>>>    examples/ptpclient/ptpclient.c |  5 +++
>>>>    lib/ethdev/rte_ethdev.c        | 57 +++++++++++++++++++++++++++++++++-
>>>>    2 files changed, 61 insertions(+), 1 deletion(-)
>>>>
>>> .
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

timesync_read_rx_timestamp
On 9/21/2023 12:59 PM, lihuisong (C) wrote:
> add ice & igc maintainers
> 
> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> Sorry for my delay reply because of taking a look at all PMDs
>>> implementation.
>>>
>>>
>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>   From the first version of ptpclient, it seems that this example
>>>>> assume that
>>>>> the PMDs support the PTP feature and enable PTP by default. Please see
>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>>> which are introduced in 2015.
>>>>>
>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>> enable or
>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>
>>>> Hi Huisong,
>>>>
>>>> As far as I know this offload is not for PTP.
>>>> PTP and TIMESTAMP are different.
>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>> offlaod for PTP.
>>>
>> Can you please detail what is "PTP offload"?
>>
> It indicates whether the device supports PTP or enable  PTP feature.
>

We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
APIs to control PTP support.

But when mention from "offload", it is something device itself does.

PTP is a protocol (IEEE 1588), and used to synchronize clocks.
What I get is protocol can be parsed by networking stack and it can be
used by application to synchronize clock.

When you are refer to "PTP offload", does it mean device (NIC)
understands the protocol and parse it to synchronize device clock with
other devices?


We have 'rte_eth_timesync_*()' APIs, my understanding is application
parses the PTP protocol, and it may use this information to configure
NIC to synchronize its clock, but it may also use PTP provided
information to sync any other clock. Is this understanding correct?


> If TIMESTAMP offload is not for PTP, I don't know what the point of this
> offload independent existence is.
>

TIMESTAMP offload request device to add timestamp to mbuf in ingress,
and use mbuf timestamp to schedule packet for egress.

Technically this time-stamping can be done by driver, but if offload
set, HW timestamp is used for it.

Rx timestamp can be used for various reasons, like debugging and
performance/latency analyses, etc..


>>
>>>> PTP is a protocol for time sync.
>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>> Yes.
>>> But a lot of PMDs actually depand on HW to report Rx timestamp releated
>>> information
>>> because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp
>>> API.
>>>
>> HW support may be required for PTP but this doesn't mean timestamp
>> offload is used.
> understand.
>>
>>>>> And then about four years later, ptpclient enable Rx timestamp offload
>>>>> because some PMDs require this offload to enable. Please see
>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>>>> offload").
>>>>>
>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>> updated
>>>> ptpclient sample to set TIMESTAMP offload.
>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3
>>> and so on.
>>>
>> Can you please point the ice & igc code, cc'ing their maintainers, we
>> can look together?
> 
> *-->igc code:*
> 
> Having following codes in igc_recv_scattered_pkts():
> 
>         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>                     uint32_t *, -IGC_TS_HDR_LEN);
>             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>                     ts[2];
>             rxm->timesync = rxq->queue_id;
>         }
> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
> 

Above code requires TIMESTAMP offload to set timesync, but this
shouldn't be a requirement. Usage seems mixed.

> *-->ice code:*
> 
> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>         if (ice_timestamp_dynflag > 0 &&
>             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>             rxq->time_high =
>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>             if (unlikely(is_tsinit)) {
>                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
> rxq->time_high);
>                 rxq->hw_time_low = (uint32_t)ts_ns;
>                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>                 is_tsinit = false;
>             } else {
>                 if (rxq->time_high < rxq->hw_time_low)
>                     rxq->hw_time_high += 1;
>                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
>                 rxq->hw_time_low = rxq->time_high;
>             }
>             rxq->hw_time_update = rte_get_timer_cycles() /
>                          (rte_get_timer_hz() / 1000);
>             *RTE_MBUF_DYNFIELD(rxm,
>                        (ice_timestamp_dynfield_offset),
>                        rte_mbuf_timestamp_t *) = ts_ns;
>             pkt_flags |= ice_timestamp_dynflag;
>         }
> 
>         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>             RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>             rxq->time_high =
>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>             rxm->timesync = rxq->queue_id;
>             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>         }
> #endif
> 
> Note: rxm->timesync and rxq->time_high will be used in
> timesync_read_rx_timestamp()
> 

This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
and flag set accordingly.

And if PTP enabled, and PTP packet type detected relevant flag set in
mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'.


Although above usage looks correct, I can see in 'ice_timesync_enable()'
TIMESTAMP offload is used requirement to enable timesync.
TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
mentioned what is done is dpaa2.


>>
>>
>>>> We need to clarify dpaa2 usage.
>>>>
>>>>> By all the records, this is more like a process of perfecting PTP
>>>>> feature.
>>>>> Not all network adaptors support PTP feature. So adding the check for
>>>>> PTP
>>>>> capability in ethdev layer is necessary.
>>>>>
>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
>>>> checked, so no additional check is needed.
>>> But only having dev_ops about PTP doesn't satisfy the use of this
>>> feature.
>>> For example,
>>> there are serveal network ports belonged to a driver on one OS, and only
>>> one port support PTP function.
>>> So driver needs one *PTP* offload.
>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>> what is causing confusion.
>>> Yes it is a little bit confusion.
>>> There are two kinds of implementation:
>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>> B:  saving "Rx timestamp related information" from Rx description when
>>> receive PTP SYNC packet and
>>>      report it in read_rx_timestamp API.
>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>> save "Rx timestamp related information.
>>> What do you think about this, Ferruh?
>>>> I would be great if you can help on clarification, and update
>>>> documentation or API comments, or what ever required, for this.
>>> ok
>>>>> ---
>>>>> v3:
>>>>>    - patch [2/3] for hns3 has been applied and so remove it.
>>>>>    - ops pointer check is closer to usage.
>>>>>
>>>>> Huisong Li (2):
>>>>>     examples/ptpclient: add the check for PTP capability
>>>>>     ethdev: add the check for the valitity of timestamp offload
>>>>>
>>>>>    examples/ptpclient/ptpclient.c |  5 +++
>>>>>    lib/ethdev/rte_ethdev.c        | 57
>>>>> +++++++++++++++++++++++++++++++++-
>>>>>    2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>> .
>> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

在 2023/11/2 7:39, Ferruh Yigit 写道:
> timesync_read_rx_timestamp
> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>> add ice & igc maintainers
>>
>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>> implementation.
>>>>
>>>>
>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>    From the first version of ptpclient, it seems that this example
>>>>>> assume that
>>>>>> the PMDs support the PTP feature and enable PTP by default. Please see
>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>>>> which are introduced in 2015.
>>>>>>
>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>> enable or
>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>
>>>>> Hi Huisong,
>>>>>
>>>>> As far as I know this offload is not for PTP.
>>>>> PTP and TIMESTAMP are different.
>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>> offlaod for PTP.
>>>>
>>> Can you please detail what is "PTP offload"?
>>>
>> It indicates whether the device supports PTP or enable  PTP feature.
>>
> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
> APIs to control PTP support.
No, this is just to control it.
we still need to like a device capablity to report application if the 
port support to call this API, right?
>
> But when mention from "offload", it is something device itself does.
>
> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
> What I get is protocol can be parsed by networking stack and it can be
> used by application to synchronize clock.
>
> When you are refer to "PTP offload", does it mean device (NIC)
> understands the protocol and parse it to synchronize device clock with
> other devices?
Good point. PTP offload is unreasonable.
But the capablity is required indeed.
What do you think of introducing a RTE_ETH_DEV_PTP in 
dev->data->dev_flags for PTP feature?
>
>
> We have 'rte_eth_timesync_*()' APIs, my understanding is application
> parses the PTP protocol, and it may use this information to configure
> NIC to synchronize its clock, but it may also use PTP provided
> information to sync any other clock. Is this understanding correct?
>
>
>> If TIMESTAMP offload is not for PTP, I don't know what the point of this
>> offload independent existence is.
>>
> TIMESTAMP offload request device to add timestamp to mbuf in ingress,
> and use mbuf timestamp to schedule packet for egress.
Agree.
>
> Technically this time-stamping can be done by driver, but if offload
> set, HW timestamp is used for it.
>
> Rx timestamp can be used for various reasons, like debugging and
> performance/latency analyses, etc..
>
>
>>>>> PTP is a protocol for time sync.
>>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>>> Yes.
>>>> But a lot of PMDs actually depand on HW to report Rx timestamp releated
>>>> information
>>>> because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp
>>>> API.
>>>>
>>> HW support may be required for PTP but this doesn't mean timestamp
>>> offload is used.
>> understand.
>>>>>> And then about four years later, ptpclient enable Rx timestamp offload
>>>>>> because some PMDs require this offload to enable. Please see
>>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>>>>> offload").
>>>>>>
>>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>>> updated
>>>>> ptpclient sample to set TIMESTAMP offload.
>>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3
>>>> and so on.
>>>>
>>> Can you please point the ice & igc code, cc'ing their maintainers, we
>>> can look together?
>> *-->igc code:*
>>
>> Having following codes in igc_recv_scattered_pkts():
>>
>>          if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>>              uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>>                      uint32_t *, -IGC_TS_HDR_LEN);
>>              rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>>                      ts[2];
>>              rxm->timesync = rxq->queue_id;
>>          }
>> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
>>
> Above code requires TIMESTAMP offload to set timesync, but this
> shouldn't be a requirement. Usage seems mixed.
>
>> *-->ice code:*
>>
>> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>>          if (ice_timestamp_dynflag > 0 &&
>>              (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>>              rxq->time_high =
>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>              if (unlikely(is_tsinit)) {
>>                  ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
>> rxq->time_high);
>>                  rxq->hw_time_low = (uint32_t)ts_ns;
>>                  rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>>                  is_tsinit = false;
>>              } else {
>>                  if (rxq->time_high < rxq->hw_time_low)
>>                      rxq->hw_time_high += 1;
>>                  ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
>>                  rxq->hw_time_low = rxq->time_high;
>>              }
>>              rxq->hw_time_update = rte_get_timer_cycles() /
>>                           (rte_get_timer_hz() / 1000);
>>              *RTE_MBUF_DYNFIELD(rxm,
>>                         (ice_timestamp_dynfield_offset),
>>                         rte_mbuf_timestamp_t *) = ts_ns;
>>              pkt_flags |= ice_timestamp_dynflag;
>>          }
>>
>>          if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>>              RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>>              rxq->time_high =
>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>              rxm->timesync = rxq->queue_id;
>>              pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>          }
>> #endif
>>
>> Note: rxm->timesync and rxq->time_high will be used in
>> timesync_read_rx_timestamp()
>>
> This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
> and flag set accordingly.
hns3 also implemented PTP as ice did.
>
> And if PTP enabled, and PTP packet type detected relevant flag set in
> mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'.
Yes.
>
>
> Although above usage looks correct, I can see in 'ice_timesync_enable()'
> TIMESTAMP offload is used requirement to enable timesync.
> TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
> mentioned what is done is dpaa2.
>
>
>>>
>>>>> We need to clarify dpaa2 usage.
>>>>>
>>>>>> By all the records, this is more like a process of perfecting PTP
>>>>>> feature.
>>>>>> Not all network adaptors support PTP feature. So adding the check for
>>>>>> PTP
>>>>>> capability in ethdev layer is necessary.
>>>>>>
>>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already
>>>>> checked, so no additional check is needed.
>>>> But only having dev_ops about PTP doesn't satisfy the use of this
>>>> feature.
>>>> For example,
>>>> there are serveal network ports belonged to a driver on one OS, and only
>>>> one port support PTP function.
>>>> So driver needs one *PTP* offload.
>>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>>> what is causing confusion.
>>>> Yes it is a little bit confusion.
>>>> There are two kinds of implementation:
>>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>>> B:  saving "Rx timestamp related information" from Rx description when
>>>> receive PTP SYNC packet and
>>>>       report it in read_rx_timestamp API.
>>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>>> save "Rx timestamp related information.
>>>> What do you think about this, Ferruh?
>>>>> I would be great if you can help on clarification, and update
>>>>> documentation or API comments, or what ever required, for this.
>>>> ok
>>>>>> ---
>>>>>> v3:
>>>>>>     - patch [2/3] for hns3 has been applied and so remove it.
>>>>>>     - ops pointer check is closer to usage.
>>>>>>
>>>>>> Huisong Li (2):
>>>>>>      examples/ptpclient: add the check for PTP capability
>>>>>>      ethdev: add the check for the valitity of timestamp offload
>>>>>>
>>>>>>     examples/ptpclient/ptpclient.c |  5 +++
>>>>>>     lib/ethdev/rte_ethdev.c        | 57
>>>>>> +++++++++++++++++++++++++++++++++-
>>>>>>     2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>> .
>>> .
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

Hi Ferruh,

在 2023/11/23 19:56, lihuisong (C) 写道:
>
> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>> timesync_read_rx_timestamp
>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>> add ice & igc maintainers
>>>
>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>> implementation.
>>>>>
>>>>>
>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>    From the first version of ptpclient, it seems that this example
>>>>>>> assume that
>>>>>>> the PMDs support the PTP feature and enable PTP by default. 
>>>>>>> Please see
>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>>>>> which are introduced in 2015.
>>>>>>>
>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>> enable or
>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>
>>>>>> Hi Huisong,
>>>>>>
>>>>>> As far as I know this offload is not for PTP.
>>>>>> PTP and TIMESTAMP are different.
>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>>> offlaod for PTP.
>>>>>
>>>> Can you please detail what is "PTP offload"?
>>>>
>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>
>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>> APIs to control PTP support.
> No, this is just to control it.
> we still need to like a device capablity to report application if the 
> port support to call this API, right?
>>
>> But when mention from "offload", it is something device itself does.
>>
>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>> What I get is protocol can be parsed by networking stack and it can be
>> used by application to synchronize clock.
>>
>> When you are refer to "PTP offload", does it mean device (NIC)
>> understands the protocol and parse it to synchronize device clock with
>> other devices?
> Good point. PTP offload is unreasonable.
> But the capablity is required indeed.
> What do you think of introducing a RTE_ETH_DEV_PTP in 
> dev->data->dev_flags for PTP feature?

Can you take a look at this discussion line again?

Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if 
the device support PTP feature.

>>
>>
>> We have 'rte_eth_timesync_*()' APIs, my understanding is application
>> parses the PTP protocol, and it may use this information to configure
>> NIC to synchronize its clock, but it may also use PTP provided
>> information to sync any other clock. Is this understanding correct?
>>
>>
>>> If TIMESTAMP offload is not for PTP, I don't know what the point of 
>>> this
>>> offload independent existence is.
>>>
>> TIMESTAMP offload request device to add timestamp to mbuf in ingress,
>> and use mbuf timestamp to schedule packet for egress.
> Agree.
>>
>> Technically this time-stamping can be done by driver, but if offload
>> set, HW timestamp is used for it.
>>
>> Rx timestamp can be used for various reasons, like debugging and
>> performance/latency analyses, etc..
>>
>>
>>>>>> PTP is a protocol for time sync.
>>>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>>>> Yes.
>>>>> But a lot of PMDs actually depand on HW to report Rx timestamp 
>>>>> releated
>>>>> information
>>>>> because of reading Rx timestamp of PTP SYNC packet in 
>>>>> read_rx_timestamp
>>>>> API.
>>>>>
>>>> HW support may be required for PTP but this doesn't mean timestamp
>>>> offload is used.
>>> understand.
>>>>>>> And then about four years later, ptpclient enable Rx timestamp 
>>>>>>> offload
>>>>>>> because some PMDs require this offload to enable. Please see
>>>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>>>>>>> offload").
>>>>>>>
>>>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>>>> updated
>>>>>> ptpclient sample to set TIMESTAMP offload.
>>>>> There are many PMDs doing like this, such as ice, igc, cnxk, 
>>>>> dpaa2, hns3
>>>>> and so on.
>>>>>
>>>> Can you please point the ice & igc code, cc'ing their maintainers, we
>>>> can look together?
>>> *-->igc code:*
>>>
>>> Having following codes in igc_recv_scattered_pkts():
>>>
>>>          if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>>>              uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>>>                      uint32_t *, -IGC_TS_HDR_LEN);
>>>              rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>>>                      ts[2];
>>>              rxm->timesync = rxq->queue_id;
>>>          }
>>> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
>>>
>> Above code requires TIMESTAMP offload to set timesync, but this
>> shouldn't be a requirement. Usage seems mixed.
>>
>>> *-->ice code:*
>>>
>>> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>>>          if (ice_timestamp_dynflag > 0 &&
>>>              (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>>>              rxq->time_high =
>>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>>              if (unlikely(is_tsinit)) {
>>>                  ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
>>> rxq->time_high);
>>>                  rxq->hw_time_low = (uint32_t)ts_ns;
>>>                  rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>>>                  is_tsinit = false;
>>>              } else {
>>>                  if (rxq->time_high < rxq->hw_time_low)
>>>                      rxq->hw_time_high += 1;
>>>                  ts_ns = (uint64_t)rxq->hw_time_high << 32 | 
>>> rxq->time_high;
>>>                  rxq->hw_time_low = rxq->time_high;
>>>              }
>>>              rxq->hw_time_update = rte_get_timer_cycles() /
>>>                           (rte_get_timer_hz() / 1000);
>>>              *RTE_MBUF_DYNFIELD(rxm,
>>>                         (ice_timestamp_dynfield_offset),
>>>                         rte_mbuf_timestamp_t *) = ts_ns;
>>>              pkt_flags |= ice_timestamp_dynflag;
>>>          }
>>>
>>>          if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>>>              RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>>>              rxq->time_high =
>>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>>              rxm->timesync = rxq->queue_id;
>>>              pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>          }
>>> #endif
>>>
>>> Note: rxm->timesync and rxq->time_high will be used in
>>> timesync_read_rx_timestamp()
>>>
>> This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
>> and flag set accordingly.
> hns3 also implemented PTP as ice did.
>>
>> And if PTP enabled, and PTP packet type detected relevant flag set in
>> mbuf, and timesyc value set to use later for 
>> 'timesync_read_rx_timestamp()'.
> Yes.
>>
>>
>> Although above usage looks correct, I can see in 'ice_timesync_enable()'
>> TIMESTAMP offload is used requirement to enable timesync.
>> TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
>> mentioned what is done is dpaa2.
>>
>>
>>>>
>>>>>> We need to clarify dpaa2 usage.
>>>>>>
>>>>>>> By all the records, this is more like a process of perfecting PTP
>>>>>>> feature.
>>>>>>> Not all network adaptors support PTP feature. So adding the 
>>>>>>> check for
>>>>>>> PTP
>>>>>>> capability in ethdev layer is necessary.
>>>>>>>
>>>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops 
>>>>>> already
>>>>>> checked, so no additional check is needed.
>>>>> But only having dev_ops about PTP doesn't satisfy the use of this
>>>>> feature.
>>>>> For example,
>>>>> there are serveal network ports belonged to a driver on one OS, 
>>>>> and only
>>>>> one port support PTP function.
>>>>> So driver needs one *PTP* offload.
>>>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>>>> what is causing confusion.
>>>>> Yes it is a little bit confusion.
>>>>> There are two kinds of implementation:
>>>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>>>> B:  saving "Rx timestamp related information" from Rx description 
>>>>> when
>>>>> receive PTP SYNC packet and
>>>>>       report it in read_rx_timestamp API.
>>>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>>>> save "Rx timestamp related information.
>>>>> What do you think about this, Ferruh?
>>>>>> I would be great if you can help on clarification, and update
>>>>>> documentation or API comments, or what ever required, for this.
>>>>> ok
>>>>>>> ---
>>>>>>> v3:
>>>>>>>     - patch [2/3] for hns3 has been applied and so remove it.
>>>>>>>     - ops pointer check is closer to usage.
>>>>>>>
>>>>>>> Huisong Li (2):
>>>>>>>      examples/ptpclient: add the check for PTP capability
>>>>>>>      ethdev: add the check for the valitity of timestamp offload
>>>>>>>
>>>>>>>     examples/ptpclient/ptpclient.c |  5 +++
>>>>>>>     lib/ethdev/rte_ethdev.c        | 57
>>>>>>> +++++++++++++++++++++++++++++++++-
>>>>>>>     2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>> .
>>>> .
>> .
>
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 1/11/2024 6:25 AM, lihuisong (C) wrote:
> Hi Ferruh,
> 
> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>
>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>> timesync_read_rx_timestamp
>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>> add ice & igc maintainers
>>>>
>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>>> Hi Ferruh,
>>>>>>
>>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>>> implementation.
>>>>>>
>>>>>>
>>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>>    From the first version of ptpclient, it seems that this example
>>>>>>>> assume that
>>>>>>>> the PMDs support the PTP feature and enable PTP by default.
>>>>>>>> Please see
>>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>>>>>> which are introduced in 2015.
>>>>>>>>
>>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>>> enable or
>>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>>
>>>>>>> Hi Huisong,
>>>>>>>
>>>>>>> As far as I know this offload is not for PTP.
>>>>>>> PTP and TIMESTAMP are different.
>>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>>>> offlaod for PTP.
>>>>>>
>>>>> Can you please detail what is "PTP offload"?
>>>>>
>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>
>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>> APIs to control PTP support.
>> No, this is just to control it.
>> we still need to like a device capablity to report application if the
>> port support to call this API, right?
>>>
>>> But when mention from "offload", it is something device itself does.
>>>
>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>> What I get is protocol can be parsed by networking stack and it can be
>>> used by application to synchronize clock.
>>>
>>> When you are refer to "PTP offload", does it mean device (NIC)
>>> understands the protocol and parse it to synchronize device clock with
>>> other devices?
>> Good point. PTP offload is unreasonable.
>> But the capablity is required indeed.
>> What do you think of introducing a RTE_ETH_DEV_PTP in
>> dev->data->dev_flags for PTP feature?
> 
> Can you take a look at this discussion line again?
> 
> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
> the device support PTP feature.
> 

Hi Huisong,

First let me try to summarize the discussion since it has been some time.

HW timer block can be used for Rx timestamp offload
(RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
different things.

This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
support, which is wrong.



After we agreed on above, your next question is to use 'dev_flag' to
report PTP capability.

First, can you please describe what is the motivation to learn if HW
supports PTP or now, what is the benefit of knowing this.

If we agree that there is a need to know the PTP capability, question is
where to report this capability.

Suggested 'dev_flags' is used for various things, some are internal
flags and some are status, I don't think overloading this variable is
not good idea.

Other option is an update 'rte_eth_dev_info_get()' for it but it has the
same problem, this function is already overloaded with many different
things.

We can have an API just to get PTP capability, but this will require a
new dev_ops, this can be an option but my concern is having a capability
dev_ops for each feature create a mess in dev_ops.

Perhaps we can have single get_capability() API, and it gets features as
flags to return if that feature is supported or not, but this requires a
wider discussion.

Instead can we deduce the capability from PTP relevant dev_ops, if they
are implemented we can say it is supported? This doesn't require new
support.

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

在 2024/1/27 0:54, Ferruh Yigit 写道:
> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>> timesync_read_rx_timestamp
>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>> add ice & igc maintainers
>>>>>
>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>>>> Hi Ferruh,
>>>>>>>
>>>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>>>> implementation.
>>>>>>>
>>>>>>>
>>>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>>>     From the first version of ptpclient, it seems that this example
>>>>>>>>> assume that
>>>>>>>>> the PMDs support the PTP feature and enable PTP by default.
>>>>>>>>> Please see
>>>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client")
>>>>>>>>> which are introduced in 2015.
>>>>>>>>>
>>>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>>>> enable or
>>>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>>>
>>>>>>>> Hi Huisong,
>>>>>>>>
>>>>>>>> As far as I know this offload is not for PTP.
>>>>>>>> PTP and TIMESTAMP are different.
>>>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>>>>>> offlaod for PTP.
>>>>>>>
>>>>>> Can you please detail what is "PTP offload"?
>>>>>>
>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>
>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>> APIs to control PTP support.
>>> No, this is just to control it.
>>> we still need to like a device capablity to report application if the
>>> port support to call this API, right?
>>>> But when mention from "offload", it is something device itself does.
>>>>
>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>> What I get is protocol can be parsed by networking stack and it can be
>>>> used by application to synchronize clock.
>>>>
>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>> understands the protocol and parse it to synchronize device clock with
>>>> other devices?
>>> Good point. PTP offload is unreasonable.
>>> But the capablity is required indeed.
>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>> dev->data->dev_flags for PTP feature?
>> Can you take a look at this discussion line again?
>>
>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>> the device support PTP feature.
>>
Hi Ferruh,

Thanks for taking your time to reply.

> Hi Huisong,
>
> First let me try to summarize the discussion since it has been some time.
>
> HW timer block can be used for Rx timestamp offload
> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
> different things.
>
> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
> support, which is wrong.
>
correct.
>
> After we agreed on above, your next question is to use 'dev_flag' to
> report PTP capability.
>
> First, can you please describe what is the motivation to learn if HW
> supports PTP or now, what is the benefit of knowing this.
There are a couple of device which have the same driver on a platform or OS.
But just allow one device to support or be responsible for PTP feature.
The firmware will report a capability to tell the device if it is 
support PTP.
But, currently, driver doesn't know how to report user which device 
support PTP feature.

In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
Whether the device supports PTP is irrelevant to this macro.
>
> If we agree that there is a need to know the PTP capability, question is
> where to report this capability.
>
> Suggested 'dev_flags' is used for various things, some are internal
> flags and some are status, I don't think overloading this variable is
> not good idea.
Yes, we need to consider  carefully.
>
> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
> same problem, this function is already overloaded with many different
> things.
>
> We can have an API just to get PTP capability, but this will require a
> new dev_ops, this can be an option but my concern is having a capability
> dev_ops for each feature create a mess in dev_ops.
>
> Perhaps we can have single get_capability() API, and it gets features as
> flags to return if that feature is supported or not, but this requires a
> wider discussion.
>
> Instead can we deduce the capability from PTP relevant dev_ops, if they
> are implemented we can say it is supported? This doesn't require new
> support.
Thank you mentioning so many ways.
For the end of advice, I don't think it is appropriate.
Because we have to modify dynamically the pointer address of all PTP 
APIs in dev_ops on the above case.

How about we use rte_eth_dev_info.dev_capa to report PTP offload?
This is specifically used to report "Non-offload capabilities" according 
to its document.


>
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 1/27/2024 1:52 AM, lihuisong (C) wrote:
> 
> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>> timesync_read_rx_timestamp
>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>> add ice & igc maintainers
>>>>>>
>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>>>>> Hi Ferruh,
>>>>>>>>
>>>>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>>>>> implementation.
>>>>>>>>
>>>>>>>>
>>>>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>>>>     From the first version of ptpclient, it seems that this
>>>>>>>>>> example
>>>>>>>>>> assume that
>>>>>>>>>> the PMDs support the PTP feature and enable PTP by default.
>>>>>>>>>> Please see
>>>>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP
>>>>>>>>>> client")
>>>>>>>>>> which are introduced in 2015.
>>>>>>>>>>
>>>>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>>>>> enable or
>>>>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>>>>
>>>>>>>>> Hi Huisong,
>>>>>>>>>
>>>>>>>>> As far as I know this offload is not for PTP.
>>>>>>>>> PTP and TIMESTAMP are different.
>>>>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add
>>>>>>>> one new
>>>>>>>> offlaod for PTP.
>>>>>>>>
>>>>>>> Can you please detail what is "PTP offload"?
>>>>>>>
>>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>>
>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>> APIs to control PTP support.
>>>> No, this is just to control it.
>>>> we still need to like a device capablity to report application if the
>>>> port support to call this API, right?
>>>>> But when mention from "offload", it is something device itself does.
>>>>>
>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>> What I get is protocol can be parsed by networking stack and it can be
>>>>> used by application to synchronize clock.
>>>>>
>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>> understands the protocol and parse it to synchronize device clock with
>>>>> other devices?
>>>> Good point. PTP offload is unreasonable.
>>>> But the capablity is required indeed.
>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>> dev->data->dev_flags for PTP feature?
>>> Can you take a look at this discussion line again?
>>>
>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>>> the device support PTP feature.
>>>
> Hi Ferruh,
> 
> Thanks for taking your time to reply.
> 
>> Hi Huisong,
>>
>> First let me try to summarize the discussion since it has been some time.
>>
>> HW timer block can be used for Rx timestamp offload
>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>> different things.
>>
>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>> support, which is wrong.
>>
> correct.
>>
>> After we agreed on above, your next question is to use 'dev_flag' to
>> report PTP capability.
>>
>> First, can you please describe what is the motivation to learn if HW
>> supports PTP or now, what is the benefit of knowing this.
> There are a couple of device which have the same driver on a platform or
> OS.
> But just allow one device to support or be responsible for PTP feature.
> The firmware will report a capability to tell the device if it is
> support PTP.
> But, currently, driver doesn't know how to report user which device
> support PTP feature.
> 

Driver can hold a private data that records if PTP supported by the
device or not, and according this value PTP dev_ops can return error or
success.

This may not be ideal but it lets user to know about the support status,
can this work?


> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
> Whether the device supports PTP is irrelevant to this macro.
>

Yes, I guess because both features use same HW, there is confusion there.

>>
>> If we agree that there is a need to know the PTP capability, question is
>> where to report this capability.
>>
>> Suggested 'dev_flags' is used for various things, some are internal
>> flags and some are status, I don't think overloading this variable is
>> not good idea.
> Yes, we need to consider  carefully.
>>
>> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
>> same problem, this function is already overloaded with many different
>> things.
>>
>> We can have an API just to get PTP capability, but this will require a
>> new dev_ops, this can be an option but my concern is having a capability
>> dev_ops for each feature create a mess in dev_ops.
>>
>> Perhaps we can have single get_capability() API, and it gets features as
>> flags to return if that feature is supported or not, but this requires a
>> wider discussion.
>>
>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>> are implemented we can say it is supported? This doesn't require new
>> support.
> Thank you mentioning so many ways.
> For the end of advice, I don't think it is appropriate.
> Because we have to modify dynamically the pointer address of all PTP
> APIs in dev_ops on the above case.
> 

I was thinking for the case application distinguish if PTP related
dev_ops set or not, but after your explanation I can see this won't help
for your case.
Because in your case PTP dev_ops implemented but some devices support it
and some don't, and you are looking for a way to distinguish it.

> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
> This is specifically used to report "Non-offload capabilities" according
> to its document.
> 

As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
cautious to expand it more.
Also I think it is a wider discussion if we want a capability reporting
in ethdev and where it should be.

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[lihuisong (C)]

在 2024/1/29 19:16, Ferruh Yigit 写道:
> On 1/27/2024 1:52 AM, lihuisong (C) wrote:
>> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>>> timesync_read_rx_timestamp
>>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>>> add ice & igc maintainers
>>>>>>>
>>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>>>>>> Hi Ferruh,
>>>>>>>>>
>>>>>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>>>>>> implementation.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>>>>>      From the first version of ptpclient, it seems that this
>>>>>>>>>>> example
>>>>>>>>>>> assume that
>>>>>>>>>>> the PMDs support the PTP feature and enable PTP by default.
>>>>>>>>>>> Please see
>>>>>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP
>>>>>>>>>>> client")
>>>>>>>>>>> which are introduced in 2015.
>>>>>>>>>>>
>>>>>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>>>>>> enable or
>>>>>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>>>>>
>>>>>>>>>> Hi Huisong,
>>>>>>>>>>
>>>>>>>>>> As far as I know this offload is not for PTP.
>>>>>>>>>> PTP and TIMESTAMP are different.
>>>>>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add
>>>>>>>>> one new
>>>>>>>>> offlaod for PTP.
>>>>>>>>>
>>>>>>>> Can you please detail what is "PTP offload"?
>>>>>>>>
>>>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>>>
>>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>>> APIs to control PTP support.
>>>>> No, this is just to control it.
>>>>> we still need to like a device capablity to report application if the
>>>>> port support to call this API, right?
>>>>>> But when mention from "offload", it is something device itself does.
>>>>>>
>>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>>> What I get is protocol can be parsed by networking stack and it can be
>>>>>> used by application to synchronize clock.
>>>>>>
>>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>>> understands the protocol and parse it to synchronize device clock with
>>>>>> other devices?
>>>>> Good point. PTP offload is unreasonable.
>>>>> But the capablity is required indeed.
>>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>>> dev->data->dev_flags for PTP feature?
>>>> Can you take a look at this discussion line again?
>>>>
>>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>>>> the device support PTP feature.
>>>>
>> Hi Ferruh,
>>
>> Thanks for taking your time to reply.
>>
>>> Hi Huisong,
>>>
>>> First let me try to summarize the discussion since it has been some time.
>>>
>>> HW timer block can be used for Rx timestamp offload
>>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>>> different things.
>>>
>>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>>> support, which is wrong.
>>>
>> correct.
>>> After we agreed on above, your next question is to use 'dev_flag' to
>>> report PTP capability.
>>>
>>> First, can you please describe what is the motivation to learn if HW
>>> supports PTP or now, what is the benefit of knowing this.
>> There are a couple of device which have the same driver on a platform or
>> OS.
>> But just allow one device to support or be responsible for PTP feature.
>> The firmware will report a capability to tell the device if it is
>> support PTP.
>> But, currently, driver doesn't know how to report user which device
>> support PTP feature.
>>
> Driver can hold a private data that records if PTP supported by the
> device or not, and according this value PTP dev_ops can return error or
> success.
>
> This may not be ideal but it lets user to know about the support status,
> can this work?
I don't think it is user friendly.
Users know which port supports the PTP feature only when the API fails 
to be invoked, right?
In addition, this is a common issue for all supported PTP device. So It 
is better to do this check in PMD.
>
>
>> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
>> Whether the device supports PTP is irrelevant to this macro.
>>
> Yes, I guess because both features use same HW, there is confusion there.
>
>>> If we agree that there is a need to know the PTP capability, question is
>>> where to report this capability.
>>>
>>> Suggested 'dev_flags' is used for various things, some are internal
>>> flags and some are status, I don't think overloading this variable is
>>> not good idea.
>> Yes, we need to consider  carefully.
>>> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
>>> same problem, this function is already overloaded with many different
>>> things.
>>>
>>> We can have an API just to get PTP capability, but this will require a
>>> new dev_ops, this can be an option but my concern is having a capability
>>> dev_ops for each feature create a mess in dev_ops.
>>>
>>> Perhaps we can have single get_capability() API, and it gets features as
>>> flags to return if that feature is supported or not, but this requires a
>>> wider discussion.
>>>
>>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>>> are implemented we can say it is supported? This doesn't require new
>>> support.
>> Thank you mentioning so many ways.
>> For the end of advice, I don't think it is appropriate.
>> Because we have to modify dynamically the pointer address of all PTP
>> APIs in dev_ops on the above case.
>>
> I was thinking for the case application distinguish if PTP related
> dev_ops set or not, but after your explanation I can see this won't help
> for your case.
> Because in your case PTP dev_ops implemented but some devices support it
> and some don't, and you are looking for a way to distinguish it.
Yes
>
>> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
>> This is specifically used to report "Non-offload capabilities" according
>> to its document.
>>
> As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
> cautious to expand it more.
> Also I think it is a wider discussion if we want a capability reporting
> in ethdev and where it should be.
How about we send a RFC patch which use rte_eth_dev_info.dev_capa to 
report PTP offload and start to disscuss this issue?
And add Thomas's patch [1] and this patch.

[1]https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/

>
> .

Re: [PATCH v3 0/2] ethdev: add the check for PTP capability

[Ferruh Yigit]

On 1/29/2024 1:58 PM, lihuisong (C) wrote:
> 
> 在 2024/1/29 19:16, Ferruh Yigit 写道:
>> On 1/27/2024 1:52 AM, lihuisong (C) wrote:
>>> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>>>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>>>> timesync_read_rx_timestamp
>>>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>>>> add ice & igc maintainers
>>>>>>>>
>>>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>>>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>>>>>>>>> Hi Ferruh,
>>>>>>>>>>
>>>>>>>>>> Sorry for my delay reply because of taking a look at all PMDs
>>>>>>>>>> implementation.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>>>>>>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>>>>>>>>>      From the first version of ptpclient, it seems that this
>>>>>>>>>>>> example
>>>>>>>>>>>> assume that
>>>>>>>>>>>> the PMDs support the PTP feature and enable PTP by default.
>>>>>>>>>>>> Please see
>>>>>>>>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP
>>>>>>>>>>>> client")
>>>>>>>>>>>> which are introduced in 2015.
>>>>>>>>>>>>
>>>>>>>>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>>>>>>>>> enable or
>>>>>>>>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>>>>>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>>>>>>>>
>>>>>>>>>>> Hi Huisong,
>>>>>>>>>>>
>>>>>>>>>>> As far as I know this offload is not for PTP.
>>>>>>>>>>> PTP and TIMESTAMP are different.
>>>>>>>>>> If TIMESTAMP offload cannot stand for PTP, we may need to add
>>>>>>>>>> one new
>>>>>>>>>> offlaod for PTP.
>>>>>>>>>>
>>>>>>>>> Can you please detail what is "PTP offload"?
>>>>>>>>>
>>>>>>>> It indicates whether the device supports PTP or enable  PTP
>>>>>>>> feature.
>>>>>>>>
>>>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>>>> APIs to control PTP support.
>>>>>> No, this is just to control it.
>>>>>> we still need to like a device capablity to report application if the
>>>>>> port support to call this API, right?
>>>>>>> But when mention from "offload", it is something device itself does.
>>>>>>>
>>>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>>>> What I get is protocol can be parsed by networking stack and it
>>>>>>> can be
>>>>>>> used by application to synchronize clock.
>>>>>>>
>>>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>>>> understands the protocol and parse it to synchronize device clock
>>>>>>> with
>>>>>>> other devices?
>>>>>> Good point. PTP offload is unreasonable.
>>>>>> But the capablity is required indeed.
>>>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>>>> dev->data->dev_flags for PTP feature?
>>>>> Can you take a look at this discussion line again?
>>>>>
>>>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to
>>>>> reveal if
>>>>> the device support PTP feature.
>>>>>
>>> Hi Ferruh,
>>>
>>> Thanks for taking your time to reply.
>>>
>>>> Hi Huisong,
>>>>
>>>> First let me try to summarize the discussion since it has been some
>>>> time.
>>>>
>>>> HW timer block can be used for Rx timestamp offload
>>>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>>>> different things.
>>>>
>>>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>>>> support, which is wrong.
>>>>
>>> correct.
>>>> After we agreed on above, your next question is to use 'dev_flag' to
>>>> report PTP capability.
>>>>
>>>> First, can you please describe what is the motivation to learn if HW
>>>> supports PTP or now, what is the benefit of knowing this.
>>> There are a couple of device which have the same driver on a platform or
>>> OS.
>>> But just allow one device to support or be responsible for PTP feature.
>>> The firmware will report a capability to tell the device if it is
>>> support PTP.
>>> But, currently, driver doesn't know how to report user which device
>>> support PTP feature.
>>>
>> Driver can hold a private data that records if PTP supported by the
>> device or not, and according this value PTP dev_ops can return error or
>> success.
>>
>> This may not be ideal but it lets user to know about the support status,
>> can this work?
> I don't think it is user friendly.
> Users know which port supports the PTP feature only when the API fails
> to be invoked, right?
> In addition, this is a common issue for all supported PTP device. So It
> is better to do this check in PMD.
>>
>>
>>> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code
>>> flow.
>>> Whether the device supports PTP is irrelevant to this macro.
>>>
>> Yes, I guess because both features use same HW, there is confusion there.
>>
>>>> If we agree that there is a need to know the PTP capability,
>>>> question is
>>>> where to report this capability.
>>>>
>>>> Suggested 'dev_flags' is used for various things, some are internal
>>>> flags and some are status, I don't think overloading this variable is
>>>> not good idea.
>>> Yes, we need to consider  carefully.
>>>> Other option is an update 'rte_eth_dev_info_get()' for it but it has
>>>> the
>>>> same problem, this function is already overloaded with many different
>>>> things.
>>>>
>>>> We can have an API just to get PTP capability, but this will require a
>>>> new dev_ops, this can be an option but my concern is having a
>>>> capability
>>>> dev_ops for each feature create a mess in dev_ops.
>>>>
>>>> Perhaps we can have single get_capability() API, and it gets
>>>> features as
>>>> flags to return if that feature is supported or not, but this
>>>> requires a
>>>> wider discussion.
>>>>
>>>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>>>> are implemented we can say it is supported? This doesn't require new
>>>> support.
>>> Thank you mentioning so many ways.
>>> For the end of advice, I don't think it is appropriate.
>>> Because we have to modify dynamically the pointer address of all PTP
>>> APIs in dev_ops on the above case.
>>>
>> I was thinking for the case application distinguish if PTP related
>> dev_ops set or not, but after your explanation I can see this won't help
>> for your case.
>> Because in your case PTP dev_ops implemented but some devices support it
>> and some don't, and you are looking for a way to distinguish it.
> Yes
>>
>>> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
>>> This is specifically used to report "Non-offload capabilities" according
>>> to its document.
>>>
>> As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
>> cautious to expand it more.
>> Also I think it is a wider discussion if we want a capability reporting
>> in ethdev and where it should be.
> How about we send a RFC patch which use rte_eth_dev_info.dev_capa to
> report PTP offload and start to disscuss this issue?
>

Ack. Lets start discussion on top of a patch. Thanks.

> And add Thomas's patch [1] and this patch.
> 
> [1]https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/
> 
>>
>> .