On Mon, Apr 11, 2022 at 03:33:38PM +0800, Hangbin Liu wrote:
> Latest Linux kernel has supported getting PHC from bond directly. This
> would help topology like VLAN over bond to get PHC natively. To achieve
> this, the new hwtstamp flag is needed when passing hwtstamp_config to
> kernel. Let's supply the flag first, and fall back without flag if user
> run on old kernel.
>
> Split port_change_phc() from function port_link_status() so the event
> functions could call it directly when find the phc changed.
I can follow this version better, thanks for that.
This refactoring...
> @@ -2636,10 +2636,48 @@ static void bc_dispatch(struct port *p, enum
> fsm_event event, int mdiff)
> }
> }
>
> +void port_change_phc(struct port *p)
> +{
> + int required_modes;
> +
> + /* Only switch phc with HW time stamping mode */
> + if (!interface_tsinfo_valid(p->iface) ||
> + interface_phc_index(p->iface) < 0)
> + return;
> +
> + required_modes = clock_required_modes(p->clock);
> + if (!interface_tsmodes_supported(p->iface, required_modes)) {
> + pr_err("interface '%s' does not support requested "
> + "timestamping mode, set link status down by force.",
> + interface_label(p->iface));
> + p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> + } else if (p->phc_from_cmdline) {
> + pr_warning("%s: taking /dev/ptp%d from the "
> + "command line, not the attached ptp%d",
> + p->log_name, p->phc_index,
> + interface_phc_index(p->iface));
> + } else if (p->phc_index != interface_phc_index(p->iface)) {
> + p->phc_index = interface_phc_index(p->iface);
> +
> + if (clock_switch_phc(p->clock, p->phc_index)) {
> + p->last_fault_type = FT_SWITCH_PHC;
> + port_dispatch(p, EV_FAULT_DETECTED, 0);
> + return;
> + }
> + clock_sync_interval(p->clock, p->log_sync_interval);
> +
> + /* Ensure TS_LABEL_CHANGED is set for failover when
> + * PHC changed as we may not call port_change_phc()
> + * from port_link_status().
> + */
> + p->link_status |= TS_LABEL_CHANGED;
> + }
> +}
> +
> void port_link_status(void *ctx, int linkup, int ts_index)
> {
> char ts_label[MAX_IFNAME_SIZE + 1] = {0};
> - int link_state, required_modes;
> + int link_state;
> const char *old_ts_label;
> struct port *p = ctx;
>
> @@ -2663,32 +2701,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> if (p->link_status & LINK_UP &&
> (p->link_status & LINK_STATE_CHANGED || p->link_status &
> TS_LABEL_CHANGED)) {
> interface_get_tsinfo(p->iface);
> -
> - /* Only switch phc with HW time stamping mode */
> - if (interface_tsinfo_valid(p->iface) &&
> - interface_phc_index(p->iface) >= 0) {
> - required_modes = clock_required_modes(p->clock);
> - if (!interface_tsmodes_supported(p->iface,
> required_modes)) {
> - pr_err("interface '%s' does not support
> requested "
> - "timestamping mode, set link status down
> by force.",
> - interface_label(p->iface));
> - p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> - } else if (p->phc_from_cmdline) {
> - pr_warning("%s: taking /dev/ptp%d from the "
> - "command line, not the attached
> ptp%d",
> - p->log_name, p->phc_index,
> - interface_phc_index(p->iface));
> - } else if (p->phc_index !=
> interface_phc_index(p->iface)) {
> - p->phc_index = interface_phc_index(p->iface);
> -
> - if (clock_switch_phc(p->clock, p->phc_index)) {
> - p->last_fault_type = FT_SWITCH_PHC;
> - port_dispatch(p, EV_FAULT_DETECTED, 0);
> - return;
> - }
> - clock_sync_interval(p->clock,
> p->log_sync_interval);
> - }
> - }
> + port_change_phc(p);
> }
... alreadly improves the readability of the code. Please put that
bit into its own patch.
> @@ -2802,7 +2815,12 @@ static enum fsm_event bc_event(struct port *p, int
> fd_index)
>
> case FD_RTNL:
> pr_debug("%s: received link status notification", p->log_name);
> - rtnl_link_status(fd, p->name, port_link_status, p);
> + if (!interface_get_tsinfo(p->iface) &&
> + (p->phc_index != interface_phc_index(p->iface)))
> + port_change_phc(p);
> + else
> + rtnl_link_status(fd, p->name, port_link_status, p);
However this part will cause issues. When the RTNL event occurs,
there can be multiple messages. The callback is invoked on each one:
rtnl_link_status()
{
...
for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
...
if (cb)
cb(ctx, link_up, slave_index);
}
}
You new logic will drop link up/down notifications (or anything else
we might add later).
In some cases, your change will only call port_change_phc() but will
avoid the important other part of port_link_status():
/*
* A port going down can affect the BMCA result.
* Force a state decision event.
*/
if (p->link_status & LINK_DOWN)
clock_set_sde(p->clock, 1);
Why not simply remove the extra test and check
(p->phc_index != interface_phc_index(p->iface))
in the port_change_phc() helper function?
What am I missing?
Thanks,
Richard
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
