Message-ID: <eddc733e-2587-45ed-a2b4-a395afc0064b@intel.com>
Date: Mon, 24 Feb 2025 12:36:18 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jacob Keller <jacob.e.keller@...el.com>, Anthony Nguyen
	<anthony.l.nguyen@...el.com>
CC: Karol Kolacinski <karol.kolacinski@...el.com>, netdev
	<netdev@...r.kernel.org>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH iwl-net] ice: ensure periodic output start time is in the
 future

On 2/13/25 00:54, Jacob Keller wrote:
> From: Karol Kolacinski <karol.kolacinski@...el.com>
> 
> On E800 series hardware, if the start time for a periodic output signal is
> programmed into GLTSYN_TGT_H and GLTSYN_TGT_L registers, the hardware logic
> locks up and the periodic output signal never starts. Any future attempt to
> reprogram the clock function is futile as the hardware will not reset until
> a power on.
> 
> The ice_ptp_cfg_perout function has logic to prevent this, as it checks if
> the requested start time is in the past. If so, a new start time is
> calculated by rounding up.
> 
> Since commit d755a7e129a5 ("ice: Cache perout/extts requests and check
> flags"), the rounding is done to the nearest multiple of the clock period,
> rather than to a full second. This is more accurate, since it ensures the
> signal matches the user request precisely.
> 
> Unfortunately, there is a race condition with this rounding logic. If the
> current time is close to the multiple of the period, we could calculate a
> target time that is extremely soon. It takes time for the software to
> program the registers, during which time this requested start time could
> become a start time in the past. If that happens, the periodic output
> signal will lock up.
> 
> For large enough periods, or for the logic prior to the mentioned commit,
> this is unlikely. However, with the new logic rounding to the period and
> with a small enough period, this becomes inevitable.
> 
> For example, attempting to enable a 10MHz signal requires a period of 100
> nanoseconds. This means in the *best* case, we have 99 nanoseconds to
> program the clock output. This is essentially impossible, and thus such a
> small period practically guarantees that the clock output function will
> lock up.
> 
> To fix this, add some slop to the clock time used to check if the start
> time is in the past. Because it is not critical that output signals start
> immediately, but it *is* critical that we do not brick the function, 0.5
> seconds is selected. This does mean that any requested output will be
> delayed by at least 0.5 seconds.
> 
> This slop is applied before rounding, so that we always round up to the
> nearest multiple of the period that is at least 0.5 seconds in the future,
> ensuring a minimum of 0.5 seconds to program the clock output registers.
> 
> Finally, to ensure that the hardware registers programming the clock output
> complete in a timely manner, add a write flush to the end of
> ice_ptp_write_perout. This ensures we don't risk any issue with PCIe
> transaction batching.
> 
> Strictly speaking, this fixes a race condition all the way back at the
> initial implementation of periodic output programming, as it is
> theoretically possible to trigger this bug even on the old logic when
> always rounding to a full second. However, the window is narrow, and the
> code has been refactored heavily since then, making a direct backport not
> apply cleanly.
> 
> Fixes: d755a7e129a5 ("ice: Cache perout/extts requests and check flags")
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>

Reviewed-by: Simon Horman <horms@...nel.org>
https://lore.kernel.org/netdev/20250215145941.GQ1615191@kernel.org/

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

> ---
>   drivers/net/ethernet/intel/ice/ice_ptp.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index e26320ce52ca17b30a9538b11b754c7cf57c9af9..a99e0fbd0b8b55ad72a2bc7d13851562a6f2f5bd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1783,6 +1783,7 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan,
>   				  8 + chan + (tmr_idx * 4));
>   
>   	wr32(hw, GLGEN_GPIO_CTL(gpio_pin), val);
> +	ice_flush(hw);
>   
>   	return 0;
>   }
> @@ -1843,9 +1844,10 @@ static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
>   		div64_u64_rem(start, period, &phase);
>   
>   	/* If we have only phase or start time is in the past, start the timer
> -	 * at the next multiple of period, maintaining phase.
> +	 * at the next multiple of period, maintaining phase at least 0.5 second
> +	 * from now, so we have time to write it to HW.
>   	 */
> -	clk = ice_ptp_read_src_clk_reg(pf, NULL);
> +	clk = ice_ptp_read_src_clk_reg(pf, NULL) + NSEC_PER_MSEC * 500;
>   	if (rq->flags & PTP_PEROUT_PHASE || start <= clk - prop_delay_ns)
>   		start = div64_u64(clk + period - 1, period) * period + phase;
>   
> 
> ---
> base-commit: e589adf5b70c07b1ab974d077046fdbf583b2f36
> change-id: 20250212-jk-gnrd-ptp-pin-patches-42561da45ec1
> 
> Best regards,

Powered by blists - more mailing lists