On Fri, 19 Nov 2021 at 06:23, Karthikkumar Valoor <kval...@altiostar.com>
wrote:
> Hi forum,
>
>
>
> Thanks for the comments.
>
> Please find our responses below.
>
>
>
> *From:* Erez <erezge...@gmail.com>
> *Sent:* 16 November 2021 21:08
> *To:* Vladimir Oltean <olte...@gmail.com>
> *Cc:* Karthikkumar Valoor <kval...@altiostar.com>;
> linuxptp-devel@lists.sourceforge.net
> *Subject:* Re: [Linuxptp-devel] [PATCH] Include phcIndex in
> PORT_PROPERTIES_NP PMC command
>
>
>
> CAUTION: This email originated from outside of Altiostar. Do not click on
> links or open attachments unless you recognize the sender and you are sure
> the content is safe. You will never be asked to reset your Altiostar
> password via email.
>
>
>
>
>
>
>
> On Tue, 16 Nov 2021 at 15:22, Vladimir Oltean <olte...@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 12:47:09PM +0000, Karthikkumar V via
> Linuxptp-devel wrote:
> > PORT_PROPERTIES_NP PMC Command will now display the phcIndex along with
> > other interface details. A typical use case will be Users can fetch the
> > phcIndex to use the clock APIs of the kernel (clock_gettime and
> clock_settime)
> > to get the PHC time from user applications.
> >
> > Signed-off-by: Karthikkumar V <kval...@altiostar.com>
> > Signed-off-by: Ramana Reddy <rre...@altiostar.com>
> > ---
>
> May I suggest that
> (a) you should not modify the binary format of management TLVs in
> incompatible ways, as that would require everybody else to adapt and
> somehow know which format ptp4l expects, and
>
> Totally agree.
>
> You should add new fields at the end of the port_properties_np structure.
>
> Make sure a new pmc tool prints the message from an old ptp4l properly.
>
> And verify an old pmc tool works with a new ptp4l.
>
>
>
> *[Karthikkumar Valoor] Valid comment. Seems we have issue whether we add
> at*
>
> *the end or middle of the structure (error thrown as “pmc[153304.857]: bad
> message”*
>
> *from pmc_recv() function due to size mismatch).*
>
> *There will always be compatibility issues if one of the binaries
> (ptp4l/pmc) are*
>
> *old versions. Not sure how do we address these issues? What is the way
> forward?*
>
>
>
You change the size comparing to support both sizes.
At least support old ptp4l with new pmc.
And mention in the comment that you can not use old pmc with new ptp4l.
Some users do not want to replace old ptp4l just for the phc index.
(b) you can deduce the PHC index already from the struct PTPText interface,
> see posix_clock_open() -> sk_get_ts_info().
>
>
>
> Changing management TLV should be done carefully.
>
> Only when we must.
>
>
>
> *[Karthikkumar Valoor] sk_get_ts_info() needs access to the host
> interface, which*
>
> *will not be visible in cloud environment for applications (for ex:
> POD/container *
>
> *trying to fetch the info).The change was added so that Client
> applications can directly*
>
> *access the /dev/ptpX device without calling the sk_get_ts_info() call.*
>
>
>
A good point.
But If you can not see the interface, how do you "know" the phc index is
the same on the client "machine"?
Why does that matter at all?
You need to fetch the phc index on the same "machine" that the ptp4l runs.
Please write your own application for that and not break everyone else's
management protocol.
>
> > pmc.c | 6 ++++--
> > port.c | 1 +
> > tlv.h | 1 +
> > 3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/pmc.c b/pmc.c
> > index a1ee787..67e33a3 100644
> > --- a/pmc.c
> > +++ b/pmc.c
> > @@ -443,11 +443,13 @@ static void pmc_show(struct ptp_message *msg, FILE
> *fp)
> > IFMT "portIdentity %s"
> > IFMT "portState %s"
> > IFMT "timestamping %s"
> > - IFMT "interface %s",
> > + IFMT "interface %s"
> > + IFMT "phcIndex %d",
> > pid2str(&ppn->portIdentity),
> > ps_str[ppn->port_state],
> > ts_str(ppn->timestamping),
> > - text2str(&ppn->interface));
> > + text2str(&ppn->interface),
> > + ppn->phc_index);
> > break;
> > case MID_PORT_STATS_NP:
> > pcp = (struct port_stats_np *) mgt->data;
> > diff --git a/port.c b/port.c
> > index 7e51e77..743b7c7 100644
> > --- a/port.c
> > +++ b/port.c
> > @@ -956,6 +956,7 @@ static int port_management_fill_response(struct port
> *target,
> > else
> > ppn->port_state = target->state;
> > ppn->timestamping = target->timestamping;
> > + ppn->phc_index = target->phc_index;
> > ts_label = interface_label(target->iface);
> > ptp_text_set(&ppn->interface, ts_label);
> > datalen = sizeof(*ppn) + ppn->interface.length;
> > diff --git a/tlv.h b/tlv.h
> > index 97615fd..0f6cf28 100644
> > --- a/tlv.h
> > +++ b/tlv.h
> > @@ -341,6 +341,7 @@ struct port_properties_np {
> > struct PortIdentity portIdentity;
> > uint8_t port_state;
> > uint8_t timestamping;
> > + Integer32 phc_index;
> > struct PTPText interface;
> > } PACKED;
> >
> > --
> > 1.8.3.1
> >
> >
> >
> > _______________________________________________
> > 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
>
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
