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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
