Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On 12/8/2022 1:20 PM, Geva, Erez wrote: > On Wed, 2022-12-07 at 06:59 -0800, Richard Cochran wrote: >> On Thu, Nov 17, 2022 at 02:15:23PM -0800, Jacob Keller wrote: >>> On 11/17/2022 1:34 PM, Geva, Erez wrote: >> 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. >>> >>> Fair. We likely have the same problem with some of the other "2" >>> ioctls, >>> since they're handled in a similar way. I think we do the Right(TM) >>> thing >>> for the sysoff.c where we probe the kernel at run-time. This could >>> be done >>> here but is probably not really worth it considering that >>> PTP_CLOCK_GETCAPS >>> functions the same way as PTP_CLOCK_GETCAPS2 for all kernels I >>> checked... So >>> I guess this is somewhat less likely. >> >> Jacob, do you want to have phc_ctl fall back to PTP_CLOCK_GETCAPS at >> run time if PTP_CLOCK_GETCAPS2 fails? > > We can use run-time fallback. > But personalty, I do not think it worth it. > Using PTP_CLOCK_GETCAPS2 is only done for one new field in a debug > tool. > We can simply wait till PTP_CLOCK_GETCAPS become obsolete or we have a > new PTP_CLOCK_GETCAPS3 to handle. > >> >>> I'm not sure if our other PTP ioctls are checked properly like this >>> at run >>> time... >> >> Maybe, but only because of sloppiness. Better to support older >> kernels at run time, as this is more user friendly. >> >> Working on embedded systems over the years, I've have often been that >> user, and, believe me, it is super annoying when the latest greatest >> App isn't backwards compatible. > > My view too :-) > >> >> Thanks, >> Richard > > > Erez Agreed. Since there's no difference to using PTP_CLOCK_GETCAPS2 at least currently, it makes sense to just keep using PTP_CLOCK_GETCAPS. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On Wed, 2022-12-07 at 06:59 -0800, Richard Cochran wrote: > On Thu, Nov 17, 2022 at 02:15:23PM -0800, Jacob Keller wrote: > > On 11/17/2022 1:34 PM, Geva, Erez wrote: > > > > 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. > > > > > > > Fair. We likely have the same problem with some of the other "2" > > ioctls, > > since they're handled in a similar way. I think we do the Right(TM) > > thing > > for the sysoff.c where we probe the kernel at run-time. This could > > be done > > here but is probably not really worth it considering that > > PTP_CLOCK_GETCAPS > > functions the same way as PTP_CLOCK_GETCAPS2 for all kernels I > > checked... So > > I guess this is somewhat less likely. > > Jacob, do you want to have phc_ctl fall back to PTP_CLOCK_GETCAPS at > run time if PTP_CLOCK_GETCAPS2 fails? We can use run-time fallback. But personalty, I do not think it worth it. Using PTP_CLOCK_GETCAPS2 is only done for one new field in a debug tool. We can simply wait till PTP_CLOCK_GETCAPS become obsolete or we have a new PTP_CLOCK_GETCAPS3 to handle. > > > I'm not sure if our other PTP ioctls are checked properly like this > > at run > > time... > > Maybe, but only because of sloppiness. Better to support older > kernels at run time, as this is more user friendly. > > Working on embedded systems over the years, I've have often been that > user, and, believe me, it is super annoying when the latest greatest > App isn't backwards compatible. My view too :-) > > Thanks, > Richard Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On Thu, Nov 17, 2022 at 02:15:23PM -0800, Jacob Keller wrote: > On 11/17/2022 1:34 PM, Geva, Erez wrote: > > 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. > > > > Fair. We likely have the same problem with some of the other "2" ioctls, > since they're handled in a similar way. I think we do the Right(TM) thing > for the sysoff.c where we probe the kernel at run-time. This could be done > here but is probably not really worth it considering that PTP_CLOCK_GETCAPS > functions the same way as PTP_CLOCK_GETCAPS2 for all kernels I checked... So > I guess this is somewhat less likely. Jacob, do you want to have phc_ctl fall back to PTP_CLOCK_GETCAPS at run time if PTP_CLOCK_GETCAPS2 fails? > I'm not sure if our other PTP ioctls are checked properly like this at run > time... Maybe, but only because of sloppiness. Better to support older kernels at run time, as this is more user friendly. Working on embedded systems over the years, I've have often been that user, and, believe me, it is super annoying when the latest greatest App isn't backwards compatible. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On 11/17/2022 1:34 PM, Geva, Erez wrote: On Thu, 2022-11-17 at 18:27 +, Keller, Jacob E wrote: -Original Message- From: Geva, Erez Sent: Thursday, November 17, 2022 9:13 AM To: Keller, Jacob E ; 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. Fair. We likely have the same problem with some of the other "2" ioctls, since they're handled in a similar way. I think we do the Right(TM) thing for the sysoff.c where we probe the kernel at run-time. This could be done here but is probably not really worth it considering that PTP_CLOCK_GETCAPS functions the same way as PTP_CLOCK_GETCAPS2 for all kernels I checked... So I guess this is somewhat less likely. I'm not sure if our other PTP ioctls are checked properly like this at run time... If I may, this is the classic gap, Hardware companies fail to understand. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On Thu, 2022-11-17 at 18:27 +, Keller, Jacob E wrote: > > > > -Original Message- > > From: Geva, Erez > > Sent: Thursday, November 17, 2022 9:13 AM > > To: Keller, Jacob E ; 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 > > > --- > > > 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
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
> -Original Message- > From: Geva, Erez > Sent: Thursday, November 17, 2022 9:13 AM > To: Keller, Jacob E ; 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. > 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 > > --- > > 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
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 :-) > 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. 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 > --- > 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(). 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
[Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
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 PTP_CLOCK_GETCAPS always zeroed the caps structure. However, it is good practice to use the newer version consistently. Signed-off-by: Jacob Keller --- 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?" 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; -- 2.38.1.420.g319605f8f00e ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel