Re: [PATCH iwl-next v2 4/4] ice: use struct timecounter for E825 and E830

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ice: perout_rqs updated outside phc.lock in
  ice_ptp_gpio_enable()
- [Medium] ice: premature update_jiffies store in ice_ptp_read_cycles()
--

commit 2dbce654e6813d30b319e71f8e68ca2b82a4b244
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: use struct timecounter for E825 and E830

This commit switches E825 and E830 devices from direct hardware PHC time
adjustments and legacy 32-bit timestamp extension to a latched
timecounter/cyclecounter stored in struct ice_phc.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a0451c1083a41..b48d1bf3bbf59 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -272,6 +272,202 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
>  	return 0;
>  }
>  
> +/**
> + * ice_ptp_read_cycles - cyclecounter read() callback function
> + * @cc: the hardware cycle counter structure
> + *
> + * Read the PHC time via ice_ptp_read_src_clk_reg() and report it for use
> + * within timecounter_read() and timecounter_init().
> + *
> + * Although the cycle counter is masked to 32-bits for timestamp extension,
> + * this still reports the full 64-bit time to enable determining the phase
> + * difference between the PHC hardware value and the timecounter properly.
> + *
> + * Return: The current PHC timer value from the hardware on success.
> + *         Since failure cannot be meaningfully reported, return the previous
> + *         cycle_last value when unable to access the device.
> + */
> +u64 ice_ptp_read_cycles(struct cyclecounter *cc)
> +{
> +	struct ice_adapter *adapter;
> +	struct ice_phc *phc;
> +	struct ice_pf *pf;
> +	u64 time;
> +	int err;
> +
> +	phc = container_of(cc, struct ice_phc, cc);
> +	adapter = container_of(phc, struct ice_adapter, phc);
> +
> +	scoped_guard(rcu) {
> +		pf = rcu_dereference(adapter->ctrl_pf);
> +		if (!pf)
> +			return phc->tc[0].cycle_last;
> +
> +		err = ice_ptp_read_src_clk_reg(pf, NULL, &time);
> +		if (err)
> +			return phc->tc[0].cycle_last;
> +
> +		/* Paired with smp_load_acquire in ice_ptp_extend_32b_ts */
> +		smp_store_release(&phc->update_jiffies, jiffies | 1);
> +		return time;
> +	}
> +}

[ ... ]

> +static void ice_phc_refresh(struct ice_phc *phc)
> +{
> +	lockdep_assert_held(&phc->lock);
> +
> +	write_seqcount_latch_begin(&phc->seq);
> +	(void)timecounter_read(&phc->tc[0]);
> +	write_seqcount_latch(&phc->seq);
> +	/* Update the secondary copy */
> +	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
> +	phc->tc[1].nsec = phc->tc[0].nsec;
> +	write_seqcount_latch_end(&phc->seq);
> +}
> +
> +/**
> + * ice_phc_init - Initialize PHC timecounter to new start value
> + * @phc: pointer to the PHC structure
> + * @init_tstamp: New initial value for the timecounter
> + *
> + * Initialize the PHC timecounter to a new offset. The latched sequence
> + * counter is used to ensure readers get a consistent view of the timecounter
> + * phase. We always pass the same cyclecounter structure, so the mask, frac,
> + * and cc fields always remain the same, and only the cycle_last and nsec
> + * fields must be propagated to the secondary structure.
> + */
> +static void ice_phc_init(struct ice_phc *phc, u64 init_tstamp)
> +{
> +	lockdep_assert_held(&phc->lock);
> +
> +	write_seqcount_latch_begin(&phc->seq);
> +	timecounter_init(&phc->tc[0], &phc->cc, init_tstamp);
> +	write_seqcount_latch(&phc->seq);
> +	/* Update the secondary copy */
> +	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
> +	phc->tc[1].nsec = phc->tc[0].nsec;
> +	write_seqcount_latch_end(&phc->seq);
> +}

[Severity: Medium]
Does storing phc->update_jiffies inside ice_ptp_read_cycles() publish
the timestamp before either phc->tc[] copy is updated?

timecounter_init() and timecounter_read() invoke cc->read() before
writing tc[0].cycle_last and tc[0].nsec, while phc->seq is odd and
directing readers to phc->tc[1]:

CPU0                                  CPU1
ice_phc_init()
  write_seqcount_latch_begin(&phc->seq)
  timecounter_init(&phc->tc[0], ...)
    ice_ptp_read_cycles()
      smp_store_release(&phc->update_jiffies, ...)
                                      ice_ptp_extend_32b_ts()
                                        smp_load_acquire(&phc->update_jiffies)
                                        ice_phc_cyc2time_with_trace()
                                          seq = read_seqcount_latch(&phc->seq)
                                          /* idx == 1 */
                                          timecounter_cyc2time(&phc->tc[1], ...)
                                            <-- uses uninitialized or stale tc[1]
    tc[0].cycle_last = ...
    tc[0].nsec = ...
  write_seqcount_latch(&phc->seq)
  phc->tc[1] = phc->tc[0]
  write_seqcount_latch_end(&phc->seq)

During initial bringup or after a reset or delayed refresh (>2s), CPU1
sees a fresh update_jiffies and extends a packet's 32-bit hardware
timestamp against an uninitialized (zero) or stale phc->tc[1] snapshot
instead of discarding it or using the updated timecounter.  Recording the
new jiffies value and calling smp_store_release(&phc->update_jiffies, ...)
after write_seqcount_latch(&phc->seq) (or write_seqcount_latch_end()) when
cc->read() succeeded would ensure readers only see the fresh timestamp
once an updated timecounter copy is active.

[ ... ]

> @@ -1974,7 +2202,10 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
>  		if (on && pf->ptp.state != ICE_PTP_READY)
>  			return -EIO;
>  
> +		/* Serialize against concurrent timecounter updates */
> +		mutex_lock(&pf->adapter->phc.lock);
>  		err = ice_ptp_cfg_perout(pf, &rq->perout, on);
> +		mutex_unlock(&pf->adapter->phc.lock);
>  		if (!err) {
>  			*cached = rq->perout;
>  		} else {

[ ... ]

> @@ -2086,6 +2319,43 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
> +static int ice_ptp_settime64_tc(struct ptp_clock_info *info,
> +				const struct timespec64 *ts)
> +{
> +	struct ice_pf *pf = ptp_info_to_pf(info);
> +	struct ice_phc *phc = &pf->adapter->phc;
> +	u64 ns;
> +
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		return -EIO;
> +
> +	ns = timespec64_to_ns(ts);
> +
> +	mutex_lock(&phc->lock);
> +
> +	ice_ptp_disable_all_perout(pf);
> +
> +	ice_phc_init(phc, ns);
> +
> +	ice_ptp_mark_all_tx_tracker_stale(pf);\n+
> +	/* Reenable periodic outputs */
> +	ice_ptp_enable_all_perout(pf);
> +
> +	mutex_unlock(&phc->lock);
> +
> +	return 0;
> +}

[Severity: Medium]
Can releasing pf->adapter->phc.lock before updating *cached
(pf->ptp.perout_rqs[rq->perout.index]) in ice_ptp_gpio_enable() race with
ice_ptp_settime64_tc() or ice_ptp_adjtime_tc()?

The POSIX clock layer only holds down_read(&clk->rwsem) across ioctl,
clock_settime, and clock_adjtime calls, so ice_ptp_gpio_enable() and
ice_ptp_settime64_tc() can run concurrently:

CPU0                                  CPU1
ice_ptp_gpio_enable()
  mutex_lock(&pf->adapter->phc.lock)
  ice_ptp_cfg_perout(pf, &rq->perout, 1)
  mutex_unlock(&pf->adapter->phc.lock)
                                      ice_ptp_settime64_tc()
                                        mutex_lock(&phc->lock)
                                        ice_ptp_disable_all_perout(pf)
                                        ice_phc_init(phc, ns)
                                        ice_ptp_enable_all_perout(pf)
                                          /* cached->period is still 0 */
                                        mutex_unlock(&phc->lock)
  *cached = rq->perout;                 <-- HW keeps pre-settime phase

When enabling an output, ice_ptp_enable_all_perout() sees a zero period in
perout_rqs[] and skips reprogramming GLTSYN_TGT for the new timecounter
phase; when disabling an output, it sees the old non-zero period and
re-enables the output in hardware right before *cached is cleared.
Writing *cached without holding phc.lock also races with
ice_ptp_disable_all_perout() and ice_ptp_enable_all_perout() reading the
struct under phc.lock.  Holding pf->adapter->phc.lock across the *cached
update in ice_ptp_gpio_enable() avoids the race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-jk-e825-e830-timecounter-v2-0-9c357a7ed349@intel.com?part=4