On Thu, 2022-11-17 at 18:27 +0000, Keller, Jacob E wrote:
>
>
> > -----Original Message-----
> > From: Geva, Erez <erez.geva....@siemens.com>
> > Sent: Thursday, November 17, 2022 9:13 AM
> > To: Keller, Jacob E <jacob.e.kel...@intel.com>; linuxptp-
> > de...@lists.sourceforge.net
> > Subject: Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use
> > PTP_CLOCK_GETCAPS2
> > ioctl if available
> >
> > On Tue, 2022-11-15 at 16:43 -0800, Jacob Keller wrote:
> > > The PTP_CLOCK_GETCAPS2 ioctl is provided by kernel as a
> > > replacement
> > > for the
> > > original PTP_CLOCK_GETCAPS ioctl. This was done in order to
> > > provide
> > > ioctls
> > > which guarantee reserved fields are properly initialized. In
> > > practice
> > > the
> > > PTP_CLOCK_GETCAPS2 and PTP_CLOCK_GETCAPS both behave identically
> > > since
> >
> > Exactly, that is why I send a patch to add the new 'adjust_phase'
> > in
> > do_caps(), without changing the ioctl itself :-)
> >
> >
>
> The reason PTP_CLOCK_GETCAPS2 was created was around the same time as
> the other ioctls were fixed to properly a) validate reserved fields
> and b) ensure that all reported reserved fields were zeroed.
>
> Functionally its 100% identical to PTP_CLOCK_GETCAPS because this
> ioctl already did the right thing.
>
> The reason I propose switching to PTP_CLOCK_GETCAPS2 over
> PTP_CLOCK_GETCAPS is because it reduces cognitive burden to REMEMBER
> that the old PTP_CLOCK_GETCAPS is ok. All I remembered was "the non-2
> ioctls might be broken and not receive new features".
>
> > > PTP_CLOCK_GETCAPS always zeroed the caps structure. However, it
> > > is
> > > good
> > > practice to use the newer version consistently.
> >
> > But, it can also breaks when using the new linuxptp version build
> > on
> > new machine, but used with old kernel.
> > It may be reasonable in few years, when these old kernels becomes
> > obsolete.
>
> This uses a fallback to PTP_CLOCK_GETCAPS if PTP_CLOCK_GETCAPS2 is
> not found.
The problem is the fallback works only on build.
But if the build system is newer than the running system, the fallback
will fail, as you will use the PTP_CLOCK_GETCAPS2 which does not exist
on the running old system.
If I may, this is the classic gap, Hardware companies fail to
understand.
Erez
>
> > But since the new PTP_CLOCK_GETCAPS2 was introduce in 2019, I think
> > we
> > should wait with it.
> > As not everyone build linuxptp, most users use pre-build.
> >
> >
> > >
> > > Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> > > ---
> > > Technically this is identical in current kernels, both ioctls
> > > return
> > > exactly
> > > the same conent.. I still think its better to use the '2'
> > > variants in
> > > principle that developer doesn't have to remember "is this
> > > variant ok
> > > to get
> > > all the new features going forward?"
> >
> > There are more 12 bytes in the reserve.
> > I guess once we pass them, we would need a non-backward flag, but
> > till
> > then, I think we can keep using the PTP_CLOCK_GETCAPS flag, and
> > leave
> > the new flags in do_caps().
> >
>
> The only motiviation for this patch is consistency in using the "2"
> style ioctls across ptp4l, because it makes it less likely someone
> will use the non-2 version of an ioctl that actually does have
> behavioral differences in the future.
>
> Thanks,
> Jake
>
> > Erez
> >
> > >
> > > missing.h | 7 +++++++
> > > phc_ctl.c | 2 +-
> > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/missing.h b/missing.h
> > > index 79a87d425993..41427d3a38b2 100644
> > > --- a/missing.h
> > > +++ b/missing.h
> > > @@ -98,6 +98,13 @@ enum {
> > > #define PTP_PEROUT_REQUEST2 PTP_PEROUT_REQUEST
> > > #endif
> > >
> > > +#ifdef PTP_CLOCK_GETCAPS2
> > > +#define PTP_CLOCK_GETCAPS_REQUEST_FAILED "PTP_CLOCK_GETCAPS2
> > failed:
> > > %m"
> > > +#else
> > > +#define PTP_CLOCK_GETCAPS_REQUEST_FAILED "PTP_CLOCK_GETCAPS
> > > failed:
> > > %m"
> > > +#define PTP_CLOCK_GETCAPS2 PTP_CLOCK_GETCAPS
> > > +#endif
> > > +
> > > #if LINUX_VERSION_CODE < KERNEL_VERSION(5,8,0)
> > >
> > > /* from upcoming Linux kernel version 5.8 */
> > > diff --git a/phc_ctl.c b/phc_ctl.c
> > > index 6a5c2f43d7c9..17a615f8aae6 100644
> > > --- a/phc_ctl.c
> > > +++ b/phc_ctl.c
> > > @@ -288,7 +288,7 @@ static int do_caps(clockid_t clkid, int cmdc,
> > > char *cmdv[])
> > > return 0;
> > > }
> > >
> > > - if (ioctl(CLOCKID_TO_FD(clkid), PTP_CLOCK_GETCAPS,
> > > &caps)) {
> > > + if (ioctl(CLOCKID_TO_FD(clkid), PTP_CLOCK_GETCAPS2,
> > > &caps)) {
> > > pr_err("get capabilities failed: %s",
> > > strerror(errno));
> > > return -1;
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
