Re: [PATCH net] net/mlx5: Use only loaded devices for PTP clock operations

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message leaves out a large change in how the PHC
  keeps time.
- [Medium] The patch makes mlx5_ptp_getcyclesx() (and
  mlx5_ptp_gettimex(), used as the getcyclesx64 fallback when cycles are…

--

net/mlx5: Use only loaded devices for PTP clock operations

This patch serializes PTP clock hardware access with the clock mutex for
all clocks, not only shared ones. PTP callbacks now return -EAGAIN when no
loaded function backs the clock. It also ties the PTP overflow worker to
mlx5_clock_load()/mlx5_clock_unload() and uses the devcom lock to
synchronize with peer loads and unloads.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
> index 5df786133e4bc..c3e53d219496f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c

[ ... ]

> @@ -580,14 +616,20 @@ static int mlx5_ptp_getcyclesx(struct ptp_clock_info *ptp,
>  						ptp_info);
>  	struct mlx5_core_dev *mdev;
>  	u64 cycles;
> +	int err = 0;
>  
>  	mlx5_clock_lock(clock);
> -	mdev = mlx5_clock_mdev_get(clock);
> +	mdev = mlx5_clock_active_mdev_get(clock);
> +	if (!mdev) {
> +		err = -EAGAIN;
> +		goto unlock;
> +	}

[Severity: Medium]
Can this corrupt PTP virtual clocks built on top of the mlx5 PHC?

When expose_cycles is set, mlx5_init_timer_clock() installs
mlx5_ptp_getcyclesx as getcyclesx64. Otherwise ptp_getcycles64() falls
back to gettime64, which is mlx5_ptp_gettimex(), and that also returns
-EAGAIN now.

ptp_vclock_read() in drivers/ptp/ptp_vclock.c ignores the return value:

	struct timespec64 ts = {};

	ptp->info->getcycles64(ptp->info, &ts);

	return timespec64_to_ns(&ts);

So while the function is unloaded, the cycle sample becomes 0.

mlx5_clock_unload() only cancels the physical clock's worker, through
ptp_cancel_worker_sync(clock->ptp). The vclock's own worker keeps
running. ptp_vclock_register() schedules it with
PTP_VCLOCK_REFRESH_INTERVAL (HZ * 2):

ptp_vclock_refresh()
  ptp_vclock_gettime()
    timecounter_read()
      timecounter_read_delta()
        ptp_vclock_read()    /* returns 0 */

timecounter_read_delta() then adds (0 - cycle_last) & mask to
vclock->tc.nsec and sets cycle_last to 0. Once the function is loaded
again, the next sample adds (real - 0) & mask. With the 32-bit vclock
mask, the two partial deltas can add up to the real delta plus 2^32, so
the vclock may jump forward by about 4.3 seconds.

Before this patch, mlx5_ptp_getcyclesx() always read
dev->iseg->internal_timer. The iseg stays mapped across
mlx5_unload()/mlx5_load() and is only unmapped in mlx5_pci_close(). So
these reads returned valid values during a devlink reload or recovery
window.

The missing error check in ptp_vclock_read() is an older weakness in the
ptp core. The new -EAGAIN return here is what makes it reachable.

Should ptp_vclock_read() handle the error? Or should the mlx5 getcycles
path keep returning valid cycles from the still-mapped iseg?

>  
>  	cycles = mlx5_read_time(mdev, sts, false);
>  	*ts = ns_to_timespec64(cycles);
> +unlock:
>  	mlx5_clock_unlock(clock);
> -	return 0;
> +	return err;
>  }

[ ... ]

> @@ -1554,18 +1646,32 @@ void mlx5_clock_load(struct mlx5_core_dev *mdev)

[ ... ]

> +	start = !cpriv->mdev->clock_state->loaded;
> +	mdev->clock_state->loaded = true;
> +	if (start) {
> +		struct mlx5_timer *timer = &clock->timer;
> +		u64 ns;
> +
> +		cpriv->mdev = mdev;
> +		if (mlx5_real_time_mode(mdev))
> +			ns = mlx5_read_time(mdev, NULL, true);
> +		else
> +			ns = timer->saved_phc_ns +
> +			     ktime_get_boottime_ns() - timer->saved_boot_ns;
> +
> +		mlx5_set_timecounter(mdev, ns);

[Severity: Medium]
The commit message says:

    To fix that, serialize clock hardware access with the clock mutex for
    all clocks and fail callbacks with -EAGAIN when there is no loaded
    function to service the clock. Additionally, tie the worker's lifetime
    to load/unload and use the devcom lock to synchronize with peer
    loads/unloads.

Should it also describe the change in how the PHC keeps time across
unload/load?

Before this patch, mlx5_init_timer_clock() set up the timecounter once,
and it carried over unload/load unchanged. mlx5_clock_load() only
registered the notifier and re-armed PPS.

With this patch, the first load re-bases the timecounter through
mlx5_set_timecounter()->timecounter_init(). In real-time mode the value
comes from the HW real-time register. In free-running mode it is
extrapolated from the saved_phc_ns/saved_boot_ns pair shown above.

The commit message does not mention these related changes either:

  - mlx5_update_clock_info_page() now calls mlx5_timer_snapshot() on every
    update. This stores tc.nsec and CLOCK_BOOTTIME in the new
    saved_phc_ns and saved_boot_ns fields of struct mlx5_timer.

  - mlx5_ptp_adjtime() now calls timecounter_read() before
    timecounter_adjtime(). This changes the cycle_last/nsec values
    published in the RDMA clock_info page.

  - mlx5_clock_unload() now calls timecounter_read() through
    mlx5_save_timecounter().

As a result, the PHC value and the clock_info cycle_last/nsec pair are
re-based after every devlink reload, health recovery and suspend/resume.
While no function is loaded, a free-running PHC also advances at the
CLOCK_BOOTTIME rate rather than its adjfine'd rate.

This looks necessary because the overflow worker is now cancelled while
unloaded, so the 41-bit counter could wrap without being observed.

Could the commit message describe this change in time behavior and the
reason for it? This matters more because the Fixes: tag will bring the
patch into stable.

>  		mlx5_clock_arm_pps_in_event(clock, mdev, cpriv->event_mdev);
> +	}
>  	mlx5_clock_unlock(clock);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021105600.3481485-1-tariqt%40nvidia.com