On 10/23/2026 6:46 PM, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [Medium] ice: stale shared phc.cached_time when clock-owning PF stops or > unbinds > -- > > commit 866055ac7173935e690af3f73268e559a0698a8d > Author: Jacob Keller <[email protected]> > > ice: store cached PHC time in ice_adapter > > This commit moves the cached PHC time from per-PF and per-Rx-ring fields > into a shared struct ice_phc embedded in struct ice_adapter, and updates > only the clock-owning PF to refresh that shared value in its periodic > work. > >> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c >> b/drivers/net/ethernet/intel/ice/ice_ptp.c >> index ec24c09280785..4b4a00beadd2c 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > [ ... ] > >> @@ -987,35 +992,26 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct >> ice_ptp_tx *tx, u8 port) >> } >> >> /** >> - * ice_ptp_update_cached_phctime - Update the cached PHC time values >> + * ice_ptp_update_cached_phctime - Update the cached PHC time value >> * @pf: Board specific private structure >> * >> - * This function updates the system time values which are cached in the PF >> - * structure and the Rx rings. >> - * >> - * This function must be called periodically to ensure that the cached value >> - * is never more than 2 seconds old. >> + * Update the PTP hardware clock time values which are cached in the PHC >> + * structure. >> * >> - * Note that the cached copy in the PF PTP structure is always updated, even >> - * if we can't update the copy in the Rx rings. >> - * >> - * Return: >> - * * 0 - OK, successfully updated >> - * * -EAGAIN - PF was busy, need to reschedule the update >> - * * -ENODEV - no control PF owns the source clock register space >> - * * -EOPNOTSUPP - PTP support is not compiled in >> + * This function must be called periodically by the clock owner to ensure >> that >> + * the cached value is never more than 2 seconds old. >> */ >> -static int ice_ptp_update_cached_phctime(struct ice_pf *pf) >> +static void ice_ptp_update_cached_phctime(struct ice_pf *pf) >> { >> + struct ice_phc *phc = &pf->adapter->phc; >> struct device *dev = ice_pf_to_dev(pf); >> unsigned long update_before; >> u64 systime; >> - int err, i; >> + int err; >> >> - update_before = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000); >> - if (pf->ptp.cached_phc_time && >> - time_is_before_jiffies(update_before)) { >> - unsigned long time_taken = jiffies - pf->ptp.cached_phc_jiffies; >> + update_before = phc->update_jiffies + msecs_to_jiffies(2000); >> + if (phc->cached_time && time_is_before_jiffies(update_before)) { >> + unsigned long time_taken = jiffies - phc->update_jiffies; >> >> dev_warn(dev, "%u msecs passed between update to cached PHC >> time\n", >> jiffies_to_msecs(time_taken)); >> @@ -1032,36 +1028,12 @@ static int ice_ptp_update_cached_phctime(struct >> ice_pf *pf) >> * longer tracks the PHC, and both treat a zero cache as >> * "no timestamp". >> */ >> - systime = 0; >> - WRITE_ONCE(pf->ptp.cached_phc_time, 0); >> + WRITE_ONCE(phc->cached_time, 0); >> } else { >> - /* Update the cached PHC time stored in the PF structure */ >> - WRITE_ONCE(pf->ptp.cached_phc_time, systime); >> - WRITE_ONCE(pf->ptp.cached_phc_jiffies, jiffies); >> + /* Update the cached PHC time stored in the PHC structure */ >> + WRITE_ONCE(phc->cached_time, systime); >> + WRITE_ONCE(phc->update_jiffies, jiffies); >> } > > [ ... ] > >> @@ -3078,24 +3025,20 @@ static void ice_ptp_periodic_work(struct >> kthread_work *work) >> { >> struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work); >> struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp); >> - int err; >> >> if (pf->ptp.state != ICE_PTP_READY) >> return; >> >> ice_ptp_tspll_monitor(pf); >> >> - err = ice_ptp_update_cached_phctime(pf); >> + if (ice_pf_src_tmr_owned(pf)) >> + ice_ptp_update_cached_phctime(pf); > > [Severity: Medium] > What happens to pf->adapter->phc.cached_time on remaining PFs if the > clock-owning PF is unbound, reset, or fails PTP rebuild? > > Previously, commit dcc2c9108e65 ("ice: Reject PTP access without a control > PF") handled control PF removal by having each PF run > ice_ptp_update_cached_phctime() from ice_ptp_periodic_work(). When the > control PF went away, ice_ptp_read_src_clk_reg() returned -ENODEV on the > secondary PFs, which cleared cached_phc_time and the Rx ring cached_phctime > copies to 0. > > With this change, ice_ptp_periodic_work() in > drivers/net/ethernet/intel/ice/ice_ptp.c only calls > ice_ptp_update_cached_phctime() when ice_pf_src_tmr_owned(pf) is true > (and the only other callers are ice_ptp_settime64() and ice_ptp_adjtime() > via ice_ptp_reset_cached_phctime(), which also only run on the clock > owner). Because ice_pf_src_tmr_owned(pf) guarantees ice_is_primary(&pf->hw) > is true, ice_ptp_read_src_clk_reg() never returns -ENODEV here, making the > error branch in ice_ptp_update_cached_phctime() that zeroes > phc->cached_time unreachable. > > Meanwhile, when the clock-owning PF stops its periodic worker in > ice_ptp_prepare_for_reset() or is unbound via > ice_ptp_release() -> ice_ptp_cleanup_adapter(): > > drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_cleanup_adapter() { > ... > if (ice_get_ctrl_pf(pf) == pf) { > rcu_assign_pointer(pf->adapter->ctrl_pf, NULL); > synchronize = true; > } > ... > } > > nothing zeroes pf->adapter->phc.cached_time. > > Because the Rx path in ice_ptp_get_rx_hwts() -> ice_ptp_extend_32b_ts() > only checks whether phc->cached_time is non-zero and does not check > phc->update_jiffies, won't active sibling or secondary PFs continue > extending 32-bit hardware Rx timestamps against the frozen > phc->cached_time indefinitely? >
It does seem reasonable to clear this value when we clear the ctrl_pf. I can fix that.
