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