On Thu, Apr 11, 2024 at 12:11 AM Sagi Maimon <maimon.sagi@xxxxxxxxx> wrote:
>
> Hi Mahesh
> What is the status of your patch?
> if your patch is upstreamed , then it will have all I need.
> But, If not , I will upstream my patch.
> BR,
>
Hi Sagi,
If you want to pursue the syscall option, then those are tangential
and please go ahead. (I cannot stop you!)
I'm interested in getting the "tight sandwich timestamps" that
gettimex64() ioctl offers and I would want enhancements to
gettimex64() done the way it was discussed in the later half of this
thread. If you want to sign-up for that please let me know.
thanks,
--mahesh..
> On Thu, Apr 11, 2024 at 5:56 AM Mahesh Bandewar (महेश बंडेवार)
> <maheshb@xxxxxxxxxx> wrote:
> >
> > On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> > > > On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > > > The modification that you have proposed (in a couple of posts back)
> > > > would work but it's still not ideal since the pre/post ts are not
> > > > close enough as they are currently (properly implemented!)
> > > > gettimex64() would have. The only way to do that would be to have
> > > > another ioctl as I have proposed which is a superset of current
> > > > gettimex64 and pre-post collection is the closest possible.
> > >
> > > Errm. What I posted as sketch _is_ using gettimex64() with the extra
> > > twist of the flag vs. a clockid (which is an implementation detail) and
> > > the difference that I carry the information in ptp_system_timestamp
> > > instead of needing a new argument clockid to all existing callbacks
> > > because the modification to ptp_read_prets() and postts() will just be
> > > sufficient, no?
> > >
> > OK, that makes sense.
> >
> > > For the case where the driver does not provide gettimex64() then the
> > > extension of the original offset ioctl is still providing a better
> > > mechanism than the proposed syscall.
> > >
> > > I also clearly said that all drivers should be converted over to
> > > gettimex64().
> > >
> > I agree. Honestly that should have been mandatory and
> > ptp_register_clock() should fail otherwise! Probably should have been
> > part of gettimex64 implementation :(
> >
> > I don't think we can do anything other than just hoping all driver
> > implementations include gettimex64 implementation.
> >
> > > > Having said that, the 'flag' modification proposal is a good backup
> > > > for the drivers that don't have good implementation (close enough but
> > > > not ideal). Also, you don't need a new ioctl-op. So if we really want
> > > > precision, I believe, we need a new ioctl op (with supporting
> > > > implementation similar to the mlx4 code above). but we want to save
> > > > the new ioctl-op and have less precision then proposed modification
> > > > would work fine.
> > >
> > > I disagree. The existing gettimex64() is good enough if the driver
> > > implements it correctly today. If not then those drivers need to be
> > > fixed independent of this.
> > >
> > > So assumed that a driver does:
> > >
> > > gettimex64()
> > > ptp_prets(sts);
> > > read_clock();
> > > ptp_postts(sts);
> > >
> > > today then having:
> > >
> > > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > > ktime_get_raw_ts64(&sts->pre_ts);
> > > else
> > > ktime_get_real_ts64(&sts->pre_ts);
> > > }
> > > }
> > >
> > > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > > ktime_get_raw_ts64(&sts->post_ts);
> > > else
> > > ktime_get_real_ts64(&sts->post_ts);
> > > }
> > > }
> > >
> > > or
> > >
> > > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > switch (sts->clockid) {
> > > case CLOCK_MONOTONIC_RAW:
> > > time_get_raw_ts64(&sts->pre_ts);
> > > break;
> > > case CLOCK_REALTIME:
> > > ktime_get_real_ts64(&sts->pre_ts);
> > > break;
> > > }
> > > }
> > > }
> > >
> > > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > switch (sts->clockid) {
> > > case CLOCK_MONOTONIC_RAW:
> > > time_get_raw_ts64(&sts->post_ts);
> > > break;
> > > case CLOCK_REALTIME:
> > > ktime_get_real_ts64(&sts->post_ts);
> > > break;
> > > }
> > > }
> > > }
> > >
> > > is doing the exact same thing as your proposal but without touching any
> > > driver which implements gettimex64() correctly at all.
> > >
> > I see. Yes, this makes sense.
> >
> > > While your proposal requires to touch every single driver for no reason,
> > > no?
> > >
> > > It is just an implementation detail whether you use a flag or a
> > > clockid. You can carry the clockid for the clocks which actually can be
> > > read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
> > >
> > > struct ptp_sys_offset_extended {
> > > unsigned int n_samples; /* Desired number of measurements. */
> > > clockid_t clockid;
> > > unsigned int rsv[2]; /* Reserved for future use. */
> > > };
> > >
> > > and in the IOCTL:
> > >
> > > if (extoff->clockid != CLOCK_MONOTONIC_RAW)
> > > return -EINVAL;
> > >
> > > sts.clockid = extoff->clockid;
> > >
> > > and it all just works, no?
> > >
> > Yes, this should work. However, I didn't check if struct
> > ptp_system_timestamp is used in some other context.
> >
> > > I have no problem to decide that PTP_SYS_OFFSET will not get this
> > > treatment and the drivers have to be converted over to
> > > PTP_SYS_OFFSET_EXTENDED.
> > >
> > > But adding yet another callback just to carry a clockid as argument is a
> > > more than pointless exercise as I demonstrated.
> > >
> > Agreed. As I said, I thought we cannot change the gettimex64() without
> > breaking the compatibility but the fact that CLOCK_REALTIME is "0"
> > works well for the backward compatibility case.
> >
> > I can spin up an updated patch/series that updates gettimex64
> > implementation instead of adding a new ioctl-op If you all agree.
> >
> > thanks,
> > --mahesh..
> >
> > > Thanks,
> > >
> > > tglx
