> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-boun...@osuosl.org> On Behalf Of Jacob
> Keller
> Sent: 20 February 2025 03:43
> To: Nguyen, Anthony L <anthony.l.ngu...@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kits...@intel.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalew...@intel.com>; Kolacinski, Karol
> <karol.kolacin...@intel.com>
Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; Keller, Jacob E
<jacob.e.kel...@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net] ice: ensure periodic output start
> time is in the future
>
> From: Karol Kolacinski <karol.kolacin...@intel.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.kolacin...@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
Tested-by: Rinitha S <sx.rini...@intel.com> (A Contingent worker at Intel)
