[PATCH v1] net/ice: fix incorrect reading of PHY timestamp

[PATCH v1] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In ICE PMD, previously the ready bitmap checking before reading
PHY timestamp was not present. This caused incorrect Tx
timestamping.

The ready bitmap checking is enabled and PHY timestamp is read once
the ready bitmap gives positive value.

Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 7b1bd163a2..2357d6e1da 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6516,14 +6516,32 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct ice_tx_queue *txq;
 	uint8_t lport;
-	uint64_t ts_ns, ns, tstamp;
+	uint64_t ts_ns, ns, tstamp, tstamp_ready = 0;
+	uint64_t start_time, curr_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
+	txq = dev->data->tx_queues[0];
 	lport = hw->port_info->lport;
 
-	ret = ice_read_phy_tstamp(hw, lport, 0, &tstamp);
+	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready(hw, lport, &tstamp_ready);
+		if (ret) {
+			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
+			return -1;
+		}
+		curr_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+		if (curr_time - start_time > 1000) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	}
+
+	ret = ice_read_phy_tstamp(hw, lport, txq->queue_id, &tstamp);
 	if (ret) {
 		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
 		return -1;
-- 
2.43.0

[PATCH v2] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In ICE PMD, previously the ready bitmap checking before reading
PHY timestamp was not present. This caused incorrect Tx
timestamping.

The ready bitmap checking is enabled and PHY timestamp is read once
the ready bitmap gives positive value.

Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 7b1bd163a2..e0db47cf28 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6517,12 +6517,28 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	uint8_t lport;
-	uint64_t ts_ns, ns, tstamp;
+	uint64_t ts_ns, ns, tstamp, tstamp_ready = 0;
+	uint64_t start_time, curr_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
 	lport = hw->port_info->lport;
 
+	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready(hw, lport, &tstamp_ready);
+		if (ret) {
+			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
+			return -1;
+		}
+		curr_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+		if (curr_time - start_time > 1000) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	}
+
 	ret = ice_read_phy_tstamp(hw, lport, 0, &tstamp);
 	if (ret) {
 		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
-- 
2.43.0

[PATCH v3] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In ICE PMD, previously the ready bitmap checking before reading
PHY timestamp was not present. This caused incorrect Tx
timestamping.

The ready bitmap checking is enabled and PHY timestamp is read once
the ready bitmap gives positive value.

Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 7b1bd163a2..e0db47cf28 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6517,12 +6517,28 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	uint8_t lport;
-	uint64_t ts_ns, ns, tstamp;
+	uint64_t ts_ns, ns, tstamp, tstamp_ready = 0;
+	uint64_t start_time, curr_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
 	lport = hw->port_info->lport;
 
+	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready(hw, lport, &tstamp_ready);
+		if (ret) {
+			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
+			return -1;
+		}
+		curr_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+		if (curr_time - start_time > 1000) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	}
+
 	ret = ice_read_phy_tstamp(hw, lport, 0, &tstamp);
 	if (ret) {
 		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
-- 
2.43.0

[PATCH v4] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In ICE PMD, previously the ready bitmap checking before reading
PHY timestamp was not present. This caused incorrect Tx
timestamping.

The ready bitmap checking is enabled and PHY timestamp is read once
the ready bitmap gives positive value.

Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 7b1bd163a2..e0db47cf28 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6517,12 +6517,28 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	uint8_t lport;
-	uint64_t ts_ns, ns, tstamp;
+	uint64_t ts_ns, ns, tstamp, tstamp_ready = 0;
+	uint64_t start_time, curr_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
 	lport = hw->port_info->lport;
 
+	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready(hw, lport, &tstamp_ready);
+		if (ret) {
+			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
+			return -1;
+		}
+		curr_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+		if (curr_time - start_time > 1000) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	}
+
 	ret = ice_read_phy_tstamp(hw, lport, 0, &tstamp);
 	if (ret) {
 		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
-- 
2.43.0

Re: [PATCH v3] net/ice: fix incorrect reading of PHY timestamp

[Bruce Richardson]

On Fri, Oct 25, 2024 at 07:32:11AM +0000, Soumyadeep Hore wrote:
> In ICE PMD, previously the ready bitmap checking before reading
> PHY timestamp was not present. This caused incorrect Tx
> timestamping.
> 
> The ready bitmap checking is enabled and PHY timestamp is read once
> the ready bitmap gives positive value.
> 
> Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 7b1bd163a2..e0db47cf28 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -6517,12 +6517,28 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
>  	struct ice_adapter *ad =
>  			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	uint8_t lport;
> -	uint64_t ts_ns, ns, tstamp;
> +	uint64_t ts_ns, ns, tstamp, tstamp_ready = 0;
> +	uint64_t start_time, curr_time;
>  	const uint64_t mask = 0xFFFFFFFF;
>  	int ret;
>  
>  	lport = hw->port_info->lport;
>  
> +	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);

Why all the division by 1000?

> +
> +	while (!(tstamp_ready & BIT_ULL(0))) {
> +		ret = ice_get_phy_tx_tstamp_ready(hw, lport, &tstamp_ready);
> +		if (ret) {
> +			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
> +			return -1;
> +		}
> +		curr_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> +		if (curr_time - start_time > 1000) {

Is 1 second not a very long time to wait for this? Surely even milliseconds
is a very long delay in this case.

In terms of the logic, rather than constantly comparing vs the start time
and doing lots of division, I think it would be simpler to just set a max
end-time. For example, to keep the current 1s limit:

	uint64_t end_time = rte_get_timer_cycles?() + rte_get_timer_hz()
	...
	if (rte_get_timer_cycles() > end_time) {
	     ...
	}

> +			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
> +			return -1;
> +		}
> +	}
> +
>  	ret = ice_read_phy_tstamp(hw, lport, 0, &tstamp);
>  	if (ret) {
>  		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
> -- 
> 2.43.0
> 

Re: [PATCH v4] net/ice: fix incorrect reading of PHY timestamp

[Bruce Richardson]

On Fri, Oct 25, 2024 at 07:50:25AM +0000, Soumyadeep Hore wrote:
> In ICE PMD, previously the ready bitmap checking before reading
> PHY timestamp was not present. This caused incorrect Tx
> timestamping.
> 
> The ready bitmap checking is enabled and PHY timestamp is read once
> the ready bitmap gives positive value.
> 
> Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
Hi Soumyadeep,

see review I made on the previous version. Also, when sending new revisions
- especially if there hasn't been previous on-list review, please include
below the cutline a summary of what has changed from previous version.

Thanks,
/Bruce

Re: [PATCH v2] net/ice: fix incorrect reading of PHY timestamp

[Bruce Richardson]

On Wed, Oct 30, 2024 at 02:16:11AM +0000, Soumyadeep Hore wrote:
> In ICE PMD, previously the ready bitmap checking before reading
> PHY timestamp was not present. This caused incorrect Tx
> timestamping.
> 
> The ready bitmap checking is enabled and PHY timestamp is read once
> the ready bitmap gives positive value.
> 
> Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
> v2:
> - Addressed Bruce's comments
> ---
>  drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 70298ac330..6d0d37b3a0 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -6597,10 +6597,26 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
>  	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	struct ice_adapter *ad =
>  			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> -	uint64_t ts_ns, tstamp;
> +	uint64_t ts_ns, tstamp, tstamp_ready = 0;
> +	uint64_t end_time;
>  	const uint64_t mask = 0xFFFFFFFF;
>  	int ret;
>  
> +	end_time = rte_get_timer_cycles() + rte_get_timer_hz();
> +

Patch code itself looks fine, but don't you think that 1sec is an
excessively long time to wait? Maybe do a V3 with a lower timeout
threshold.

/Bruce

[PATCH v2] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In ICE PMD, previously the ready bitmap checking before reading
PHY timestamp was not present. This caused incorrect Tx
timestamping.

The ready bitmap checking is enabled and PHY timestamp is read once
the ready bitmap gives positive value.

Fixes: 881169950d80 ("net/ice/base: implement initial PTP support for E830")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
v2:
- Addressed Bruce's comments
---
 drivers/net/ice/ice_ethdev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 70298ac330..6d0d37b3a0 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6597,10 +6597,26 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
-	uint64_t ts_ns, tstamp;
+	uint64_t ts_ns, tstamp, tstamp_ready = 0;
+	uint64_t end_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
+	end_time = rte_get_timer_cycles() + rte_get_timer_hz();
+
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready(hw, ad->ptp_tx_block, &tstamp_ready);
+		if (ret) {
+			PMD_DRV_LOG(ERR, "Failed to get phy ready for timestamp");
+			return -1;
+		}
+
+		if (rte_get_timer_cycles() > end_time) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	}
+
 	ret = ice_read_phy_tstamp(hw, ad->ptp_tx_block, ad->ptp_tx_index, &tstamp);
 	if (ret || tstamp == 0) {
 		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");
-- 
2.43.0