On Tue, Nov 10, 2020 at 02:21:42PM -0800, Richard Cochran wrote:
> This patch renames the function to have the module prefix and tries to
> put into words what it does.
Try, well said.
>
> Signed-off-by: Richard Cochran <richardcoch...@gmail.com>
> ---
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 528d4ee..94ef9ca 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -336,33 +336,6 @@ int run_pmc_clock_identity(struct pmc_agent *node, int
> timeout)
> return 1;
> }
>
> -/* Returns: -1 in case of error, 0 otherwise */
> -int update_pmc_node(struct pmc_agent *node)
> -{
> - struct timespec tp;
> - uint64_t ts;
> -
> - if (!node->pmc) {
> - return 0;
> - }
> - if (clock_gettime(CLOCK_MONOTONIC, &tp)) {
> - pr_err("failed to read clock: %m");
> - return -1;
> - }
> - ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
> -
> - if (ts - node->pmc_last_update >= PMC_UPDATE_INTERVAL) {
> - if (node->subscription_active) {
> - renew_subscription(node, 0);
> - }
> - if (run_pmc_get_utc_offset(node, 0) > 0) {
> - node->pmc_last_update = ts;
> - }
> - }
> -
> - return 0;
> -}
> -
> int init_pmc_node(struct config *cfg, struct pmc_agent *node, const char
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context)
> {
> @@ -414,6 +387,32 @@ int pmc_agent_subscribe(struct pmc_agent *node, int
> timeout)
> return renew_subscription(node, timeout);
> }
>
> +int pmc_agent_update(struct pmc_agent *node)
> +{
> + struct timespec tp;
> + uint64_t ts;
> +
> + if (!node->pmc) {
> + return 0;
> + }
> + if (clock_gettime(CLOCK_MONOTONIC, &tp)) {
> + pr_err("failed to read clock: %m");
> + return -errno;
This little change here snuck in.
> + }
> + ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
> +
> + if (ts - node->pmc_last_update >= PMC_UPDATE_INTERVAL) {
> + if (node->subscription_active) {
> + renew_subscription(node, 0);
> + }
> + if (run_pmc_get_utc_offset(node, 0) > 0) {
> + node->pmc_last_update = ts;
> + }
> + }
> +
> + return 0;
> +}
> +
> bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent)
> {
> return agent->utc_offset_traceable;
> diff --git a/pmc_agent.h b/pmc_agent.h
> index e4d3c3c..0466076 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -33,7 +33,6 @@ typedef int pmc_node_recv_subscribed_t(void *context,
> struct ptp_message *msg,
>
> int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context);
> -int update_pmc_node(struct pmc_agent *agent);
> int run_pmc_clock_identity(struct pmc_agent *agent, int timeout);
> int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
> int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
> @@ -85,6 +84,22 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent,
> int offset);
> */
> int pmc_agent_subscribe(struct pmc_agent *agent, int timeout);
>
> +/**
> + * Queries the local ptp4l instance to update the TAI-UTC offset and
> + * the current leap adjustment.
Do you know exactly why we retrieve the UTC offset implicitly and not
explicitly, considering that phc2sys already calls run_pmc_get_utc_offset
enough times directly already?
Does the PTP management protocol require that we send some dummy
keepalive requests or something?
> + *
> + * In addition:
> + *
> + * - Any active port state subscription will be renewed.
> + * - The port state notification callback might be invoked.
Which one is that? You mean ->recv_subscribed?
"Might" be called? That's the best we can say right now, right. Asking
"why" it would be called might be too deep of a question.
> + *
> + * Note that the PMC agent rate limits the query to once per minute.
Somehow the description doesn't say the most important thing from a user
perspective. Could you please mention that this needs to be called
periodically to avoid expiry of the subscription? Maybe move this
comment from where it is right now:
#define PMC_UPDATE_INTERVAL (60 * NS_PER_SEC)
#define PMC_SUBSCRIBE_DURATION 180 /* 3 minutes */
/* Note that PMC_SUBSCRIBE_DURATION has to be longer than
* PMC_UPDATE_INTERVAL otherwise subscription will time out before it is
* renewed.
*/
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
