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

[Bruce Richardson <bruce.richardson@intel.com>]

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