[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 | 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..efc6db4346 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 start_time, curr_time;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
+	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, ad->ptp_tx_block, &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, 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

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

[Singh, Aman Deep]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]


On 25-10-2024 15:56, 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>

Acked-by: Aman Singh<aman.deep.singh@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 70298ac330..efc6db4346 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 start_time, curr_time;
>   	const uint64_t mask = 0xFFFFFFFF;
>   	int ret;
>   
> +	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, ad->ptp_tx_block, &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, ad->ptp_tx_block, ad->ptp_tx_index, &tstamp);
>   	if (ret || tstamp == 0) {
>   		PMD_DRV_LOG(ERR, "Failed to read phy timestamp");

[-- Attachment #2: Type: text/html, Size: 2659 bytes --]

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

[Bruce Richardson]

On Tue, Oct 29, 2024 at 04:07:45PM +0530, Singh, Aman Deep wrote:
>    On 25-10-2024 15:56, 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: [1]stable@dpdk.org
> 
> Signed-off-by: Soumyadeep Hore [2]<soumyadeep.hore@intel.com>
> 
> Acked-by: Aman Singh [3]<aman.deep.singh@intel.com>
> 
Is this a v5 of this patch, or a resubmission of v1? There are 3 different
v1's of this patch, plus a v2, v3 and v4 listed in patchwork, see [1].
I had feedback on v3 of this patch, which I believe is still relevant to
this version.

Please keep all revisions of a patch in the one thread and with increasing
version number, saves us reviewers getting confused! :-) 
Also, with each new version, please include a summary below the cutline of
what has changed from previous, so we know what to look out for when
reviewing.

/Bruce

[1] https://patches.dpdk.org/project/dpdk/list/?series=&submitter=&state=*&q=fix+incorrect+reading+of+PHY&archive=&delegate=

[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

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 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>
---
v3:
- Decreased the end time delay from 1 second to 10 microseconds
---
v2:
- Addressed Bruce's comments
---
 drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 70298ac330..3c768b6d0b 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6597,10 +6597,27 @@ 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;
 
+	/* Set the end time with a delay of 10 microseconds */
+	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
+
+	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

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

[Bruce Richardson]

On Mon, Nov 04, 2024 at 10:31:12AM +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>
> ---
> v3:
> - Decreased the end time delay from 1 second to 10 microseconds
> ---
> v2:
> - Addressed Bruce's comments
> ---
>  drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 70298ac330..3c768b6d0b 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -6597,10 +6597,27 @@ 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;
>  
> +	/* Set the end time with a delay of 10 microseconds */
> +	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
> +
> +	while (!(tstamp_ready & BIT_ULL(0))) {

Nit: in DPDK, we recommend using "== 0" explicitly for numeric comparisons.
The "!" should only be used for boolean values, not ints.

> +		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;
> +		}

Sorry for the last minute feedback here, but shouldn't these two conditions
be the other way around? Right now, if you call ice_get_phy_tx_tstamp_ready
just as the timer expires, and if the timestamp is actually ready this
time, you will exit with error instead of handling the timestamp.

Also, a very minor issue, but since you always want to go through the loop
at least once, a do { } while(0) might be a better construct. It would
avoid you having to initialize tstamp_ready to zero (not that it matters).

	do {
		if (rte_get_timer_cycles() > end_time) {
			/* log error and return */
		}

		if (ice_get_phy_tx_tstamp_ready(...) != 0) {
			/* error log and return */
		}
	} while((tstamp_ready & BIT_ULL(0)) == 0);

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

Regards,
/Bruce

[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>
---
v4:
- Addressed Bruce comments for do while loop introduction
---
v3:
- Decreased the end time delay from 1 second to 10 microseconds
---
v2:
- Addressed Bruce's comments
---
 drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 70298ac330..7e2c6107ff 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6597,10 +6597,27 @@ 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;
 
+	/* Set the end time with a delay of 10 microseconds */
+	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
+
+	do {
+		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;
+		}
+	} while ((tstamp_ready & BIT_ULL(0)) == 0);
+
 	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

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

[Bruce Richardson]

On Tue, Nov 05, 2024 at 10:14:19AM +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>
> ---
> v4:
> - Addressed Bruce comments for do while loop introduction
> ---
> v3:
> - Decreased the end time delay from 1 second to 10 microseconds
> ---
> v2:
> - Addressed Bruce's comments
> ---
>  drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 70298ac330..7e2c6107ff 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -6597,10 +6597,27 @@ 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;
>  
> +	/* Set the end time with a delay of 10 microseconds */
> +	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
> +
> +	do {
> +		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;
> +		}
> +	} while ((tstamp_ready & BIT_ULL(0)) == 0);
> +

Unfortunately, this is still not quite right. The important change in my
last feedback was not the introduction of the do-while, but the switching
of the two blocks inside the while loop, or the adding of an additional
check to the timer expiry. You need to check that the bit is
still not set before checking for timer expiry. When "get_timer_cycles() >
end_time", it may also be the case that (tstamp_ready & BIT_ULL(0)) != 0, in
which case you want to continue rather than returning error.

/Bruce

[PATCH v5] 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>
---
v5:
- Addressed edge case as specified in Bruce's comment
---
v4:
- Addressed Bruce comments for do while loop introduction
---
v3:
- Decreased the end time delay from 1 second to 10 microseconds
---
v2:
- Addressed Bruce's comments
---
 drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 70298ac330..4a7fb2b656 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6597,10 +6597,27 @@ 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;
 
+	/* Set the end time with a delay of 10 microseconds */
+	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
+
+	do {
+		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 ((tstamp_ready & BIT_ULL(0)) == 0 && rte_get_timer_cycles() > end_time) {
+			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
+			return -1;
+		}
+	} while ((tstamp_ready & BIT_ULL(0)) == 0);
+
 	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

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

[Bruce Richardson]

On Wed, Nov 06, 2024 at 08:22:29AM +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>

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
> v5:
> - Addressed edge case as specified in Bruce's comment
> ---
> v4:
> - Addressed Bruce comments for do while loop introduction
> ---
> v3:
> - Decreased the end time delay from 1 second to 10 microseconds
> ---
> v2:
> - Addressed Bruce's comments
> ---
>  drivers/net/ice/ice_ethdev.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 70298ac330..4a7fb2b656 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -6597,10 +6597,27 @@ 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;
>  
> +	/* Set the end time with a delay of 10 microseconds */
> +	end_time = rte_get_timer_cycles() + (rte_get_timer_hz() / 100000);
> +
> +	do {
> +		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 ((tstamp_ready & BIT_ULL(0)) == 0 && rte_get_timer_cycles() > end_time) {
> +			PMD_DRV_LOG(ERR, "Timeout to get phy ready for timestamp");
> +			return -1;
> +		}
> +	} while ((tstamp_ready & BIT_ULL(0)) == 0);
> +
>  	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
> 

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

[Bruce Richardson]

On Wed, Nov 06, 2024 at 10:59:47AM +0000, Bruce Richardson wrote:
> On Wed, Nov 06, 2024 at 08:22:29AM +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>
> 
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> 
Applied to dpdk-next-net-intel.

Thanks,
/Bruce

[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 v1] net/ice: fix incorrect reading of PHY timestamp

[Soumyadeep Hore]

In E830 adapters, PHY timestamp for Tx packets should be read once
the ready status of PHY timestamp registers is 1.

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/base/ice_ptp_hw.c | 68 ++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ice/base/ice_ptp_hw.c b/drivers/net/ice/base/ice_ptp_hw.c
index 004f659eae..41367105b2 100644
--- a/drivers/net/ice/base/ice_ptp_hw.c
+++ b/drivers/net/ice/base/ice_ptp_hw.c
@@ -5526,6 +5526,27 @@ ice_ptp_port_cmd_e830(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd,
 					   lock_sbq);
 }
 
+/**
+ * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register
+ * @hw: pointer to the HW struct
+ * @port: the PHY port to read
+ * @tstamp_ready: contents of the Tx memory status register
+ *
+ */
+static int
+ice_get_phy_tx_tstamp_ready_e830(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
+{
+	u64 hi;
+	u32 lo;
+
+	lo = rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_L);
+	hi = (u64)rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_H) << 32;
+
+	*tstamp_ready = hi | lo;
+
+	return 0;
+}
+
 /**
  * ice_read_phy_tstamp_e830 - Read a PHY timestamp out of the external PHY
  * @hw: pointer to the HW struct
@@ -5539,10 +5560,30 @@ ice_ptp_port_cmd_e830(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd,
 static int
 ice_read_phy_tstamp_e830(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
 {
-	u32 hi_addr = E830_HIGH_TX_MEMORY_BANK(idx, lport);
-	u32 lo_addr = E830_LOW_TX_MEMORY_BANK(idx, lport);
+	u32 hi_addr, lo_addr;
 	u32 lo_val, hi_val, lo;
-	u8 hi;
+	u8 hi, ret;
+	u64 start_time, curr_time;
+	u64 tstamp_ready = 0;
+
+	start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
+
+	/* To check the ready status of HY Timestamp register for fetching timestamp */
+	while (!(tstamp_ready & BIT_ULL(0))) {
+		ret = ice_get_phy_tx_tstamp_ready_e830(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;
+		}
+	}
+
+	hi_addr = E830_HIGH_TX_MEMORY_BANK(idx, lport);
+	lo_addr = E830_LOW_TX_MEMORY_BANK(idx, lport);
 
 	lo_val = rd32(hw, lo_addr);
 	hi_val = rd32(hw, hi_addr);
@@ -5558,27 +5599,6 @@ ice_read_phy_tstamp_e830(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
 	return 0;
 }
 
-/**
- * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register
- * @hw: pointer to the HW struct
- * @port: the PHY port to read
- * @tstamp_ready: contents of the Tx memory status register
- *
- */
-static int
-ice_get_phy_tx_tstamp_ready_e830(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
-{
-	u64 hi;
-	u32 lo;
-
-	lo = rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_L);
-	hi = (u64)rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_H) << 32;
-
-	*tstamp_ready = hi | lo;
-
-	return 0;
-}
-
 /* Device agnostic functions
  *
  * The following functions implement shared behavior common to both E822/E823
-- 
2.43.0

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

[Bruce Richardson]

On Fri, Aug 23, 2024 at 11:01:33AM +0000, Soumyadeep Hore wrote:
> In E830 adapters, PHY timestamp for Tx packets should be read once
> the ready status of PHY timestamp registers is 1.
> 
> 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/base/ice_ptp_hw.c | 68 ++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 24 deletions(-)
> 
Since this is a patch to the base code for "ice", should it be, or can it
be, included in the patchset for the base code update for this release [1].

[1] https://patches.dpdk.org/project/dpdk/list/?series=32832

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

[Hore, Soumyadeep]

Hi Bruce,

Let's keep it separate for better tracking of the bug fix.

On Fri, Aug 23, 2024 at 11:01:33AM +0000, Soumyadeep Hore wrote:
> In E830 adapters, PHY timestamp for Tx packets should be read once the 
> ready status of PHY timestamp registers is 1.
> 
> 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/base/ice_ptp_hw.c | 68 
> ++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 24 deletions(-)
> 
Since this is a patch to the base code for "ice", should it be, or can it be, included in the patchset for the base code update for this release [1].

[1] https://patches.dpdk.org/project/dpdk/list/?series=32832

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

[Patrick Robb]

Recheck-request: iol-marvell-Functional

On Fri, Aug 23, 2024 at 7:56 AM Soumyadeep Hore
<soumyadeep.hore@intel.com> wrote:
>
> In E830 adapters, PHY timestamp for Tx packets should be read once
> the ready status of PHY timestamp registers is 1.
>
> 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/base/ice_ptp_hw.c | 68 ++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ice/base/ice_ptp_hw.c b/drivers/net/ice/base/ice_ptp_hw.c
> index 004f659eae..41367105b2 100644
> --- a/drivers/net/ice/base/ice_ptp_hw.c
> +++ b/drivers/net/ice/base/ice_ptp_hw.c
> @@ -5526,6 +5526,27 @@ ice_ptp_port_cmd_e830(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd,
>                                            lock_sbq);
>  }
>
> +/**
> + * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register
> + * @hw: pointer to the HW struct
> + * @port: the PHY port to read
> + * @tstamp_ready: contents of the Tx memory status register
> + *
> + */
> +static int
> +ice_get_phy_tx_tstamp_ready_e830(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
> +{
> +       u64 hi;
> +       u32 lo;
> +
> +       lo = rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_L);
> +       hi = (u64)rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_H) << 32;
> +
> +       *tstamp_ready = hi | lo;
> +
> +       return 0;
> +}
> +
>  /**
>   * ice_read_phy_tstamp_e830 - Read a PHY timestamp out of the external PHY
>   * @hw: pointer to the HW struct
> @@ -5539,10 +5560,30 @@ ice_ptp_port_cmd_e830(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd,
>  static int
>  ice_read_phy_tstamp_e830(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
>  {
> -       u32 hi_addr = E830_HIGH_TX_MEMORY_BANK(idx, lport);
> -       u32 lo_addr = E830_LOW_TX_MEMORY_BANK(idx, lport);
> +       u32 hi_addr, lo_addr;
>         u32 lo_val, hi_val, lo;
> -       u8 hi;
> +       u8 hi, ret;
> +       u64 start_time, curr_time;
> +       u64 tstamp_ready = 0;
> +
> +       start_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> +
> +       /* To check the ready status of HY Timestamp register for fetching timestamp */
> +       while (!(tstamp_ready & BIT_ULL(0))) {
> +               ret = ice_get_phy_tx_tstamp_ready_e830(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;
> +               }
> +       }
> +
> +       hi_addr = E830_HIGH_TX_MEMORY_BANK(idx, lport);
> +       lo_addr = E830_LOW_TX_MEMORY_BANK(idx, lport);
>
>         lo_val = rd32(hw, lo_addr);
>         hi_val = rd32(hw, hi_addr);
> @@ -5558,27 +5599,6 @@ ice_read_phy_tstamp_e830(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
>         return 0;
>  }
>
> -/**
> - * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register
> - * @hw: pointer to the HW struct
> - * @port: the PHY port to read
> - * @tstamp_ready: contents of the Tx memory status register
> - *
> - */
> -static int
> -ice_get_phy_tx_tstamp_ready_e830(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
> -{
> -       u64 hi;
> -       u32 lo;
> -
> -       lo = rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_L);
> -       hi = (u64)rd32(hw, E830_PRTMAC_TS_TX_MEM_VALID_H) << 32;
> -
> -       *tstamp_ready = hi | lo;
> -
> -       return 0;
> -}
> -
>  /* Device agnostic functions
>   *
>   * The following functions implement shared behavior common to both E822/E823
> --
> 2.43.0
>