| Message ID | 20230904120209.741207-4-lukma@denx.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: hsr: Enable HSR HW offloading for KSZ9477 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Sep 04, 2023 at 02:02:08PM +0200, Lukasz Majewski wrote: > This patch adds functions for providing in KSZ9477 switch HSR > (High-availability Seamless Redundancy) hardware offloading. > > According to AN3474 application note following features are provided: > - TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP) > - RX packet duplication discarding > - Prevention of packet loop > > For last two ones - there is a probability that some packets will not > be filtered in HW (in some special cases). Hence, the HSR core code > shall be used to discard those not caught frames. > > Moreover, some switch registers adjustments are required - like setting > MAC address of HSR network interface. > > Additionally, the KSZ9477 switch has been configured to forward frames > between HSR ports (1,2) members. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > Changes for v2: > - Use struct ksz_device to store hsr ports information (not struct dsa) > > Changes for v3: > - Enable in-switch forwarding of frames between HSR ports (i.e. enable > bridging of those two ports) > > - The NETIF_F_HW_HSR_FWD flag has been marked as supported by the HSR > network device > > - Remove ETH MAC address validity check as it is done earlier in the net > driver > > - Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag > --- > drivers/net/dsa/microchip/ksz9477.c | 103 ++++++++++++++++++++++++++++ > drivers/net/dsa/microchip/ksz9477.h | 4 ++ > 2 files changed, 107 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c > index 83b7f2d5c1ea..c4ed89c1de48 100644 > --- a/drivers/net/dsa/microchip/ksz9477.c > +++ b/drivers/net/dsa/microchip/ksz9477.c > @@ -1141,6 +1141,109 @@ int ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val) > return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val); > } > > +/* The KSZ9477 provides following HW features to accelerate > + * HSR frames handling: > + * > + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH > + * 2. RX PACKET DUPLICATION DISCARDING > + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING > + * > + * Only one from point 1. has the NETIF_F* flag available. > + * > + * Ones from point 2 and 3 are "best effort" - i.e. those will > + * work correctly most of the time, but it may happen that some > + * frames will not be caught. Hence, the SW needs to handle those > + * special cases. However, the speed up gain is considerable when > + * above features are used. > + * > + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as HSR frames > + * can be forwarded in the switch fabric between HSR ports. How do these 2 concepts (autonomous forwarding + software-based elimination of some frames) work together? If software is not the sole receiver of traffic which needs to be filtered further, and duplicates also get forwarded to the network, does this not break the HSR ring? What are the causes due to which self-address filtering and duplicate elimination only work "most of the time"? > + */ > +#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_FWD) > + > +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > + struct dsa_port *partner) > +{ > + struct ksz_device *dev = ds->priv; > + struct net_device *slave; > + u8 i, data; > + int ret; > + > + /* Program which ports shall support HSR */ > + dev->hsr_ports = BIT(port) | BIT(partner->index); > + ksz_write32(dev, REG_HSR_PORT_MAP__4, dev->hsr_ports); > + > + /* Forward frames between HSR ports (i.e. bridge together HSR ports) */ > + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, > + dev->hsr_ports); > + ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4, > + dev->hsr_ports, dev->hsr_ports); Call ksz9477_cfg_port_member() instead? > + > + /* Enable discarding of received HSR frames */ > + ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data); > + data |= HSR_DUPLICATE_DISCARD; > + data &= ~HSR_NODE_UNICAST; > + ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data); > + > + /* Self MAC address filtering for HSR frames to avoid > + * traverse of the HSR ring more than once. > + * > + * The HSR port (i.e. hsr0) MAC address is used. > + */ > + for (i = 0; i < ETH_ALEN; i++) { > + ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, hsr->dev_addr[i]); > + if (ret) > + return ret; FWIW: https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/ Some coordination will be required regarding the MAC address that the switch driver needs to program to these registers. It seems that it is not single purpose. > + } > + > + /* Enable global self-address filtering if not yet done during switch > + * start > + */ > + ksz_read8(dev, REG_SW_LUE_CTRL_1, &data); > + if (!(data & SW_SRC_ADDR_FILTER)) { > + data |= SW_SRC_ADDR_FILTER; > + ksz_write8(dev, REG_SW_LUE_CTRL_1, data); > + } If there is no way that SW_SRC_ADDR_FILTER can be unset after ksz9477_reset_switch() is called, then this is dead code which should be removed. > + > + /* Enable per port self-address filtering */ > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, true); > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > + PORT_SRC_ADDR_FILTER, true); > + > + /* Setup HW supported features for lan HSR ports */ > + slave = dsa_to_port(ds, port)->slave; > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > + > + slave = dsa_to_port(ds, partner->index)->slave; > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; Can the code that is duplicated for the partner port be moved to the caller? > + > + pr_debug("%s: HSR join port: %d partner: %d port_map: 0x%x\n", __func__, > + port, partner->index, dev->hsr_ports); > + > + return 0; > +} > + > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr, > + struct dsa_port *partner) > +{ > + struct ksz_device *dev = ds->priv; > + > + /* Clear ports HSR support */ > + ksz_write32(dev, REG_HSR_PORT_MAP__4, 0); > + > + /* Disable forwarding frames between HSR ports */ > + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0); > + ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4, > + dev->hsr_ports, 0); > + > + /* Disable per port self-address filtering */ > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false); > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > + PORT_SRC_ADDR_FILTER, false); > + > + return 0; > +} > + > int ksz9477_switch_init(struct ksz_device *dev) > { > u8 data8; > diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h > index b6f7e3c46e3f..634262efb73c 100644 > --- a/drivers/net/dsa/microchip/ksz9477.h > +++ b/drivers/net/dsa/microchip/ksz9477.h > @@ -58,5 +58,9 @@ int ksz9477_dsa_init(struct ksz_device *dev); > int ksz9477_switch_init(struct ksz_device *dev); > void ksz9477_switch_exit(struct ksz_device *dev); > void ksz9477_port_queue_split(struct ksz_device *dev, int port); > +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > + struct dsa_port *partner); > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr, > + struct dsa_port *partner); > > #endif > -- > 2.20.1 >
Hi Vladimir, > On Mon, Sep 04, 2023 at 02:02:08PM +0200, Lukasz Majewski wrote: > > This patch adds functions for providing in KSZ9477 switch HSR > > (High-availability Seamless Redundancy) hardware offloading. > > > > According to AN3474 application note following features are > > provided: > > - TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP) > > - RX packet duplication discarding > > - Prevention of packet loop > > > > For last two ones - there is a probability that some packets will > > not be filtered in HW (in some special cases). Hence, the HSR core > > code shall be used to discard those not caught frames. > > > > Moreover, some switch registers adjustments are required - like > > setting MAC address of HSR network interface. > > > > Additionally, the KSZ9477 switch has been configured to forward > > frames between HSR ports (1,2) members. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > Changes for v2: > > - Use struct ksz_device to store hsr ports information (not struct > > dsa) > > > > Changes for v3: > > - Enable in-switch forwarding of frames between HSR ports (i.e. > > enable bridging of those two ports) > > > > - The NETIF_F_HW_HSR_FWD flag has been marked as supported by the > > HSR network device > > > > - Remove ETH MAC address validity check as it is done earlier in > > the net driver > > > > - Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag > > --- > > drivers/net/dsa/microchip/ksz9477.c | 103 > > ++++++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz9477.h | > > 4 ++ 2 files changed, 107 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz9477.c > > b/drivers/net/dsa/microchip/ksz9477.c index > > 83b7f2d5c1ea..c4ed89c1de48 100644 --- > > a/drivers/net/dsa/microchip/ksz9477.c +++ > > b/drivers/net/dsa/microchip/ksz9477.c @@ -1141,6 +1141,109 @@ int > > ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val) > > return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val); > > } > > +/* The KSZ9477 provides following HW features to accelerate > > + * HSR frames handling: > > + * > > + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH > > + * 2. RX PACKET DUPLICATION DISCARDING > > + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING > > + * > > + * Only one from point 1. has the NETIF_F* flag available. > > + * > > + * Ones from point 2 and 3 are "best effort" - i.e. those will > > + * work correctly most of the time, but it may happen that some > > + * frames will not be caught. Hence, the SW needs to handle those > > + * special cases. However, the speed up gain is considerable when > > + * above features are used. > > + * > > + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as > > HSR frames > > + * can be forwarded in the switch fabric between HSR ports. > > How do these 2 concepts (autonomous forwarding + software-based > elimination of some frames) work together? If software is not the sole > receiver of traffic which needs to be filtered further, and duplicates > also get forwarded to the network, does this not break the HSR ring? > Autonomous forwarding is based on KSZ9477, having the HSR ports "bridged" to send frames between them. Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX packet duplication discarding which will discard duplicated frames. Last but not least the - packet loop prevention. My understanding is as follows: 1. RX packet duplication removes copy of a frame, which is addressed to cpu port of switch. 2. The "bridge" of HSR passes frames in-KSZ9477, which are not addressed to this cpu host (between other HSR nodes). 3. Packet loop prevention - the HSR packet with SA of note which sent it - is not further forwarded. > What are the causes due to which self-address filtering and duplicate > elimination only work "most of the time"? Please refer to section "KSZ9477 CHIP LIMITATIONS" in: https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf > > > + */ > > +#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP | > > NETIF_F_HW_HSR_FWD) + > > +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct > > net_device *hsr, > > + struct dsa_port *partner) > > +{ > > + struct ksz_device *dev = ds->priv; > > + struct net_device *slave; > > + u8 i, data; > > + int ret; > > + > > + /* Program which ports shall support HSR */ > > + dev->hsr_ports = BIT(port) | BIT(partner->index); > > + ksz_write32(dev, REG_HSR_PORT_MAP__4, dev->hsr_ports); > > + > > + /* Forward frames between HSR ports (i.e. bridge together > > HSR ports) */ > > + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, > > dev->hsr_ports, > > + dev->hsr_ports); > > + ksz_prmw32(dev, partner->index, > > REG_PORT_VLAN_MEMBERSHIP__4, > > + dev->hsr_ports, dev->hsr_ports); > > Call ksz9477_cfg_port_member() instead? +1 Thanks for the information. > > > + > > + /* Enable discarding of received HSR frames */ > > + ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data); > > + data |= HSR_DUPLICATE_DISCARD; > > + data &= ~HSR_NODE_UNICAST; > > + ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data); > > + > > + /* Self MAC address filtering for HSR frames to avoid > > + * traverse of the HSR ring more than once. > > + * > > + * The HSR port (i.e. hsr0) MAC address is used. > > + */ > > + for (i = 0; i < ETH_ALEN; i++) { > > + ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, > > hsr->dev_addr[i]); > > + if (ret) > > + return ret; > > FWIW: > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/ > Some coordination will be required regarding the MAC address that the > switch driver needs to program to these registers. Writing of this MAC address is _required_ for PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING feature. In the ifconfig output - the lan1, lan2 and hsr0 shall all have the same MAC address assigned. I simply take the hsr0 mac address. > It seems that it > is not single purpose. At least in the case of HSR it looks like single purpose (for the loop prevention). > > > + } > > + > > + /* Enable global self-address filtering if not yet done > > during switch > > + * start > > + */ > > + ksz_read8(dev, REG_SW_LUE_CTRL_1, &data); > > + if (!(data & SW_SRC_ADDR_FILTER)) { > > + data |= SW_SRC_ADDR_FILTER; > > + ksz_write8(dev, REG_SW_LUE_CTRL_1, data); > > + } > > If there is no way that SW_SRC_ADDR_FILTER can be unset after > ksz9477_reset_switch() is called, then this is dead code which should > be removed. Yes. Correct. I will remove it. > > > + > > + /* Enable per port self-address filtering */ > > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, > > PORT_SRC_ADDR_FILTER, true); > > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > > + PORT_SRC_ADDR_FILTER, true); > > + > > + /* Setup HW supported features for lan HSR ports */ > > + slave = dsa_to_port(ds, port)->slave; > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > + > > + slave = dsa_to_port(ds, partner->index)->slave; > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > Can the code that is duplicated for the partner port be moved to the > caller? I've followed the convention from xrs700x driver, where we only make setup when we are sure that on both HSR ports the "join" has been called. > > > + > > + pr_debug("%s: HSR join port: %d partner: %d port_map: > > 0x%x\n", __func__, > > + port, partner->index, dev->hsr_ports); > > + > > + return 0; > > +} > > + > > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct > > net_device *hsr, > > + struct dsa_port *partner) > > +{ > > + struct ksz_device *dev = ds->priv; > > + > > + /* Clear ports HSR support */ > > + ksz_write32(dev, REG_HSR_PORT_MAP__4, 0); > > + > > + /* Disable forwarding frames between HSR ports */ > > + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, > > dev->hsr_ports, 0); > > + ksz_prmw32(dev, partner->index, > > REG_PORT_VLAN_MEMBERSHIP__4, > > + dev->hsr_ports, 0); > > + > > + /* Disable per port self-address filtering */ > > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, > > PORT_SRC_ADDR_FILTER, false); > > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > > + PORT_SRC_ADDR_FILTER, false); > > + > > + return 0; > > +} > > + > > int ksz9477_switch_init(struct ksz_device *dev) > > { > > u8 data8; > > diff --git a/drivers/net/dsa/microchip/ksz9477.h > > b/drivers/net/dsa/microchip/ksz9477.h index > > b6f7e3c46e3f..634262efb73c 100644 --- > > a/drivers/net/dsa/microchip/ksz9477.h +++ > > b/drivers/net/dsa/microchip/ksz9477.h @@ -58,5 +58,9 @@ int > > ksz9477_dsa_init(struct ksz_device *dev); int > > ksz9477_switch_init(struct ksz_device *dev); void > > ksz9477_switch_exit(struct ksz_device *dev); void > > ksz9477_port_queue_split(struct ksz_device *dev, int port); +int > > ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device > > *hsr, > > + struct dsa_port *partner); > > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct > > net_device *hsr, > > + struct dsa_port *partner); > > > > #endif > > -- > > 2.20.1 > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Sep 05, 2023 at 01:11:03PM +0200, Lukasz Majewski wrote: > > > +/* The KSZ9477 provides following HW features to accelerate > > > + * HSR frames handling: > > > + * > > > + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH > > > + * 2. RX PACKET DUPLICATION DISCARDING > > > + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING > > > + * > > > + * Only one from point 1. has the NETIF_F* flag available. > > > + * > > > + * Ones from point 2 and 3 are "best effort" - i.e. those will > > > + * work correctly most of the time, but it may happen that some > > > + * frames will not be caught. Hence, the SW needs to handle those > > > + * special cases. However, the speed up gain is considerable when > > > + * above features are used. > > > + * > > > + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as HSR frames > > > + * can be forwarded in the switch fabric between HSR ports. > > > > How do these 2 concepts (autonomous forwarding + software-based > > elimination of some frames) work together? If software is not the sole > > receiver of traffic which needs to be filtered further, and duplicates > > also get forwarded to the network, does this not break the HSR ring? > > > > Autonomous forwarding is based on KSZ9477, having the HSR ports > "bridged" to send frames between them. > > Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX > packet duplication discarding which will discard duplicated frames. > > Last but not least the - packet loop prevention. > > My understanding is as follows: > > 1. RX packet duplication removes copy of a frame, which is addressed to > cpu port of switch. Does the duplicate elimination function only do that, as you say, or is it supposed to also eliminate duplicates for packets flooded to 2 ports as well (the host port, and the other ring port)? If the latter, then it will fail to eliminate duplicates for packets that didn't visit the CPU. So the ring will rely on the self-address filtering capability of the other devices, in order not to collapse. I see that the software implementation also offers self-address filtering in hsr_handle_frame() -> hsr_addr_is_self(), but I don't see anything making that mandatory in IEC 62439-3:2018. Can we assume that the other ring members will know how to deal with it? > 2. The "bridge" of HSR passes frames in-KSZ9477, which are not > addressed to this cpu host (between other HSR nodes). How is the switch supposed to know which packets are addressed to this CPU port and which are not? I expect separate answers for unicast, multicast and broadcast. > 3. Packet loop prevention - the HSR packet with SA of note which sent > it - is not further forwarded. > > > What are the causes due to which self-address filtering and duplicate > > elimination only work "most of the time"? > > Please refer to section "KSZ9477 CHIP LIMITATIONS" in: > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf Ok, so the limitation is a race condition in hardware such that, when duplicate packets are received on member ports very close in time to each other, the hardware fails to detect that they're duplicates. > > > + /* Enable discarding of received HSR frames */ > > > + ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data); > > > + data |= HSR_DUPLICATE_DISCARD; > > > + data &= ~HSR_NODE_UNICAST; > > > + ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data); > > > + > > > + /* Self MAC address filtering for HSR frames to avoid > > > + * traverse of the HSR ring more than once. > > > + * > > > + * The HSR port (i.e. hsr0) MAC address is used. > > > + */ > > > + for (i = 0; i < ETH_ALEN; i++) { > > > + ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, hsr->dev_addr[i]); > > > + if (ret) > > > + return ret; > > > > FWIW: > > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/ > > Some coordination will be required regarding the MAC address that the > > switch driver needs to program to these registers. > > Writing of this MAC address is _required_ for PREVENTING PACKET LOOP IN > THE RING BY SELF-ADDRESS FILTERING feature. In case it was not clear, I was talking about coordination between you and Oleksij. He needs to program the same register for Wake on LAN. > In the ifconfig output - the lan1, lan2 and hsr0 shall all have the > same MAC address assigned. > > I simply take the hsr0 mac address. > > > It seems that it is not single purpose. > > At least in the case of HSR it looks like single purpose (for the loop > prevention). And for WoL, REG_SW_MAC_ADDR_0 is also single purpose. And for the MAC SA in the generated PAUSE frames, also single purpose. Single + single + single = ? Being a common register for multiple functions, I hope that it won't be the users who discover than when multiple functionalities are used in tandem (like WoL+HSR), they partially overwrite what the other has done. So, by coordination, I mean something like a cohesive way of thinking out the driver. For WoL/pause frames, the linked discussion suggested that the switch MAC address could be kept in sync with the DSA master's MAC address. Could that also work here, and could we add a restriction to say "Offload not supported for HSR device with a MAC address different from the DSA master" and return -EOPNOTSUPP in ksz_hsr_join()? Then ksz9477_hsr_join() would not modify this register. > > > + /* Enable per port self-address filtering */ > > > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, > > > PORT_SRC_ADDR_FILTER, true); > > > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > > > + PORT_SRC_ADDR_FILTER, true); > > > + > > > + /* Setup HW supported features for lan HSR ports */ > > > + slave = dsa_to_port(ds, port)->slave; > > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > > + > > > + slave = dsa_to_port(ds, partner->index)->slave; > > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > > > Can the code that is duplicated for the partner port be moved to the > > caller? > > I've followed the convention from xrs700x driver, where we only make > setup when we are sure that on both HSR ports the "join" has been > called. If code that is duplicated for both member ports can be moved to code paths that get executed for each individual port, then the resulting driver code may turn out to be less complex. It's not a big deal if it can't, and maintaining a sane operating mode in that transient state (HSR device with a single member port) is obviously much more important. But the question was if it can, and I don't think you've answered that?
On Tue, Sep 05, 2023 at 04:03:55PM +0300, Vladimir Oltean wrote: > > > What are the causes due to which self-address filtering and duplicate > > > elimination only work "most of the time"? > > > > Please refer to section "KSZ9477 CHIP LIMITATIONS" in: > > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf > > Ok, so the limitation is a race condition in hardware such that, when > duplicate packets are received on member ports very close in time to > each other, the hardware fails to detect that they're duplicates. It would be good to leave at least the link as part of the comment, if not also a short summary (in case the PDF URL gets moved/removed).
Hi Vladimir, > On Tue, Sep 05, 2023 at 01:11:03PM +0200, Lukasz Majewski wrote: > > > > +/* The KSZ9477 provides following HW features to accelerate > > > > + * HSR frames handling: > > > > + * > > > > + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH > > > > + * 2. RX PACKET DUPLICATION DISCARDING > > > > + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS > > > > FILTERING > > > > + * > > > > + * Only one from point 1. has the NETIF_F* flag available. > > > > + * > > > > + * Ones from point 2 and 3 are "best effort" - i.e. those will > > > > + * work correctly most of the time, but it may happen that some > > > > + * frames will not be caught. Hence, the SW needs to handle > > > > those > > > > + * special cases. However, the speed up gain is considerable > > > > when > > > > + * above features are used. > > > > + * > > > > + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, > > > > as HSR frames > > > > + * can be forwarded in the switch fabric between HSR ports. > > > > > > How do these 2 concepts (autonomous forwarding + software-based > > > elimination of some frames) work together? If software is not the > > > sole receiver of traffic which needs to be filtered further, and > > > duplicates also get forwarded to the network, does this not break > > > the HSR ring? > > > > Autonomous forwarding is based on KSZ9477, having the HSR ports > > "bridged" to send frames between them. > > > > Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX > > packet duplication discarding which will discard duplicated frames. > > > > Last but not least the - packet loop prevention. > > > > My understanding is as follows: > > > > 1. RX packet duplication removes copy of a frame, which is > > addressed to cpu port of switch. > > Does the duplicate elimination function only do that, as you say, or > is it supposed to also eliminate duplicates for packets flooded to 2 > ports as well (the host port, and the other ring port)? > In the "RX PACKET DUPLICATION DISCARDING" (from [1]) the hash of DA and SA and seq number is used to assess if received frame is a duplicated. ----8<------- If a matching (based on above??) frame has already been received on the other port, the frame is dropped. If not, then standard forwarding rules apply. ---->8------- > If the latter, then it will fail to eliminate duplicates for packets > that didn't visit the CPU. So the ring will rely on the self-address > filtering capability of the other devices, in order not to collapse. > I see that the software implementation also offers self-address > filtering in hsr_handle_frame() -> hsr_addr_is_self(), but I don't see > anything making that mandatory in IEC 62439-3:2018. Can we assume that > the other ring members will know how to deal with it? > > > 2. The "bridge" of HSR passes frames in-KSZ9477, which are not > > addressed to this cpu host (between other HSR nodes). > > How is the switch supposed to know which packets are addressed to this > CPU port and which are not? I expect separate answers for unicast, > multicast and broadcast. > > > 3. Packet loop prevention - the HSR packet with SA of note which > > sent it - is not further forwarded. > > > > > What are the causes due to which self-address filtering and > > > duplicate elimination only work "most of the time"? > > > > Please refer to section "KSZ9477 CHIP LIMITATIONS" in: > > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf > > > > Ok, so the limitation is a race condition in hardware such that, when > duplicate packets are received on member ports very close in time to > each other, the hardware fails to detect that they're duplicates. +1. > > > > > + /* Enable discarding of received HSR frames */ > > > > + ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data); > > > > + data |= HSR_DUPLICATE_DISCARD; > > > > + data &= ~HSR_NODE_UNICAST; > > > > + ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data); > > > > + > > > > + /* Self MAC address filtering for HSR frames to avoid > > > > + * traverse of the HSR ring more than once. > > > > + * > > > > + * The HSR port (i.e. hsr0) MAC address is used. > > > > + */ > > > > + for (i = 0; i < ETH_ALEN; i++) { > > > > + ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, > > > > hsr->dev_addr[i]); > > > > + if (ret) > > > > + return ret; > > > > > > FWIW: > > > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/ > > > Some coordination will be required regarding the MAC address that > > > the switch driver needs to program to these registers. > > > > Writing of this MAC address is _required_ for PREVENTING PACKET > > LOOP IN THE RING BY SELF-ADDRESS FILTERING feature. > > In case it was not clear, I was talking about coordination between you > and Oleksij. He needs to program the same register for Wake on LAN. Ok. Thanks for the information. > > > In the ifconfig output - the lan1, lan2 and hsr0 shall all have the > > same MAC address assigned. > > > > I simply take the hsr0 mac address. > > > > > It seems that it is not single purpose. > > > > At least in the case of HSR it looks like single purpose (for the > > loop prevention). > > And for WoL, REG_SW_MAC_ADDR_0 is also single purpose. And for the > MAC SA in the generated PAUSE frames, also single purpose. Single + > single + single = ? > > Being a common register for multiple functions, I hope that it won't > be the users who discover than when multiple functionalities are used > in tandem (like WoL+HSR), they partially overwrite what the other has > done. This cannot be excluded, yes. However, I think that HSR ports will not use WoL... > > So, by coordination, I mean something like a cohesive way of thinking > out the driver. > > For WoL/pause frames, the linked discussion suggested that the switch > MAC address could be kept in sync with the DSA master's MAC address. > > Could that also work here, and could we add a restriction to say > "Offload not supported for HSR device with a MAC address different > from the DSA master" and return -EOPNOTSUPP in ksz_hsr_join()? Then > ksz9477_hsr_join() would not modify this register. In my case - the lanX ports (and hsr0) gets the same MAC address as it is assigned to eth0 (the cpu port). So, yes I can add such code to avoid problems. > > > > > + /* Enable per port self-address filtering */ > > > > + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, > > > > PORT_SRC_ADDR_FILTER, true); > > > > + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, > > > > + PORT_SRC_ADDR_FILTER, true); > > > > + > > > > + /* Setup HW supported features for lan HSR ports */ > > > > + slave = dsa_to_port(ds, port)->slave; > > > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > > > + > > > > + slave = dsa_to_port(ds, partner->index)->slave; > > > > + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; > > > > > > Can the code that is duplicated for the partner port be moved to > > > the caller? > > > > I've followed the convention from xrs700x driver, where we only make > > setup when we are sure that on both HSR ports the "join" has been > > called. > > If code that is duplicated for both member ports can be moved to code > paths that get executed for each individual port, then the resulting > driver code may turn out to be less complex. > > It's not a big deal if it can't, and maintaining a sane operating mode > in that transient state (HSR device with a single member port) is > obviously much more important. But the question was if it can, and I > don't think you've answered that? I think that it would be possible to have single execution patch per joining port (for example join is called on lan1 and then on lan2). I'm concerned with writing common registers - for example REG_HSR_PORT_MAP. If there are no side effects (like hsr port support must be set for both at once), we can try to make single execution path. Links: [1] - https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Vladimir, > On Tue, Sep 05, 2023 at 04:03:55PM +0300, Vladimir Oltean wrote: > > > > What are the causes due to which self-address filtering and > > > > duplicate elimination only work "most of the time"? > > > > > > Please refer to section "KSZ9477 CHIP LIMITATIONS" in: > > > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf > > > > > > > Ok, so the limitation is a race condition in hardware such that, > > when duplicate packets are received on member ports very close in > > time to each other, the hardware fails to detect that they're > > duplicates. > > It would be good to leave at least the link as part of the comment, > if not also a short summary (in case the PDF URL gets moved/removed). Ok. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 83b7f2d5c1ea..c4ed89c1de48 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -1141,6 +1141,109 @@ int ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val) return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val); } +/* The KSZ9477 provides following HW features to accelerate + * HSR frames handling: + * + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH + * 2. RX PACKET DUPLICATION DISCARDING + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING + * + * Only one from point 1. has the NETIF_F* flag available. + * + * Ones from point 2 and 3 are "best effort" - i.e. those will + * work correctly most of the time, but it may happen that some + * frames will not be caught. Hence, the SW needs to handle those + * special cases. However, the speed up gain is considerable when + * above features are used. + * + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as HSR frames + * can be forwarded in the switch fabric between HSR ports. + */ +#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_FWD) + +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, + struct dsa_port *partner) +{ + struct ksz_device *dev = ds->priv; + struct net_device *slave; + u8 i, data; + int ret; + + /* Program which ports shall support HSR */ + dev->hsr_ports = BIT(port) | BIT(partner->index); + ksz_write32(dev, REG_HSR_PORT_MAP__4, dev->hsr_ports); + + /* Forward frames between HSR ports (i.e. bridge together HSR ports) */ + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, + dev->hsr_ports); + ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4, + dev->hsr_ports, dev->hsr_ports); + + /* Enable discarding of received HSR frames */ + ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data); + data |= HSR_DUPLICATE_DISCARD; + data &= ~HSR_NODE_UNICAST; + ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data); + + /* Self MAC address filtering for HSR frames to avoid + * traverse of the HSR ring more than once. + * + * The HSR port (i.e. hsr0) MAC address is used. + */ + for (i = 0; i < ETH_ALEN; i++) { + ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, hsr->dev_addr[i]); + if (ret) + return ret; + } + + /* Enable global self-address filtering if not yet done during switch + * start + */ + ksz_read8(dev, REG_SW_LUE_CTRL_1, &data); + if (!(data & SW_SRC_ADDR_FILTER)) { + data |= SW_SRC_ADDR_FILTER; + ksz_write8(dev, REG_SW_LUE_CTRL_1, data); + } + + /* Enable per port self-address filtering */ + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, true); + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, + PORT_SRC_ADDR_FILTER, true); + + /* Setup HW supported features for lan HSR ports */ + slave = dsa_to_port(ds, port)->slave; + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; + + slave = dsa_to_port(ds, partner->index)->slave; + slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES; + + pr_debug("%s: HSR join port: %d partner: %d port_map: 0x%x\n", __func__, + port, partner->index, dev->hsr_ports); + + return 0; +} + +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr, + struct dsa_port *partner) +{ + struct ksz_device *dev = ds->priv; + + /* Clear ports HSR support */ + ksz_write32(dev, REG_HSR_PORT_MAP__4, 0); + + /* Disable forwarding frames between HSR ports */ + ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0); + ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4, + dev->hsr_ports, 0); + + /* Disable per port self-address filtering */ + ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false); + ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL, + PORT_SRC_ADDR_FILTER, false); + + return 0; +} + int ksz9477_switch_init(struct ksz_device *dev) { u8 data8; diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h index b6f7e3c46e3f..634262efb73c 100644 --- a/drivers/net/dsa/microchip/ksz9477.h +++ b/drivers/net/dsa/microchip/ksz9477.h @@ -58,5 +58,9 @@ int ksz9477_dsa_init(struct ksz_device *dev); int ksz9477_switch_init(struct ksz_device *dev); void ksz9477_switch_exit(struct ksz_device *dev); void ksz9477_port_queue_split(struct ksz_device *dev, int port); +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, + struct dsa_port *partner); +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr, + struct dsa_port *partner); #endif
This patch adds functions for providing in KSZ9477 switch HSR (High-availability Seamless Redundancy) hardware offloading. According to AN3474 application note following features are provided: - TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP) - RX packet duplication discarding - Prevention of packet loop For last two ones - there is a probability that some packets will not be filtered in HW (in some special cases). Hence, the HSR core code shall be used to discard those not caught frames. Moreover, some switch registers adjustments are required - like setting MAC address of HSR network interface. Additionally, the KSZ9477 switch has been configured to forward frames between HSR ports (1,2) members. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- Changes for v2: - Use struct ksz_device to store hsr ports information (not struct dsa) Changes for v3: - Enable in-switch forwarding of frames between HSR ports (i.e. enable bridging of those two ports) - The NETIF_F_HW_HSR_FWD flag has been marked as supported by the HSR network device - Remove ETH MAC address validity check as it is done earlier in the net driver - Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag --- drivers/net/dsa/microchip/ksz9477.c | 103 ++++++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz9477.h | 4 ++ 2 files changed, 107 insertions(+)