From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2194D126BF7 for ; Sun, 24 May 2026 12:36:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779626200; cv=none; b=lqFQ2MDXSlLfXqaEbZLusAcyj7EHp9shJQ0LiyXV97YbYfUnmp4S4TSRNsEKVr86MtYsoYKM0Ifb6L8UJQ3OoBg4pINDTjvdZP9T9+rFr1JYjBXdgMG660VzBGQblWdWL4jce0wpB55na7v0a7RD4RekBhA1wf0DnHCx8BFpLLQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779626200; c=relaxed/simple; bh=/cJY7qkr0uFrS68XWSlDyxGgshAs05k0eeNTavbUhjk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=FhfxYj5BrTOpxT26pw9Ja2hXXReTCtw+xVgMi9ACFmYVLICpTBlGsEiyisJK+bLUuIN49GRx4QDf82r7eAJqM5xGdSo2Yz7aRhWetBuk+g2Pce4XFycXAj/ZMOnKZbdNN2Ct6zqXd1bLtQPas62QXDB+TWQQrpP3SmK6u4iQv8U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=MsrDa/K/; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="MsrDa/K/" Received: by smtp.kernel.org (Postfix) with UTF8SMTPSA id 011921F000E9; Sun, 24 May 2026 12:36:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1779626198; bh=YxWcuAdZ6QysoNlM1+u6HlEj2JMjjhboPkEGIR0TpUg=; h=From:To:Cc:Subject:In-Reply-To:References:Date; b=MsrDa/K/19uZmBbE1ID/3IkkAxHPHk2bgx2t6QcyTKfY+zOt6xYiKKOsoMs/6lJ1T smZWRbJa9pihu92TRFspMbIs1xbCqhkaZtxPLWeefvzyNaduiveF/ip4chrGRVd7PH /pLKkae059UnET58W7b8UK/AyjbzJi6rllPCK0NB0oAbP9tu80WnhH96vY1D6+IB5R O+0+l+z7IOHVk3xnOZ+WkmkfMqqh2uahJm/FFRTx7padkMMs4Lt1AMSkoktZx3TUju wPSxqD0ihqBQGM4iyVum2C0A0P2GGJmPFE9yUv829nJjiGDkB0uIGrssZcjXVZza5v mXNj/rmshY3gQ== From: Thomas Gleixner To: David Woodhouse , Miroslav Lichvar Cc: Richard Cochran , Wen Gu , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , John Stultz , Stephen Boyd , Anna-Maria Behnsen , Frederic Weisbecker , Shuah Khan , Peter Zijlstra , Thomas =?utf-8?Q?Wei=C3=9Fschuh?= , Arnd Bergmann , Julien Ridoux , Ryan Luu , linux-kernel@vger.kernel.org, Marcelo Tosatti Subject: Re: [RFC PATCH v2 0/8] timekeeping: Fix draft tracking precision and add feed-forward discipline via vmclock In-Reply-To: References: <20260517220326.4625-1-dwmw2@infradead.org> <0d32da75fa88c92ac0225ef23a9045afdf2ac9fe.camel@infradead.org> <87o6i8vcni.ffs@tglx> <6c7137748ccea288fdf55e7c2e24817ccc1673ef.camel@infradead.org> <874ijzvpl8.ffs@tglx> <00609fa255442648e11e2b5596a3b80a29edcfc8.camel@infradead.org> <87v7cftqey.ffs@tglx> Date: Sun, 24 May 2026 14:36:35 +0200 Message-ID: <87se7ht25o.ffs@tglx> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Fri, May 22 2026 at 17:23, David Woodhouse wrote: > On Fri, 2026-05-22 at 17:28 +0200, Thomas Gleixner wrote: >> This raises an interesting question. Must any of the existing PTM using >> drivers mplement that new extended getcrosstimestampattr() callback, in >> order to expose the cycles/csid in attr or can you fallback to the >> existing callback and have the rest of the fields 0? >> >> Same question arises if you change the pre/post timestamp helpers to >> utilize ktime_get_snapshot_id(). All existing drivers which use them >> will then automatically retrieve cs_cycles/cs_id. > > Taking those in reverse order... yes, this means that with a new > variant of PTP_SYS_OFFSET_EXTENDED, userspace can see actual counter > values even for the system parts of those ABA timestamps, even for non- > PTM clocks, and discipline the TSC/archcounter against the external > clock. Correct. > Currently I have userspace which literally does rdtsc() either side of > calling the ioctl :) Why am I not surprised? :) > And PTM devices can be used with PTP_SYS_OFFSET_PRECISE, which goes > through get_device_system_crosststamp() as described, and all just > works? It's just that we now allow userspace to *see* the counter value > that the driver was already generating. A new variant of PRECISE > So to your questions: although there's new userspace ioctl support, the > *drivers* don't need any modification for that (as long as they use the > standard prets/postts helpers). Yes. > The remaining question is the device timestamp part (the 'B' in the ABA > sandwich) for PTP_SYS_OFFSET_EXTENDED with PTM-capable drivers. Should > that get a counterval? PTM-capable driver support cross timestamps, which will with a new version of PTP_SYS_OFFSET_PRECISE expose the system counterval. No ABA for that as it's hardware latched AB. > I don't have a strong opinion. On one hand we'd have to find a way to > convert it from PTM for devices where it actually *is* PTM, and that's > what PTP_SYS_OFFSET_PRECISE is *for*. Correct. > But on the other hand, can't the conversion be a whole lot simpler than > get_device_system_crosststamp() because it's not actually dealing with > any timekeepers; it's basically only invoking convert_base_to_cs()? But what for? If you have PTM, use PRECISE. There is _zero_ value of having pre/post timestamps when the hardware already does the correlated precise sampling, no? > And the ioctl should support it *all* but just have a clear way of > indicating that any of the optional fields including the attrs are > *not* populated (or use 0/max values maybe?). Yes. > So no, I don't think any driver *has* to add any attr support in order > to expose counter values to userspace. The only reason I asked Arthur > to mix those things up was for the *userspace* API, to avoid adding yet > another ioctl over and over again. And now I feel bad for doing so :) I think you can create _one_ data structure variant, which fits both EXTENDED_ATTR and PRECISE_ATTR: struct attrs { u32 valid; u32 error_bound; .... u32 reserved[N]; }; @valid tells user space, which of the attributes has been filled in by the driver. That avoids bounds based validity checks, which are a pain as you might end up with different bounds for every attribute. Having a valid flags field avoids that completely. struct devtime { ptp_clock_time device_time; struct attrs attrs; }; struct systime { u64 sys_systime; u64 sys_rawtime; u64 sys_counter; u32 sys_counter_id; u32 reserved; }; Exposing @sys_counter_id requires to expose CSID_* in the user space ABI reliably, as otherwise a kernel internal CSID enum change would blow up the user space guess work. Your ptp_counter_id approach is error prone. struct timestamp { union { struct systime systime; struct systime pre_systime; }; struct devtime devtime; struct systime post_systime; }; struct request { u32 valid; clockid_t clock_id; unsigned int num_samples; u32 reserved[N]; }; I rather have @valid here too. The 'zero the reserved' members approach is a pain as new kernels have to map 0 to default behavior instead of being free to make 0 mean what they intend. @valid allows you to use other sizes than u32 for future fields. All you have to take care of is to keep the existing fields at the same place as before. struct ioctl_data { struct request request; struct timestamp timestamps[]; }; So for both PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS user space allocates enough space to accomodate data::request::num_samples. For PTP_SYS_OFFSET_PRECISE_ATTRS num_samples has to be 1 and data::timestamps[0].post_systime is zeroed by the kernel because it has no meaning. So now in the kernel you do: ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user *argptr) { struct ioctl_data __user *data = argptr; struct request; if (copy_from_user(&request, &data->request, sizeof(request))) return -EFAULT; if (!extattr_request_valid(request)) return -EINVAL; for (unsigned int i; i < request.num_samples; i++) { struct ptp_system_timestamp sts = { .clock_id = request.clock_id, }; struct timestamp uts = { }; struct timespec64 devts; if (ptp->info->gettimex64_attr) ret = ptp->info->gettimex64_attr(ptp->info, &dev_ts, &sts, &uts.attr); else if (ptp->info->gettimex64) ret = ptp->info->gettimex64(ptp->info, &dev_ts, &sts); else return -ENOTSUPP; if (ret) return ret; uts.pre_systime = mangle(sts.pre_systime); uts.devtime.device_time = mangle(dev_ts); uts.post_systime = mangle(sts.post_systime); if (!copy_to_user(&data->timestamps[i], uts, sizeof(uts))) return -EFAULT; } return 0; } ptp_sys_offset_precise_attrs(struct ptp_clock *ptp, void __user *argptr) { struct ioctl_data __user *data = argptr; struct request; if (copy_from_user(&request, &data->request, sizeof(request))) return -EFAULT; if (!preciseattr_request_valid(request)) return -EINVAL; struct system_device_crosststamp xtstamp = { .clock_id = request.clock_id, }; struct timestamp uts = { }; if (ptp->info->getcrosststamp_attr) ret = ptp->info->getcrosststamp_attr(ptp->info, &xtstamp, &uts.attr); else if (ptp->info->getcrosststamp) ret = ptp->info->getcrosststamp(ptp->info, &xtstamp); else return -ENOTSUPP; if (ret) return ret; uts.systime = mangle(xtstamp.systime); uts.devtime.device_time = mangle(xtstamp.device); if (!copy_to_user(&data->timestamps[0], uts, sizeof(uts))) return -EFAULT; return 0; } Or something like this, which immediately enables the functionality for all drivers which implement the getcrosststamp() or the gettimex64() callbacks with a unified user space data structure. The attributes.valid bits are all zero and and once drivers implement the _attr callback variants, those attributes supported by the driver will magically appear with the corresponding valid bits set. Hmm? Thanks, tglx