* [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic @ 2021-12-09 17:33 Kurt Kanzenbach 2021-12-10 0:07 ` Tobias Waldekranz 0 siblings, 1 reply; 16+ messages in thread From: Kurt Kanzenbach @ 2021-12-09 17:33 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, netdev, Richard Cochran, Kurt Kanzenbach A time aware switch should trap PTP traffic to the management CPU. User space daemons such as ptp4l will process these messages to implement Boundary (or Transparent) Clocks. At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages which leads to confusion when multiple end devices are connected to the switch. Therefore, setup PTP traps. Leverage the already implemented policy mechanism based on destination addresses. Configure the traps only if timestamping is enabled so that non time aware use case is still functioning. Introduce an additional PTP operation in case other devices need special handling in regards to trapping as well. Tested on Marvell Topaz (mv88e6341) switch with multiple end devices connected like this: |# DSA setup |$ ip link set eth0 up |$ ip link set lan0 up |$ ip link set lan1 up |$ ip link set lan2 up |$ ip link add name br0 type bridge |$ ip link set dev lan0 master br0 |$ ip link set dev lan1 master br0 |$ ip link set dev lan2 master br0 |$ ip link set lan0 up |$ ip link set lan1 up |$ ip link set lan2 up |$ ip link set br0 up |$ dhclient br0 |# Configure bridge routing |$ ebtables --table broute --append BROUTING --protocol 0x88F7 --jump DROP |# Start linuxptp |$ ptp4l -H -2 -i lan0 -i lan1 -i lan2 --tx_timestamp_timeout=40 -m Verified added policies with mv88e6xxx_dump. Signed-off-by: Kurt Kanzenbach <kurt@kmk-computers.de> --- drivers/net/dsa/mv88e6xxx/chip.c | 12 +++--- drivers/net/dsa/mv88e6xxx/chip.h | 5 +++ drivers/net/dsa/mv88e6xxx/hwtstamp.c | 7 ++++ drivers/net/dsa/mv88e6xxx/ptp.c | 59 ++++++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/ptp.h | 2 + 5 files changed, 80 insertions(+), 5 deletions(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 7fadbf987b23..ab50ebd85f1f 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1816,8 +1816,8 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port, return mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry); } -static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, - const struct mv88e6xxx_policy *policy) +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, + const struct mv88e6xxx_policy *policy) { enum mv88e6xxx_policy_mapping mapping = policy->mapping; enum mv88e6xxx_policy_action action = policy->action; @@ -1835,10 +1835,12 @@ static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, case MV88E6XXX_POLICY_MAPPING_SA: if (action == MV88E6XXX_POLICY_ACTION_NORMAL) state = 0; /* Dissociate the port and address */ - else if (action == MV88E6XXX_POLICY_ACTION_DISCARD && + else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD || + action == MV88E6XXX_POLICY_ACTION_TRAP) && is_multicast_ether_addr(addr)) state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_POLICY; - else if (action == MV88E6XXX_POLICY_ACTION_DISCARD && + else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD || + action == MV88E6XXX_POLICY_ACTION_TRAP) && is_unicast_ether_addr(addr)) state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC_POLICY; else @@ -4589,7 +4591,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = { .serdes_irq_status = mv88e6390_serdes_irq_status, .gpio_ops = &mv88e6352_gpio_ops, .avb_ops = &mv88e6390_avb_ops, - .ptp_ops = &mv88e6352_ptp_ops, + .ptp_ops = &mv88e6341_ptp_ops, .serdes_get_sset_count = mv88e6390_serdes_get_sset_count, .serdes_get_strings = mv88e6390_serdes_get_strings, .serdes_get_stats = mv88e6390_serdes_get_stats, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 8271b8aa7b71..795ae5a56834 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -673,6 +673,8 @@ struct mv88e6xxx_ptp_ops { int (*port_disable)(struct mv88e6xxx_chip *chip, int port); int (*global_enable)(struct mv88e6xxx_chip *chip); int (*global_disable)(struct mv88e6xxx_chip *chip); + int (*setup_ptp_traps)(struct mv88e6xxx_chip *chip, int port, + bool enable); int n_ext_ts; int arr0_sts_reg; int arr1_sts_reg; @@ -760,4 +762,7 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip) int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap); +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, + const struct mv88e6xxx_policy *policy); + #endif /* _MV88E6XXX_CHIP_H */ diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c index 8f74ffc7a279..617aeb6cbaac 100644 --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c @@ -94,6 +94,7 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port, const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops; struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port]; bool tstamp_enable = false; + int ret; /* Prevent the TX/RX paths from trying to interact with the * timestamp hardware while we reconfigure it. @@ -161,6 +162,12 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port, if (chip->enable_count == 0 && ptp_ops->global_disable) ptp_ops->global_disable(chip); } + + if (ptp_ops->setup_ptp_traps) { + ret = ptp_ops->setup_ptp_traps(chip, port, tstamp_enable); + if (tstamp_enable && ret) + dev_warn(chip->dev, "Failed to setup PTP traps. PTP might not work as desired!\n"); + } mv88e6xxx_reg_unlock(chip); /* Once hardware has been configured, enable timestamp checks diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index d838c174dc0d..8d6ff03d37c8 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.c +++ b/drivers/net/dsa/mv88e6xxx/ptp.c @@ -345,6 +345,37 @@ static int mv88e6352_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin, return 0; } +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port, + bool enable) +{ + static const u8 ptp_destinations[][ETH_ALEN] = { + { 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */ + { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */ + { 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */ + { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */ + { 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */ + { 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */ + }; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) { + struct mv88e6xxx_policy policy = { }; + + policy.mapping = MV88E6XXX_POLICY_MAPPING_DA; + policy.action = enable ? MV88E6XXX_POLICY_ACTION_TRAP : + MV88E6XXX_POLICY_ACTION_NORMAL; + policy.port = port; + policy.vid = 0; + ether_addr_copy(policy.addr, ptp_destinations[i]); + + ret = mv88e6xxx_policy_apply(chip, port, &policy); + if (ret) + return ret; + } + + return 0; +} + const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = { .clock_read = mv88e6165_ptp_clock_read, .global_enable = mv88e6165_global_enable, @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, }; +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = { + .clock_read = mv88e6352_ptp_clock_read, + .ptp_enable = mv88e6352_ptp_enable, + .ptp_verify = mv88e6352_ptp_verify, + .event_work = mv88e6352_tai_event_work, + .port_enable = mv88e6352_hwtstamp_port_enable, + .port_disable = mv88e6352_hwtstamp_port_disable, + .setup_ptp_traps = mv88e6341_setup_ptp_traps, + .n_ext_ts = 1, + .arr0_sts_reg = MV88E6XXX_PORT_PTP_ARR0_STS, + .arr1_sts_reg = MV88E6XXX_PORT_PTP_ARR1_STS, + .dep_sts_reg = MV88E6XXX_PORT_PTP_DEP_STS, + .rx_filters = (1 << HWTSTAMP_FILTER_NONE) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) | + (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), + .cc_shift = MV88E6XXX_CC_SHIFT, + .cc_mult = MV88E6XXX_CC_MULT, + .cc_mult_num = MV88E6XXX_CC_MULT_NUM, + .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, +}; + static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc) { struct mv88e6xxx_chip *chip = cc_to_chip(cc); diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h index 269d5d16a466..badcb72d10a6 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.h +++ b/drivers/net/dsa/mv88e6xxx/ptp.h @@ -151,6 +151,7 @@ void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip); extern const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops; extern const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops; extern const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops; +extern const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops; #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */ @@ -171,6 +172,7 @@ static inline void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip) static const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {}; static const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {}; static const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {}; +static const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {}; #endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-09 17:33 [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic Kurt Kanzenbach @ 2021-12-10 0:07 ` Tobias Waldekranz 2021-12-10 17:39 ` Kurt Kanzenbach 2021-12-11 5:14 ` Jakub Kicinski 0 siblings, 2 replies; 16+ messages in thread From: Tobias Waldekranz @ 2021-12-10 0:07 UTC (permalink / raw) To: Kurt Kanzenbach, Andrew Lunn, Vivien Didelot Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, netdev, Richard Cochran, Kurt Kanzenbach On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote: > A time aware switch should trap PTP traffic to the management CPU. User space > daemons such as ptp4l will process these messages to implement Boundary (or > Transparent) Clocks. > > At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages > which leads to confusion when multiple end devices are connected to the > switch. Therefore, setup PTP traps. Leverage the already implemented policy > mechanism based on destination addresses. Configure the traps only if > timestamping is enabled so that non time aware use case is still functioning. Do we know how PTP is supposed to work in relation to things like STP? I.e should you be able to run PTP over a link that is currently in blocking? It seems like being able to sync your clock before a topology change occurs would be nice. In that case, these addresses should be added to the ATU as MGMT instead of policy entries. > Introduce an additional PTP operation in case other devices need special > handling in regards to trapping as well. > > Tested on Marvell Topaz (mv88e6341) switch with multiple end devices connected > like this: > > |# DSA setup > |$ ip link set eth0 up > |$ ip link set lan0 up > |$ ip link set lan1 up > |$ ip link set lan2 up > |$ ip link add name br0 type bridge > |$ ip link set dev lan0 master br0 > |$ ip link set dev lan1 master br0 > |$ ip link set dev lan2 master br0 > |$ ip link set lan0 up > |$ ip link set lan1 up > |$ ip link set lan2 up > |$ ip link set br0 up > |$ dhclient br0 > |# Configure bridge routing > |$ ebtables --table broute --append BROUTING --protocol 0x88F7 --jump DROP > |# Start linuxptp > |$ ptp4l -H -2 -i lan0 -i lan1 -i lan2 --tx_timestamp_timeout=40 -m > > Verified added policies with mv88e6xxx_dump. > > Signed-off-by: Kurt Kanzenbach <kurt@kmk-computers.de> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 12 +++--- > drivers/net/dsa/mv88e6xxx/chip.h | 5 +++ > drivers/net/dsa/mv88e6xxx/hwtstamp.c | 7 ++++ > drivers/net/dsa/mv88e6xxx/ptp.c | 59 ++++++++++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/ptp.h | 2 + > 5 files changed, 80 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 7fadbf987b23..ab50ebd85f1f 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -1816,8 +1816,8 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port, > return mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry); > } > > -static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, > - const struct mv88e6xxx_policy *policy) > +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, > + const struct mv88e6xxx_policy *policy) > { > enum mv88e6xxx_policy_mapping mapping = policy->mapping; > enum mv88e6xxx_policy_action action = policy->action; > @@ -1835,10 +1835,12 @@ static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, > case MV88E6XXX_POLICY_MAPPING_SA: > if (action == MV88E6XXX_POLICY_ACTION_NORMAL) > state = 0; /* Dissociate the port and address */ > - else if (action == MV88E6XXX_POLICY_ACTION_DISCARD && > + else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD || > + action == MV88E6XXX_POLICY_ACTION_TRAP) && > is_multicast_ether_addr(addr)) > state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_POLICY; > - else if (action == MV88E6XXX_POLICY_ACTION_DISCARD && > + else if ((action == MV88E6XXX_POLICY_ACTION_DISCARD || > + action == MV88E6XXX_POLICY_ACTION_TRAP) && > is_unicast_ether_addr(addr)) > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC_POLICY; > else > @@ -4589,7 +4591,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = { > .serdes_irq_status = mv88e6390_serdes_irq_status, > .gpio_ops = &mv88e6352_gpio_ops, > .avb_ops = &mv88e6390_avb_ops, > - .ptp_ops = &mv88e6352_ptp_ops, > + .ptp_ops = &mv88e6341_ptp_ops, > .serdes_get_sset_count = mv88e6390_serdes_get_sset_count, > .serdes_get_strings = mv88e6390_serdes_get_strings, > .serdes_get_stats = mv88e6390_serdes_get_stats, > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > index 8271b8aa7b71..795ae5a56834 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.h > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -673,6 +673,8 @@ struct mv88e6xxx_ptp_ops { > int (*port_disable)(struct mv88e6xxx_chip *chip, int port); > int (*global_enable)(struct mv88e6xxx_chip *chip); > int (*global_disable)(struct mv88e6xxx_chip *chip); > + int (*setup_ptp_traps)(struct mv88e6xxx_chip *chip, int port, > + bool enable); > int n_ext_ts; > int arr0_sts_reg; > int arr1_sts_reg; > @@ -760,4 +762,7 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip) > > int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap); > > +int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port, > + const struct mv88e6xxx_policy *policy); > + > #endif /* _MV88E6XXX_CHIP_H */ > diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c > index 8f74ffc7a279..617aeb6cbaac 100644 > --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c > +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c > @@ -94,6 +94,7 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port, > const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops; > struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port]; > bool tstamp_enable = false; > + int ret; > > /* Prevent the TX/RX paths from trying to interact with the > * timestamp hardware while we reconfigure it. > @@ -161,6 +162,12 @@ static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port, > if (chip->enable_count == 0 && ptp_ops->global_disable) > ptp_ops->global_disable(chip); > } > + > + if (ptp_ops->setup_ptp_traps) { > + ret = ptp_ops->setup_ptp_traps(chip, port, tstamp_enable); > + if (tstamp_enable && ret) > + dev_warn(chip->dev, "Failed to setup PTP traps. PTP might not work as desired!\n"); > + } > mv88e6xxx_reg_unlock(chip); > > /* Once hardware has been configured, enable timestamp checks > diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c > index d838c174dc0d..8d6ff03d37c8 100644 > --- a/drivers/net/dsa/mv88e6xxx/ptp.c > +++ b/drivers/net/dsa/mv88e6xxx/ptp.c > @@ -345,6 +345,37 @@ static int mv88e6352_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin, > return 0; > } > > +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port, > + bool enable) > +{ > + static const u8 ptp_destinations[][ETH_ALEN] = { > + { 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */ > + { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */ > + { 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */ > + { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */ > + { 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */ > + { 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */ How does the L3 entries above play together with IGMP/MLD? I.e. what happens if, after launching ptp4l, an IGMP report comes in on lanX, requesting that same group? Would the policy entry not be overwritten by mv88e6xxx_port_mdb_add? Eventually I think we will have many interfaces to configure static entries in the ATU: 1. MDB entries from a bridge (already in place) 2. User-configured entries through ethtool's rxnfc (already in place) 3. Driver-internal consumers (this patch, MRP, etc.) 4. User-configured entries through TC. Seems to me like we need to start tracking the owners for these to stop them from stomping on one another. > + }; > + int ret, i; > + > + for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) { > + struct mv88e6xxx_policy policy = { }; > + > + policy.mapping = MV88E6XXX_POLICY_MAPPING_DA; > + policy.action = enable ? MV88E6XXX_POLICY_ACTION_TRAP : > + MV88E6XXX_POLICY_ACTION_NORMAL; > + policy.port = port; > + policy.vid = 0; > + ether_addr_copy(policy.addr, ptp_destinations[i]); > + > + ret = mv88e6xxx_policy_apply(chip, port, &policy); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = { > .clock_read = mv88e6165_ptp_clock_read, > .global_enable = mv88e6165_global_enable, > @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { > .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, > }; > > +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = { > + .clock_read = mv88e6352_ptp_clock_read, > + .ptp_enable = mv88e6352_ptp_enable, > + .ptp_verify = mv88e6352_ptp_verify, > + .event_work = mv88e6352_tai_event_work, > + .port_enable = mv88e6352_hwtstamp_port_enable, > + .port_disable = mv88e6352_hwtstamp_port_disable, > + .setup_ptp_traps = mv88e6341_setup_ptp_traps, Is there any reason why this could not be added to the existing ops for 6352 instead? Their ATU's are compatible, IIRC. > + .n_ext_ts = 1, > + .arr0_sts_reg = MV88E6XXX_PORT_PTP_ARR0_STS, > + .arr1_sts_reg = MV88E6XXX_PORT_PTP_ARR1_STS, > + .dep_sts_reg = MV88E6XXX_PORT_PTP_DEP_STS, > + .rx_filters = (1 << HWTSTAMP_FILTER_NONE) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) | > + (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | > + (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | > + (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), > + .cc_shift = MV88E6XXX_CC_SHIFT, > + .cc_mult = MV88E6XXX_CC_MULT, > + .cc_mult_num = MV88E6XXX_CC_MULT_NUM, > + .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, > +}; > + > static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc) > { > struct mv88e6xxx_chip *chip = cc_to_chip(cc); > diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h > index 269d5d16a466..badcb72d10a6 100644 > --- a/drivers/net/dsa/mv88e6xxx/ptp.h > +++ b/drivers/net/dsa/mv88e6xxx/ptp.h > @@ -151,6 +151,7 @@ void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip); > extern const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops; > extern const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops; > extern const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops; > +extern const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops; > > #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */ > > @@ -171,6 +172,7 @@ static inline void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip) > static const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {}; > static const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {}; > static const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {}; > +static const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = {}; > > #endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */ > > -- > 2.34.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-10 0:07 ` Tobias Waldekranz @ 2021-12-10 17:39 ` Kurt Kanzenbach 2021-12-11 23:02 ` Tobias Waldekranz 2021-12-11 5:14 ` Jakub Kicinski 1 sibling, 1 reply; 16+ messages in thread From: Kurt Kanzenbach @ 2021-12-10 17:39 UTC (permalink / raw) To: Tobias Waldekranz, Andrew Lunn, Vivien Didelot Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, netdev, Richard Cochran On Fri Dec 10 2021, Tobias Waldekranz wrote: > On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote: >> A time aware switch should trap PTP traffic to the management CPU. User space >> daemons such as ptp4l will process these messages to implement Boundary (or >> Transparent) Clocks. >> >> At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages >> which leads to confusion when multiple end devices are connected to the >> switch. Therefore, setup PTP traps. Leverage the already implemented policy >> mechanism based on destination addresses. Configure the traps only if >> timestamping is enabled so that non time aware use case is still functioning. > > Do we know how PTP is supposed to work in relation to things like STP? > I.e should you be able to run PTP over a link that is currently in > blocking? It seems like being able to sync your clock before a topology > change occurs would be nice. In that case, these addresses should be > added to the ATU as MGMT instead of policy entries. Given the fact that the l2 p2p address is already considered as management traffic (see mv88e6390_g1_mgmt_rsvd2cpu()) maybe all PTP addresses could be treated as such. [snip] >> +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port, >> + bool enable) >> +{ >> + static const u8 ptp_destinations[][ETH_ALEN] = { >> + { 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */ >> + { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */ >> + { 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */ >> + { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */ >> + { 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */ >> + { 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */ > > How does the L3 entries above play together with IGMP/MLD? I.e. what > happens if, after launching ptp4l, an IGMP report comes in on lanX, > requesting that same group? Would the policy entry not be overwritten by > mv88e6xxx_port_mdb_add? Just tested this. Yes it is overwritten without any detection or errors. Actually I did test UDP as well and didn't notice it. It obviously depends on the order of events. > > Eventually I think we will have many interfaces to configure static > entries in the ATU: > > 1. MDB entries from a bridge (already in place) > 2. User-configured entries through ethtool's rxnfc (already in place) > 3. Driver-internal consumers (this patch, MRP, etc.) > 4. User-configured entries through TC. > > Seems to me like we need to start tracking the owners for these to stop > them from stomping on one another. Agreed. Some mechanism is required. Any idea how to implement it? In case of PTP the management/policy entries should take precedence. > >> + }; >> + int ret, i; >> + >> + for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) { >> + struct mv88e6xxx_policy policy = { }; >> + >> + policy.mapping = MV88E6XXX_POLICY_MAPPING_DA; >> + policy.action = enable ? MV88E6XXX_POLICY_ACTION_TRAP : >> + MV88E6XXX_POLICY_ACTION_NORMAL; >> + policy.port = port; >> + policy.vid = 0; >> + ether_addr_copy(policy.addr, ptp_destinations[i]); >> + >> + ret = mv88e6xxx_policy_apply(chip, port, &policy); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = { >> .clock_read = mv88e6165_ptp_clock_read, >> .global_enable = mv88e6165_global_enable, >> @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { >> .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, >> }; >> >> +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = { >> + .clock_read = mv88e6352_ptp_clock_read, >> + .ptp_enable = mv88e6352_ptp_enable, >> + .ptp_verify = mv88e6352_ptp_verify, >> + .event_work = mv88e6352_tai_event_work, >> + .port_enable = mv88e6352_hwtstamp_port_enable, >> + .port_disable = mv88e6352_hwtstamp_port_disable, >> + .setup_ptp_traps = mv88e6341_setup_ptp_traps, > > Is there any reason why this could not be added to the existing ops for > 6352 instead? Their ATU's are compatible, IIRC. No particular reason except that I don't have access to a 6352 device to test it. Thanks, Kurt ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-10 17:39 ` Kurt Kanzenbach @ 2021-12-11 23:02 ` Tobias Waldekranz 2021-12-13 18:54 ` Kurt Kanzenbach 0 siblings, 1 reply; 16+ messages in thread From: Tobias Waldekranz @ 2021-12-11 23:02 UTC (permalink / raw) To: Kurt Kanzenbach, Andrew Lunn, Vivien Didelot Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, netdev, Richard Cochran On Fri, Dec 10, 2021 at 18:39, Kurt Kanzenbach <kurt@kmk-computers.de> wrote: > On Fri Dec 10 2021, Tobias Waldekranz wrote: >> On Thu, Dec 09, 2021 at 18:33, Kurt Kanzenbach <kurt@kmk-computers.de> wrote: >>> A time aware switch should trap PTP traffic to the management CPU. User space >>> daemons such as ptp4l will process these messages to implement Boundary (or >>> Transparent) Clocks. >>> >>> At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages >>> which leads to confusion when multiple end devices are connected to the >>> switch. Therefore, setup PTP traps. Leverage the already implemented policy >>> mechanism based on destination addresses. Configure the traps only if >>> timestamping is enabled so that non time aware use case is still functioning. >> >> Do we know how PTP is supposed to work in relation to things like STP? >> I.e should you be able to run PTP over a link that is currently in >> blocking? It seems like being able to sync your clock before a topology >> change occurs would be nice. In that case, these addresses should be >> added to the ATU as MGMT instead of policy entries. > > Given the fact that the l2 p2p address is already considered as > management traffic (see mv88e6390_g1_mgmt_rsvd2cpu()) maybe all PTP > addresses could be treated as such. The feedback from both Jakub and Richard seems to support that. > >>> +static int mv88e6341_setup_ptp_traps(struct mv88e6xxx_chip *chip, int port, >>> + bool enable) >>> +{ >>> + static const u8 ptp_destinations[][ETH_ALEN] = { >>> + { 0x01, 0x1b, 0x19, 0x00, 0x00, 0x00 }, /* L2 PTP */ >>> + { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e }, /* L2 P2P */ >>> + { 0x01, 0x00, 0x5e, 0x00, 0x01, 0x81 }, /* IPv4 PTP */ >>> + { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* IPv4 P2P */ >>> + { 0x33, 0x33, 0x00, 0x00, 0x01, 0x81 }, /* IPv6 PTP */ >>> + { 0x33, 0x33, 0x00, 0x00, 0x00, 0x6b }, /* IPv6 P2P */ >> >> How does the L3 entries above play together with IGMP/MLD? I.e. what >> happens if, after launching ptp4l, an IGMP report comes in on lanX, >> requesting that same group? Would the policy entry not be overwritten by >> mv88e6xxx_port_mdb_add? > > Just tested this. Yes it is overwritten without any detection or > errors. Actually I did test UDP as well and didn't notice it. It > obviously depends on the order of events. > >> >> Eventually I think we will have many interfaces to configure static >> entries in the ATU: >> >> 1. MDB entries from a bridge (already in place) >> 2. User-configured entries through ethtool's rxnfc (already in place) >> 3. Driver-internal consumers (this patch, MRP, etc.) >> 4. User-configured entries through TC. >> >> Seems to me like we need to start tracking the owners for these to stop >> them from stomping on one another. > > Agreed. Some mechanism is required. Any idea how to implement it? In > case of PTP the management/policy entries should take precedence. One approach would be to create a cache containing all static ATU entries. That way we can easily track the owner of each entry. There are also major performance advantages of being able to update memberships of group entries without having to read the entry back from the ATU first. This is especially important once we start handling router ports correctly, in which case you have to update all active entries on every add/remove. Before going down that route though, I would suggest getting some initial feedback from Andrew. A complicating factor, no matter the implementation, is the relationship between the bridge MDB and all other consumers of ATU entries. As an example: If the driver first receives an MDB add for one of the L3 PTP groups, and then a user starts up ptp4l, the driver can't then go back to the bridge and say "remember that group entry that I said I loaded, well I have removed it now". So whatever implementation we choose, I think it needs to keep a shadow entry for the MDB that can be re-inserted if the corresponding management or policy entry is removed. You may simply want to allow all consumers to register any given group with the cache. The cache would then internally elect the "best" entry and install that to the ATU. Sort of what zebra/quagga/FRR does for dynamic routing. The priority would probably be something like: 1. Management entry 2. Policy entry 3. MDB entry This should still result in the proper forwarding of a registered groups that are shadowed by management or policy entries. The bridge would know (via skb->offload_fwd_mark) that the packet had not been forwarded in hardware and would fallback to software forwarding. If the policy entry was later removed (e.g. PTP was shut down) the MDB entry could be reinstalled and offloading resumed. >> >>> + }; >>> + int ret, i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(ptp_destinations); ++i) { >>> + struct mv88e6xxx_policy policy = { }; >>> + >>> + policy.mapping = MV88E6XXX_POLICY_MAPPING_DA; >>> + policy.action = enable ? MV88E6XXX_POLICY_ACTION_TRAP : >>> + MV88E6XXX_POLICY_ACTION_NORMAL; >>> + policy.port = port; >>> + policy.vid = 0; >>> + ether_addr_copy(policy.addr, ptp_destinations[i]); >>> + >>> + ret = mv88e6xxx_policy_apply(chip, port, &policy); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = { >>> .clock_read = mv88e6165_ptp_clock_read, >>> .global_enable = mv88e6165_global_enable, >>> @@ -419,6 +450,34 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { >>> .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, >>> }; >>> >>> +const struct mv88e6xxx_ptp_ops mv88e6341_ptp_ops = { >>> + .clock_read = mv88e6352_ptp_clock_read, >>> + .ptp_enable = mv88e6352_ptp_enable, >>> + .ptp_verify = mv88e6352_ptp_verify, >>> + .event_work = mv88e6352_tai_event_work, >>> + .port_enable = mv88e6352_hwtstamp_port_enable, >>> + .port_disable = mv88e6352_hwtstamp_port_disable, >>> + .setup_ptp_traps = mv88e6341_setup_ptp_traps, >> >> Is there any reason why this could not be added to the existing ops for >> 6352 instead? Their ATU's are compatible, IIRC. > > No particular reason except that I don't have access to a 6352 device to > test it. Got it. Well I can hopefully be of assistance there. Anyway, I think we can safely assume that they are compatible with respect to the ATU. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-11 23:02 ` Tobias Waldekranz @ 2021-12-13 18:54 ` Kurt Kanzenbach 0 siblings, 0 replies; 16+ messages in thread From: Kurt Kanzenbach @ 2021-12-13 18:54 UTC (permalink / raw) To: Tobias Waldekranz, Andrew Lunn, Vivien Didelot Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, netdev, Richard Cochran On Sun Dec 12 2021, Tobias Waldekranz wrote: >> Agreed. Some mechanism is required. Any idea how to implement it? In >> case of PTP the management/policy entries should take precedence. > > One approach would be to create a cache containing all static ATU > entries. That way we can easily track the owner of each entry. There are > also major performance advantages of being able to update memberships of > group entries without having to read the entry back from the ATU > first. This is especially important once we start handling router ports > correctly, in which case you have to update all active entries on every > add/remove. > > Before going down that route though, I would suggest getting some > initial feedback from Andrew. > > A complicating factor, no matter the implementation, is the relationship > between the bridge MDB and all other consumers of ATU entries. As an > example: If the driver first receives an MDB add for one of the L3 PTP > groups, and then a user starts up ptp4l, the driver can't then go back > to the bridge and say "remember that group entry that I said I loaded, > well I have removed it now". So whatever implementation we choose, I > think it needs to keep a shadow entry for the MDB that can be > re-inserted if the corresponding management or policy entry is removed. > > You may simply want to allow all consumers to register any given group > with the cache. The cache would then internally elect the "best" entry > and install that to the ATU. Sort of what zebra/quagga/FRR does for > dynamic routing. The priority would probably be something like: > > 1. Management entry > 2. Policy entry > 3. MDB entry > > This should still result in the proper forwarding of a registered groups > that are shadowed by management or policy entries. The bridge would know > (via skb->offload_fwd_mark) that the packet had not been forwarded in > hardware and would fallback to software forwarding. If the policy entry > was later removed (e.g. PTP was shut down) the MDB entry could be > reinstalled and offloading resumed. Thanks. This approach makes sense and should solve the problem at hand. Thanks, Kurt ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-10 0:07 ` Tobias Waldekranz 2021-12-10 17:39 ` Kurt Kanzenbach @ 2021-12-11 5:14 ` Jakub Kicinski 2021-12-11 15:39 ` Richard Cochran 1 sibling, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2021-12-11 5:14 UTC (permalink / raw) To: Tobias Waldekranz Cc: Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev, Richard Cochran On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: > > At the moment the mv88e6xxx driver for mv88e6341 doesn't trap these messages > > which leads to confusion when multiple end devices are connected to the > > switch. Therefore, setup PTP traps. Leverage the already implemented policy > > mechanism based on destination addresses. Configure the traps only if > > timestamping is enabled so that non time aware use case is still functioning. > > Do we know how PTP is supposed to work in relation to things like STP? > I.e should you be able to run PTP over a link that is currently in > blocking? Not sure if I'm missing the real question but IIRC the standard calls out that PTP clock distribution tree can be different that the STP tree, ergo PTP ignores STP forwarding state. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-11 5:14 ` Jakub Kicinski @ 2021-12-11 15:39 ` Richard Cochran 2021-12-12 15:16 ` Kurt Kanzenbach 2021-12-13 12:10 ` Richard Cochran 0 siblings, 2 replies; 16+ messages in thread From: Richard Cochran @ 2021-12-11 15:39 UTC (permalink / raw) To: Jakub Kicinski Cc: Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote: > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: > > Do we know how PTP is supposed to work in relation to things like STP? > > I.e should you be able to run PTP over a link that is currently in > > blocking? > > Not sure if I'm missing the real question but IIRC the standard > calls out that PTP clock distribution tree can be different that > the STP tree, ergo PTP ignores STP forwarding state. That is correct. The PTP will form its own spanning tree, and that might be different than the STP. In fact, the Layer2 PTP messages have special MAC addresses that are supposed to be sent unconditionally, even over blocked ports. Thanks, Richard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-11 15:39 ` Richard Cochran @ 2021-12-12 15:16 ` Kurt Kanzenbach 2021-12-13 12:10 ` Richard Cochran 1 sibling, 0 replies; 16+ messages in thread From: Kurt Kanzenbach @ 2021-12-12 15:16 UTC (permalink / raw) To: Richard Cochran, Jakub Kicinski Cc: Tobias Waldekranz, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Sat Dec 11 2021, Richard Cochran wrote: > On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote: >> On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: >> > Do we know how PTP is supposed to work in relation to things like STP? >> > I.e should you be able to run PTP over a link that is currently in >> > blocking? >> >> Not sure if I'm missing the real question but IIRC the standard >> calls out that PTP clock distribution tree can be different that >> the STP tree, ergo PTP ignores STP forwarding state. > > That is correct. The PTP will form its own spanning tree, and that > might be different than the STP. In fact, the Layer2 PTP messages > have special MAC addresses that are supposed to be sent > unconditionally, even over blocked ports. Thanks for clarification. This needs fixing in hellcreek too, as pass_blocked is currently not set for the ptp fdb entries. Thanks, Kurt ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-11 15:39 ` Richard Cochran 2021-12-12 15:16 ` Kurt Kanzenbach @ 2021-12-13 12:10 ` Richard Cochran 2021-12-13 12:31 ` Vladimir Oltean ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Richard Cochran @ 2021-12-13 12:10 UTC (permalink / raw) To: Jakub Kicinski Cc: Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote: > On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote: > > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: > > > Do we know how PTP is supposed to work in relation to things like STP? > > > I.e should you be able to run PTP over a link that is currently in > > > blocking? > > > > Not sure if I'm missing the real question but IIRC the standard > > calls out that PTP clock distribution tree can be different that > > the STP tree, ergo PTP ignores STP forwarding state. > > That is correct. The PTP will form its own spanning tree, and that > might be different than the STP. In fact, the Layer2 PTP messages > have special MAC addresses that are supposed to be sent > unconditionally, even over blocked ports. Let me correct that statement. I looked at 1588 again, and for E2E TC it states, "All PTP version 2 messages shall be forwarded according to the addressing rules of the network." I suppose that includes the STP state. For P2P TC, there is an exception that the peer delay messages are not forwarded. These are generated and consumed by the switch. The PTP spanning tree still is formed by the Boundary Clocks (BC), and a Transparent Clock (TC) does not participate in forming the PTP spanning tree. What does this mean for Linux DSA switch drivers? 1. All incoming PTP frames should be forwarded to the CPU port, so that the software stack may perform its BC or TC functions. 2. When used as a BC, the PTP frames sent from the CPU port should not be dropped. 3. When used as a TC, PTP frames sent from the CPU port can be dropped on blocked external ports, except for the Peer Delay messages. Now maybe a particular switch implementation makes it hard to form rules for #3 that still work for UDP over IPv4/6. Thanks, Richard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 12:10 ` Richard Cochran @ 2021-12-13 12:31 ` Vladimir Oltean 2021-12-13 15:27 ` Andrew Lunn 2021-12-13 17:11 ` Richard Cochran 2021-12-13 16:44 ` Jakub Kicinski 2021-12-13 18:40 ` Kurt Kanzenbach 2 siblings, 2 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-12-13 12:31 UTC (permalink / raw) To: Richard Cochran Cc: Jakub Kicinski, Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller, netdev Hi Richard, On Mon, Dec 13, 2021 at 04:10:45AM -0800, Richard Cochran wrote: > On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote: > > On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote: > > > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: > > > > Do we know how PTP is supposed to work in relation to things like STP? > > > > I.e should you be able to run PTP over a link that is currently in > > > > blocking? > > > > > > Not sure if I'm missing the real question but IIRC the standard > > > calls out that PTP clock distribution tree can be different that > > > the STP tree, ergo PTP ignores STP forwarding state. > > > > That is correct. The PTP will form its own spanning tree, and that > > might be different than the STP. In fact, the Layer2 PTP messages > > have special MAC addresses that are supposed to be sent > > unconditionally, even over blocked ports. > > Let me correct that statement. > > I looked at 1588 again, and for E2E TC it states, "All PTP version 2 > messages shall be forwarded according to the addressing rules of the > network." I suppose that includes the STP state. > > For P2P TC, there is an exception that the peer delay messages are not > forwarded. These are generated and consumed by the switch. > > The PTP spanning tree still is formed by the Boundary Clocks (BC), and > a Transparent Clock (TC) does not participate in forming the PTP > spanning tree. > > What does this mean for Linux DSA switch drivers? > > 1. All incoming PTP frames should be forwarded to the CPU port, so > that the software stack may perform its BC or TC functions. > > 2. When used as a BC, the PTP frames sent from the CPU port should not > be dropped. > > 3. When used as a TC, PTP frames sent from the CPU port can be dropped > on blocked external ports, except for the Peer Delay messages. > > Now maybe a particular switch implementation makes it hard to form > rules for #3 that still work for UDP over IPv4/6. With other drivers, all packets injected from the CPU port act as if in "god mode", bypassing any STP state. It then becomes the responsibility of the software to not send packets on a port that is blocking, except for packets for control protocols. Would you agree that ptp4l should consider monitoring whether its ports are under a bridge, and what STP state that bridge port is in? I think this isn't even specific to DSA, the same thing would happen with software bridging: packets sent through sockets opened on the bridge would have egress prevented on blocking ports by virtue of the bridge driver code, but packets sent through sockets opened on the bridge port directly, I don't think the bridge driver has any saying about the STP state. So similarly, it becomes the application's responsibility. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 12:31 ` Vladimir Oltean @ 2021-12-13 15:27 ` Andrew Lunn 2021-12-13 17:11 ` Richard Cochran 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2021-12-13 15:27 UTC (permalink / raw) To: Vladimir Oltean Cc: Richard Cochran, Jakub Kicinski, Tobias Waldekranz, Kurt Kanzenbach, Vivien Didelot, Florian Fainelli, David S. Miller, netdev > I think this isn't even specific to DSA, the same thing would > happen with software bridging And pure switchdev switches. I wonder what the Mellanox switch does in this case? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 12:31 ` Vladimir Oltean 2021-12-13 15:27 ` Andrew Lunn @ 2021-12-13 17:11 ` Richard Cochran 2021-12-13 18:58 ` Vladimir Oltean 1 sibling, 1 reply; 16+ messages in thread From: Richard Cochran @ 2021-12-13 17:11 UTC (permalink / raw) To: Vladimir Oltean Cc: Jakub Kicinski, Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller, netdev On Mon, Dec 13, 2021 at 02:31:47PM +0200, Vladimir Oltean wrote: > With other drivers, all packets injected from the CPU port act as if in > "god mode", bypassing any STP state. It then becomes the responsibility > of the software to not send packets on a port that is blocking, > except for packets for control protocols. Would you agree that ptp4l > should consider monitoring whether its ports are under a bridge, and > what STP state that bridge port is in? Perhaps. linuxptp TC mode will forward frames out all configured interfaces. If the bridge can't drop the PTP frames automatically, then this could cause loops. So if switch HW in general won't drop them, then, yes, the TC user space stack will need to follow the STP state. > I think this isn't even specific > to DSA, the same thing would happen with software bridging: (Linux doesn't support even SW time stamping on SW bridges, so you can't have a TC running in this case.) Thanks, Richard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 17:11 ` Richard Cochran @ 2021-12-13 18:58 ` Vladimir Oltean 0 siblings, 0 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-12-13 18:58 UTC (permalink / raw) To: Richard Cochran Cc: Jakub Kicinski, Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller, netdev On Mon, Dec 13, 2021 at 09:11:40AM -0800, Richard Cochran wrote: > On Mon, Dec 13, 2021 at 02:31:47PM +0200, Vladimir Oltean wrote: > > > With other drivers, all packets injected from the CPU port act as if in > > "god mode", bypassing any STP state. It then becomes the responsibility > > of the software to not send packets on a port that is blocking, > > except for packets for control protocols. Would you agree that ptp4l > > should consider monitoring whether its ports are under a bridge, and > > what STP state that bridge port is in? > > Perhaps. linuxptp TC mode will forward frames out all configured > interfaces. If the bridge can't drop the PTP frames automatically, > then this could cause loops. Considering that in this configuration: ip link add br0 type bridge ip link set swp0 master br0 ip link set swp1 master br0 ip link set swp2 master br0 ip link set swp3 master br0 ptp4l -i swp0 -i swp1 -i swp2 -i swp3 -f configs/P2P-TC.cfg -m the kernel code path for PTP packets has nothing to do with the bridge driver (unless maybe an explicit netfilter rule to tell the bridge RX handler to not steal those packets from the physical port), I think it's safe to say that yes, the bridge can't drop the PTP frames automatically. > So if switch HW in general won't drop them, then, yes, the TC user > space stack will need to follow the STP state. The hardware offloads tend to follow the software model as closely as possible, and as mentioned, I think this has to do with the software model in this case, rather than a hardware quirk. I would consider the hardware to be quirky quite in the opposite case: you send a packet through a physical port rather than the bridge, and that one is affected by the STP state. We have switch drivers like that too, but let's not go there, they're rather the exception. > > I think this isn't even specific > > to DSA, the same thing would happen with software bridging: > > (Linux doesn't support even SW time stamping on SW bridges, so you > can't have a TC running in this case.) As also rephrased in the replies above, the point was that STP state has nothing to do, in general, with whether this is a DSA switch or not. It is a bridging service concept, and applies to data plane packets, which ptp4l isn't sending/receiving, since as you very well point out, your socket is opened on the physical port and not on the bridge device. So don't expect the STP state of the port to do something here. If you send a packet on a socket opened on an interface that is backed by a physical port, expect that packet to be sent - is the main point. The fact that I used the "software bridging" term was just to clarify that it hasn't got anything to do with whether the bridging is offloaded or not. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 12:10 ` Richard Cochran 2021-12-13 12:31 ` Vladimir Oltean @ 2021-12-13 16:44 ` Jakub Kicinski 2021-12-13 17:04 ` Richard Cochran 2021-12-13 18:40 ` Kurt Kanzenbach 2 siblings, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2021-12-13 16:44 UTC (permalink / raw) To: Richard Cochran Cc: Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Mon, 13 Dec 2021 04:10:45 -0800 Richard Cochran wrote: > I looked at 1588 again, and for E2E TC it states, "All PTP version 2 > messages shall be forwarded according to the addressing rules of the > network." I suppose that includes the STP state. > > For P2P TC, there is an exception that the peer delay messages are not > forwarded. These are generated and consumed by the switch. Indeed, looks like a v1 vs v2 change :S ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 16:44 ` Jakub Kicinski @ 2021-12-13 17:04 ` Richard Cochran 0 siblings, 0 replies; 16+ messages in thread From: Richard Cochran @ 2021-12-13 17:04 UTC (permalink / raw) To: Jakub Kicinski Cc: Tobias Waldekranz, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Mon, Dec 13, 2021 at 08:44:25AM -0800, Jakub Kicinski wrote: > Indeed, looks like a v1 vs v2 change :S FWIW, the peer delay mechanism was new in 1588v2. Thanks, Richard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic 2021-12-13 12:10 ` Richard Cochran 2021-12-13 12:31 ` Vladimir Oltean 2021-12-13 16:44 ` Jakub Kicinski @ 2021-12-13 18:40 ` Kurt Kanzenbach 2 siblings, 0 replies; 16+ messages in thread From: Kurt Kanzenbach @ 2021-12-13 18:40 UTC (permalink / raw) To: Richard Cochran, Jakub Kicinski Cc: Tobias Waldekranz, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, netdev On Mon Dec 13 2021, Richard Cochran wrote: > On Sat, Dec 11, 2021 at 07:39:26AM -0800, Richard Cochran wrote: >> On Fri, Dec 10, 2021 at 09:14:10PM -0800, Jakub Kicinski wrote: >> > On Fri, 10 Dec 2021 01:07:59 +0100 Tobias Waldekranz wrote: >> > > Do we know how PTP is supposed to work in relation to things like STP? >> > > I.e should you be able to run PTP over a link that is currently in >> > > blocking? >> > >> > Not sure if I'm missing the real question but IIRC the standard >> > calls out that PTP clock distribution tree can be different that >> > the STP tree, ergo PTP ignores STP forwarding state. >> >> That is correct. The PTP will form its own spanning tree, and that >> might be different than the STP. In fact, the Layer2 PTP messages >> have special MAC addresses that are supposed to be sent >> unconditionally, even over blocked ports. > > Let me correct that statement. > > I looked at 1588 again, and for E2E TC it states, "All PTP version 2 > messages shall be forwarded according to the addressing rules of the > network." I suppose that includes the STP state. > > For P2P TC, there is an exception that the peer delay messages are not > forwarded. These are generated and consumed by the switch. > > The PTP spanning tree still is formed by the Boundary Clocks (BC), and > a Transparent Clock (TC) does not participate in forming the PTP > spanning tree. > > What does this mean for Linux DSA switch drivers? > > 1. All incoming PTP frames should be forwarded to the CPU port, so > that the software stack may perform its BC or TC functions. > > 2. When used as a BC, the PTP frames sent from the CPU port should not > be dropped. > > 3. When used as a TC, PTP frames sent from the CPU port can be dropped > on blocked external ports, except for the Peer Delay messages. Maybe I'm missing something, but how is #2 and #3 supposed to work with DSA? The switch driver doesn't know whether the user wants to run BC or TC. For #2 the STP state is ignored, for #3 it is not except for peer delay measurements. Thanks, Kurt ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-12-13 18:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-09 17:33 [PATCH net-next v1] net: dsa: mv88e6xxx: Trap PTP traffic Kurt Kanzenbach 2021-12-10 0:07 ` Tobias Waldekranz 2021-12-10 17:39 ` Kurt Kanzenbach 2021-12-11 23:02 ` Tobias Waldekranz 2021-12-13 18:54 ` Kurt Kanzenbach 2021-12-11 5:14 ` Jakub Kicinski 2021-12-11 15:39 ` Richard Cochran 2021-12-12 15:16 ` Kurt Kanzenbach 2021-12-13 12:10 ` Richard Cochran 2021-12-13 12:31 ` Vladimir Oltean 2021-12-13 15:27 ` Andrew Lunn 2021-12-13 17:11 ` Richard Cochran 2021-12-13 18:58 ` Vladimir Oltean 2021-12-13 16:44 ` Jakub Kicinski 2021-12-13 17:04 ` Richard Cochran 2021-12-13 18:40 ` Kurt Kanzenbach
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.