Re: [PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer
From: Jacob Keller <jacob.e.keller@intel.com>
Date: 2026-08-24 23:55:03
Also in:
intel-wired-lan
On 8/21/2026 5:13 PM, Jacob Keller wrote:
quoted hunk ↗ jump to hunk
To ensure that the E825 PHY timer begins in a clean state, initiate a PHY soft reset prior to programming the PHY. This ensures that we clear any outstanding Tx timestamp memory, and ensures that the PHY internal state has been completely reset. Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products") Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Maciek Machnikowski <redacted> --- drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index b7d217ac31f3..10e973876608 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c@@ -2119,9 +2119,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset) * @hw: pointer to the HW struct * @port: the PHY port to start * - * Start the clock of a PHY port. This must be done as part of the flow to - * re-calibrate Tx and Rx timestamping offsets whenever the clock time is - * initialized or when link speed changes. + * Perform a PHY soft reset and then start the clock for the PHY port. + * + * This must be done as part of the flow to re-calibrate Tx and Rx + * timestamping offsets whenever the clock time is initialized or when link + * speed changes. * * Return: * * %0 - success@@ -2137,6 +2139,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port) tmr_idx = ice_get_ptp_src_clock_index(hw); + err = ice_ptp_phy_soft_reset_eth56g(hw, port); + if (err) + return err; + err = ice_stop_phy_timer_eth56g(hw, port, false); if (err) return err;
Sashiko points out a couple of pre-existing issues that are unrelated to this change:
This is a pre-existing issue, but does ice_ptp_update_cached_phctime() use
the ICE_CFG_BUSY bit flag as an ad-hoc lock?
In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_update_cached_phctime(),
the flag guards iteration over the vsi array:
if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
return -EAGAIN;
and then clears it on exit:
clear_bit(ICE_CFG_BUSY, pf->state);
Does using a bit flag instead of a proper lock bypass lockdep and deadlock
detection?This likely could and should be improved, but I am not certain if it makes sense to try and do so in this series, as this affects much larger portions of the driver and is a long standing issue with the ice driver (and Intel drivers in general) :\
This is a pre-existing issue, but does the lockless read of
tx->tstamps[idx].start in ice_ptp_process_tx_tstamp() race with its
initialization?
In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_request_ts(), the in_use
bit is set before the start timestamp is initialized:
set_bit(idx, tx->in_use);
clear_bit(idx, tx->stale);
tx->tstamps[idx].start = jiffies;
If a concurrent reader in ice_ptp_process_tx_tstamp() iterates over the
in_use bits locklessly:
for_each_set_bit(idx, tx->in_use, tx->len) {
...
if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
drop_ts = true;
can it observe the newly set in_use bit and read a stale start value,
causing it to erroneously drop a valid SKB?This should be fixed, and we can fix it by correctly ordering so that the start time is updated before setting the bit, (possibly with a barrier?). I will add the fix to the series.