Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Jun 2023 11:39:16 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Alex Maftei <alex.maftei@amd.com>
CC:     <richardcochran@gmail.com>, <shuah@kernel.org>,
        <linux-kselftest@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>
Subject: Re: [PATCH net 1/2] selftests/ptp: Add -x option for testing
 PTP_SYS_OFFSET_EXTENDED
Message-ID: <20230619093916.xxfkzj576hwz4tjq@soft-dev3-1>
References: <cover.1686955631.git.alex.maftei@amd.com>
 <e3e14166f0e92065d08a024159e29160b815d2bf.1686955631.git.alex.maftei@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <e3e14166f0e92065d08a024159e29160b815d2bf.1686955631.git.alex.maftei@amd.com>
Precedence: bulk

The 06/16/2023 23:48, Alex Maftei wrote:

Hi Alex,

As I can see you will need to send another patch, I have just a small
comment bellow.


> The -x option (where 'x' stands for eXtended) takes an argument which
> represents the number of samples to request from the PTP device.
> The help message will display the maximum number of samples allowed.
> Providing an invalid argument will also display the maximum number of
> samples allowed.
> 
> Signed-off-by: Alex Maftei <alex.maftei@amd.com>
> ---
>  tools/testing/selftests/ptp/testptp.c | 42 +++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
> index cfa9562f3cd8..2a99973ffc1b 100644
> --- a/tools/testing/selftests/ptp/testptp.c
> +++ b/tools/testing/selftests/ptp/testptp.c
> @@ -142,8 +142,9 @@ static void usage(char *progname)
>                 " -S         set the system time from the ptp clock time\n"
>                 " -t val     shift the ptp clock time by 'val' seconds\n"
>                 " -T val     set the ptp clock time to 'val' seconds\n"
> +               " -x val     get an extended ptp clock time with the desired number of samples (up to %d)\n"
>                 " -z         test combinations of rising/falling external time stamp flags\n",
> -               progname);
> +               progname, PTP_MAX_SAMPLES);
>  }
> 
>  int main(int argc, char *argv[])
> @@ -157,6 +158,7 @@ int main(int argc, char *argv[])
>         struct timex tx;
>         struct ptp_clock_time *pct;
>         struct ptp_sys_offset *sysoff;
> +       struct ptp_sys_offset_extended *soe;
> 
>         char *progname;
>         unsigned int i;
> @@ -174,6 +176,7 @@ int main(int argc, char *argv[])
>         int index = 0;
>         int list_pins = 0;
>         int pct_offset = 0;
> +       int getextended = 0;
>         int n_samples = 0;
>         int pin_index = -1, pin_func;
>         int pps = -1;
> @@ -188,7 +191,7 @@ int main(int argc, char *argv[])
> 
>         progname = strrchr(argv[0], '/');
>         progname = progname ? 1+progname : argv[0];
> -       while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:p:P:sSt:T:w:z"))) {
> +       while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:p:P:sSt:T:w:x:Xz"))) {

The 'X' needs to be part of the next patch, as you introduce here only 'x'.

>                 switch (c) {
>                 case 'c':
>                         capabilities = 1;
> @@ -250,6 +253,13 @@ int main(int argc, char *argv[])
>                 case 'w':
>                         pulsewidth = atoi(optarg);
>                         break;
> +               case 'x':
> +                       getextended = atoi(optarg);
> +                       if (getextended < 1 || getextended > PTP_MAX_SAMPLES) {
> +                               fprintf(stderr, "number of extended timestamp samples must be between 1 and %d; was asked for %d\n", PTP_MAX_SAMPLES, getextended);
> +                               return -1;
> +                       }
> +                       break;
>                 case 'z':
>                         flagtest = 1;
>                         break;
> @@ -516,6 +526,34 @@ int main(int argc, char *argv[])
>                 free(sysoff);
>         }
> 
> +       if (getextended) {
> +               soe = calloc(1, sizeof(*soe));
> +               if (!soe) {
> +                       perror("calloc");
> +                       return -1;
> +               }
> +
> +               soe->n_samples = getextended;
> +
> +               if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED, soe))
> +                       perror("PTP_SYS_OFFSET_EXTENDED");
> +               else {
> +                       printf("extended timestamp request returned %d samples\n",
> +                               getextended);
> +
> +                       for (i = 0; i < getextended; i++) {
> +                               printf("sample #%2d: system time before: %lld.%09u\n",
> +                               i, soe->ts[i][0].sec, soe->ts[i][0].nsec);
> +                               printf("            phc time: %lld.%09u\n",
> +                               soe->ts[i][1].sec, soe->ts[i][1].nsec);
> +                               printf("            system time after: %lld.%09u\n",
> +                               soe->ts[i][2].sec, soe->ts[i][2].nsec);
> +                       }
> +               }
> +
> +               free(soe);
> +       }
> +
>         close(fd);
>         return 0;
>  }
> --
> 2.28.0
> 
> 

-- 
/Horatiu