Re: [Linuxptp-devel] [PATCH] phc2sys: Notify kernel if clock is not in sync
On Wed, Mar 04, 2020 at 08:49:45AM -0800, Richard Cochran wrote: > On Mon, Feb 17, 2020 at 01:46:26PM +0100, Miroslav Lichvar wrote: > > On Mon, Feb 17, 2020 at 02:34:14PM +0200, Heikkinen, Ville (Nokia - > > FI/Espoo) wrote: > > > (https://www.eecis.udel.edu/~mills/database/reports/kern/kernb.pdf). In > > > there, it's specified that "STA_UNSYNC set/cleared by the caller to > > > indicate > > > clock unsynchronized (e.g., when no peers are reachable)". So, this is the > > > reasoning why the flag could be used like this. > > > > Good find. > > > > FWIW, I'm not aware of any software that would unset the flag when the > > you meant, "set", right? Right. > > > clock stops being updated. That includes software that was maintained > > by Dr. Mills. > > > > It seems wrong to me to do that, but I'm not strongly opposed. > > I think we can go ahead and give a flag a meaning. If the kernel > meant to enforce some other kind of meaning, then it would have had to > check the value that user space provides. I'd stick with the meaning "the clock has is believed to have an error larger than 16 seconds". With ptp4l/phc2sys, if someone needs to check if the clock was updated in the last X seconds, they can check the maxerror value if it's not larger than X * 500us. No need to set the UNSYNC flag. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] phc2sys: Notify kernel if clock is not in sync
On Fri, Feb 14, 2020 at 01:23:44PM +0200, Ville Heikkinen wrote: > In case there is no connection to the server, notify the kernel > that the clock is currently unsynchronized. > > Signed-off-by: Ville Heikkinen Applied. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 00/30] Convert open coded interface into proper module.
On Tue, Feb 18, 2020 at 10:03:38AM -0800, Jacob Keller wrote: > > Nice. A lot of patches, but they sound reasonable. Will review them this > afternoon. I really appreciate your careful review. It is quite a chore. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 28/30] interface: Hide the implementation details.
On Tue, Feb 18, 2020 at 01:32:01PM -0800, Jacob Keller wrote: > I'd appreciate if there was some way to ensure we catch the interface > structure layout changing such that the definitions in clock.c and > config.c aren't compatible with interface.c anymore. > > Perhaps there isn't a good solution for that besides code review? I am not too worried about the safety, because moving the list head will cause the whole thing to explode. But there would be a better way. The struct port has the same design, and so I'll address this issue with a follow up series. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 24/30] interface: Introduce methods to create and destroy instances.
On Tue, Feb 18, 2020 at 01:25:00PM -0800, Jacob Keller wrote: > > I still think it would be good to have the functions guarantee the NULL > by manually assigning or using one of the string copy implementations > that will guarantee it. That way they don't have to rely on this assumption. In the final version, we have, char name[MAX_IFNAME_SIZE + 1]; char ts_label[MAX_IFNAME_SIZE + 1]; strncpy(iface->name, name, MAX_IFNAME_SIZE); memcpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE); strncpy(iface->ts_label, label, MAX_IFNAME_SIZE); so I think it is clear enough. The use of MAX_IFNAME_SIZE is key. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 20/30] interface: Introduce a method to test the time stamping information validity.
On Tue, Feb 18, 2020 at 01:19:09PM -0800, Jacob Keller wrote:
> > +bool interface_tsinfo_valid(struct interface *iface)
> > +{
> > + return iface->ts_info.valid ? true : false;
> > +}
>
> Do you actually need the ternary here? shouldn't ts_info.valid get
> converted to true or false because we are returning a boolean?
Right.
> I don't think this is harmful and you may consider it improving
> readability though.
Yeah, that is the reason. I like to have it spelled out in this case.
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 14/30] interface: Introduce a method to set the name.
On Tue, Feb 18, 2020 at 01:07:52PM -0800, Jacob Keller wrote: > > Good, the name is marked as constant. Side note, for those interface_* > functions that don't modify the interface, does it make sense to mark > them as taking a const interface pointer? I have heard C++ people insisting on this, but frankly IMHO it doesn't make any sense. If the implementation of the "class" is completely hidden, as it should be, then the caller should not care what happens offstage. That is none of the caller's business. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 07/30] Convert call sites to the proper method for getting interface names.
On Tue, Feb 18, 2020 at 11:38:43AM -0800, Jacob Keller wrote: > > Is there a way to generate the network of how interconnected the various > object files are? The .d files have the includes. I once wrote a script to turn the .d files into a dotty graph. But I guess the utility is low. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 06/30] interface: Introduce an access method for the name field.
On Tue, Feb 18, 2020 at 11:33:32AM -0800, Jacob Keller wrote: > So the interface.o isn't being added to something like $(CONFIG)? I like the idea, but I'll leave that as a follow-on task. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] phc2sys: Notify kernel if clock is not in sync
On Mon, Feb 17, 2020 at 01:46:26PM +0100, Miroslav Lichvar wrote: > On Mon, Feb 17, 2020 at 02:34:14PM +0200, Heikkinen, Ville (Nokia - FI/Espoo) > wrote: > > (https://www.eecis.udel.edu/~mills/database/reports/kern/kernb.pdf). In > > there, it's specified that "STA_UNSYNC set/cleared by the caller to indicate > > clock unsynchronized (e.g., when no peers are reachable)". So, this is the > > reasoning why the flag could be used like this. > > Good find. > > FWIW, I'm not aware of any software that would unset the flag when the you meant, "set", right? > clock stops being updated. That includes software that was maintained > by Dr. Mills. > > It seems wrong to me to do that, but I'm not strongly opposed. I think we can go ahead and give a flag a meaning. If the kernel meant to enforce some other kind of meaning, then it would have had to check the value that user space provides. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
