Re: [Linuxptp-devel] [PATCH_SNMPD_v2 6/6] snmpd: Add data collection from ptp4l program for some mib get requests
On Tue, May 29, 2018 at 11:42:17PM +0200, Anders Selhammer wrote: > Three random picked mib object from ptpbase_mib is initialized in sub > agent and data is received from ptp4l. This looks pretty good. Can we have the whole MIB please? > +/* > + * function declarations > + */ > +struct ptp_message* run_pmc(char *cmd); Can we call it snmp_run_pmc() or something to make it different from the function in phc2sys ? Thanks, Richard -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH_SNMPD_v2 5/6] snmpd: Added communication to ptp4l via the UDS port.
On Tue, May 29, 2018 at 11:42:16PM +0200, Anders Selhammer wrote:
> UDS transport is configured using pmc_common funtions.
"functions."
> +static int open_pmc(struct config *cfg)
> +{
> + char uds_local[MAX_IFNAME_SIZE + 1];
> + snprintf(uds_local, sizeof(uds_local), "/var/run/snmpd.%d", getpid());
> +
> + pmc = pmc_create(cfg, TRANS_UDS, uds_local, 0, 1);
> +
> + return (pmc) ? 0 : -1;
Don't need parenthesis here.
> +}
> +
> static int open_snmp()
> {
> snmp_enable_calllog();
> @@ -35,23 +50,99 @@ static int open_snmp()
> return 0;
> }
>
> +static void usage(char *progname)
> +{
> + fprintf(stderr,
> + "\nusage: %s [options]\n\n"
> + " -f [file] read configuration from 'file'\n"
> + " -hprints this message and exits\n"
> + " -mprint messages to stdout\n"
It would be useful to have "-q" as well, just like the other programs.
> + "\n",
> + progname);
> +}
> +
Thanks,
Richard
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH_SNMPD_v2 4/6] snmpd: Add snmp sub agent
On Tue, May 29, 2018 at 11:42:15PM +0200, Anders Selhammer wrote:
> +int main(int argc, char *argv[])
> +{
> + int err = 0;
> +
> + if (handle_term_signals()) {
> + return -1;
> + }
> +
> + if (open_snmp()) {
> + return -1;
> + }
> +
> + while (is_running()) {
> + agent_check_and_process(1);
> + }
> +
> + snmp_shutdown("linuxptpAgent");
> +
> + return err;
> +}
Please forgive my ignorance, but this minimal program is functional,
right?
What does it do? How does one test it to see that it is working?
Thanks,
Richard
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH_SNMPD_v2 4/6] snmpd: Add snmp sub agent
On Tue, May 29, 2018 at 11:42:15PM +0200, Anders Selhammer wrote:
> diff --git a/incdefs.sh b/incdefs.sh
> index 19e620e..d8ae7d5 100755
> --- a/incdefs.sh
> +++ b/incdefs.sh
> @@ -88,5 +88,16 @@ kernel_flags()
> fi
> }
>
> -flags="$(user_flags)$(kernel_flags)"
> +#
> +# Look for libsnmp presence.
> +#
> +snmp_flags()
> +{
> + libsnmp=/usr/include/net-snmp/
This won't work when cross compiling. That is an important use case,
and we should support it.
> + if [ -d ${libsnmp} ]; then
> + printf " -I. `net-snmp-config --cflags`"
Does libsnmp deliver pkgconfig files? If so we can use them.
Or is net-snmp-config a shell script (or similar) that delivers
correct paths when cross compiling?
> + fi
> +}
> +
> +flags="$(user_flags)$(kernel_flags)$(snmp_flags)"
> echo "$flags"
Thanks,
Richard
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] The daemon should not downgrade the RX filter. An AVB/TSN application may use it. Follow Cliff Spradlin recommendation and add a configuration option.
On Wed, Jun 13, 2018 at 8:39 PM Richard Cochran wrote: > 1. normal - current code > 2. check - read only ioctl; quit if setting not rich enough for the HW time stamping mode Yeah, I'd suggest naming these modes 'overwrite' and 'check'. On Tue, Jun 12, 2018 at 12:40:06PM +, Geva, Erez wrote: > +++ b/ptp4l.8 In the documentation, I suggest explaining that the reason this option is needed is that the hardware timestamping setting on the network interface is global - changing it affects the behavior of the interface for any program on the system. > + cfg.tx_type= tx_type; > + if (ftm == HWTS_FILTER_IGNORE || > + cfg.rx_filter != HWTSTAMP_FILTER_ALL) > + cfg.rx_filter = rx_filter; I think the logic would be a lot simpler if you make this new readonly option apply to both tx and rx. You'd either: a) read the values, or b) attempt to write the values -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] port: don't clockcheck timestamps from other domains
On Fri, Jun 08, 2018 at 11:01:37AM -0700, Cliff Spradlin via Linuxptp-devel wrote: > ptp4l runs clockcheck on an incoming PTP message before checking its > domain number. If the time on another domain is different, then > clockcheck will trigger spurious synchronization faults. > > This patch reorders the logic so that clockcheck only runs on messages > in the same time domain. Applied. Thanks, Richard -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH_SNMPD_v2 1/6] pmc: Refactoring of in arguments in pmc_create
On Tue, May 29, 2018 at 11:42:12PM +0200, Anders Selhammer wrote:
> @@ -124,20 +120,25 @@ void pmc_destroy(struct pmc *pmc)
>
> static struct ptp_message *pmc_message(struct pmc *pmc, uint8_t action)
> {
> + UInteger8 transportSpecific;
> struct ptp_message *msg;
> int pdulen;
>
> msg = msg_allocate();
> - if (!msg)
> + if (!msg) {
> return NULL;
> + }
> +
> + transportSpecific = config_get_int(pmc->cfg, NULL, "transportSpecific");
I don't like this patch. It isn't required for SNMP support, and it
makes every PMC message do a hash table lookup once ...
> + transportSpecific <<= 4;
>
> pdulen = sizeof(struct management_msg);
> msg->hwts.type = TS_SOFTWARE;
>
> - msg->header.tsmt = MANAGEMENT | pmc->transport_specific;
> + msg->header.tsmt = MANAGEMENT | transportSpecific;
> msg->header.ver= PTP_VERSION;
> msg->header.messageLength = pdulen;
> - msg->header.domainNumber = pmc->domain_number;
> + msg->header.domainNumber = config_get_int(pmc->cfg, NULL,
> "domainNumber");
... twice for every sent message. That is a waste of CPU time,
especially for clients that must send many messages. The original
code pre-computed those values which is the right thing to do.
> msg->header.sourcePortIdentity = pmc->port_identity;
> msg->header.sequenceId = pmc->sequence_id++;
> msg->header.control= CTL_MANAGEMENT;
Thanks,
Richard
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] The daemon should not downgrade the RX filter. An AVB/TSN application may use it. Follow Cliff Spradlin recommendation and add a configuration option.
On Tue, Jun 12, 2018 at 12:40:06PM +, Geva, Erez wrote:
> Signed-off-by: Erez Geva
Please write a proper commit message with a succinct subject line.
> +static struct config_enum hwts_filter_enu[] = {
> + { "default", HWTS_FILTER_DEF },
"DEF" looks silly. Please spell out "DEFAULT".
> + { "ignore", HWTS_FILTER_IGNORE },
> + { "upgrade", HWTS_FILTER_UPGRADE },
> + { "check", HWTS_FILTER_CHECK },
Why four different modes?
What we discussed was:
> My preference would be to have an option to not adjust it. Then you could
> use hwstamp_ctl or another program before running ptp4l to set it to the
> value you want. In this mode, ptp4l would simply check to see whether the
> filter was set to a sufficient value for PTP operations and fail with an
> error if it was not.
That means just two modes:
1. normal - current code
2. check - read only ioctl; quit if setting not rich enough for the HW
time stamping mode
> + { NULL, 0 },
> +};
> +
> struct config_item config_tab[] = {
> PORT_ITEM_INT("announceReceiptTimeout", 3, 2, UINT8_MAX),
> GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
> @@ -268,6 +276,7 @@ struct config_item config_tab[] = {
> GLOB_ITEM_STR("userDescription", ""),
> GLOB_ITEM_INT("utc_offset", CURRENT_UTC_OFFSET, 0, INT_MAX),
> GLOB_ITEM_INT("verbose", 0, 0, 1),
> + PORT_ITEM_ENU("hwts_filter", HWTS_FILTER_DEF, hwts_filter_enu),
Keep list in alphabetical order please.
> };
>
> static enum parser_result
> diff --git a/ptp4l.8 b/ptp4l.8
> index ae6e491..4e2e87e 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -624,6 +624,14 @@ The time source is a single byte code that gives an idea
> of the kind
> of local clock in use. The value is purely informational, having no
> effect on the outcome of the Best Master Clock algorithm, and is
> advertised when the clock becomes grand master.
> +.TP
> +.B hwts_filter
> +Select the hardware time stamp filter setting mode;
> +Possible values are ignore, upgrade, check.
That is only three values, but you added four.
> +Some AVB/TSN applications may need the receiving hardware time stamp.
This has nothing to do with AVB/TSN. Instead, we want ptp4l to play
nicely with *any* other program that needs time stamping.
> +Upgrade mode prevent downgrading the RX filter.
> +Check mode only check but do not set.
> +The default is ignore.
You have the default as HWTS_FILTER_DEF which is distinct from
HWTS_FILTER_IGNORE.
> .SH TIME SCALE USAGE
>
> diff --git a/raw.c b/raw.c
> index 8dc50bc..ff60fea 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -200,7 +200,8 @@ static void addr_to_mac(void *mac, struct address *addr)
> }
>
> static int raw_open(struct transport *t, struct interface *iface,
> - struct fdarray *fda, enum timestamp_type ts_type)
> + struct fdarray *fda, enum timestamp_type ts_type,
> + enum hwts_filter_mode ftm)
Let's not tack on other parameter to the transports. This is not a
per-port setting. Instead, we want a global variable in sk.c along
with sk_tx_timeout and sk_check_fupsync.
> @@ -55,12 +56,27 @@ static int hwts_init(int fd, const char *device, int
> rx_filter, int tx_type)
> strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name) - 1);
>
> ifreq.ifr_data = (void *)
> - cfg.tx_type= tx_type;
> - cfg.rx_filter = rx_filter;
> - req = cfg;
> - err = ioctl(fd, SIOCSHWTSTAMP, );
> - if (err < 0)
> - return err;
> +
> + if (ftm != HWTS_FILTER_IGNORE) {
> + if (ioctl(fd, SIOCGHWTSTAMP, ) < 0) {
> + pr_err("ioctl SIOCGHWTSTAMP failed: %m");
> + return -1;
> + }
> + }
> +
> + if (ftm == HWTS_FILTER_CHECK) {
> + req.tx_type = tx_type;
> + req.rx_filter = rx_filter;
> + } else {
> + cfg.tx_type= tx_type;
> + if (ftm == HWTS_FILTER_IGNORE ||
> + cfg.rx_filter != HWTSTAMP_FILTER_ALL)
> + cfg.rx_filter = rx_filter;
> + req = cfg;
> + err = ioctl(fd, SIOCSHWTSTAMP, );
> + if (err < 0)
> + return err;
> + }
This is too convoluted and super confusing.
...
> +enum hwts_filter_mode {
> + HWTS_FILTER_DEF, /* default value for real value detection */
Sorry, I really can't follow these comments.
> + HWTS_FILTER_IGNORE,/* ignore previous mode and set new mode */
> + HWTS_FILTER_UPGRADE, /* upgrade rx filter */
> + HWTS_FILTER_CHECK, /* check filters but do not change them */
> +};
What we really want is a Boolean that makes the ioctl read only.
Thanks,
Richard
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel
