Discussion:
[Linuxptp-devel] [PATCH v3 01/11] uds: don't output "Connection refused"
Jiri Benc
2014-05-02 10:37:44 UTC
Permalink
When phc2sys is started before ptp4l or it is interrupted before ptp4l has
a chance to reply to its query, the "uds: sendto failed: Connection refused"
message is output. This is not an interesting message.

Also, don't output the "failed to send message" error from pmc_send, as
all transports output errors in their send routine.

Signed-off-by: Jiri Benc <***@redhat.com>
---
pmc_common.c | 10 +++-------
uds.c | 2 +-
2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/pmc_common.c b/pmc_common.c
index a7201f92454d..2c75074c8fb7 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -143,18 +143,14 @@ static struct ptp_message *pmc_message(struct pmc *pmc, uint8_t action)

static int pmc_send(struct pmc *pmc, struct ptp_message *msg, int pdulen)
{
- int cnt, err;
+ int err;
+
err = msg_pre_send(msg);
if (err) {
pr_err("msg_pre_send failed");
return -1;
}
- cnt = transport_send(pmc->transport, &pmc->fdarray, 0, msg);
- if (cnt < 0) {
- pr_err("failed to send message");
- return -1;
- }
- return 0;
+ return transport_send(pmc->transport, &pmc->fdarray, 0, msg);
}

static int pmc_tlv_datalen(struct pmc *pmc, int id)
diff --git a/uds.c b/uds.c
index e88b333e4f00..e98e32c0351d 100644
--- a/uds.c
+++ b/uds.c
@@ -111,7 +111,7 @@ static int uds_send(struct transport *t, struct fdarray *fda, int event,
addr = &uds->address;

cnt = sendto(fd, buf, buflen, 0, &addr->sa, addr->len);
- if (cnt <= 0) {
+ if (cnt <= 0 && errno != ECONNREFUSED) {
pr_err("uds: sendto failed: %m");
}
return cnt;
--
1.7.6.5
Jiri Benc
2014-05-02 10:37:45 UTC
Permalink
Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 15 ++++++++++++---
port.c | 6 +++---
port.h | 3 ++-
3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/clock.c b/clock.c
index c5dd873e64c5..c541951dcc68 100644
--- a/clock.c
+++ b/clock.c
@@ -798,7 +798,7 @@ static void clock_forward_mgmt_msg(struct clock *c, struct port *p, struct ptp_m

int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
{
- int changed = 0, i;
+ int changed = 0, i, res, answers;
struct management_tlv *mgt;
struct ClockIdentity *tcid, wildcard = {
{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
@@ -883,9 +883,18 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
clock_management_send_error(p, msg, NOT_SUPPORTED);
break;
default:
+ answers = 0;
for (i = 0; i < c->nports; i++) {
- if (port_manage(c->port[i], p, msg))
- break;
+ res = port_manage(c->port[i], p, msg);
+ if (res < 0)
+ return changed;
+ if (res > 0)
+ answers++;
+ }
+ if (!answers) {
+ /* IEEE 1588 Interpretation #21 suggests to use
+ * WRONG_VALUE for ports that do not exist */
+ clock_management_send_error(p, msg, WRONG_VALUE);
}
break;
}
diff --git a/port.c b/port.c
index 390aa389adc4..e84067395f53 100644
--- a/port.c
+++ b/port.c
@@ -2198,11 +2198,11 @@ int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
switch (management_action(msg)) {
case GET:
if (port_management_get_response(p, ingress, mgt->id, msg))
- return 0;
+ return 1;
break;
case SET:
if (port_management_set(p, ingress, mgt->id, msg))
- return 0;
+ return 1;
break;
case COMMAND:
break;
@@ -2234,7 +2234,7 @@ int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
port_management_send_error(p, ingress, msg, NO_SUCH_ID);
return -1;
}
- return 0;
+ return 1;
}

int port_management_error(struct PortIdentity pid, struct port *ingress,
diff --git a/port.h b/port.h
index 9d1ddc9e94a3..804e4631d00c 100644
--- a/port.h
+++ b/port.h
@@ -116,7 +116,8 @@ struct PortIdentity port_identity(struct port *p);
* @param p A pointer previously obtained via port_open().
* @param ingress The port on which 'msg' was received.
* @param msg A management message.
- * @return Zero if the message is valid, non-zero otherwise.
+ * @return 1 if the message was responded to, 0 if it did not apply
+ * to the port, -1 if it was invalid.
*/
int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg);
--
1.7.6.5
Richard Cochran
2014-05-03 13:39:48 UTC
Permalink
---
Applied.

Thanks,
Richard
Jiri Benc
2014-05-02 10:37:47 UTC
Permalink
Signed-off-by: Jiri Benc <***@redhat.com>
---
port.c | 7 +++++++
port.h | 8 ++++++++
2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/port.c b/port.c
index 164de9b6898d..d5650ac7172e 100644
--- a/port.c
+++ b/port.c
@@ -2170,6 +2170,13 @@ int port_forward(struct port *p, struct ptp_message *msg)
return cnt <= 0 ? -1 : 0;
}

+int port_forward_to(struct port *p, struct ptp_message *msg)
+{
+ int cnt;
+ cnt = transport_sendto(p->trp, &p->fda, 0, msg);
+ return cnt <= 0 ? -1 : 0;
+}
+
int port_prepare_and_send(struct port *p, struct ptp_message *msg, int event)
{
int cnt;
diff --git a/port.h b/port.h
index 86a6f173bf98..34103a6a24ee 100644
--- a/port.h
+++ b/port.h
@@ -92,6 +92,14 @@ enum fsm_event port_event(struct port *port, int fd_index);
int port_forward(struct port *p, struct ptp_message *msg);

/**
+ * Forward a message on a given port to the address stored in the message.
+ * @param port A pointer previously obtained via port_open().
+ * @param msg The message to send. Must be in network byte order.
+ * @return Zero on success, non-zero otherwise.
+ */
+int port_forward_to(struct port *p, struct ptp_message *msg);
+
+/**
* Prepare message for transmission and send it to a given port. Note that
* a single message cannot be sent several times using this function, that
* would lead to corrupted data being sent. Use msg_pre_send and
--
1.7.6.5
Richard Cochran
2014-05-03 13:41:16 UTC
Permalink
---
Applied.

Thanks,
Richard
Jiri Benc
2014-05-02 10:37:46 UTC
Permalink
This parameter was made obsolete by the common address refactoring.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 2 +-
port.c | 2 +-
port.h | 3 +--
3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/clock.c b/clock.c
index c541951dcc68..a85efdec2857 100644
--- a/clock.c
+++ b/clock.c
@@ -785,7 +785,7 @@ static void clock_forward_mgmt_msg(struct clock *c, struct port *p, struct ptp_m
msg->management.boundaryHops--;
msg_pre_send(msg);
}
- if (port_forward(fwd, msg, pdulen))
+ if (port_forward(fwd, msg))
pr_err("port %d: management forward failed", i + 1);
}
}
diff --git a/port.c b/port.c
index e84067395f53..164de9b6898d 100644
--- a/port.c
+++ b/port.c
@@ -2163,7 +2163,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
return event;
}

-int port_forward(struct port *p, struct ptp_message *msg, int msglen)
+int port_forward(struct port *p, struct ptp_message *msg)
{
int cnt;
cnt = transport_send(p->trp, &p->fda, 0, msg);
diff --git a/port.h b/port.h
index 804e4631d00c..86a6f173bf98 100644
--- a/port.h
+++ b/port.h
@@ -87,10 +87,9 @@ enum fsm_event port_event(struct port *port, int fd_index);
* Forward a message on a given port.
* @param port A pointer previously obtained via port_open().
* @param msg The message to send. Must be in network byte order.
- * @param msglen The length of the message in bytes.
* @return Zero on success, non-zero otherwise.
*/
-int port_forward(struct port *p, struct ptp_message *msg, int msglen);
+int port_forward(struct port *p, struct ptp_message *msg);

/**
* Prepare message for transmission and send it to a given port. Note that
--
1.7.6.5
Richard Cochran
2014-05-03 13:40:24 UTC
Permalink
Post by Jiri Benc
This parameter was made obsolete by the common address refactoring.
---
Applied.

Thanks,
Richard
Jiri Benc
2014-05-02 10:37:48 UTC
Permalink
This puts groundwork for event subscription and notification. The individual
events are added by subsequent patches.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
clock.h | 11 +++++
notification.h | 27 +++++++++++
tlv.c | 4 ++
tlv.h | 8 +++
5 files changed, 186 insertions(+), 0 deletions(-)
create mode 100644 notification.h

diff --git a/clock.c b/clock.c
index a85efdec2857..600fc058a391 100644
--- a/clock.c
+++ b/clock.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <time.h>

+#include "address.h"
#include "bmc.h"
#include "clock.h"
#include "clockadj.h"
@@ -59,6 +60,14 @@ struct clock_stats {
unsigned int max_count;
};

+struct clock_subscriber {
+ LIST_ENTRY(clock_subscriber) list;
+ uint8_t events[EVENT_BITMASK_CNT];
+ struct PortIdentity targetPortIdentity;
+ struct address addr;
+ UInteger16 sequenceId;
+};
+
struct clock {
clockid_t clkid;
struct servo *servo;
@@ -99,6 +108,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface uds_interface;
+ LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
};

struct clock the_clock;
@@ -110,9 +120,96 @@ static int cid_eq(struct ClockIdentity *a, struct ClockIdentity *b)
return 0 == memcmp(a, b, sizeof(*a));
}

+#ifndef LIST_FOREACH_SAFE
+#define LIST_FOREACH_SAFE(var, head, field, tvar) \
+ for ((var) = LIST_FIRST((head)); \
+ (var) && ((tvar) = LIST_NEXT((var), field), 1); \
+ (var) = (tvar))
+#endif
+
+static void remove_subscriber(struct clock_subscriber *s)
+{
+ LIST_REMOVE(s, list);
+ free(s);
+}
+
+static void clock_update_subscription(struct clock *c, struct ptp_message *req,
+ uint8_t *bitmask)
+{
+ struct clock_subscriber *s;
+ int i, remove = 1;
+
+ for (i = 0; i < EVENT_BITMASK_CNT; i++) {
+ if (bitmask[i]) {
+ remove = 0;
+ break;
+ }
+ }
+
+ LIST_FOREACH(s, &c->subscribers, list) {
+ if (!memcmp(&s->targetPortIdentity, &req->header.sourcePortIdentity,
+ sizeof(struct PortIdentity))) {
+ /* Found, update the transport address and event
+ * mask. */
+ if (!remove) {
+ s->addr = req->address;
+ memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ } else {
+ remove_subscriber(s);
+ }
+ return;
+ }
+ }
+ if (remove)
+ return;
+ /* Not present yet, add the subscriber. */
+ s = malloc(sizeof(*s));
+ if (!s) {
+ pr_err("failed to allocate memory for a subscriber");
+ return;
+ }
+ s->targetPortIdentity = req->header.sourcePortIdentity;
+ s->addr = req->address;
+ memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ s->sequenceId = 0;
+ LIST_INSERT_HEAD(&c->subscribers, s, list);
+}
+
+static void clock_flush_subscriptions(struct clock *c)
+{
+ struct clock_subscriber *s, *tmp;
+
+ LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) {
+ remove_subscriber(s);
+ }
+}
+
+void clock_send_notification(struct clock *c, struct ptp_message *msg,
+ int msglen, enum notification event)
+{
+ unsigned int event_pos = event / 8;
+ uint8_t mask = 1 << (event % 8);
+ struct port *uds = c->port[c->nports];
+ struct clock_subscriber *s;
+
+ LIST_FOREACH(s, &c->subscribers, list) {
+ if (!(s->events[event_pos] & mask))
+ continue;
+ /* send event */
+ msg->header.sequenceId = htons(s->sequenceId);
+ s->sequenceId++;
+ msg->management.targetPortIdentity.clockIdentity = s->targetPortIdentity.clockIdentity;
+ msg->management.targetPortIdentity.portNumber = htons(s->targetPortIdentity.portNumber);
+ msg->address = s->addr;
+ port_forward_to(uds, msg);
+ }
+}
+
void clock_destroy(struct clock *c)
{
int i;
+
+ clock_flush_subscriptions(c);
for (i = 0; i < c->nports; i++) {
port_close(c->port[i]);
close(c->fault_fd[i]);
@@ -328,6 +425,40 @@ static int clock_management_set(struct clock *c, struct port *p,
return respond ? 1 : 0;
}

+static int clock_management_cmd_response(struct clock *c, struct port *p,
+ int id, struct ptp_message *req)
+{
+ int respond = 0;
+ struct ptp_message *rsp = NULL;
+ struct management_tlv *tlv;
+ struct subscribe_events_np *sen;
+
+ tlv = (struct management_tlv *)req->management.suffix;
+
+ switch (id) {
+ case SUBSCRIBE_EVENTS_NP:
+ if (p != c->port[c->nports]) {
+ /* Only the UDS port allowed. */
+ break;
+ }
+ sen = (struct subscribe_events_np *)tlv->data;
+ clock_update_subscription(c, req, sen->bitmask);
+ respond = 1;
+ break;
+ }
+ if (respond) {
+ rsp = port_management_reply(port_identity(p), p, req);
+ if (!rsp) {
+ pr_err("failed to allocate response message");
+ return 1;
+ }
+ if (port_prepare_and_send(p, rsp, 0))
+ pr_err("failed to send response message");
+ msg_put(rsp);
+ }
+ return respond ? 1 : 0;
+}
+
static void clock_stats_update(struct clock_stats *s,
int64_t offset, double freq)
{
@@ -669,6 +800,8 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,

clock_sync_interval(c, 0);

+ LIST_INIT(&c->subscribers);
+
for (i = 0; i < count; i++) {
c->port[i] = port_open(phc_index, timestamping, 1+i, &iface[i], c);
if (!c->port[i]) {
@@ -842,6 +975,8 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
return changed;
break;
case COMMAND:
+ if (clock_management_cmd_response(c, p, mgt->id, msg))
+ return changed;
break;
default:
return changed;
@@ -880,6 +1015,7 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
case PRIMARY_DOMAIN:
case TIME_STATUS_NP:
case GRANDMASTER_SETTINGS_NP:
+ case SUBSCRIBE_EVENTS_NP:
clock_management_send_error(p, msg, NOT_SUPPORTED);
break;
default:
diff --git a/clock.h b/clock.h
index 804640bdb8f4..8718f2db715b 100644
--- a/clock.h
+++ b/clock.h
@@ -23,6 +23,7 @@
#include "dm.h"
#include "ds.h"
#include "config.h"
+#include "notification.h"
#include "servo.h"
#include "tlv.h"
#include "tmv.h"
@@ -134,6 +135,16 @@ void clock_install_fda(struct clock *c, struct port *p, struct fdarray fda);
int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg);

/**
+ * Send notification about an event to all subscribers.
+ * @param c The clock instance.
+ * @param msg The PTP message to send, in network byte order.
+ * @param msglen The length of the message in bytes.
+ * @param event The event that occured.
+ */
+void clock_send_notification(struct clock *c, struct ptp_message *msg,
+ int msglen, enum notification event);
+
+/**
* Obtain a clock's parent data set.
* @param c The clock instance.
* @return A pointer to the parent data set of the clock.
diff --git a/notification.h b/notification.h
new file mode 100644
index 000000000000..57e7a7856360
--- /dev/null
+++ b/notification.h
@@ -0,0 +1,27 @@
+/**
+ * @file notification.h
+ * @brief Definitions for the notification framework.
+ * @note Copyright (C) 2014 Red Hat, Inc., Jiri Benc <***@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_NOTIFICATION_H
+#define HAVE_NOTIFICATION_H
+
+enum notification {
+ NOTIFY_DUMMY,
+};
+
+#endif
diff --git a/tlv.c b/tlv.c
index b8cdd3959c9b..430410f75397 100644
--- a/tlv.c
+++ b/tlv.c
@@ -242,6 +242,10 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
pdsnp->neighborPropDelayThresh = ntohl(pdsnp->neighborPropDelayThresh);
pdsnp->asCapable = ntohl(pdsnp->asCapable);
break;
+ case SUBSCRIBE_EVENTS_NP:
+ if (data_len != sizeof(struct subscribe_events_np))
+ goto bad_length;
+ break;
case SAVE_IN_NON_VOLATILE_STORAGE:
case RESET_NON_VOLATILE_STORAGE:
case INITIALIZE:
diff --git a/tlv.h b/tlv.h
index 60c937db02ef..10ed301fdc04 100644
--- a/tlv.h
+++ b/tlv.h
@@ -79,6 +79,7 @@ enum management_action {
#define PRIMARY_DOMAIN 0x4002
#define TIME_STATUS_NP 0xC000
#define GRANDMASTER_SETTINGS_NP 0xC001
+#define SUBSCRIBE_EVENTS_NP 0xC003

/* Port management ID values */
#define NULL_MANAGEMENT 0x0000
@@ -196,6 +197,13 @@ struct port_ds_np {
Integer32 asCapable;
} PACKED;

+
+#define EVENT_BITMASK_CNT 32
+
+struct subscribe_events_np {
+ uint8_t bitmask[EVENT_BITMASK_CNT];
+} PACKED;
+
enum clock_type {
CLOCK_TYPE_ORDINARY = 0x8000,
CLOCK_TYPE_BOUNDARY = 0x4000,
--
1.7.6.5
Richard Cochran
2014-05-03 18:19:10 UTC
Permalink
Post by Jiri Benc
This puts groundwork for event subscription and notification. The individual
events are added by subsequent patches.
I like this patch, but I still would want a few changes,
mostly minor ones.
Post by Jiri Benc
---
clock.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
clock.h | 11 +++++
notification.h | 27 +++++++++++
tlv.c | 4 ++
tlv.h | 8 +++
5 files changed, 186 insertions(+), 0 deletions(-)
create mode 100644 notification.h
diff --git a/clock.c b/clock.c
index a85efdec2857..600fc058a391 100644
--- a/clock.c
+++ b/clock.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <time.h>
+#include "address.h"
#include "bmc.h"
#include "clock.h"
#include "clockadj.h"
@@ -59,6 +60,14 @@ struct clock_stats {
unsigned int max_count;
};
+struct clock_subscriber {
+ LIST_ENTRY(clock_subscriber) list;
+ uint8_t events[EVENT_BITMASK_CNT];
+ struct PortIdentity targetPortIdentity;
+ struct address addr;
+ UInteger16 sequenceId;
+};
+
struct clock {
clockid_t clkid;
struct servo *servo;
@@ -99,6 +108,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface uds_interface;
+ LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
};
struct clock the_clock;
@@ -110,9 +120,96 @@ static int cid_eq(struct ClockIdentity *a, struct ClockIdentity *b)
return 0 == memcmp(a, b, sizeof(*a));
}
+#ifndef LIST_FOREACH_SAFE
+#define LIST_FOREACH_SAFE(var, head, field, tvar) \
+ for ((var) = LIST_FIRST((head)); \
+ (var) && ((tvar) = LIST_NEXT((var), field), 1); \
+ (var) = (tvar))
+#endif
I think this extra macro is overkill, see below.

[ but it is really needed by the later patch. ]
Post by Jiri Benc
+
+static void remove_subscriber(struct clock_subscriber *s)
+{
+ LIST_REMOVE(s, list);
+ free(s);
+}
+
+static void clock_update_subscription(struct clock *c, struct ptp_message *req,
+ uint8_t *bitmask)
+{
+ struct clock_subscriber *s;
+ int i, remove = 1;
+
+ for (i = 0; i < EVENT_BITMASK_CNT; i++) {
+ if (bitmask[i]) {
+ remove = 0;
+ break;
+ }
+ }
+
+ LIST_FOREACH(s, &c->subscribers, list) {
+ if (!memcmp(&s->targetPortIdentity, &req->header.sourcePortIdentity,
+ sizeof(struct PortIdentity))) {
+ /* Found, update the transport address and event
+ * mask. */
+ if (!remove) {
+ s->addr = req->address;
+ memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ } else {
+ remove_subscriber(s);
+ }
+ return;
+ }
+ }
+ if (remove)
+ return;
+ /* Not present yet, add the subscriber. */
+ s = malloc(sizeof(*s));
+ if (!s) {
+ pr_err("failed to allocate memory for a subscriber");
+ return;
+ }
+ s->targetPortIdentity = req->header.sourcePortIdentity;
+ s->addr = req->address;
+ memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ s->sequenceId = 0;
+ LIST_INSERT_HEAD(&c->subscribers, s, list);
+}
+
+static void clock_flush_subscriptions(struct clock *c)
+{
+ struct clock_subscriber *s, *tmp;
+
+ LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) {
+ remove_subscriber(s);
+ }
You are removing every item in the list, and so can just do this...

while ((s = LIST_FIRST(&c->subscribers)) != NULL) {
LIST_REMOVE(s, list);
/* or */
remove_subscriber(s);
}

can't you?
Post by Jiri Benc
+}
+
+void clock_send_notification(struct clock *c, struct ptp_message *msg,
+ int msglen, enum notification event)
+{
+ unsigned int event_pos = event / 8;
+ uint8_t mask = 1 << (event % 8);
+ struct port *uds = c->port[c->nports];
+ struct clock_subscriber *s;
+
+ LIST_FOREACH(s, &c->subscribers, list) {
+ if (!(s->events[event_pos] & mask))
+ continue;
+ /* send event */
+ msg->header.sequenceId = htons(s->sequenceId);
+ s->sequenceId++;
+ msg->management.targetPortIdentity.clockIdentity = s->targetPortIdentity.clockIdentity;
+ msg->management.targetPortIdentity.portNumber = htons(s->targetPortIdentity.portNumber);
Lines too long.
Post by Jiri Benc
+ msg->address = s->addr;
+ port_forward_to(uds, msg);
+ }
+}
+
void clock_destroy(struct clock *c)
{
int i;
+
+ clock_flush_subscriptions(c);
for (i = 0; i < c->nports; i++) {
port_close(c->port[i]);
close(c->fault_fd[i]);
@@ -328,6 +425,40 @@ static int clock_management_set(struct clock *c, struct port *p,
return respond ? 1 : 0;
}
+static int clock_management_cmd_response(struct clock *c, struct port *p,
+ int id, struct ptp_message *req)
+{
+ int respond = 0;
+ struct ptp_message *rsp = NULL;
+ struct management_tlv *tlv;
+ struct subscribe_events_np *sen;
+
+ tlv = (struct management_tlv *)req->management.suffix;
+
+ switch (id) {
+ if (p != c->port[c->nports]) {
+ /* Only the UDS port allowed. */
+ break;
+ }
+ sen = (struct subscribe_events_np *)tlv->data;
+ clock_update_subscription(c, req, sen->bitmask);
+ respond = 1;
+ break;
+ }
+ if (respond) {
+ rsp = port_management_reply(port_identity(p), p, req);
+ if (!rsp) {
+ pr_err("failed to allocate response message");
+ return 1;
+ }
+ if (port_prepare_and_send(p, rsp, 0))
+ pr_err("failed to send response message");
+ msg_put(rsp);
+ }
+ return respond ? 1 : 0;
+}
+
static void clock_stats_update(struct clock_stats *s,
int64_t offset, double freq)
{
@@ -669,6 +800,8 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
clock_sync_interval(c, 0);
+ LIST_INIT(&c->subscribers);
+
for (i = 0; i < count; i++) {
c->port[i] = port_open(phc_index, timestamping, 1+i, &iface[i], c);
if (!c->port[i]) {
@@ -842,6 +975,8 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
return changed;
break;
+ if (clock_management_cmd_response(c, p, mgt->id, msg))
+ return changed;
So does this really have to be a COMMAND action? It feels to me more
like a SET action, configuring a per-client table of events.

None of the existing COMMAND actions send any data (except for that
useless initialization key) or configure anything. Instead, they are
some sort of imperative like "restart!" or "reset!"
Post by Jiri Benc
break;
return changed;
@@ -880,6 +1015,7 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
clock_management_send_error(p, msg, NOT_SUPPORTED);
break;
diff --git a/clock.h b/clock.h
index 804640bdb8f4..8718f2db715b 100644
--- a/clock.h
+++ b/clock.h
@@ -23,6 +23,7 @@
#include "dm.h"
#include "ds.h"
#include "config.h"
+#include "notification.h"
#include "servo.h"
#include "tlv.h"
#include "tmv.h"
@@ -134,6 +135,16 @@ void clock_install_fda(struct clock *c, struct port *p, struct fdarray fda);
int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg);
/**
+ * Send notification about an event to all subscribers.
+ */
+void clock_send_notification(struct clock *c, struct ptp_message *msg,
+ int msglen, enum notification event);
+
+/**
* Obtain a clock's parent data set.
diff --git a/notification.h b/notification.h
new file mode 100644
index 000000000000..57e7a7856360
--- /dev/null
+++ b/notification.h
@@ -0,0 +1,27 @@
+/**
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_NOTIFICATION_H
+#define HAVE_NOTIFICATION_H
+
+enum notification {
+ NOTIFY_DUMMY,
+};
+
+#endif
diff --git a/tlv.c b/tlv.c
index b8cdd3959c9b..430410f75397 100644
--- a/tlv.c
+++ b/tlv.c
@@ -242,6 +242,10 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
pdsnp->neighborPropDelayThresh = ntohl(pdsnp->neighborPropDelayThresh);
pdsnp->asCapable = ntohl(pdsnp->asCapable);
break;
+ if (data_len != sizeof(struct subscribe_events_np))
+ goto bad_length;
+ break;
diff --git a/tlv.h b/tlv.h
index 60c937db02ef..10ed301fdc04 100644
--- a/tlv.h
+++ b/tlv.h
@@ -79,6 +79,7 @@ enum management_action {
#define PRIMARY_DOMAIN 0x4002
#define TIME_STATUS_NP 0xC000
#define GRANDMASTER_SETTINGS_NP 0xC001
+#define SUBSCRIBE_EVENTS_NP 0xC003
/* Port management ID values */
#define NULL_MANAGEMENT 0x0000
@@ -196,6 +197,13 @@ struct port_ds_np {
Integer32 asCapable;
} PACKED;
+
+#define EVENT_BITMASK_CNT 32
Let's make this 64, for 512 event types.

Thanks,
Richard
Jiri Benc
2014-05-05 12:39:58 UTC
Permalink
Post by Richard Cochran
Post by Jiri Benc
+static void clock_flush_subscriptions(struct clock *c)
+{
+ struct clock_subscriber *s, *tmp;
+
+ LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) {
+ remove_subscriber(s);
+ }
You are removing every item in the list, and so can just do this...
while ((s = LIST_FIRST(&c->subscribers)) != NULL) {
LIST_REMOVE(s, list);
/* or */
remove_subscriber(s);
}
can't you?
As you said, we'll need LIST_FOREACH_SAFE anyway (and it's actually
already included in some sys/queue.h implementations). I think the way
I used makes clearer what's happening here. The additional variable will
likely be stored in a register by the compiler and this is not a
performance critical path anyway.
Post by Richard Cochran
Post by Jiri Benc
+}
+
+void clock_send_notification(struct clock *c, struct ptp_message *msg,
+ int msglen, enum notification event)
+{
+ unsigned int event_pos = event / 8;
+ uint8_t mask = 1 << (event % 8);
+ struct port *uds = c->port[c->nports];
+ struct clock_subscriber *s;
+
+ LIST_FOREACH(s, &c->subscribers, list) {
+ if (!(s->events[event_pos] & mask))
+ continue;
+ /* send event */
+ msg->header.sequenceId = htons(s->sequenceId);
+ s->sequenceId++;
+ msg->management.targetPortIdentity.clockIdentity = s->targetPortIdentity.clockIdentity;
+ msg->management.targetPortIdentity.portNumber = htons(s->targetPortIdentity.portNumber);
Lines too long.
I'll wrap them but the result will look uglier than this.
Post by Richard Cochran
Post by Jiri Benc
@@ -842,6 +975,8 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
return changed;
break;
+ if (clock_management_cmd_response(c, p, mgt->id, msg))
+ return changed;
So does this really have to be a COMMAND action? It feels to me more
like a SET action, configuring a per-client table of events.
The border between COMMAND and SET is fuzzy here. I don't have too
strong preference, except that I'd need to rewrite the set for the
4th time and retest it again :-/
Post by Richard Cochran
None of the existing COMMAND actions send any data (except for that
useless initialization key) or configure anything. Instead, they are
some sort of imperative like "restart!" or "reset!"
Except that the INITIALIZE command does have data. I thought about this
more like a "send me notifications!" command.

As a counter-argument, none of the existing SET actions act per-client,
they are used to set the (global) clock or port state.

In fact, neither of those fits. The closest thing in the standard is
unicast negotiation which uses a different TLV than management which is
not applicable here. I won't argue about this but I'm not thrilled
about rewriting and retesting this again for something that's just a
matter of taste.
Post by Richard Cochran
Post by Jiri Benc
+#define EVENT_BITMASK_CNT 32
Let's make this 64, for 512 event types.
Okay, whatever.

Jiri
--
Jiri Benc
Richard Cochran
2014-05-05 14:25:14 UTC
Permalink
Post by Jiri Benc
I'll wrap them but the result will look uglier than this.
You version looks like this:

msg->management.targetPortIdentity.clockIdentity = s->targetPortIdentity.clockIdentity;
msg->management.targetPortIdentity.portNumber = htons(s->targetPortIdentity.portNumber);

Those are 103 and 104 columns. In contrast

msg->management.targetPortIdentity.clockIdentity =
s->targetPortIdentity.clockIdentity;
msg->management.targetPortIdentity.portNumber =
htons(s->targetPortIdentity.portNumber);

doesn't look so ugly to me. (Try an 80x25 xterm.)
Post by Jiri Benc
The border between COMMAND and SET is fuzzy here. I don't have too
strong preference, except that I'd need to rewrite the set for the
4th time and retest it again :-/
Sorry for being a pain, but I really don't want to have any COMMANDs.
Post by Jiri Benc
Post by Richard Cochran
None of the existing COMMAND actions send any data (except for that
useless initialization key) or configure anything. Instead, they are
some sort of imperative like "restart!" or "reset!"
Except that the INITIALIZE command does have data.
It has a key with *one* allowed value. That is virtually the same as
not having any data. The information content is zero.
Post by Jiri Benc
In fact, neither of those fits. The closest thing in the standard is
unicast negotiation which uses a different TLV than management which is
not applicable here. I won't argue about this but I'm not thrilled
about rewriting and retesting this again for something that's just a
matter of taste.
You are right that the signal TLV is closest. I wouldn't object to
having this TLV be a signal message instead. (I am planning on
implementing unicast negotiation.) But if it is a management message,
it really doesn't match the other commands.

- SAVE_IN_NON_VOLATILE_STORAGE
- RESET_NON_VOLATILE_STORAGE
- INITIALIZE
- FAULT_LOG_RESET
- ENABLE_PORT
- DISABLE_PORT

If you can't find the time to reform this patch, then I'll do it, if
you want. That won't spare you any testing, however.

Thanks,
Richard

Jiri Benc
2014-05-02 10:37:49 UTC
Permalink
The standard requires management TLV in replies to commands:

An acknowledge management message is a response to a command
management message. The value of the managementId shall be identical
to that in the command message.
(Table 38)

Just copy the TLV from the request.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 3 +++
msg.c | 17 +++++++++++++++++
msg.h | 11 +++++++++++
3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/clock.c b/clock.c
index 600fc058a391..c66a04064ff8 100644
--- a/clock.c
+++ b/clock.c
@@ -18,6 +18,7 @@
*/
#include <errno.h>
#include <poll.h>
+#include <stddef.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
@@ -452,6 +453,8 @@ static int clock_management_cmd_response(struct clock *c, struct port *p,
pr_err("failed to allocate response message");
return 1;
}
+ msg_copy_tlv(rsp, offsetof(struct ptp_message, management.suffix),
+ req, 0);
if (port_prepare_and_send(p, rsp, 0))
pr_err("failed to send response message");
msg_put(rsp);
diff --git a/msg.c b/msg.c
index 7edbdd2619b5..7b958e30bef9 100644
--- a/msg.c
+++ b/msg.c
@@ -470,3 +470,20 @@ int msg_sots_missing(struct ptp_message *m)
}
return (!m->hwts.ts.tv_sec && !m->hwts.ts.tv_nsec) ? 1 : 0;
}
+
+void msg_copy_tlv(struct ptp_message *dest, unsigned int dest_base,
+ struct ptp_message *src, unsigned int src_base)
+{
+ unsigned int len;
+
+ if (!src_base)
+ src_base = dest_base;
+
+ len = src->header.messageLength;
+ if (len <= src_base)
+ return;
+ len -= src_base;
+ memcpy((void *)dest + dest_base, (void *)src + src_base, len);
+ dest->header.messageLength = dest_base + len;
+ dest->tlv_count = src->tlv_count;
+}
diff --git a/msg.h b/msg.h
index 3fa58338f07a..4a58f065b214 100644
--- a/msg.h
+++ b/msg.h
@@ -339,6 +339,17 @@ void msg_put(struct ptp_message *m);
int msg_sots_missing(struct ptp_message *m);

/**
+ * Copy TLVs from one message into another.
+ * @param dest Destination message.
+ * @param dest_base Offset of the suffix in the destination message.
+ * @param src Source message.
+ * @param src_base Offset of the suffix in the source message; 0 to use
+ * the value in dest_base.
+ */
+void msg_copy_tlv(struct ptp_message *dest, unsigned int dest_base,
+ struct ptp_message *src, unsigned int src_base);
+
+/**
* Work around buggy 802.1AS switches.
*/
extern int assume_two_step;
--
1.7.6.5
Richard Cochran
2014-05-03 18:22:31 UTC
Permalink
Post by Jiri Benc
An acknowledge management message is a response to a command
management message. The value of the managementId shall be identical
to that in the command message.
(Table 38)
If we make a SET action instead, then we won't need this patch.

[ I have no plans to implement any of the standard COMMAND actions, as
they are so useless. ]

Thanks,
Richard
Jiri Benc
2014-05-02 10:37:50 UTC
Permalink
Add expiration time to subscriptions; they need to be renewed before they
expiry. This way, the subscription automatically times out when phc2sys is
killed.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 27 +++++++++++++++++++++++++--
tlv.c | 8 ++++++++
tlv.h | 1 +
3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/clock.c b/clock.c
index c66a04064ff8..29d58b0c2461 100644
--- a/clock.c
+++ b/clock.c
@@ -67,6 +67,7 @@ struct clock_subscriber {
struct PortIdentity targetPortIdentity;
struct address addr;
UInteger16 sequenceId;
+ time_t expiration;
};

struct clock {
@@ -135,10 +136,11 @@ static void remove_subscriber(struct clock_subscriber *s)
}

static void clock_update_subscription(struct clock *c, struct ptp_message *req,
- uint8_t *bitmask)
+ uint8_t *bitmask, unsigned int valid_time)
{
struct clock_subscriber *s;
int i, remove = 1;
+ struct timespec now;

for (i = 0; i < EVENT_BITMASK_CNT; i++) {
if (bitmask[i]) {
@@ -155,6 +157,8 @@ static void clock_update_subscription(struct clock *c, struct ptp_message *req,
if (!remove) {
s->addr = req->address;
memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ s->expiration = now.tv_sec + valid_time;
} else {
remove_subscriber(s);
}
@@ -172,6 +176,8 @@ static void clock_update_subscription(struct clock *c, struct ptp_message *req,
s->targetPortIdentity = req->header.sourcePortIdentity;
s->addr = req->address;
memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ s->expiration = now.tv_sec + valid_time;
s->sequenceId = 0;
LIST_INSERT_HEAD(&c->subscribers, s, list);
}
@@ -185,6 +191,21 @@ static void clock_flush_subscriptions(struct clock *c)
}
}

+static void clock_prune_subscriptions(struct clock *c)
+{
+ struct clock_subscriber *s, *tmp;
+ struct timespec now;
+
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) {
+ if (s->expiration <= now.tv_sec) {
+ pr_info("subscriber %s timed out",
+ pid2str(&s->targetPortIdentity));
+ remove_subscriber(s);
+ }
+ }
+}
+
void clock_send_notification(struct clock *c, struct ptp_message *msg,
int msglen, enum notification event)
{
@@ -443,7 +464,8 @@ static int clock_management_cmd_response(struct clock *c, struct port *p,
break;
}
sen = (struct subscribe_events_np *)tlv->data;
- clock_update_subscription(c, req, sen->bitmask);
+ clock_update_subscription(c, req, sen->bitmask,
+ sen->valid_time);
respond = 1;
break;
}
@@ -1108,6 +1130,7 @@ int clock_poll(struct clock *c)
if (sde)
handle_state_decision_event(c);

+ clock_prune_subscriptions(c);
return 0;
}

diff --git a/tlv.c b/tlv.c
index 430410f75397..60df3575b5f0 100644
--- a/tlv.c
+++ b/tlv.c
@@ -62,6 +62,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct port_ds_np *pdsnp;
struct time_status_np *tsn;
struct grandmaster_settings_np *gsn;
+ struct subscribe_events_np *sen;
struct mgmt_clock_description *cd;
int extra_len = 0, len;
uint8_t *buf;
@@ -245,6 +246,8 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
case SUBSCRIBE_EVENTS_NP:
if (data_len != sizeof(struct subscribe_events_np))
goto bad_length;
+ sen = (struct subscribe_events_np *)m->data;
+ sen->valid_time = ntohs(sen->valid_time);
break;
case SAVE_IN_NON_VOLATILE_STORAGE:
case RESET_NON_VOLATILE_STORAGE:
@@ -277,6 +280,7 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
struct port_ds_np *pdsnp;
struct time_status_np *tsn;
struct grandmaster_settings_np *gsn;
+ struct subscribe_events_np *sen;
struct mgmt_clock_description *cd;
switch (m->id) {
case CLOCK_DESCRIPTION:
@@ -341,6 +345,10 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
pdsnp->neighborPropDelayThresh = htonl(pdsnp->neighborPropDelayThresh);
pdsnp->asCapable = htonl(pdsnp->asCapable);
break;
+ case SUBSCRIBE_EVENTS_NP:
+ sen = (struct subscribe_events_np *)m->data;
+ sen->valid_time = htons(sen->valid_time);
+ break;
}
}

diff --git a/tlv.h b/tlv.h
index 10ed301fdc04..c383c822684b 100644
--- a/tlv.h
+++ b/tlv.h
@@ -201,6 +201,7 @@ struct port_ds_np {
#define EVENT_BITMASK_CNT 32

struct subscribe_events_np {
+ uint16_t valid_time; /* seconds */
uint8_t bitmask[EVENT_BITMASK_CNT];
} PACKED;
--
1.7.6.5
Richard Cochran
2014-05-03 18:32:01 UTC
Permalink
Post by Jiri Benc
Add expiration time to subscriptions; they need to be renewed before they
expiry. This way, the subscription automatically times out when phc2sys is
killed.
---
clock.c | 27 +++++++++++++++++++++++++--
tlv.c | 8 ++++++++
tlv.h | 1 +
3 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/clock.c b/clock.c
index c66a04064ff8..29d58b0c2461 100644
--- a/clock.c
+++ b/clock.c
@@ -67,6 +67,7 @@ struct clock_subscriber {
struct PortIdentity targetPortIdentity;
struct address addr;
UInteger16 sequenceId;
+ time_t expiration;
};
struct clock {
@@ -135,10 +136,11 @@ static void remove_subscriber(struct clock_subscriber *s)
}
static void clock_update_subscription(struct clock *c, struct ptp_message *req,
- uint8_t *bitmask)
+ uint8_t *bitmask, unsigned int valid_time)
When reading this patch, I was confused by the name "valid_time" ...
Post by Jiri Benc
struct subscribe_events_np {
+ uint16_t valid_time; /* seconds */
uint8_t bitmask[EVENT_BITMASK_CNT];
} PACKED;
I think "duration" would be a better name. Also I have two questions:

1. It is a good idea to let the client set the duration?

2. Should we perhaps use a 32 bit field for this?
As is, it allows only 18 hours.

Thanks,
Richard
Jiri Benc
2014-05-05 12:54:04 UTC
Permalink
Post by Richard Cochran
1. It is a good idea to let the client set the duration?
You wanted to get rid of the magic constant in ptp4l, this allowed me to
move it to phc2sys ;-)

Okay, seriously: I don't think this is a problem. It is limited to UDS
and the worst thing that can happen is attempting to send notifications
to a client when the client is long dead because it set too big value
and crashed.

As it is the client who knows when it will be able to renew the
subscription, it makes sense to be the client who sets the duration.

Obviously, the hyper correct way would be for the server to ack or deny
the request based on the duration value and internal policy, similarly
to e.g. bandwidth allocation in various protocols. I think it's
overkill for this case.
Post by Richard Cochran
2. Should we perhaps use a 32 bit field for this?
As is, it allows only 18 hours.
I even thought about limiting the maximum value to a hour or so. The
client is supposed to renew the subscription, otherwise the time limit
would be useless.

Jiri
--
Jiri Benc
Richard Cochran
2014-05-05 14:03:47 UTC
Permalink
Post by Jiri Benc
I even thought about limiting the maximum value to a hour or so. The
client is supposed to renew the subscription, otherwise the time limit
would be useless.
Fair enough.

Thanks,
Richard
Jiri Benc
2014-05-02 10:37:52 UTC
Permalink
Split management message creation to more fine-grained functions to allow
notification messages to be created.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----------
clock.h | 8 ++++++++
port.c | 8 +++++++-
port.h | 14 ++++++++++++++
4 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/clock.c b/clock.c
index 29d58b0c2461..e585074daea3 100644
--- a/clock.c
+++ b/clock.c
@@ -290,22 +290,16 @@ static void clock_management_send_error(struct port *p,
pr_err("failed to send management error status");
}

-static int clock_management_get_response(struct clock *c, struct port *p,
- int id, struct ptp_message *req)
+static int clock_management_fill_response(struct clock *c,
+ struct ptp_message *rsp, int id)
{
int datalen = 0, respond = 0;
struct management_tlv *tlv;
struct management_tlv_datum *mtd;
- struct ptp_message *rsp;
struct time_status_np *tsn;
struct grandmaster_settings_np *gsn;
- struct PortIdentity pid = port_identity(p);
struct PTPText *text;

- rsp = port_management_reply(pid, p, req);
- if (!rsp) {
- return 0;
- }
tlv = (struct management_tlv *) rsp->management.suffix;
tlv->type = TLV_MANAGEMENT;
tlv->id = id;
@@ -416,10 +410,26 @@ static int clock_management_get_response(struct clock *c, struct port *p,
tlv->length = sizeof(tlv->id) + datalen;
rsp->header.messageLength += sizeof(*tlv) + datalen;
rsp->tlv_count = 1;
- port_prepare_and_send(p, rsp, 0);
}
+ return respond;
+}
+
+static int clock_management_get_response(struct clock *c, struct port *p,
+ int id, struct ptp_message *req)
+{
+ struct PortIdentity pid = port_identity(p);
+ struct ptp_message *rsp;
+ int respond;
+
+ rsp = port_management_reply(pid, p, req);
+ if (!rsp) {
+ return 0;
+ }
+ respond = clock_management_fill_response(c, rsp, id);
+ if (respond)
+ port_prepare_and_send(p, rsp, 0);
msg_put(rsp);
- return respond ? 1 : 0;
+ return respond;
}

static int clock_management_set(struct clock *c, struct port *p,
@@ -1062,6 +1072,34 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
return changed;
}

+void clock_notify_event(struct clock *c, enum notification event)
+{
+ struct port *uds = c->port[c->nports];
+ struct PortIdentity pid = port_identity(uds);
+ struct ptp_message *msg;
+ UInteger16 msg_len;
+ int id;
+
+ switch (event) {
+ /* set id */
+ default:
+ return;
+ }
+ /* targetPortIdentity and sequenceId will be filled by
+ * clock_send_notification */
+ msg = port_management_notify(pid, uds);
+ if (!msg)
+ return;
+ if (!clock_management_fill_response(c, msg, id))
+ goto err;
+ msg_len = msg->header.messageLength;
+ if (msg_pre_send(msg))
+ goto err;
+ clock_send_notification(c, msg, msg_len, event);
+err:
+ msg_put(msg);
+}
+
struct parent_ds *clock_parent_ds(struct clock *c)
{
return &c->dad;
diff --git a/clock.h b/clock.h
index 8718f2db715b..92ec163d962f 100644
--- a/clock.h
+++ b/clock.h
@@ -145,6 +145,14 @@ void clock_send_notification(struct clock *c, struct ptp_message *msg,
int msglen, enum notification event);

/**
+ * Construct and send notification to subscribers about an event that
+ * occured on the clock.
+ * @param c The clock instance.
+ * @param event The identification of the event.
+ */
+void clock_notify_event(struct clock *c, enum notification event);
+
+/**
* Obtain a clock's parent data set.
* @param c The clock instance.
* @return A pointer to the parent data set of the clock.
diff --git a/port.c b/port.c
index d48d275ba1a3..29e98ceb66ce 100644
--- a/port.c
+++ b/port.c
@@ -2336,6 +2336,12 @@ struct ptp_message *port_management_reply(struct PortIdentity pid,
management_action(req));
}

+struct ptp_message *port_management_notify(struct PortIdentity pid,
+ struct port *port)
+{
+ return port_management_construct(pid, port, 0, NULL, 1, GET);
+}
+
void port_notify_event(struct port *p, enum notification event)
{
struct PortIdentity pid = port_identity(p);
@@ -2350,7 +2356,7 @@ void port_notify_event(struct port *p, enum notification event)
}
/* targetPortIdentity and sequenceId will be filled by
* clock_send_notification */
- msg = port_management_construct(pid, p, 0, NULL, 1, GET);
+ msg = port_management_notify(pid, p);
if (!msg)
return;
if (!port_management_fill_response(p, msg, id))
diff --git a/port.h b/port.h
index 34c18a3e2d0c..a1b8ad795040 100644
--- a/port.h
+++ b/port.h
@@ -157,6 +157,20 @@ struct ptp_message *port_management_reply(struct PortIdentity pid,
struct ptp_message *req);

/**
+ * Allocate a standalone reply management message.
+ *
+ * See note in @ref port_management_reply description about freeing the
+ * message. Also note that the constructed message does not have
+ * targetPortIdentity and sequenceId filled.
+ *
+ * @param pid The id of the responding port.
+ * @param port The port to which the message will be sent.
+ * @return Pointer to a message on success, NULL otherwise.
+ */
+struct ptp_message *port_management_notify(struct PortIdentity pid,
+ struct port *port);
+
+/**
* Construct and send notification to subscribers about an event that
* occured on the port.
* @param p The port.
--
1.7.6.5
Jiri Benc
2014-05-02 10:37:51 UTC
Permalink
Split management message creation to more fine-grained functions to allow
notification messages to be created.

Signed-off-by: Jiri Benc <***@redhat.com>
---
port.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------
port.h | 9 ++++++
2 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/port.c b/port.c
index d5650ac7172e..d48d275ba1a3 100644
--- a/port.c
+++ b/port.c
@@ -603,26 +603,19 @@ static void port_management_send_error(struct port *p, struct port *ingress,
static const Octet profile_id_drr[] = {0x00, 0x1B, 0x19, 0x00, 0x01, 0x00};
static const Octet profile_id_p2p[] = {0x00, 0x1B, 0x19, 0x00, 0x02, 0x00};

-static int port_management_get_response(struct port *target,
- struct port *ingress, int id,
- struct ptp_message *req)
+static int port_management_fill_response(struct port *target,
+ struct ptp_message *rsp, int id)
{
int datalen = 0, respond = 0;
struct management_tlv *tlv;
struct management_tlv_datum *mtd;
- struct ptp_message *rsp;
struct portDS *pds;
struct port_ds_np *pdsnp;
- struct PortIdentity pid = port_identity(target);
struct clock_description *desc;
struct mgmt_clock_description *cd;
uint8_t *buf;
uint16_t u16;

- rsp = port_management_reply(pid, ingress, req);
- if (!rsp) {
- return 0;
- }
tlv = (struct management_tlv *) rsp->management.suffix;
tlv->type = TLV_MANAGEMENT;
tlv->id = id;
@@ -776,10 +769,27 @@ static int port_management_get_response(struct port *target,
tlv->length = sizeof(tlv->id) + datalen;
rsp->header.messageLength += sizeof(*tlv) + datalen;
rsp->tlv_count = 1;
- port_prepare_and_send(ingress, rsp, 0);
}
+ return respond;
+}
+
+static int port_management_get_response(struct port *target,
+ struct port *ingress, int id,
+ struct ptp_message *req)
+{
+ struct PortIdentity pid = port_identity(target);
+ struct ptp_message *rsp;
+ int respond;
+
+ rsp = port_management_reply(pid, ingress, req);
+ if (!rsp) {
+ return 0;
+ }
+ respond = port_management_fill_response(target, rsp, id);
+ if (respond)
+ port_prepare_and_send(ingress, rsp, 0);
msg_put(rsp);
- return respond ? 1 : 0;
+ return respond;
}

static int port_management_set(struct port *target,
@@ -2271,9 +2281,11 @@ int port_management_error(struct PortIdentity pid, struct port *ingress,
return err;
}

-struct ptp_message *port_management_reply(struct PortIdentity pid,
- struct port *ingress,
- struct ptp_message *req)
+static struct ptp_message *
+port_management_construct(struct PortIdentity pid, struct port *ingress,
+ UInteger16 sequenceId,
+ struct PortIdentity *targetPortIdentity,
+ UInteger8 boundaryHops, uint8_t action)
{
struct ptp_message *msg;
int pdulen;
@@ -2290,16 +2302,16 @@ struct ptp_message *port_management_reply(struct PortIdentity pid,
msg->header.messageLength = pdulen;
msg->header.domainNumber = clock_domain_number(ingress->clock);
msg->header.sourcePortIdentity = pid;
- msg->header.sequenceId = req->header.sequenceId;
+ msg->header.sequenceId = sequenceId;
msg->header.control = CTL_MANAGEMENT;
msg->header.logMessageInterval = 0x7f;

- msg->management.targetPortIdentity = req->header.sourcePortIdentity;
- msg->management.startingBoundaryHops =
- req->management.startingBoundaryHops - req->management.boundaryHops;
- msg->management.boundaryHops = msg->management.startingBoundaryHops;
+ if (targetPortIdentity)
+ msg->management.targetPortIdentity = *targetPortIdentity;
+ msg->management.startingBoundaryHops = boundaryHops;
+ msg->management.boundaryHops = boundaryHops;

- switch (management_action(req)) {
+ switch (action) {
case GET: case SET:
msg->management.flags = RESPONSE;
break;
@@ -2310,6 +2322,47 @@ struct ptp_message *port_management_reply(struct PortIdentity pid,
return msg;
}

+struct ptp_message *port_management_reply(struct PortIdentity pid,
+ struct port *ingress,
+ struct ptp_message *req)
+{
+ UInteger8 boundaryHops;
+
+ boundaryHops = req->management.startingBoundaryHops - req->management.boundaryHops;
+ return port_management_construct(pid, ingress,
+ req->header.sequenceId,
+ &req->header.sourcePortIdentity,
+ boundaryHops,
+ management_action(req));
+}
+
+void port_notify_event(struct port *p, enum notification event)
+{
+ struct PortIdentity pid = port_identity(p);
+ struct ptp_message *msg;
+ UInteger16 msg_len;
+ int id;
+
+ switch (event) {
+ /* set id */
+ default:
+ return;
+ }
+ /* targetPortIdentity and sequenceId will be filled by
+ * clock_send_notification */
+ msg = port_management_construct(pid, p, 0, NULL, 1, GET);
+ if (!msg)
+ return;
+ if (!port_management_fill_response(p, msg, id))
+ goto err;
+ msg_len = msg->header.messageLength;
+ if (msg_pre_send(msg))
+ goto err;
+ clock_send_notification(p->clock, msg, msg_len, event);
+err:
+ msg_put(msg);
+}
+
struct port *port_open(int phc_index,
enum timestamp_type timestamping,
int number,
diff --git a/port.h b/port.h
index 34103a6a24ee..34c18a3e2d0c 100644
--- a/port.h
+++ b/port.h
@@ -23,6 +23,7 @@
#include "fd.h"
#include "foreign.h"
#include "fsm.h"
+#include "notification.h"
#include "transport.h"

/* forward declarations */
@@ -156,6 +157,14 @@ struct ptp_message *port_management_reply(struct PortIdentity pid,
struct ptp_message *req);

/**
+ * Construct and send notification to subscribers about an event that
+ * occured on the port.
+ * @param p The port.
+ * @param event The identification of the event.
+ */
+void port_notify_event(struct port *p, enum notification event);
+
+/**
* Open a network port.
* @param phc_index The PHC device index for the network device.
* @param timestamping The timestamping mode for this port.
--
1.7.6.5
Richard Cochran
2014-05-03 18:49:16 UTC
Permalink
Post by Jiri Benc
Split management message creation to more fine-grained functions to allow
notification messages to be created.
I must admit I didn't understand what you did, at first reading. Can
you lose a few words about how you split the code and why?

Is it that the 'fill' method is shared by regular management replies
and by the event notifiers?

...
Post by Jiri Benc
+struct ptp_message *port_management_reply(struct PortIdentity pid,
+ struct port *ingress,
+ struct ptp_message *req)
+{
+ UInteger8 boundaryHops;
+
+ boundaryHops = req->management.startingBoundaryHops - req->management.boundaryHops;
Line too long.

Thanks,
Richard
Jiri Benc
2014-05-05 13:00:51 UTC
Permalink
Post by Richard Cochran
Post by Jiri Benc
Split management message creation to more fine-grained functions to allow
notification messages to be created.
I must admit I didn't understand what you did, at first reading. Can
you lose a few words about how you split the code and why?
Is it that the 'fill' method is shared by regular management replies
and by the event notifiers?
Yes. And I wanted to avoid code duplication, thus I extracted the
generic part of port_management_get_response to a separate function.

The new port_management_fill_response is called from
port_management_get_response (so the function behaves exactly the same
as before this patch) and from a new port_notify_event function. The
difference is port_management_get_response uses the request message to
construct the reply message, while port_notify_event constructs the
reply message based on the notification id.
Post by Richard Cochran
Post by Jiri Benc
+struct ptp_message *port_management_reply(struct PortIdentity pid,
+ struct port *ingress,
+ struct ptp_message *req)
+{
+ UInteger8 boundaryHops;
+
+ boundaryHops = req->management.startingBoundaryHops - req->management.boundaryHops;
Line too long.
Will split it.

Jiri
--
Jiri Benc
Jiri Benc
2014-05-02 10:37:53 UTC
Permalink
Notify subscribers about port state changes.

Signed-off-by: Jiri Benc <***@redhat.com>
---
notification.h | 2 +-
port.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/notification.h b/notification.h
index 57e7a7856360..47c9b56c4f7e 100644
--- a/notification.h
+++ b/notification.h
@@ -21,7 +21,7 @@
#define HAVE_NOTIFICATION_H

enum notification {
- NOTIFY_DUMMY,
+ NOTIFY_PORT_STATE,
};

#endif
diff --git a/port.c b/port.c
index 29e98ceb66ce..64c01be127e3 100644
--- a/port.c
+++ b/port.c
@@ -2038,6 +2038,7 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
if (next == PS_LISTENING && p->delayMechanism == DM_P2P) {
port_set_delay_tmo(p);
}
+ port_notify_event(p, NOTIFY_PORT_STATE);
return 1;
}

@@ -2053,6 +2054,7 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
}

p->state = next;
+ port_notify_event(p, NOTIFY_PORT_STATE);
return 0;
}

@@ -2350,7 +2352,9 @@ void port_notify_event(struct port *p, enum notification event)
int id;

switch (event) {
- /* set id */
+ case NOTIFY_PORT_STATE:
+ id = PORT_DATA_SET;
+ break;
default:
return;
}
--
1.7.6.5
Jiri Benc
2014-05-02 10:37:54 UTC
Permalink
Will be used by phc2sys to find out interfaces corresponding to ports.

Signed-off-by: Jiri Benc <***@redhat.com>
---
clock.c | 9 +++++++++
port.c | 13 +++++++++++++
tlv.c | 14 ++++++++++++++
tlv.h | 8 ++++++++
4 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/clock.c b/clock.c
index e585074daea3..db96cb3f4b8e 100644
--- a/clock.c
+++ b/clock.c
@@ -1018,6 +1018,15 @@ int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
}

switch (mgt->id) {
+ case PORT_PROPERTIES_NP:
+ if (p != c->port[c->nports]) {
+ /* Only the UDS port allowed. */
+ clock_management_send_error(p, msg, NOT_SUPPORTED);
+ return 0;
+ }
+ }
+
+ switch (mgt->id) {
case USER_DESCRIPTION:
case SAVE_IN_NON_VOLATILE_STORAGE:
case RESET_NON_VOLATILE_STORAGE:
diff --git a/port.c b/port.c
index 64c01be127e3..b6abd0eb7fa3 100644
--- a/port.c
+++ b/port.c
@@ -611,6 +611,7 @@ static int port_management_fill_response(struct port *target,
struct management_tlv_datum *mtd;
struct portDS *pds;
struct port_ds_np *pdsnp;
+ struct port_properties_np *ppn;
struct clock_description *desc;
struct mgmt_clock_description *cd;
uint8_t *buf;
@@ -760,6 +761,18 @@ static int port_management_fill_response(struct port *target,
datalen = sizeof(*pdsnp);
respond = 1;
break;
+ case PORT_PROPERTIES_NP:
+ ppn = (struct port_properties_np *)tlv->data;
+ ppn->portIdentity = target->portIdentity;
+ if (target->state == PS_GRAND_MASTER)
+ ppn->port_state = PS_MASTER;
+ else
+ ppn->port_state = target->state;
+ ppn->timestamping = target->timestamping;
+ ptp_text_set(&ppn->interface, target->name);
+ datalen = sizeof(*ppn) + ppn->interface.length;
+ respond = 1;
+ break;
}
if (respond) {
if (datalen % 2) {
diff --git a/tlv.c b/tlv.c
index 60df3575b5f0..d8c680f4640f 100644
--- a/tlv.c
+++ b/tlv.c
@@ -63,6 +63,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct time_status_np *tsn;
struct grandmaster_settings_np *gsn;
struct subscribe_events_np *sen;
+ struct port_properties_np *ppn;
struct mgmt_clock_description *cd;
int extra_len = 0, len;
uint8_t *buf;
@@ -249,6 +250,14 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
sen = (struct subscribe_events_np *)m->data;
sen->valid_time = ntohs(sen->valid_time);
break;
+ case PORT_PROPERTIES_NP:
+ if (data_len < sizeof(struct port_properties_np))
+ goto bad_length;
+ ppn = (struct port_properties_np *)m->data;
+ ppn->portIdentity.portNumber = ntohs(ppn->portIdentity.portNumber);
+ extra_len = sizeof(struct port_properties_np);
+ extra_len += ppn->interface.length;
+ break;
case SAVE_IN_NON_VOLATILE_STORAGE:
case RESET_NON_VOLATILE_STORAGE:
case INITIALIZE:
@@ -281,6 +290,7 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
struct time_status_np *tsn;
struct grandmaster_settings_np *gsn;
struct subscribe_events_np *sen;
+ struct port_properties_np *ppn;
struct mgmt_clock_description *cd;
switch (m->id) {
case CLOCK_DESCRIPTION:
@@ -349,6 +359,10 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
sen = (struct subscribe_events_np *)m->data;
sen->valid_time = htons(sen->valid_time);
break;
+ case PORT_PROPERTIES_NP:
+ ppn = (struct port_properties_np *)m->data;
+ ppn->portIdentity.portNumber = htons(ppn->portIdentity.portNumber);
+ break;
}
}

diff --git a/tlv.h b/tlv.h
index c383c822684b..8d52a4224781 100644
--- a/tlv.h
+++ b/tlv.h
@@ -100,6 +100,7 @@ enum management_action {
#define DELAY_MECHANISM 0x6000
#define LOG_MIN_PDELAY_REQ_INTERVAL 0x6001
#define PORT_DATA_SET_NP 0xC002
+#define PORT_PROPERTIES_NP 0xC004

/* Management error ID values */
#define RESPONSE_TOO_BIG 0x0001
@@ -205,6 +206,13 @@ struct subscribe_events_np {
uint8_t bitmask[EVENT_BITMASK_CNT];
} PACKED;

+struct port_properties_np {
+ struct PortIdentity portIdentity;
+ uint8_t port_state;
+ uint8_t timestamping;
+ struct PTPText interface;
+} PACKED;
+
enum clock_type {
CLOCK_TYPE_ORDINARY = 0x8000,
CLOCK_TYPE_BOUNDARY = 0x4000,
--
1.7.6.5
Richard Cochran
2014-05-03 13:39:10 UTC
Permalink
Post by Jiri Benc
When phc2sys is started before ptp4l or it is interrupted before ptp4l has
a chance to reply to its query, the "uds: sendto failed: Connection refused"
message is output. This is not an interesting message.
Also, don't output the "failed to send message" error from pmc_send, as
all transports output errors in their send routine.
---
Applied.

Thanks,
Richard
Loading...