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

[Carolina Jubran <cjubran@nvidia.com>]

On 23/10/2026 13:58, netdev-bot+sashiko@kernel.org wrote:
> 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?


I think the error should be handled in the PTP core rather than accessing
the hardware while the backing function is unloaded.

There is already a series addressing this issue
https://lore.kernel.org/netdev/20261022-ptp-vclock-sampling-v5-0-06ceace17bac@gmail.com/


>
>>   
>>   	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.

Fair point. I will update the commit message.

Thanks,

Carolina