Re: [Linuxptp-devel] [PATCH v3 1/3] phc: dynamically try PTP_PIN_SETFUNC2 and fallback to PTP_PIN_SETFUNC
On Fri, 23 Jun 2023 at 18:55, Jacob Keller wrote:
>
>
> On 6/23/2023 1:44 AM, Erez wrote:
> > On Fri, 23 Jun 2023 at 09:07, Jacob Keller
> wrote:
> >
> >> The phc library currently selects whether to use PTP_PIN_SETFUNC2 over
> >> PTP_PIN_SETFUNC based on whether the kernel headers it is compiled
> against
> >> have the PTP_PIN_SETFUNC2 defined.
> >>
> >> This means that the use of PTP_PIN_SETFUNC vs PTP_PIN_SETFUNC2 depends
> on
> >> whether the headers we compiled against have the appropriate definition,
> >> but not whether the running kernel supports the new ioctl.
> >>
> >> Fix this to use dynamic fallback. If PTP_PIN_SETFUNC2 returns -ENOTTY,
> then
> >> try PTP_PIN_SETFUNC. This approach will work on a wider range of kernels
> >> and doesn't need the headers to be up to date.
> >>
> >
> >
> > I look in the kernel
> >
> https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/ptp/ptp_chardev.c#L399
> > Using PTP_PIN_SETFUNC2 at the moment does not add anything.
> >
> > Perhaps in the future.
> > It is better to checkthe ptp_pin_desc structure
> >
> https://elixir.bootlin.com/linux/v6.4-rc7/source/include/uapi/linux/ptp_clock.h#L174
> > Once we have new properties there.
> > Then we can use PTP_PIN_SETFUNC2.
> >
> > Erez
> >
>
> The reason to use PTP_PIN_SETFUNC2 is that if it *does* exist we *know*
> the kernel sanitized the data structures and returned zeros in the
> reserved fields.
>
And we do not use these reserved fields.
Once the kernel uses these reserved fields, then it will be interesting.
>
> Obviously currently we aren't using any reserved fields so the behavior
> should be identical.
>
> If we use the old command the kernel sanitizes the reserved fields from
> user space as zero and forwards that to the drivers:
>
> See:
>
> > if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> > || pd.rsv[3] || pd.rsv[4])
> > && cmd == PTP_PIN_SETFUNC2) {
> > err = -EINVAL;
> > break;
> > } else if (cmd == PTP_PIN_SETFUNC) {
> > pd.rsv[0] = 0;
> > pd.rsv[1] = 0;
> > pd.rsv[2] = 0;
> > pd.rsv[3] = 0;
> > pd.rsv[4] = 0;
> > }
>
> and note the checks for PTP_PIN_SETFUNC2 and PTP_PIN_SETFUNC.
>
> This *is* a behavioral difference and I think we should prefer the
> PTP_PIN_SETFUNC2 operation where its available.
>
It is not a different behavior as we zero the structure before we call and
ignore the reserved fields after we get the reply.
It is a waste of time to use PTP_PIN_SETFUNC2 at the moment as long as the
reserved fields are not used.
The application is NOT a kernel verification tool!
Erez
> Unlike PTP_GETCAPS which is EXACTLY the same as PTP_GETCAPS2 I think its
> important to use these variants.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 1/3] phc: dynamically try PTP_PIN_SETFUNC2 and fallback to PTP_PIN_SETFUNC
On 6/23/2023 1:44 AM, Erez wrote:
> On Fri, 23 Jun 2023 at 09:07, Jacob Keller wrote:
>
>> The phc library currently selects whether to use PTP_PIN_SETFUNC2 over
>> PTP_PIN_SETFUNC based on whether the kernel headers it is compiled against
>> have the PTP_PIN_SETFUNC2 defined.
>>
>> This means that the use of PTP_PIN_SETFUNC vs PTP_PIN_SETFUNC2 depends on
>> whether the headers we compiled against have the appropriate definition,
>> but not whether the running kernel supports the new ioctl.
>>
>> Fix this to use dynamic fallback. If PTP_PIN_SETFUNC2 returns -ENOTTY, then
>> try PTP_PIN_SETFUNC. This approach will work on a wider range of kernels
>> and doesn't need the headers to be up to date.
>>
>
>
> I look in the kernel
> https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/ptp/ptp_chardev.c#L399
> Using PTP_PIN_SETFUNC2 at the moment does not add anything.
>
> Perhaps in the future.
> It is better to checkthe ptp_pin_desc structure
> https://elixir.bootlin.com/linux/v6.4-rc7/source/include/uapi/linux/ptp_clock.h#L174
> Once we have new properties there.
> Then we can use PTP_PIN_SETFUNC2.
>
> Erez
>
The reason to use PTP_PIN_SETFUNC2 is that if it *does* exist we *know*
the kernel sanitized the data structures and returned zeros in the
reserved fields.
Obviously currently we aren't using any reserved fields so the behavior
should be identical.
If we use the old command the kernel sanitizes the reserved fields from
user space as zero and forwards that to the drivers:
See:
> if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> || pd.rsv[3] || pd.rsv[4])
> && cmd == PTP_PIN_SETFUNC2) {
> err = -EINVAL;
> break;
> } else if (cmd == PTP_PIN_SETFUNC) {
> pd.rsv[0] = 0;
> pd.rsv[1] = 0;
> pd.rsv[2] = 0;
> pd.rsv[3] = 0;
> pd.rsv[4] = 0;
> }
and note the checks for PTP_PIN_SETFUNC2 and PTP_PIN_SETFUNC.
This *is* a behavioral difference and I think we should prefer the
PTP_PIN_SETFUNC2 operation where its available.
Unlike PTP_GETCAPS which is EXACTLY the same as PTP_GETCAPS2 I think its
important to use these variants.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 1/3] phc: dynamically try PTP_PIN_SETFUNC2 and fallback to PTP_PIN_SETFUNC
On Fri, 23 Jun 2023 at 09:07, Jacob Keller wrote:
> The phc library currently selects whether to use PTP_PIN_SETFUNC2 over
> PTP_PIN_SETFUNC based on whether the kernel headers it is compiled against
> have the PTP_PIN_SETFUNC2 defined.
>
> This means that the use of PTP_PIN_SETFUNC vs PTP_PIN_SETFUNC2 depends on
> whether the headers we compiled against have the appropriate definition,
> but not whether the running kernel supports the new ioctl.
>
> Fix this to use dynamic fallback. If PTP_PIN_SETFUNC2 returns -ENOTTY, then
> try PTP_PIN_SETFUNC. This approach will work on a wider range of kernels
> and doesn't need the headers to be up to date.
>
I look in the kernel
https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/ptp/ptp_chardev.c#L399
Using PTP_PIN_SETFUNC2 at the moment does not add anything.
Perhaps in the future.
It is better to checkthe ptp_pin_desc structure
https://elixir.bootlin.com/linux/v6.4-rc7/source/include/uapi/linux/ptp_clock.h#L174
Once we have new properties there.
Then we can use PTP_PIN_SETFUNC2.
Erez
>
> Signed-off-by: Jacob Keller
> ---
> missing.h | 7 ++-
> phc.c | 7 ---
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/missing.h b/missing.h
> index 79a87d425993..9b37dc258c0f 100644
> --- a/missing.h
> +++ b/missing.h
> @@ -244,11 +244,8 @@ struct ptp_pin_desc {
>
> #endif /*!PTP_PIN_SETFUNC*/
>
> -#ifdef PTP_PIN_SETFUNC2
> -#define PTP_PIN_SETFUNC_FAILED "PTP_PIN_SETFUNC2 failed: %m"
> -#else
> -#define PTP_PIN_SETFUNC_FAILED "PTP_PIN_SETFUNC failed: %m"
> -#define PTP_PIN_SETFUNC2 PTP_PIN_SETFUNC
> +#ifndef PTP_PIN_SETFUNC2
> +#define PTP_PIN_SETFUNC2 _IOW(PTP_CLK_MAGIC, 16, struct ptp_pin_desc)
> #endif
>
> #ifndef LIST_FOREACH_SAFE
> diff --git a/phc.c b/phc.c
> index 37f6b9f6db3c..49e44d1aad65 100644
> --- a/phc.c
> +++ b/phc.c
> @@ -113,9 +113,10 @@ int phc_number_pins(clockid_t clkid)
> int phc_pin_setfunc(clockid_t clkid, struct ptp_pin_desc *desc)
> {
> int err = ioctl(CLOCKID_TO_FD(clkid), PTP_PIN_SETFUNC2, desc);
> - if (err) {
> - fprintf(stderr, PTP_PIN_SETFUNC_FAILED "\n");
> - }
> + if (err == -ENOTTY)
> + err = ioctl(CLOCKID_TO_FD(clkid), PTP_PIN_SETFUNC, desc);
> + if (err)
> + fprintf(stderr, "failed to set pin configuration, error:
> %m\n");
> return err;
> }
>
> --
> 2.41.0.1.g9857a21e0017.dirty
>
>
>
> ___
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
