Re: [PATCH v4 net-next 3/7] selftests/ptp: Add testptp support for attributes ioctls

[Arthur Kiyanovski <akiyano@amazon.com>]

On 2026-07-16 16:00:49+03:00, saeed bishara wrote:
> >                 " -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"
> > +               " -a val     get extended timestamps with attributes (error_bound,\n"
> 
> timestamps -> ptp clock to be consistent
> also, have you considered making the "-a" additional flag to -x and -A
> instead of an exclusive option?
> 
> > +               printf("sample #%2d: unknown clock %d %s: %lld.%09u\n",
> > +                      sample_num, clockid, when, sec, nsec);
> 
> In the case of an unknown clock, will the additional parameters
> (when/sec/..) be useful?
> 
> >                 break;
> >         }
> >  }
> > @@ -188,6 +193,7 @@ int main(int argc, char *argv[])
> >         struct ptp_sys_offset *sysoff;
> >         struct ptp_sys_offset_extended *soe;
> >         struct ptp_sys_offset_precise *xts;
> > +       struct ptp_sys_offset_attrs *attrs_data;
> >
> >         char *progname;
> >         unsigned int i;
> > @@ -208,7 +214,9 @@ int main(int argc, char *argv[])
> >         int list_pins = 0;
> >         int pct_offset = 0;
> >         int getextended = 0;
> > +       int getextendedattrs = 0;
> >         int getcross = 0;
> > +       int getcrossattrs = 0;
> >         int n_samples = 0;
> >         int pin_index = -1, pin_func;
> >         int pps = -1;
> > @@ -226,7 +234,8 @@ int main(int argc, char *argv[])
> >
> >         progname = strrchr(argv[0], '/');
> >         progname = progname ? 1+progname : argv[0];
> > -       while (EOF != (c = getopt(argc, argv, "cd:e:E:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xy:z"))) {
> > +       while (EOF != (c = getopt(argc, argv,
> > +                                 "a:Acd:e:E:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xy:z"))) {
> >                 switch (c) {
> >                 case 'c':
> >                         capabilities = 1;
> > @@ -311,9 +320,22 @@ int main(int argc, char *argv[])
> >                                 return -1;
> >                         }
> >                         break;
> > +               case 'a':
> > +                       getextendedattrs = atoi(optarg);
> > +                       if (getextendedattrs < 1 ||
> > +                           getextendedattrs > PTP_MAX_SAMPLES) {
> > +                               fprintf(stderr,
> > +                                       "number of extended attrs timestamp samples must be between 1 and %d; was asked for %d\n",
> > +                                       PTP_MAX_SAMPLES, getextendedattrs);
> > +                               return -1;
> > +                       }
> > +                       break;
> >                 case 'X':
> >                         getcross = 1;
> >                         break;
> > +               case 'A':
> > +                       getcrossattrs = 1;
> > +                       break;
> >                 case 'y':
> >                         if (!strcasecmp(optarg, "realtime"))
> >                                 ext_clockid = CLOCK_REALTIME;
> > @@ -367,6 +389,8 @@ int main(int argc, char *argv[])
> >                                "  %d programmable pins\n"
> >                                "  %d cross timestamping\n"
> >                                "  %d adjust_phase\n"
> > +                              "  %d extended_attrs\n"
> > +                              "  %d precise_attrs\n"
> >                                "  %d maximum phase adjustment (ns)\n",
> >                                caps.max_adj,
> >                                caps.n_alarm,
> > @@ -376,6 +400,8 @@ int main(int argc, char *argv[])
> >                                caps.n_pins,
> >                                caps.cross_timestamping,
> >                                caps.adjust_phase,
> > +                              caps.extended_attrs,
> > +                              caps.precise_attrs,
> >                                caps.max_phase_adj);
> >                 }
> >         }
> > @@ -648,6 +674,49 @@ int main(int argc, char *argv[])
> >                 free(soe);
> >         }
> >
> > +       if (getextendedattrs) {
> > +               attrs_data = calloc(1, sizeof(*attrs_data) +
> > +                                   getextendedattrs * sizeof(struct ptp_timestamp));
> > +               if (!attrs_data) {
> > +                       perror("calloc");
> > +                       return -1;
> > +               }
> > +
> > +               attrs_data->request.num_samples = getextendedattrs;
> > +               attrs_data->request.clock_id = ext_clockid;
> > +
> > +               if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED_ATTRS, attrs_data)) {
> > +                       perror("PTP_SYS_OFFSET_EXTENDED_ATTRS");
> > +               } else {
> > +                       printf("extended attrs timestamp request returned %d samples\n",
> > +                              getextendedattrs);
> > +
> > +                       for (i = 0; i < getextendedattrs; i++) {
> > +                               struct ptp_timestamp *ts = &attrs_data->timestamps[i];
> > +
> > +                               printf("  sample #%u:\n", i);
> > +                               printf("    sys before: %lld ns\n",
> > +                                      (long long)ts->pre_systime.sys_time);
> > +                               printf("    phc time:   %lld.%09u\n",
> > +                                      ts->devtime.device_time.sec,
> > +                                      ts->devtime.device_time.nsec);
> > +                               if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_ERROR_BOUND)
> > +                                       printf("    error_bound: %u ns\n",
> > +                                              ts->devtime.attrs.error_bound);
> 
> in case device doesn't report error bound, I think it's better to
> print explicit message

All addressed in v5: -a is now a modifier on -x/-X (dropped -A, use
-X -a) as you suggested; usage text updated; the unknown-clock line is
trimmed to just the clock id; and attributes the device doesn't report
now print "not reported".