Re: [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/23/2026 6:46 PM, sashiko-bot@kernel.org 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 <jacob.e.keller@intel.com>
> 
> 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.