* [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches
@ 2021-02-01 14:04 George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag George McCollister
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: George McCollister @ 2021-02-01 14:04 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
Jonathan Corbet, netdev, George McCollister
Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
removal, forwarding and duplication on DSA switches.
This series adds offloading to the xrs700x DSA driver.
Resent. Jakub says "looks like this is not showing up in patchwork
probably because of the ML server issue - please repost"
Changes since RFC:
* Split hsr and dsa patches. (Florian Fainelli)
George McCollister (4):
net: hsr: generate supervision frame without HSR tag
net: hsr: add offloading support
net: dsa: add support for offloading HSR
net: dsa: xrs700x: add HSR offloading support
Documentation/networking/netdev-features.rst | 20 +++++
drivers/net/dsa/xrs700x/xrs700x.c | 106 +++++++++++++++++++++++++++
drivers/net/dsa/xrs700x/xrs700x_reg.h | 5 ++
include/linux/if_hsr.h | 22 ++++++
include/linux/netdev_features.h | 9 +++
include/linux/netdevice.h | 13 ++++
include/net/dsa.h | 13 ++++
net/dsa/dsa_priv.h | 11 +++
net/dsa/port.c | 34 +++++++++
net/dsa/slave.c | 13 ++++
net/dsa/switch.c | 24 ++++++
net/dsa/tag_xrs700x.c | 7 +-
net/ethtool/common.c | 4 +
net/hsr/hsr_device.c | 44 ++---------
net/hsr/hsr_forward.c | 37 ++++++++--
net/hsr/hsr_forward.h | 1 +
net/hsr/hsr_main.c | 14 ++++
net/hsr/hsr_main.h | 8 +-
net/hsr/hsr_slave.c | 13 +++-
19 files changed, 343 insertions(+), 55 deletions(-)
create mode 100644 include/linux/if_hsr.h
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-01 14:04 [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches George McCollister
@ 2021-02-01 14:05 ` George McCollister
2021-02-01 14:59 ` Vladimir Oltean
2021-02-01 14:05 ` [RESEND PATCH net-next 2/4] net: hsr: add offloading support George McCollister
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: George McCollister @ 2021-02-01 14:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
Jonathan Corbet, netdev, George McCollister
Generate supervision frame without HSR/PRP tag and rely on existing
code which inserts it later.
This will allow HSR/PRP tag insertions to be offloaded in the future.
Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
net/hsr/hsr_device.c | 32 ++++----------------------------
net/hsr/hsr_forward.c | 10 +++++++---
2 files changed, 11 insertions(+), 31 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index ab953a1a0d6c..161b8da6a21d 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
* being, for PRP it is a trailer and for HSR it is a
* header
*/
- skb = dev_alloc_skb(sizeof(struct hsr_tag) +
- sizeof(struct hsr_sup_tag) +
+ skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
sizeof(struct hsr_sup_payload) + hlen + tlen);
if (!skb)
@@ -275,12 +274,10 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
{
struct hsr_priv *hsr = master->hsr;
__u8 type = HSR_TLV_LIFE_CHECK;
- struct hsr_tag *hsr_tag = NULL;
struct hsr_sup_payload *hsr_sp;
struct hsr_sup_tag *hsr_stag;
unsigned long irqflags;
struct sk_buff *skb;
- u16 proto;
*interval = msecs_to_jiffies(HSR_LIFE_CHECK_INTERVAL);
if (hsr->announce_count < 3 && hsr->prot_version == 0) {
@@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
hsr->announce_count++;
}
- if (!hsr->prot_version)
- proto = ETH_P_PRP;
- else
- proto = ETH_P_HSR;
-
- skb = hsr_init_skb(master, proto);
+ skb = hsr_init_skb(master, ETH_P_PRP);
if (!skb) {
WARN_ONCE(1, "HSR: Could not send supervision frame\n");
return;
}
- if (hsr->prot_version > 0) {
- hsr_tag = skb_put(skb, sizeof(struct hsr_tag));
- hsr_tag->encap_proto = htons(ETH_P_PRP);
- set_hsr_tag_LSDU_size(hsr_tag, HSR_V1_SUP_LSDUSIZE);
- }
-
hsr_stag = skb_put(skb, sizeof(struct hsr_sup_tag));
set_hsr_stag_path(hsr_stag, (hsr->prot_version ? 0x0 : 0xf));
set_hsr_stag_HSR_ver(hsr_stag, hsr->prot_version);
@@ -315,8 +301,6 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
if (hsr->prot_version > 0) {
hsr_stag->sequence_nr = htons(hsr->sup_sequence_nr);
hsr->sup_sequence_nr++;
- hsr_tag->sequence_nr = htons(hsr->sequence_nr);
- hsr->sequence_nr++;
} else {
hsr_stag->sequence_nr = htons(hsr->sequence_nr);
hsr->sequence_nr++;
@@ -332,7 +316,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
- if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))
+ if (skb_put_padto(skb, ETH_ZLEN))
return;
hsr_forward_skb(skb, master);
@@ -348,8 +332,6 @@ static void send_prp_supervision_frame(struct hsr_port *master,
struct hsr_sup_tag *hsr_stag;
unsigned long irqflags;
struct sk_buff *skb;
- struct prp_rct *rct;
- u8 *tail;
skb = hsr_init_skb(master, ETH_P_PRP);
if (!skb) {
@@ -373,17 +355,11 @@ static void send_prp_supervision_frame(struct hsr_port *master,
hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
- if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN)) {
+ if (skb_put_padto(skb, ETH_ZLEN)) {
spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
return;
}
- tail = skb_tail_pointer(skb) - HSR_HLEN;
- rct = (struct prp_rct *)tail;
- rct->PRP_suffix = htons(ETH_P_PRP);
- set_prp_LSDU_size(rct, HSR_V1_SUP_LSDUSIZE);
- rct->sequence_nr = htons(hsr->sequence_nr);
- hsr->sequence_nr++;
spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
hsr_forward_skb(skb, master);
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index cadfccd7876e..a5566b2245a0 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -454,8 +454,10 @@ static void handle_std_frame(struct sk_buff *skb,
void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
struct hsr_frame_info *frame)
{
- if (proto == htons(ETH_P_PRP) ||
- proto == htons(ETH_P_HSR)) {
+ struct hsr_port *port = frame->port_rcv;
+
+ if (port->type != HSR_PT_MASTER &&
+ (proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
/* HSR tagged frame :- Data or Supervision */
frame->skb_std = NULL;
frame->skb_prp = NULL;
@@ -473,8 +475,10 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
{
/* Supervision frame */
struct prp_rct *rct = skb_get_PRP_rct(skb);
+ struct hsr_port *port = frame->port_rcv;
- if (rct &&
+ if (port->type != HSR_PT_MASTER &&
+ rct &&
prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
frame->skb_hsr = NULL;
frame->skb_std = NULL;
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RESEND PATCH net-next 2/4] net: hsr: add offloading support
2021-02-01 14:04 [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag George McCollister
@ 2021-02-01 14:05 ` George McCollister
2021-02-01 15:23 ` Vladimir Oltean
2021-02-01 14:05 ` [RESEND PATCH net-next 3/4] net: dsa: add support for offloading HSR George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support George McCollister
3 siblings, 1 reply; 17+ messages in thread
From: George McCollister @ 2021-02-01 14:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
Jonathan Corbet, netdev, George McCollister
Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
tag removal, duplicate generation and forwarding.
Use a new netdev_priv_flag IFF_HSR to indicate that a device is an HSR
device so DSA can tell them apart from other devices in
dsa_slave_changeupper.
For HSR, insertion involves the switch adding a 6 byte HSR header after
the 14 byte Ethernet header. For PRP it adds a 6 byte trailer.
Tag removal involves automatically stripping the HSR/PRP header/trailer
in the switch. This is possible when the switch also preforms auto
deduplication using the HSR/PRP header/trailer (making it no longer
required).
Forwarding involves automatically forwarding between redundant ports in
an HSR. This is crucial because delay is accumulated as a frame passes
through each node in the ring.
Duplication involves the switch automatically sending a single frame
from the CPU port to both redundant ports. This is required because the
inserted HSR/PRP header/trailer must contain the same sequence number
on the frames sent out both redundant ports.
Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
Documentation/networking/netdev-features.rst | 20 ++++++++++++++++++++
include/linux/if_hsr.h | 22 ++++++++++++++++++++++
include/linux/netdev_features.h | 9 +++++++++
include/linux/netdevice.h | 13 +++++++++++++
net/ethtool/common.c | 4 ++++
net/hsr/hsr_device.c | 12 +++---------
net/hsr/hsr_forward.c | 27 ++++++++++++++++++++++++---
net/hsr/hsr_forward.h | 1 +
net/hsr/hsr_main.c | 14 ++++++++++++++
net/hsr/hsr_main.h | 8 +-------
net/hsr/hsr_slave.c | 13 +++++++++----
11 files changed, 120 insertions(+), 23 deletions(-)
create mode 100644 include/linux/if_hsr.h
diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
index a2d7d7160e39..4eab45405031 100644
--- a/Documentation/networking/netdev-features.rst
+++ b/Documentation/networking/netdev-features.rst
@@ -182,3 +182,23 @@ stricter than Hardware LRO. A packet stream merged by Hardware GRO must
be re-segmentable by GSO or TSO back to the exact original packet stream.
Hardware GRO is dependent on RXCSUM since every packet successfully merged
by hardware must also have the checksum verified by hardware.
+
+* hsr-tag-ins-offload
+
+This should be set for devices which insert an HSR (highspeed ring) tag
+automatically when in HSR mode.
+
+* hsr-tag-rm-offload
+
+This should be set for devices which remove HSR (highspeed ring) tags
+automatically when in HSR mode.
+
+* hsr-fwd-offload
+
+This should be set for devices which forward HSR (highspeed ring) frames from
+one port to another in hardware.
+
+* hsr-dup-offload
+
+This should be set for devices which duplicate outgoing HSR (highspeed ring)
+frames in hardware.
diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
new file mode 100644
index 000000000000..eec9079efab0
--- /dev/null
+++ b/include/linux/if_hsr.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_IF_HSR_H_
+#define _LINUX_IF_HSR_H_
+
+/* used to differentiate various protocols */
+enum hsr_version {
+ HSR_V0 = 0,
+ HSR_V1,
+ PRP_V1,
+};
+
+#if IS_ENABLED(CONFIG_HSR)
+extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
+#else
+static inline int hsr_get_version(struct net_device *dev,
+ enum hsr_version *ver)
+{
+ return -EINVAL;
+}
+#endif /* CONFIG_HSR */
+
+#endif /*_LINUX_IF_HSR_H_*/
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index c06d6aaba9df..3de38d6a0aea 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -86,6 +86,11 @@ enum {
NETIF_F_HW_MACSEC_BIT, /* Offload MACsec operations */
NETIF_F_GRO_UDP_FWD_BIT, /* Allow UDP GRO for forwarding */
+ NETIF_F_HW_HSR_TAG_INS_BIT, /* Offload HSR tag insertion */
+ NETIF_F_HW_HSR_TAG_RM_BIT, /* Offload HSR tag removal */
+ NETIF_F_HW_HSR_FWD_BIT, /* Offload HSR forwarding */
+ NETIF_F_HW_HSR_DUP_BIT, /* Offload HSR duplication */
+
/*
* Add your fresh new feature above and remember to update
* netdev_features_strings[] in net/core/ethtool.c and maybe
@@ -159,6 +164,10 @@ enum {
#define NETIF_F_GSO_FRAGLIST __NETIF_F(GSO_FRAGLIST)
#define NETIF_F_HW_MACSEC __NETIF_F(HW_MACSEC)
#define NETIF_F_GRO_UDP_FWD __NETIF_F(GRO_UDP_FWD)
+#define NETIF_F_HW_HSR_TAG_INS __NETIF_F(HW_HSR_TAG_INS)
+#define NETIF_F_HW_HSR_TAG_RM __NETIF_F(HW_HSR_TAG_RM)
+#define NETIF_F_HW_HSR_FWD __NETIF_F(HW_HSR_FWD)
+#define NETIF_F_HW_HSR_DUP __NETIF_F(HW_HSR_DUP)
/* Finds the next feature with the highest number of the range of start till 0.
*/
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e9e7ada07ea1..9ac6f30c4a51 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1526,6 +1526,7 @@ struct net_device_ops {
* @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
* @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
* @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
+ * @IFF_HSR: device is an hsr device
*/
enum netdev_priv_flags {
IFF_802_1Q_VLAN = 1<<0,
@@ -1559,6 +1560,7 @@ enum netdev_priv_flags {
IFF_FAILOVER_SLAVE = 1<<28,
IFF_L3MDEV_RX_HANDLER = 1<<29,
IFF_LIVE_RENAME_OK = 1<<30,
+ IFF_HSR = 1<<31,
};
#define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
@@ -1591,6 +1593,7 @@ enum netdev_priv_flags {
#define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
#define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER
#define IFF_LIVE_RENAME_OK IFF_LIVE_RENAME_OK
+#define IFF_HSR IFF_HSR
/**
* struct net_device - The DEVICE structure.
@@ -5003,6 +5006,16 @@ static inline bool netif_is_failover_slave(const struct net_device *dev)
return dev->priv_flags & IFF_FAILOVER_SLAVE;
}
+static inline bool netif_is_hsr_master(const struct net_device *dev)
+{
+ return dev->flags & IFF_MASTER && dev->priv_flags & IFF_HSR;
+}
+
+static inline bool netif_is_hsr_slave(const struct net_device *dev)
+{
+ return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_HSR;
+}
+
/* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
static inline void netif_keep_dst(struct net_device *dev)
{
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 181220101a6e..0298e5635ace 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -69,6 +69,10 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
[NETIF_F_GRO_FRAGLIST_BIT] = "rx-gro-list",
[NETIF_F_HW_MACSEC_BIT] = "macsec-hw-offload",
[NETIF_F_GRO_UDP_FWD_BIT] = "rx-udp-gro-forwarding",
+ [NETIF_F_HW_HSR_TAG_INS_BIT] = "hsr-tag-ins-offload",
+ [NETIF_F_HW_HSR_TAG_RM_BIT] = "hsr-tag-rm-offload",
+ [NETIF_F_HW_HSR_FWD_BIT] = "hsr-fwd-offload",
+ [NETIF_F_HW_HSR_DUP_BIT] = "hsr-dup-offload",
};
const char
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 161b8da6a21d..d9b033e9b18c 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -418,6 +418,7 @@ static struct hsr_proto_ops hsr_ops = {
.send_sv_frame = send_hsr_supervision_frame,
.create_tagged_frame = hsr_create_tagged_frame,
.get_untagged_frame = hsr_get_untagged_frame,
+ .drop_frame = hsr_drop_frame,
.fill_frame_info = hsr_fill_frame_info,
.invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
};
@@ -521,15 +522,8 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
hsr->prot_version = protocol_version;
- /* FIXME: should I modify the value of these?
- *
- * - hsr_dev->flags - i.e.
- * IFF_MASTER/SLAVE?
- * - hsr_dev->priv_flags - i.e.
- * IFF_EBRIDGE?
- * IFF_TX_SKB_SHARING?
- * IFF_HSR_MASTER/SLAVE?
- */
+ hsr_dev->flags |= IFF_MASTER;
+ hsr_dev->priv_flags |= IFF_HSR;
/* Make sure the 1st call to netif_carrier_on() gets through */
netif_carrier_off(hsr_dev);
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index a5566b2245a0..9c79d602c4e0 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -247,6 +247,8 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
/* set the lane id properly */
hsr_set_path_id(hsr_ethhdr, port);
return skb_clone(frame->skb_hsr, GFP_ATOMIC);
+ } else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
+ return skb_clone(frame->skb_std, GFP_ATOMIC);
}
/* Create the new skb with enough headroom to fit the HSR tag */
@@ -341,6 +343,14 @@ bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
port->type == HSR_PT_SLAVE_A));
}
+bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
+{
+ if (port->dev->features & NETIF_F_HW_HSR_FWD)
+ return prp_drop_frame(frame, port);
+
+ return false;
+}
+
/* Forward the frame through all devices except:
* - Back through the receiving device
* - If it's a HSR frame: through a device where it has passed before
@@ -357,6 +367,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
{
struct hsr_port *port;
struct sk_buff *skb;
+ bool sent = false;
hsr_for_each_port(frame->port_rcv->hsr, port) {
struct hsr_priv *hsr = port->hsr;
@@ -372,6 +383,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
continue;
+ /* If hardware duplicate generation is enabled, only send out
+ * one port.
+ */
+ if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
+ continue;
+
/* Don't send frame over port where it has been sent before.
* Also fro SAN, this shouldn't be done.
*/
@@ -403,10 +420,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
}
skb->dev = port->dev;
- if (port->type == HSR_PT_MASTER)
+ if (port->type == HSR_PT_MASTER) {
hsr_deliver_master(skb, port->dev, frame->node_src);
- else
- hsr_xmit(skb, port, frame);
+ } else {
+ if (!hsr_xmit(skb, port, frame))
+ sent = true;
+ }
}
}
@@ -457,6 +476,7 @@ void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
struct hsr_port *port = frame->port_rcv;
if (port->type != HSR_PT_MASTER &&
+ !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
(proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
/* HSR tagged frame :- Data or Supervision */
frame->skb_std = NULL;
@@ -478,6 +498,7 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
struct hsr_port *port = frame->port_rcv;
if (port->type != HSR_PT_MASTER &&
+ !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
rct &&
prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
frame->skb_hsr = NULL;
diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
index 618140d484ad..b6acaafa83fc 100644
--- a/net/hsr/hsr_forward.h
+++ b/net/hsr/hsr_forward.h
@@ -23,6 +23,7 @@ struct sk_buff *hsr_get_untagged_frame(struct hsr_frame_info *frame,
struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
struct hsr_port *port);
bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
+bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
struct hsr_frame_info *frame);
void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
index 2fd1976e5b1c..0e7b5b18b5e3 100644
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -131,6 +131,20 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
return NULL;
}
+int hsr_get_version(struct net_device *dev, enum hsr_version *ver)
+{
+ struct hsr_priv *hsr;
+
+ if (!(dev->priv_flags & IFF_HSR))
+ return -EINVAL;
+
+ hsr = netdev_priv(dev);
+ *ver = hsr->prot_version;
+
+ return 0;
+}
+EXPORT_SYMBOL(hsr_get_version);
+
static struct notifier_block hsr_nb = {
.notifier_call = hsr_netdev_notify, /* Slave event notifications */
};
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index 7dc92ce5a134..7369b2febe0f 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -13,6 +13,7 @@
#include <linux/netdevice.h>
#include <linux/list.h>
#include <linux/if_vlan.h>
+#include <linux/if_hsr.h>
/* Time constants as specified in the HSR specification (IEC-62439-3 2010)
* Table 8.
@@ -171,13 +172,6 @@ struct hsr_port {
enum hsr_port_type type;
};
-/* used by driver internally to differentiate various protocols */
-enum hsr_version {
- HSR_V0 = 0,
- HSR_V1,
- PRP_V1,
-};
-
struct hsr_frame_info;
struct hsr_node;
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 36d5fcf09c61..59f8d2b68376 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
goto finish_consume;
}
- /* For HSR, only tagged frames are expected, but for PRP
- * there could be non tagged frames as well from Single
- * attached nodes (SANs).
+ /* For HSR, only tagged frames are expected (unless the device offloads
+ * HSR tag removal), but for PRP there could be non tagged frames as
+ * well from Single attached nodes (SANs).
*/
protocol = eth_hdr(skb)->h_proto;
- if (hsr->proto_ops->invalid_dan_ingress_frame &&
+
+ if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
+ hsr->proto_ops->invalid_dan_ingress_frame &&
hsr->proto_ops->invalid_dan_ingress_frame(protocol))
goto finish_pass;
@@ -137,6 +139,9 @@ static int hsr_portdev_setup(struct hsr_priv *hsr, struct net_device *dev,
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
hsr_dev = master->dev;
+ dev->flags |= IFF_SLAVE;
+ dev->priv_flags |= IFF_HSR;
+
res = netdev_upper_dev_link(dev, hsr_dev, extack);
if (res)
goto fail_upper_dev_link;
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RESEND PATCH net-next 3/4] net: dsa: add support for offloading HSR
2021-02-01 14:04 [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 2/4] net: hsr: add offloading support George McCollister
@ 2021-02-01 14:05 ` George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support George McCollister
3 siblings, 0 replies; 17+ messages in thread
From: George McCollister @ 2021-02-01 14:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
Jonathan Corbet, netdev, George McCollister
Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
tag removal, duplicate generation and forwarding on DSA switches.
Add DSA_NOTIFIER_HSR_JOIN and DSA_NOTIFIER_HSR_LEAVE which trigger calls
to .port_hsr_join and .port_hsr_leave in the DSA driver for the switch.
The DSA switch driver should then set netdev feature flags for the
HSR/PRP operation that it offloads.
NETIF_F_HW_HSR_TAG_INS
NETIF_F_HW_HSR_TAG_RM
NETIF_F_HW_HSR_FWD
NETIF_F_HW_HSR_DUP
Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
include/net/dsa.h | 13 +++++++++++++
net/dsa/dsa_priv.h | 11 +++++++++++
net/dsa/port.c | 34 ++++++++++++++++++++++++++++++++++
net/dsa/slave.c | 13 +++++++++++++
net/dsa/switch.c | 24 ++++++++++++++++++++++++
5 files changed, 95 insertions(+)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 2f5435d3d1db..584e2b5c02e0 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -167,6 +167,10 @@ struct dsa_switch_tree {
list_for_each_entry((_dp), &(_dst)->ports, list) \
if ((_dp)->lag_dev == (_lag))
+#define dsa_hsr_foreach_port(_dp, _ds, _hsr) \
+ list_for_each_entry((_dp), &(_ds)->dst->ports, list) \
+ if ((_dp)->ds == (_ds) && (_dp)->hsr_dev == (_hsr))
+
static inline struct net_device *dsa_lag_dev(struct dsa_switch_tree *dst,
unsigned int id)
{
@@ -257,6 +261,7 @@ struct dsa_port {
struct phylink_config pl_config;
struct net_device *lag_dev;
bool lag_tx_enabled;
+ struct net_device *hsr_dev;
struct list_head list;
@@ -753,6 +758,14 @@ struct dsa_switch_ops {
struct netdev_lag_upper_info *info);
int (*port_lag_leave)(struct dsa_switch *ds, int port,
struct net_device *lag);
+
+ /*
+ * HSR integration
+ */
+ int (*port_hsr_join)(struct dsa_switch *ds, int port,
+ struct net_device *hsr);
+ void (*port_hsr_leave)(struct dsa_switch *ds, int port,
+ struct net_device *hsr);
};
#define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes) \
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2ce46bb87703..9006f62f69cd 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -20,6 +20,8 @@ enum {
DSA_NOTIFIER_BRIDGE_LEAVE,
DSA_NOTIFIER_FDB_ADD,
DSA_NOTIFIER_FDB_DEL,
+ DSA_NOTIFIER_HSR_JOIN,
+ DSA_NOTIFIER_HSR_LEAVE,
DSA_NOTIFIER_LAG_CHANGE,
DSA_NOTIFIER_LAG_JOIN,
DSA_NOTIFIER_LAG_LEAVE,
@@ -94,6 +96,13 @@ struct dsa_switchdev_event_work {
u16 vid;
};
+/* DSA_NOTIFIER_HSR_* */
+struct dsa_notifier_hsr_info {
+ struct net_device *hsr;
+ int sw_index;
+ int port;
+};
+
struct dsa_slave_priv {
/* Copy of CPU port xmit for faster access in slave transmit hot path */
struct sk_buff * (*xmit)(struct sk_buff *skb,
@@ -174,6 +183,8 @@ int dsa_port_vlan_del(struct dsa_port *dp,
const struct switchdev_obj_port_vlan *vlan);
int dsa_port_link_register_of(struct dsa_port *dp);
void dsa_port_link_unregister_of(struct dsa_port *dp);
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
+void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
static inline bool dsa_port_offloads_netdev(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index f5b0f72ee7cd..09738af4d32e 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -870,3 +870,37 @@ int dsa_port_get_phy_sset_count(struct dsa_port *dp)
return ret;
}
EXPORT_SYMBOL_GPL(dsa_port_get_phy_sset_count);
+
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr)
+{
+ struct dsa_notifier_hsr_info info = {
+ .sw_index = dp->ds->index,
+ .port = dp->index,
+ .hsr = hsr,
+ };
+ int err;
+
+ dp->hsr_dev = hsr;
+
+ err = dsa_port_notify(dp, DSA_NOTIFIER_HSR_JOIN, &info);
+ if (err)
+ dp->hsr_dev = NULL;
+
+ return err;
+}
+
+void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr)
+{
+ struct dsa_notifier_hsr_info info = {
+ .sw_index = dp->ds->index,
+ .port = dp->index,
+ .hsr = hsr,
+ };
+ int err;
+
+ dp->hsr_dev = NULL;
+
+ err = dsa_port_notify(dp, DSA_NOTIFIER_HSR_LEAVE, &info);
+ if (err)
+ pr_err("DSA: failed to notify DSA_NOTIFIER_HSR_LEAVE\n");
+}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f2fb433f3828..fc7e3ff11c5c 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1924,6 +1924,19 @@ static int dsa_slave_changeupper(struct net_device *dev,
dsa_port_lag_leave(dp, info->upper_dev);
err = NOTIFY_OK;
}
+ } else if (netif_is_hsr_master(info->upper_dev)) {
+ if (info->linking) {
+ err = dsa_port_hsr_join(dp, info->upper_dev);
+ if (err == -EOPNOTSUPP) {
+ NL_SET_ERR_MSG_MOD(info->info.extack,
+ "Offloading not supported");
+ err = 0;
+ }
+ err = notifier_from_errno(err);
+ } else {
+ dsa_port_hsr_leave(dp, info->upper_dev);
+ err = NOTIFY_OK;
+ }
}
return err;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index cc0b25f3adea..c1e5083f2cfc 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -166,6 +166,24 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
return ds->ops->port_fdb_del(ds, port, info->addr, info->vid);
}
+static int dsa_switch_hsr_join(struct dsa_switch *ds,
+ struct dsa_notifier_hsr_info *info)
+{
+ if (ds->index == info->sw_index && ds->ops->port_hsr_join)
+ return ds->ops->port_hsr_join(ds, info->port, info->hsr);
+
+ return 0;
+}
+
+static int dsa_switch_hsr_leave(struct dsa_switch *ds,
+ struct dsa_notifier_hsr_info *info)
+{
+ if (ds->index == info->sw_index && ds->ops->port_hsr_leave)
+ ds->ops->port_hsr_leave(ds, info->port, info->hsr);
+
+ return 0;
+}
+
static int dsa_switch_lag_change(struct dsa_switch *ds,
struct dsa_notifier_lag_info *info)
{
@@ -319,6 +337,12 @@ static int dsa_switch_event(struct notifier_block *nb,
case DSA_NOTIFIER_FDB_DEL:
err = dsa_switch_fdb_del(ds, info);
break;
+ case DSA_NOTIFIER_HSR_JOIN:
+ err = dsa_switch_hsr_join(ds, info);
+ break;
+ case DSA_NOTIFIER_HSR_LEAVE:
+ err = dsa_switch_hsr_leave(ds, info);
+ break;
case DSA_NOTIFIER_LAG_CHANGE:
err = dsa_switch_lag_change(ds, info);
break;
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support
2021-02-01 14:04 [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches George McCollister
` (2 preceding siblings ...)
2021-02-01 14:05 ` [RESEND PATCH net-next 3/4] net: dsa: add support for offloading HSR George McCollister
@ 2021-02-01 14:05 ` George McCollister
2021-02-01 15:29 ` Vladimir Oltean
3 siblings, 1 reply; 17+ messages in thread
From: George McCollister @ 2021-02-01 14:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
Jonathan Corbet, netdev, George McCollister
Add offloading for HSR/PRP (IEC 62439-3) tag insertion, tag removal
forwarding and duplication supported by the xrs7000 series switches.
Only HSR v1 and PRP v1 are supported by the xrs7000 series switches (HSR
v0 is not).
Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
drivers/net/dsa/xrs700x/xrs700x.c | 106 ++++++++++++++++++++++++++++++++++
drivers/net/dsa/xrs700x/xrs700x_reg.h | 5 ++
net/dsa/tag_xrs700x.c | 7 ++-
3 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 259f5e657c46..566ce9330903 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -7,6 +7,8 @@
#include <net/dsa.h>
#include <linux/if_bridge.h>
#include <linux/of_device.h>
+#include <linux/netdev_features.h>
+#include <linux/if_hsr.h>
#include "xrs700x.h"
#include "xrs700x_reg.h"
@@ -496,6 +498,108 @@ static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
xrs700x_bridge_common(ds, port, bridge, false);
}
+static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
+ struct net_device *hsr)
+{
+ unsigned int val = XRS_HSR_CFG_HSR_PRP;
+ struct dsa_port *partner = NULL, *dp;
+ struct xrs700x *priv = ds->priv;
+ struct net_device *slave;
+ enum hsr_version ver;
+ int ret;
+
+ ret = hsr_get_version(hsr, &ver);
+ if (ret)
+ return ret;
+
+ if (ver == HSR_V1)
+ val |= XRS_HSR_CFG_HSR;
+ else if (ver == PRP_V1)
+ val |= XRS_HSR_CFG_PRP;
+ else
+ return -EOPNOTSUPP;
+
+ dsa_hsr_foreach_port(dp, ds, hsr) {
+ partner = dp;
+ }
+
+ /* We can't enable redundancy on the switch until both
+ * redundant ports have signed up.
+ */
+ if (!partner)
+ return 0;
+
+ regmap_fields_write(priv->ps_forward, partner->index,
+ XRS_PORT_DISABLED);
+ regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
+
+ regmap_write(priv->regmap, XRS_HSR_CFG(partner->index),
+ val | XRS_HSR_CFG_LANID_A);
+ regmap_write(priv->regmap, XRS_HSR_CFG(port),
+ val | XRS_HSR_CFG_LANID_B);
+
+ /* Clear bits for both redundant ports (HSR only) and the CPU port to
+ * enable forwarding.
+ */
+ val = GENMASK(ds->num_ports - 1, 0);
+ if (ver == HSR_V1) {
+ val &= ~BIT(partner->index);
+ val &= ~BIT(port);
+ }
+ val &= ~BIT(dsa_upstream_port(ds, port));
+ regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
+ regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
+
+ regmap_fields_write(priv->ps_forward, partner->index,
+ XRS_PORT_FORWARDING);
+ regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
+
+ slave = dsa_to_port(ds, port)->slave;
+
+ slave->features |= NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
+ NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP;
+
+ return 0;
+}
+
+static void xrs700x_hsr_leave(struct dsa_switch *ds, int port,
+ struct net_device *hsr)
+{
+ struct dsa_port *partner = NULL, *dp;
+ struct xrs700x *priv = ds->priv;
+ struct net_device *slave;
+ unsigned int val;
+
+ dsa_hsr_foreach_port(dp, ds, hsr) {
+ partner = dp;
+ }
+
+ if (!partner)
+ return;
+
+ regmap_fields_write(priv->ps_forward, partner->index,
+ XRS_PORT_DISABLED);
+ regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
+
+ regmap_write(priv->regmap, XRS_HSR_CFG(partner->index), 0);
+ regmap_write(priv->regmap, XRS_HSR_CFG(port), 0);
+
+ /* Clear bit for the CPU port to enable forwarding. */
+ val = GENMASK(ds->num_ports - 1, 0);
+ val &= ~BIT(dsa_upstream_port(ds, port));
+ regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
+ regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
+
+ regmap_fields_write(priv->ps_forward, partner->index,
+ XRS_PORT_FORWARDING);
+ regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
+
+ slave = dsa_to_port(ds, port)->slave;
+
+ slave->features &= ~(NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
+ NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP);
+}
+
static const struct dsa_switch_ops xrs700x_ops = {
.get_tag_protocol = xrs700x_get_tag_protocol,
.setup = xrs700x_setup,
@@ -509,6 +613,8 @@ static const struct dsa_switch_ops xrs700x_ops = {
.get_stats64 = xrs700x_get_stats64,
.port_bridge_join = xrs700x_bridge_join,
.port_bridge_leave = xrs700x_bridge_leave,
+ .port_hsr_join = xrs700x_hsr_join,
+ .port_hsr_leave = xrs700x_hsr_leave,
};
static int xrs700x_detect(struct xrs700x *priv)
diff --git a/drivers/net/dsa/xrs700x/xrs700x_reg.h b/drivers/net/dsa/xrs700x/xrs700x_reg.h
index a135d4d92b6d..470d00e07f15 100644
--- a/drivers/net/dsa/xrs700x/xrs700x_reg.h
+++ b/drivers/net/dsa/xrs700x/xrs700x_reg.h
@@ -49,6 +49,11 @@
/* Port Configuration Registers - HSR/PRP */
#define XRS_HSR_CFG(x) (XRS_PORT_HSR_BASE(x) + 0x0)
+#define XRS_HSR_CFG_HSR_PRP BIT(0)
+#define XRS_HSR_CFG_HSR 0
+#define XRS_HSR_CFG_PRP BIT(8)
+#define XRS_HSR_CFG_LANID_A 0
+#define XRS_HSR_CFG_LANID_B BIT(10)
/* Port Configuration Registers - PTP */
#define XRS_PTP_RX_SYNC_DELAY_NS_LO(x) (XRS_PORT_PTP_BASE(x) + 0x2)
diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c
index db0ed1a5fcb7..858cdf9d2913 100644
--- a/net/dsa/tag_xrs700x.c
+++ b/net/dsa/tag_xrs700x.c
@@ -11,12 +11,17 @@
static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev)
{
- struct dsa_port *dp = dsa_slave_to_port(dev);
+ struct dsa_port *partner, *dp = dsa_slave_to_port(dev);
u8 *trailer;
trailer = skb_put(skb, 1);
trailer[0] = BIT(dp->index);
+ if (dp->hsr_dev)
+ dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
+ if (partner != dp)
+ trailer[0] |= BIT(partner->index);
+
return skb;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-01 14:05 ` [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag George McCollister
@ 2021-02-01 14:59 ` Vladimir Oltean
2021-02-01 19:43 ` George McCollister
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-01 14:59 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 01, 2021 at 08:05:00AM -0600, George McCollister wrote:
> Generate supervision frame without HSR/PRP tag and rely on existing
> code which inserts it later.
> This will allow HSR/PRP tag insertions to be offloaded in the future.
>
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---
I'm not sure I understand why this change is correct, you'll have to
write a more convincing commit message, and if that takes up too much
space (I hope it will), you'll have to break this up into multiple
trivial changes.
Just so we're on the same page, here is the call path:
hsr_announce
-> hsr->proto_ops->send_sv_frame
-> hsr_init_skb
-> hsr_forward_skb
-> fill_frame_info
-> hsr->proto_ops->fill_frame_info
-> hsr_forward_do
-> hsr_handle_sup_frame
-> hsr->proto_ops->create_tagged_frame
-> hsr_xmit
> net/hsr/hsr_device.c | 32 ++++----------------------------
> net/hsr/hsr_forward.c | 10 +++++++---
> 2 files changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index ab953a1a0d6c..161b8da6a21d 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
> * being, for PRP it is a trailer and for HSR it is a
> * header
> */
> - skb = dev_alloc_skb(sizeof(struct hsr_tag) +
> - sizeof(struct hsr_sup_tag) +
> + skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
> sizeof(struct hsr_sup_payload) + hlen + tlen);
Question 1: why are you no longer allocating struct hsr_tag (or struct prp_rct,
which has the same size)?
In hsr->proto_ops->fill_frame_info in the call path above, the skb is
still put either into frame->skb_hsr or into frame->skb_prp, but not
into frame->skb_std, even if it does not contain a struct hsr_tag.
Also, which code exactly will insert the hsr_tag later? I assume
hsr_fill_tag via hsr->proto_ops->create_tagged_frame?
But I don't think that's how it works, unless I'm misunderstanding
something.. The code path in hsr_create_tagged_frame is:
if (frame->skb_hsr) { <- it will take this branch
struct hsr_ethhdr *hsr_ethhdr =
(struct hsr_ethhdr *)skb_mac_header(frame->skb_hsr);
/* set the lane id properly */
hsr_set_path_id(hsr_ethhdr, port);
return skb_clone(frame->skb_hsr, GFP_ATOMIC);
}
not this one
|
v
/* Create the new skb with enough headroom to fit the HSR tag */
skb = __pskb_copy(frame->skb_std,
skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
if (!skb)
return NULL;
skb_reset_mac_header(skb);
if (skb->ip_summed == CHECKSUM_PARTIAL)
skb->csum_start += HSR_HLEN;
movelen = ETH_HLEN;
if (frame->is_vlan)
movelen += VLAN_HLEN;
src = skb_mac_header(skb);
dst = skb_push(skb, HSR_HLEN);
memmove(dst, src, movelen);
skb_reset_mac_header(skb);
/* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
* that case
*/
return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
Otherwise said, it assumes that a frame->skb_hsr already has a struct
hsr_tag, no? Where does hsr_set_path_id() write?
>
> if (!skb)
> @@ -275,12 +274,10 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> {
> struct hsr_priv *hsr = master->hsr;
> __u8 type = HSR_TLV_LIFE_CHECK;
> - struct hsr_tag *hsr_tag = NULL;
> struct hsr_sup_payload *hsr_sp;
> struct hsr_sup_tag *hsr_stag;
> unsigned long irqflags;
> struct sk_buff *skb;
> - u16 proto;
>
> *interval = msecs_to_jiffies(HSR_LIFE_CHECK_INTERVAL);
> if (hsr->announce_count < 3 && hsr->prot_version == 0) {
> @@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> hsr->announce_count++;
> }
>
> - if (!hsr->prot_version)
> - proto = ETH_P_PRP;
> - else
> - proto = ETH_P_HSR;
> -
> - skb = hsr_init_skb(master, proto);
> + skb = hsr_init_skb(master, ETH_P_PRP);
Question 2: why is this correct, setting skb->protocol to ETH_P_PRP
(HSR v0) regardless of prot_version? Also, why is the change necessary?
Why is it such a big deal if supervision frames have HSR/PRP tag or not?
> if (!skb) {
> WARN_ONCE(1, "HSR: Could not send supervision frame\n");
> return;
> }
>
> - if (hsr->prot_version > 0) {
> - hsr_tag = skb_put(skb, sizeof(struct hsr_tag));
> - hsr_tag->encap_proto = htons(ETH_P_PRP);
> - set_hsr_tag_LSDU_size(hsr_tag, HSR_V1_SUP_LSDUSIZE);
> - }
> -
> hsr_stag = skb_put(skb, sizeof(struct hsr_sup_tag));
> set_hsr_stag_path(hsr_stag, (hsr->prot_version ? 0x0 : 0xf));
> set_hsr_stag_HSR_ver(hsr_stag, hsr->prot_version);
> @@ -315,8 +301,6 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> if (hsr->prot_version > 0) {
> hsr_stag->sequence_nr = htons(hsr->sup_sequence_nr);
> hsr->sup_sequence_nr++;
> - hsr_tag->sequence_nr = htons(hsr->sequence_nr);
> - hsr->sequence_nr++;
> } else {
> hsr_stag->sequence_nr = htons(hsr->sequence_nr);
> hsr->sequence_nr++;
> @@ -332,7 +316,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
>
> - if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))
> + if (skb_put_padto(skb, ETH_ZLEN))
> return;
>
> hsr_forward_skb(skb, master);
> @@ -348,8 +332,6 @@ static void send_prp_supervision_frame(struct hsr_port *master,
> struct hsr_sup_tag *hsr_stag;
> unsigned long irqflags;
> struct sk_buff *skb;
> - struct prp_rct *rct;
> - u8 *tail;
>
> skb = hsr_init_skb(master, ETH_P_PRP);
> if (!skb) {
> @@ -373,17 +355,11 @@ static void send_prp_supervision_frame(struct hsr_port *master,
> hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
>
> - if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN)) {
> + if (skb_put_padto(skb, ETH_ZLEN)) {
> spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
> return;
> }
>
> - tail = skb_tail_pointer(skb) - HSR_HLEN;
> - rct = (struct prp_rct *)tail;
> - rct->PRP_suffix = htons(ETH_P_PRP);
> - set_prp_LSDU_size(rct, HSR_V1_SUP_LSDUSIZE);
> - rct->sequence_nr = htons(hsr->sequence_nr);
> - hsr->sequence_nr++;
> spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
>
> hsr_forward_skb(skb, master);
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index cadfccd7876e..a5566b2245a0 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -454,8 +454,10 @@ static void handle_std_frame(struct sk_buff *skb,
> void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> struct hsr_frame_info *frame)
> {
> - if (proto == htons(ETH_P_PRP) ||
> - proto == htons(ETH_P_HSR)) {
> + struct hsr_port *port = frame->port_rcv;
> +
> + if (port->type != HSR_PT_MASTER &&
> + (proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
Why is this change necessary? Are you trying to force fill_frame_info to
call handle_std_frame for supervision frames, which will fix up the
kludge I asked about earlier? And why does checking for HSR_PT_MASTER
fixing it?
> /* HSR tagged frame :- Data or Supervision */
> frame->skb_std = NULL;
> frame->skb_prp = NULL;
> @@ -473,8 +475,10 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> {
> /* Supervision frame */
> struct prp_rct *rct = skb_get_PRP_rct(skb);
> + struct hsr_port *port = frame->port_rcv;
>
> - if (rct &&
> + if (port->type != HSR_PT_MASTER &&
> + rct &&
> prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
> frame->skb_hsr = NULL;
> frame->skb_std = NULL;
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 2/4] net: hsr: add offloading support
2021-02-01 14:05 ` [RESEND PATCH net-next 2/4] net: hsr: add offloading support George McCollister
@ 2021-02-01 15:23 ` Vladimir Oltean
2021-02-01 20:15 ` George McCollister
2021-02-03 20:43 ` George McCollister
0 siblings, 2 replies; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-01 15:23 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 01, 2021 at 08:05:01AM -0600, George McCollister wrote:
> Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
> tag removal, duplicate generation and forwarding.
>
> Use a new netdev_priv_flag IFF_HSR to indicate that a device is an HSR
> device so DSA can tell them apart from other devices in
> dsa_slave_changeupper.
My opinion can easily be overridden by somebody with more experience,
but for the purposes of determining whether a net_device is DSA or not,
DSA did not use a priv_flags bit, but simply exported dsa_slave_dev_check
which looks at its netdev_ops.
I am not sure what is the history of the IFF_EBRIDGE/BONDING/... bits,
but it seems a bit wasteful to me to consume the last bit of priv_flags
on something that can be derived from other sources.
I am basing this comment on the fact that your patch series does not
make use of the IFF_SLAVE bit and of netif_is_hsr_slave, and therefore
that you should probably not add a function that is not used, or the
mechanism for it.
>
> For HSR, insertion involves the switch adding a 6 byte HSR header after
> the 14 byte Ethernet header. For PRP it adds a 6 byte trailer.
>
> Tag removal involves automatically stripping the HSR/PRP header/trailer
> in the switch. This is possible when the switch also preforms auto
~~~~~~~~
performs
> deduplication using the HSR/PRP header/trailer (making it no longer
> required).
>
> Forwarding involves automatically forwarding between redundant ports in
> an HSR. This is crucial because delay is accumulated as a frame passes
> through each node in the ring.
>
> Duplication involves the switch automatically sending a single frame
> from the CPU port to both redundant ports. This is required because the
> inserted HSR/PRP header/trailer must contain the same sequence number
> on the frames sent out both redundant ports.
>
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---
> Documentation/networking/netdev-features.rst | 20 ++++++++++++++++++++
> include/linux/if_hsr.h | 22 ++++++++++++++++++++++
> include/linux/netdev_features.h | 9 +++++++++
> include/linux/netdevice.h | 13 +++++++++++++
> net/ethtool/common.c | 4 ++++
> net/hsr/hsr_device.c | 12 +++---------
> net/hsr/hsr_forward.c | 27 ++++++++++++++++++++++++---
> net/hsr/hsr_forward.h | 1 +
> net/hsr/hsr_main.c | 14 ++++++++++++++
> net/hsr/hsr_main.h | 8 +-------
> net/hsr/hsr_slave.c | 13 +++++++++----
> 11 files changed, 120 insertions(+), 23 deletions(-)
> create mode 100644 include/linux/if_hsr.h
>
> diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
> index a2d7d7160e39..4eab45405031 100644
> --- a/Documentation/networking/netdev-features.rst
> +++ b/Documentation/networking/netdev-features.rst
> @@ -182,3 +182,23 @@ stricter than Hardware LRO. A packet stream merged by Hardware GRO must
> be re-segmentable by GSO or TSO back to the exact original packet stream.
> Hardware GRO is dependent on RXCSUM since every packet successfully merged
> by hardware must also have the checksum verified by hardware.
> +
> +* hsr-tag-ins-offload
> +
> +This should be set for devices which insert an HSR (highspeed ring) tag
~~~~~~~~~~~~~~
The one thing I know about HSR is that it stands for High-availability
Seamless Redundancy, not High Speed Ring.
Also, it would be good to mention that these features apply to PRP too.
> +automatically when in HSR mode.
> +
> +* hsr-tag-rm-offload
> +
> +This should be set for devices which remove HSR (highspeed ring) tags
> +automatically when in HSR mode.
> +
> +* hsr-fwd-offload
> +
> +This should be set for devices which forward HSR (highspeed ring) frames from
> +one port to another in hardware.
> +
> +* hsr-dup-offload
> +
> +This should be set for devices which duplicate outgoing HSR (highspeed ring)
> +frames in hardware.
I don't have a strong opinion on creating ethtool features that are so
fine-grained, but do we really expect to see a net_device capable of
hsr-tag-rm-offload but not hsr-tag-ins-offload, or hsr-dup-offload but
not hsr-fwd-offload?
> diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
> new file mode 100644
> index 000000000000..eec9079efab0
> --- /dev/null
> +++ b/include/linux/if_hsr.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_IF_HSR_H_
> +#define _LINUX_IF_HSR_H_
> +
> +/* used to differentiate various protocols */
> +enum hsr_version {
> + HSR_V0 = 0,
> + HSR_V1,
> + PRP_V1,
> +};
> +
> +#if IS_ENABLED(CONFIG_HSR)
> +extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
> +#else
> +static inline int hsr_get_version(struct net_device *dev,
> + enum hsr_version *ver)
> +{
> + return -EINVAL;
> +}
> +#endif /* CONFIG_HSR */
> +
> +#endif /*_LINUX_IF_HSR_H_*/
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index c06d6aaba9df..3de38d6a0aea 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -86,6 +86,11 @@ enum {
> NETIF_F_HW_MACSEC_BIT, /* Offload MACsec operations */
> NETIF_F_GRO_UDP_FWD_BIT, /* Allow UDP GRO for forwarding */
>
> + NETIF_F_HW_HSR_TAG_INS_BIT, /* Offload HSR tag insertion */
> + NETIF_F_HW_HSR_TAG_RM_BIT, /* Offload HSR tag removal */
> + NETIF_F_HW_HSR_FWD_BIT, /* Offload HSR forwarding */
> + NETIF_F_HW_HSR_DUP_BIT, /* Offload HSR duplication */
> +
> /*
> * Add your fresh new feature above and remember to update
> * netdev_features_strings[] in net/core/ethtool.c and maybe
> @@ -159,6 +164,10 @@ enum {
> #define NETIF_F_GSO_FRAGLIST __NETIF_F(GSO_FRAGLIST)
> #define NETIF_F_HW_MACSEC __NETIF_F(HW_MACSEC)
> #define NETIF_F_GRO_UDP_FWD __NETIF_F(GRO_UDP_FWD)
> +#define NETIF_F_HW_HSR_TAG_INS __NETIF_F(HW_HSR_TAG_INS)
> +#define NETIF_F_HW_HSR_TAG_RM __NETIF_F(HW_HSR_TAG_RM)
> +#define NETIF_F_HW_HSR_FWD __NETIF_F(HW_HSR_FWD)
> +#define NETIF_F_HW_HSR_DUP __NETIF_F(HW_HSR_DUP)
>
> /* Finds the next feature with the highest number of the range of start till 0.
> */
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index e9e7ada07ea1..9ac6f30c4a51 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1526,6 +1526,7 @@ struct net_device_ops {
> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
> * @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
> + * @IFF_HSR: device is an hsr device
> */
> enum netdev_priv_flags {
> IFF_802_1Q_VLAN = 1<<0,
> @@ -1559,6 +1560,7 @@ enum netdev_priv_flags {
> IFF_FAILOVER_SLAVE = 1<<28,
> IFF_L3MDEV_RX_HANDLER = 1<<29,
> IFF_LIVE_RENAME_OK = 1<<30,
> + IFF_HSR = 1<<31,
> };
>
> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> @@ -1591,6 +1593,7 @@ enum netdev_priv_flags {
> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> #define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER
> #define IFF_LIVE_RENAME_OK IFF_LIVE_RENAME_OK
> +#define IFF_HSR IFF_HSR
>
> /**
> * struct net_device - The DEVICE structure.
> @@ -5003,6 +5006,16 @@ static inline bool netif_is_failover_slave(const struct net_device *dev)
> return dev->priv_flags & IFF_FAILOVER_SLAVE;
> }
>
> +static inline bool netif_is_hsr_master(const struct net_device *dev)
> +{
> + return dev->flags & IFF_MASTER && dev->priv_flags & IFF_HSR;
> +}
> +
> +static inline bool netif_is_hsr_slave(const struct net_device *dev)
> +{
> + return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_HSR;
> +}
> +
> /* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
> static inline void netif_keep_dst(struct net_device *dev)
> {
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 181220101a6e..0298e5635ace 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -69,6 +69,10 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
> [NETIF_F_GRO_FRAGLIST_BIT] = "rx-gro-list",
> [NETIF_F_HW_MACSEC_BIT] = "macsec-hw-offload",
> [NETIF_F_GRO_UDP_FWD_BIT] = "rx-udp-gro-forwarding",
> + [NETIF_F_HW_HSR_TAG_INS_BIT] = "hsr-tag-ins-offload",
> + [NETIF_F_HW_HSR_TAG_RM_BIT] = "hsr-tag-rm-offload",
> + [NETIF_F_HW_HSR_FWD_BIT] = "hsr-fwd-offload",
> + [NETIF_F_HW_HSR_DUP_BIT] = "hsr-dup-offload",
> };
>
> const char
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 161b8da6a21d..d9b033e9b18c 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -418,6 +418,7 @@ static struct hsr_proto_ops hsr_ops = {
> .send_sv_frame = send_hsr_supervision_frame,
> .create_tagged_frame = hsr_create_tagged_frame,
> .get_untagged_frame = hsr_get_untagged_frame,
> + .drop_frame = hsr_drop_frame,
> .fill_frame_info = hsr_fill_frame_info,
> .invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
> };
> @@ -521,15 +522,8 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
>
> hsr->prot_version = protocol_version;
>
> - /* FIXME: should I modify the value of these?
> - *
> - * - hsr_dev->flags - i.e.
> - * IFF_MASTER/SLAVE?
> - * - hsr_dev->priv_flags - i.e.
> - * IFF_EBRIDGE?
> - * IFF_TX_SKB_SHARING?
> - * IFF_HSR_MASTER/SLAVE?
> - */
> + hsr_dev->flags |= IFF_MASTER;
> + hsr_dev->priv_flags |= IFF_HSR;
>
> /* Make sure the 1st call to netif_carrier_on() gets through */
> netif_carrier_off(hsr_dev);
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index a5566b2245a0..9c79d602c4e0 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -247,6 +247,8 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> /* set the lane id properly */
> hsr_set_path_id(hsr_ethhdr, port);
> return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> + } else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> + return skb_clone(frame->skb_std, GFP_ATOMIC);
> }
>
> /* Create the new skb with enough headroom to fit the HSR tag */
> @@ -341,6 +343,14 @@ bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
> port->type == HSR_PT_SLAVE_A));
> }
>
> +bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
> +{
> + if (port->dev->features & NETIF_F_HW_HSR_FWD)
> + return prp_drop_frame(frame, port);
> +
> + return false;
> +}
> +
> /* Forward the frame through all devices except:
> * - Back through the receiving device
> * - If it's a HSR frame: through a device where it has passed before
> @@ -357,6 +367,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> {
> struct hsr_port *port;
> struct sk_buff *skb;
> + bool sent = false;
>
> hsr_for_each_port(frame->port_rcv->hsr, port) {
> struct hsr_priv *hsr = port->hsr;
> @@ -372,6 +383,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
> continue;
>
> + /* If hardware duplicate generation is enabled, only send out
> + * one port.
> + */
> + if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
> + continue;
> +
Is this right? It looks to me that you are not bypassing only duplicate
generation, but also duplicate elimination.
I think this is duplicate elimination:
/* Don't send frame over port where it has been sent before.
* Also fro SAN, this shouldn't be done.
*/
if (!frame->is_from_san &&
hsr_register_frame_out(port, frame->node_src,
frame->sequence_nr))
continue;
and this is duplicate generation:
hsr_for_each_port(frame->port_rcv->hsr, port) {
...
skb->dev = port->dev;
if (port->type == HSR_PT_MASTER)
hsr_deliver_master(skb, port->dev, frame->node_src);
else
hsr_xmit(skb, port, frame);
}
So if this is the description of NETIF_F_HW_HSR_DUP:
| Duplication involves the switch automatically sending a single frame
| from the CPU port to both redundant ports. This is required because the
| inserted HSR/PRP header/trailer must contain the same sequence number
| on the frames sent out both redundant ports.
then NETIF_F_HW_HSR_DUP is a misnomer. You should think of either
grouping duplicate elimination and generation together, or rethinking
the whole features system.
> /* Don't send frame over port where it has been sent before.
> * Also fro SAN, this shouldn't be done.
> */
> @@ -403,10 +420,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> }
>
> skb->dev = port->dev;
> - if (port->type == HSR_PT_MASTER)
> + if (port->type == HSR_PT_MASTER) {
> hsr_deliver_master(skb, port->dev, frame->node_src);
> - else
> - hsr_xmit(skb, port, frame);
> + } else {
> + if (!hsr_xmit(skb, port, frame))
> + sent = true;
> + }
> }
> }
>
> @@ -457,6 +476,7 @@ void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> struct hsr_port *port = frame->port_rcv;
>
> if (port->type != HSR_PT_MASTER &&
> + !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> (proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
> /* HSR tagged frame :- Data or Supervision */
> frame->skb_std = NULL;
> @@ -478,6 +498,7 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> struct hsr_port *port = frame->port_rcv;
>
> if (port->type != HSR_PT_MASTER &&
> + !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> rct &&
> prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
> frame->skb_hsr = NULL;
> diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
> index 618140d484ad..b6acaafa83fc 100644
> --- a/net/hsr/hsr_forward.h
> +++ b/net/hsr/hsr_forward.h
> @@ -23,6 +23,7 @@ struct sk_buff *hsr_get_untagged_frame(struct hsr_frame_info *frame,
> struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
> struct hsr_port *port);
> bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> +bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> struct hsr_frame_info *frame);
> void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> index 2fd1976e5b1c..0e7b5b18b5e3 100644
> --- a/net/hsr/hsr_main.c
> +++ b/net/hsr/hsr_main.c
> @@ -131,6 +131,20 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
> return NULL;
> }
>
> +int hsr_get_version(struct net_device *dev, enum hsr_version *ver)
> +{
> + struct hsr_priv *hsr;
> +
> + if (!(dev->priv_flags & IFF_HSR))
> + return -EINVAL;
> +
> + hsr = netdev_priv(dev);
> + *ver = hsr->prot_version;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hsr_get_version);
> +
> static struct notifier_block hsr_nb = {
> .notifier_call = hsr_netdev_notify, /* Slave event notifications */
> };
> diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> index 7dc92ce5a134..7369b2febe0f 100644
> --- a/net/hsr/hsr_main.h
> +++ b/net/hsr/hsr_main.h
> @@ -13,6 +13,7 @@
> #include <linux/netdevice.h>
> #include <linux/list.h>
> #include <linux/if_vlan.h>
> +#include <linux/if_hsr.h>
>
> /* Time constants as specified in the HSR specification (IEC-62439-3 2010)
> * Table 8.
> @@ -171,13 +172,6 @@ struct hsr_port {
> enum hsr_port_type type;
> };
>
> -/* used by driver internally to differentiate various protocols */
> -enum hsr_version {
> - HSR_V0 = 0,
> - HSR_V1,
> - PRP_V1,
> -};
> -
> struct hsr_frame_info;
> struct hsr_node;
>
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index 36d5fcf09c61..59f8d2b68376 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
> goto finish_consume;
> }
>
> - /* For HSR, only tagged frames are expected, but for PRP
> - * there could be non tagged frames as well from Single
> - * attached nodes (SANs).
> + /* For HSR, only tagged frames are expected (unless the device offloads
> + * HSR tag removal), but for PRP there could be non tagged frames as
> + * well from Single attached nodes (SANs).
> */
> protocol = eth_hdr(skb)->h_proto;
> - if (hsr->proto_ops->invalid_dan_ingress_frame &&
> +
> + if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> + hsr->proto_ops->invalid_dan_ingress_frame &&
> hsr->proto_ops->invalid_dan_ingress_frame(protocol))
> goto finish_pass;
>
> @@ -137,6 +139,9 @@ static int hsr_portdev_setup(struct hsr_priv *hsr, struct net_device *dev,
> master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> hsr_dev = master->dev;
>
> + dev->flags |= IFF_SLAVE;
> + dev->priv_flags |= IFF_HSR;
> +
> res = netdev_upper_dev_link(dev, hsr_dev, extack);
> if (res)
> goto fail_upper_dev_link;
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support
2021-02-01 14:05 ` [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support George McCollister
@ 2021-02-01 15:29 ` Vladimir Oltean
2021-02-01 21:25 ` George McCollister
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-01 15:29 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 01, 2021 at 08:05:03AM -0600, George McCollister wrote:
> Add offloading for HSR/PRP (IEC 62439-3) tag insertion, tag removal
> forwarding and duplication supported by the xrs7000 series switches.
>
> Only HSR v1 and PRP v1 are supported by the xrs7000 series switches (HSR
> v0 is not).
>
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---
Does this switch discard duplicates or does it not? If it does, what
algorithm does it use? Does it not need some sort of runtime
communication with the hsr master, like for the nodes table?
How many streams can it keep track of? What happens when the ring is
larger than the switch can keep track of in its internal Link Redundancy
Entity?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-01 14:59 ` Vladimir Oltean
@ 2021-02-01 19:43 ` George McCollister
2021-02-02 0:37 ` Vladimir Oltean
0 siblings, 1 reply; 17+ messages in thread
From: George McCollister @ 2021-02-01 19:43 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 1, 2021 at 8:59 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Feb 01, 2021 at 08:05:00AM -0600, George McCollister wrote:
> > Generate supervision frame without HSR/PRP tag and rely on existing
> > code which inserts it later.
> > This will allow HSR/PRP tag insertions to be offloaded in the future.
> >
> > Signed-off-by: George McCollister <george.mccollister@gmail.com>
> > ---
>
> I'm not sure I understand why this change is correct, you'll have to
> write a more convincing commit message, and if that takes up too much
> space (I hope it will), you'll have to break this up into multiple
> trivial changes.
Okay, I'll work on this. Not sure if this can be broken up into
trivial changes if we want it to remain working after each commit.
>
> Just so we're on the same page, here is the call path:
>
> hsr_announce
> -> hsr->proto_ops->send_sv_frame
> -> hsr_init_skb
> -> hsr_forward_skb
> -> fill_frame_info
> -> hsr->proto_ops->fill_frame_info
> -> hsr_forward_do
> -> hsr_handle_sup_frame
> -> hsr->proto_ops->create_tagged_frame
> -> hsr_xmit
>
> > net/hsr/hsr_device.c | 32 ++++----------------------------
> > net/hsr/hsr_forward.c | 10 +++++++---
> > 2 files changed, 11 insertions(+), 31 deletions(-)
> >
> > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> > index ab953a1a0d6c..161b8da6a21d 100644
> > --- a/net/hsr/hsr_device.c
> > +++ b/net/hsr/hsr_device.c
> > @@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
> > * being, for PRP it is a trailer and for HSR it is a
> > * header
> > */
> > - skb = dev_alloc_skb(sizeof(struct hsr_tag) +
> > - sizeof(struct hsr_sup_tag) +
> > + skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
> > sizeof(struct hsr_sup_payload) + hlen + tlen);
>
> Question 1: why are you no longer allocating struct hsr_tag (or struct prp_rct,
> which has the same size)?
Because the tag is no longer being included in the supervisory frame
here. If I understand correctly hsr_create_tagged_frame and
prp_create_tagged_frame will create a new skb with HSR_HLEN added
later.
>
> In hsr->proto_ops->fill_frame_info in the call path above, the skb is
> still put either into frame->skb_hsr or into frame->skb_prp, but not
> into frame->skb_std, even if it does not contain a struct hsr_tag.
Are you sure? My patch changes hsr_fill_frame_info and
prp_fill_frame_info not to do that if port->type is HSR_PT_MASTER
which I'm pretty certain it always is when sending supervisory frames
like this. If I've overlooked something let me know.
>
> Also, which code exactly will insert the hsr_tag later? I assume
> hsr_fill_tag via hsr->proto_ops->create_tagged_frame?
Correct.
>
> But I don't think that's how it works, unless I'm misunderstanding
> something.. The code path in hsr_create_tagged_frame is:
>
> if (frame->skb_hsr) { <- it will take this branch
it shouldn't be taking this branch because skb_hsr and skb_prp
shouldn't be getting filled out. Let's resolve that part of the
discussion above.
> struct hsr_ethhdr *hsr_ethhdr =
> (struct hsr_ethhdr *)skb_mac_header(frame->skb_hsr);
>
> /* set the lane id properly */
> hsr_set_path_id(hsr_ethhdr, port);
> return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> }
>
> not this one
> |
> v
>
> /* Create the new skb with enough headroom to fit the HSR tag */
> skb = __pskb_copy(frame->skb_std,
> skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
> if (!skb)
> return NULL;
> skb_reset_mac_header(skb);
>
> if (skb->ip_summed == CHECKSUM_PARTIAL)
> skb->csum_start += HSR_HLEN;
>
> movelen = ETH_HLEN;
> if (frame->is_vlan)
> movelen += VLAN_HLEN;
>
> src = skb_mac_header(skb);
> dst = skb_push(skb, HSR_HLEN);
> memmove(dst, src, movelen);
> skb_reset_mac_header(skb);
>
> /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
> * that case
> */
> return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
>
> Otherwise said, it assumes that a frame->skb_hsr already has a struct
> hsr_tag, no? Where does hsr_set_path_id() write?
>
> >
> > if (!skb)
> > @@ -275,12 +274,10 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> > {
> > struct hsr_priv *hsr = master->hsr;
> > __u8 type = HSR_TLV_LIFE_CHECK;
> > - struct hsr_tag *hsr_tag = NULL;
> > struct hsr_sup_payload *hsr_sp;
> > struct hsr_sup_tag *hsr_stag;
> > unsigned long irqflags;
> > struct sk_buff *skb;
> > - u16 proto;
> >
> > *interval = msecs_to_jiffies(HSR_LIFE_CHECK_INTERVAL);
> > if (hsr->announce_count < 3 && hsr->prot_version == 0) {
> > @@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> > hsr->announce_count++;
> > }
> >
> > - if (!hsr->prot_version)
> > - proto = ETH_P_PRP;
> > - else
> > - proto = ETH_P_HSR;
> > -
> > - skb = hsr_init_skb(master, proto);
> > + skb = hsr_init_skb(master, ETH_P_PRP);
>
> Question 2: why is this correct, setting skb->protocol to ETH_P_PRP
> (HSR v0) regardless of prot_version? Also, why is the change necessary?
This part is not intuitive and I don't have a copy of the documents
where v0 was defined. It's unfortunate this code even supports v0
because AFAIK no one else uses it; but it's in here so we have to keep
supporting it I guess.
In v1 the tag has an eth type of 0x892f and the encapsulated
supervisory frame has a type of 0x88fb. In v0 0x88fb is used for the
eth type and there is no encapsulation type. So... this is correct
however I compared supervisory frame generation before and after this
patch for v0 and I found a problem. My changes make it add 0x88fb
again later for v0 which it's not supposed to do. I'll have to fix
that part somehow.
>
> Why is it such a big deal if supervision frames have HSR/PRP tag or not?
Because if the switch does automatic HSR/PRP tag insertion it will end
up in there twice. You simply can't send anything with an HSR/PRP tag
if this is offloaded.
>
> > if (!skb) {
> > WARN_ONCE(1, "HSR: Could not send supervision frame\n");
> > return;
> > }
> >
> > - if (hsr->prot_version > 0) {
> > - hsr_tag = skb_put(skb, sizeof(struct hsr_tag));
> > - hsr_tag->encap_proto = htons(ETH_P_PRP);
> > - set_hsr_tag_LSDU_size(hsr_tag, HSR_V1_SUP_LSDUSIZE);
> > - }
> > -
> > hsr_stag = skb_put(skb, sizeof(struct hsr_sup_tag));
> > set_hsr_stag_path(hsr_stag, (hsr->prot_version ? 0x0 : 0xf));
> > set_hsr_stag_HSR_ver(hsr_stag, hsr->prot_version);
> > @@ -315,8 +301,6 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> > if (hsr->prot_version > 0) {
> > hsr_stag->sequence_nr = htons(hsr->sup_sequence_nr);
> > hsr->sup_sequence_nr++;
> > - hsr_tag->sequence_nr = htons(hsr->sequence_nr);
> > - hsr->sequence_nr++;
> > } else {
> > hsr_stag->sequence_nr = htons(hsr->sequence_nr);
> > hsr->sequence_nr++;
> > @@ -332,7 +316,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> > hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> > ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
> >
> > - if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))
> > + if (skb_put_padto(skb, ETH_ZLEN))
> > return;
> >
> > hsr_forward_skb(skb, master);
> > @@ -348,8 +332,6 @@ static void send_prp_supervision_frame(struct hsr_port *master,
> > struct hsr_sup_tag *hsr_stag;
> > unsigned long irqflags;
> > struct sk_buff *skb;
> > - struct prp_rct *rct;
> > - u8 *tail;
> >
> > skb = hsr_init_skb(master, ETH_P_PRP);
> > if (!skb) {
> > @@ -373,17 +355,11 @@ static void send_prp_supervision_frame(struct hsr_port *master,
> > hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> > ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
> >
> > - if (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN)) {
> > + if (skb_put_padto(skb, ETH_ZLEN)) {
> > spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
> > return;
> > }
> >
> > - tail = skb_tail_pointer(skb) - HSR_HLEN;
> > - rct = (struct prp_rct *)tail;
> > - rct->PRP_suffix = htons(ETH_P_PRP);
> > - set_prp_LSDU_size(rct, HSR_V1_SUP_LSDUSIZE);
> > - rct->sequence_nr = htons(hsr->sequence_nr);
> > - hsr->sequence_nr++;
> > spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
> >
> > hsr_forward_skb(skb, master);
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index cadfccd7876e..a5566b2245a0 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> > @@ -454,8 +454,10 @@ static void handle_std_frame(struct sk_buff *skb,
> > void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > struct hsr_frame_info *frame)
> > {
> > - if (proto == htons(ETH_P_PRP) ||
> > - proto == htons(ETH_P_HSR)) {
> > + struct hsr_port *port = frame->port_rcv;
> > +
> > + if (port->type != HSR_PT_MASTER &&
> > + (proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
>
> Why is this change necessary? Are you trying to force fill_frame_info to
> call handle_std_frame for supervision frames, which will fix up the
> kludge I asked about earlier? And why does checking for HSR_PT_MASTER
> fixing it?
The eth type for the tag in v0 is the same type used for supervisory
frames in v1 so if we generate supervisory frames without a tag the
existing check wasn't sufficient. Anyway, no point in talking about it
now since I might have to change the way this works to fix v0.
>
> > /* HSR tagged frame :- Data or Supervision */
> > frame->skb_std = NULL;
> > frame->skb_prp = NULL;
> > @@ -473,8 +475,10 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > {
> > /* Supervision frame */
> > struct prp_rct *rct = skb_get_PRP_rct(skb);
> > + struct hsr_port *port = frame->port_rcv;
> >
> > - if (rct &&
> > + if (port->type != HSR_PT_MASTER &&
> > + rct &&
> > prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
> > frame->skb_hsr = NULL;
> > frame->skb_std = NULL;
> > --
> > 2.11.0
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 2/4] net: hsr: add offloading support
2021-02-01 15:23 ` Vladimir Oltean
@ 2021-02-01 20:15 ` George McCollister
2021-02-03 20:43 ` George McCollister
1 sibling, 0 replies; 17+ messages in thread
From: George McCollister @ 2021-02-01 20:15 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 1, 2021 at 9:23 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Feb 01, 2021 at 08:05:01AM -0600, George McCollister wrote:
> > Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
> > tag removal, duplicate generation and forwarding.
> >
> > Use a new netdev_priv_flag IFF_HSR to indicate that a device is an HSR
> > device so DSA can tell them apart from other devices in
> > dsa_slave_changeupper.
>
> My opinion can easily be overridden by somebody with more experience,
> but for the purposes of determining whether a net_device is DSA or not,
> DSA did not use a priv_flags bit, but simply exported dsa_slave_dev_check
> which looks at its netdev_ops.
I didn't think of that. I'll try doing it and report back if there is
some reason it won't work.
>
> I am not sure what is the history of the IFF_EBRIDGE/BONDING/... bits,
> but it seems a bit wasteful to me to consume the last bit of priv_flags
> on something that can be derived from other sources.
>
> I am basing this comment on the fact that your patch series does not
> make use of the IFF_SLAVE bit and of netif_is_hsr_slave, and therefore
> that you should probably not add a function that is not used, or the
> mechanism for it.
I did figure it would be a good idea to implement it similar to
bonding but it's not used so I guess I'll take it out
>
> >
> > For HSR, insertion involves the switch adding a 6 byte HSR header after
> > the 14 byte Ethernet header. For PRP it adds a 6 byte trailer.
> >
> > Tag removal involves automatically stripping the HSR/PRP header/trailer
> > in the switch. This is possible when the switch also preforms auto
> ~~~~~~~~
> performs
oops. thanks
> > deduplication using the HSR/PRP header/trailer (making it no longer
> > required).
> >
> > Forwarding involves automatically forwarding between redundant ports in
> > an HSR. This is crucial because delay is accumulated as a frame passes
> > through each node in the ring.
> >
> > Duplication involves the switch automatically sending a single frame
> > from the CPU port to both redundant ports. This is required because the
> > inserted HSR/PRP header/trailer must contain the same sequence number
> > on the frames sent out both redundant ports.
> >
> > Signed-off-by: George McCollister <george.mccollister@gmail.com>
> > ---
> > Documentation/networking/netdev-features.rst | 20 ++++++++++++++++++++
> > include/linux/if_hsr.h | 22 ++++++++++++++++++++++
> > include/linux/netdev_features.h | 9 +++++++++
> > include/linux/netdevice.h | 13 +++++++++++++
> > net/ethtool/common.c | 4 ++++
> > net/hsr/hsr_device.c | 12 +++---------
> > net/hsr/hsr_forward.c | 27 ++++++++++++++++++++++++---
> > net/hsr/hsr_forward.h | 1 +
> > net/hsr/hsr_main.c | 14 ++++++++++++++
> > net/hsr/hsr_main.h | 8 +-------
> > net/hsr/hsr_slave.c | 13 +++++++++----
> > 11 files changed, 120 insertions(+), 23 deletions(-)
> > create mode 100644 include/linux/if_hsr.h
> >
> > diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
> > index a2d7d7160e39..4eab45405031 100644
> > --- a/Documentation/networking/netdev-features.rst
> > +++ b/Documentation/networking/netdev-features.rst
> > @@ -182,3 +182,23 @@ stricter than Hardware LRO. A packet stream merged by Hardware GRO must
> > be re-segmentable by GSO or TSO back to the exact original packet stream.
> > Hardware GRO is dependent on RXCSUM since every packet successfully merged
> > by hardware must also have the checksum verified by hardware.
> > +
> > +* hsr-tag-ins-offload
> > +
> > +This should be set for devices which insert an HSR (highspeed ring) tag
> ~~~~~~~~~~~~~~
>
> The one thing I know about HSR is that it stands for High-availability
> Seamless Redundancy, not High Speed Ring.
Yeah. Brain fart. thanks.
>
> Also, it would be good to mention that these features apply to PRP too.
Good idea.
>
> > +automatically when in HSR mode.
> > +
> > +* hsr-tag-rm-offload
> > +
> > +This should be set for devices which remove HSR (highspeed ring) tags
> > +automatically when in HSR mode.
> > +
> > +* hsr-fwd-offload
> > +
> > +This should be set for devices which forward HSR (highspeed ring) frames from
> > +one port to another in hardware.
> > +
> > +* hsr-dup-offload
> > +
> > +This should be set for devices which duplicate outgoing HSR (highspeed ring)
> > +frames in hardware.
>
> I don't have a strong opinion on creating ethtool features that are so
> fine-grained, but do we really expect to see a net_device capable of
> hsr-tag-rm-offload but not hsr-tag-ins-offload, or hsr-dup-offload but
> not hsr-fwd-offload?
This was discussed briefly with the RFC patch (wish you would have
commented on some of this then) but no-one followed up on my latest
comment. Would it be possible for you to go back, read that and give
me your take after reading it?
>
> > diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
> > new file mode 100644
> > index 000000000000..eec9079efab0
> > --- /dev/null
> > +++ b/include/linux/if_hsr.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_IF_HSR_H_
> > +#define _LINUX_IF_HSR_H_
> > +
> > +/* used to differentiate various protocols */
> > +enum hsr_version {
> > + HSR_V0 = 0,
> > + HSR_V1,
> > + PRP_V1,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_HSR)
> > +extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
> > +#else
> > +static inline int hsr_get_version(struct net_device *dev,
> > + enum hsr_version *ver)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif /* CONFIG_HSR */
> > +
> > +#endif /*_LINUX_IF_HSR_H_*/
> > diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> > index c06d6aaba9df..3de38d6a0aea 100644
> > --- a/include/linux/netdev_features.h
> > +++ b/include/linux/netdev_features.h
> > @@ -86,6 +86,11 @@ enum {
> > NETIF_F_HW_MACSEC_BIT, /* Offload MACsec operations */
> > NETIF_F_GRO_UDP_FWD_BIT, /* Allow UDP GRO for forwarding */
> >
> > + NETIF_F_HW_HSR_TAG_INS_BIT, /* Offload HSR tag insertion */
> > + NETIF_F_HW_HSR_TAG_RM_BIT, /* Offload HSR tag removal */
> > + NETIF_F_HW_HSR_FWD_BIT, /* Offload HSR forwarding */
> > + NETIF_F_HW_HSR_DUP_BIT, /* Offload HSR duplication */
> > +
> > /*
> > * Add your fresh new feature above and remember to update
> > * netdev_features_strings[] in net/core/ethtool.c and maybe
> > @@ -159,6 +164,10 @@ enum {
> > #define NETIF_F_GSO_FRAGLIST __NETIF_F(GSO_FRAGLIST)
> > #define NETIF_F_HW_MACSEC __NETIF_F(HW_MACSEC)
> > #define NETIF_F_GRO_UDP_FWD __NETIF_F(GRO_UDP_FWD)
> > +#define NETIF_F_HW_HSR_TAG_INS __NETIF_F(HW_HSR_TAG_INS)
> > +#define NETIF_F_HW_HSR_TAG_RM __NETIF_F(HW_HSR_TAG_RM)
> > +#define NETIF_F_HW_HSR_FWD __NETIF_F(HW_HSR_FWD)
> > +#define NETIF_F_HW_HSR_DUP __NETIF_F(HW_HSR_DUP)
> >
> > /* Finds the next feature with the highest number of the range of start till 0.
> > */
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index e9e7ada07ea1..9ac6f30c4a51 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1526,6 +1526,7 @@ struct net_device_ops {
> > * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> > * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
> > * @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
> > + * @IFF_HSR: device is an hsr device
> > */
> > enum netdev_priv_flags {
> > IFF_802_1Q_VLAN = 1<<0,
> > @@ -1559,6 +1560,7 @@ enum netdev_priv_flags {
> > IFF_FAILOVER_SLAVE = 1<<28,
> > IFF_L3MDEV_RX_HANDLER = 1<<29,
> > IFF_LIVE_RENAME_OK = 1<<30,
> > + IFF_HSR = 1<<31,
> > };
> >
> > #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> > @@ -1591,6 +1593,7 @@ enum netdev_priv_flags {
> > #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> > #define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER
> > #define IFF_LIVE_RENAME_OK IFF_LIVE_RENAME_OK
> > +#define IFF_HSR IFF_HSR
> >
> > /**
> > * struct net_device - The DEVICE structure.
> > @@ -5003,6 +5006,16 @@ static inline bool netif_is_failover_slave(const struct net_device *dev)
> > return dev->priv_flags & IFF_FAILOVER_SLAVE;
> > }
> >
> > +static inline bool netif_is_hsr_master(const struct net_device *dev)
> > +{
> > + return dev->flags & IFF_MASTER && dev->priv_flags & IFF_HSR;
> > +}
> > +
> > +static inline bool netif_is_hsr_slave(const struct net_device *dev)
> > +{
> > + return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_HSR;
> > +}
> > +
> > /* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
> > static inline void netif_keep_dst(struct net_device *dev)
> > {
> > diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> > index 181220101a6e..0298e5635ace 100644
> > --- a/net/ethtool/common.c
> > +++ b/net/ethtool/common.c
> > @@ -69,6 +69,10 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
> > [NETIF_F_GRO_FRAGLIST_BIT] = "rx-gro-list",
> > [NETIF_F_HW_MACSEC_BIT] = "macsec-hw-offload",
> > [NETIF_F_GRO_UDP_FWD_BIT] = "rx-udp-gro-forwarding",
> > + [NETIF_F_HW_HSR_TAG_INS_BIT] = "hsr-tag-ins-offload",
> > + [NETIF_F_HW_HSR_TAG_RM_BIT] = "hsr-tag-rm-offload",
> > + [NETIF_F_HW_HSR_FWD_BIT] = "hsr-fwd-offload",
> > + [NETIF_F_HW_HSR_DUP_BIT] = "hsr-dup-offload",
> > };
> >
> > const char
> > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> > index 161b8da6a21d..d9b033e9b18c 100644
> > --- a/net/hsr/hsr_device.c
> > +++ b/net/hsr/hsr_device.c
> > @@ -418,6 +418,7 @@ static struct hsr_proto_ops hsr_ops = {
> > .send_sv_frame = send_hsr_supervision_frame,
> > .create_tagged_frame = hsr_create_tagged_frame,
> > .get_untagged_frame = hsr_get_untagged_frame,
> > + .drop_frame = hsr_drop_frame,
> > .fill_frame_info = hsr_fill_frame_info,
> > .invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
> > };
> > @@ -521,15 +522,8 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
> >
> > hsr->prot_version = protocol_version;
> >
> > - /* FIXME: should I modify the value of these?
> > - *
> > - * - hsr_dev->flags - i.e.
> > - * IFF_MASTER/SLAVE?
> > - * - hsr_dev->priv_flags - i.e.
> > - * IFF_EBRIDGE?
> > - * IFF_TX_SKB_SHARING?
> > - * IFF_HSR_MASTER/SLAVE?
> > - */
> > + hsr_dev->flags |= IFF_MASTER;
> > + hsr_dev->priv_flags |= IFF_HSR;
> >
> > /* Make sure the 1st call to netif_carrier_on() gets through */
> > netif_carrier_off(hsr_dev);
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index a5566b2245a0..9c79d602c4e0 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> > @@ -247,6 +247,8 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> > /* set the lane id properly */
> > hsr_set_path_id(hsr_ethhdr, port);
> > return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> > + } else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> > + return skb_clone(frame->skb_std, GFP_ATOMIC);
> > }
> >
> > /* Create the new skb with enough headroom to fit the HSR tag */
> > @@ -341,6 +343,14 @@ bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
> > port->type == HSR_PT_SLAVE_A));
> > }
> >
> > +bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
> > +{
> > + if (port->dev->features & NETIF_F_HW_HSR_FWD)
> > + return prp_drop_frame(frame, port);
> > +
> > + return false;
> > +}
> > +
> > /* Forward the frame through all devices except:
> > * - Back through the receiving device
> > * - If it's a HSR frame: through a device where it has passed before
> > @@ -357,6 +367,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > {
> > struct hsr_port *port;
> > struct sk_buff *skb;
> > + bool sent = false;
> >
> > hsr_for_each_port(frame->port_rcv->hsr, port) {
> > struct hsr_priv *hsr = port->hsr;
> > @@ -372,6 +383,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
> > continue;
> >
> > + /* If hardware duplicate generation is enabled, only send out
> > + * one port.
> > + */
> > + if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
> > + continue;
> > +
>
> Is this right? It looks to me that you are not bypassing only duplicate
> generation, but also duplicate elimination.
>
> I think this is duplicate elimination:
>
> /* Don't send frame over port where it has been sent before.
> * Also fro SAN, this shouldn't be done.
> */
> if (!frame->is_from_san &&
> hsr_register_frame_out(port, frame->node_src,
> frame->sequence_nr))
> continue;
>
> and this is duplicate generation:
>
> hsr_for_each_port(frame->port_rcv->hsr, port) {
> ...
> skb->dev = port->dev;
> if (port->type == HSR_PT_MASTER)
> hsr_deliver_master(skb, port->dev, frame->node_src);
> else
> hsr_xmit(skb, port, frame);
> }
>
> So if this is the description of NETIF_F_HW_HSR_DUP:
>
> | Duplication involves the switch automatically sending a single frame
> | from the CPU port to both redundant ports. This is required because the
> | inserted HSR/PRP header/trailer must contain the same sequence number
> | on the frames sent out both redundant ports.
>
> then NETIF_F_HW_HSR_DUP is a misnomer. You should think of either
> grouping duplicate elimination and generation together, or rethinking
> the whole features system.
>
> > /* Don't send frame over port where it has been sent before.
> > * Also fro SAN, this shouldn't be done.
> > */
> > @@ -403,10 +420,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > }
> >
> > skb->dev = port->dev;
> > - if (port->type == HSR_PT_MASTER)
> > + if (port->type == HSR_PT_MASTER) {
> > hsr_deliver_master(skb, port->dev, frame->node_src);
> > - else
> > - hsr_xmit(skb, port, frame);
> > + } else {
> > + if (!hsr_xmit(skb, port, frame))
> > + sent = true;
> > + }
> > }
> > }
> >
> > @@ -457,6 +476,7 @@ void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > struct hsr_port *port = frame->port_rcv;
> >
> > if (port->type != HSR_PT_MASTER &&
> > + !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> > (proto == htons(ETH_P_PRP) || proto == htons(ETH_P_HSR))) {
> > /* HSR tagged frame :- Data or Supervision */
> > frame->skb_std = NULL;
> > @@ -478,6 +498,7 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > struct hsr_port *port = frame->port_rcv;
> >
> > if (port->type != HSR_PT_MASTER &&
> > + !(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> > rct &&
> > prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
> > frame->skb_hsr = NULL;
> > diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
> > index 618140d484ad..b6acaafa83fc 100644
> > --- a/net/hsr/hsr_forward.h
> > +++ b/net/hsr/hsr_forward.h
> > @@ -23,6 +23,7 @@ struct sk_buff *hsr_get_untagged_frame(struct hsr_frame_info *frame,
> > struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
> > struct hsr_port *port);
> > bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> > +bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> > void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > struct hsr_frame_info *frame);
> > void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> > diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> > index 2fd1976e5b1c..0e7b5b18b5e3 100644
> > --- a/net/hsr/hsr_main.c
> > +++ b/net/hsr/hsr_main.c
> > @@ -131,6 +131,20 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
> > return NULL;
> > }
> >
> > +int hsr_get_version(struct net_device *dev, enum hsr_version *ver)
> > +{
> > + struct hsr_priv *hsr;
> > +
> > + if (!(dev->priv_flags & IFF_HSR))
> > + return -EINVAL;
> > +
> > + hsr = netdev_priv(dev);
> > + *ver = hsr->prot_version;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(hsr_get_version);
> > +
> > static struct notifier_block hsr_nb = {
> > .notifier_call = hsr_netdev_notify, /* Slave event notifications */
> > };
> > diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> > index 7dc92ce5a134..7369b2febe0f 100644
> > --- a/net/hsr/hsr_main.h
> > +++ b/net/hsr/hsr_main.h
> > @@ -13,6 +13,7 @@
> > #include <linux/netdevice.h>
> > #include <linux/list.h>
> > #include <linux/if_vlan.h>
> > +#include <linux/if_hsr.h>
> >
> > /* Time constants as specified in the HSR specification (IEC-62439-3 2010)
> > * Table 8.
> > @@ -171,13 +172,6 @@ struct hsr_port {
> > enum hsr_port_type type;
> > };
> >
> > -/* used by driver internally to differentiate various protocols */
> > -enum hsr_version {
> > - HSR_V0 = 0,
> > - HSR_V1,
> > - PRP_V1,
> > -};
> > -
> > struct hsr_frame_info;
> > struct hsr_node;
> >
> > diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> > index 36d5fcf09c61..59f8d2b68376 100644
> > --- a/net/hsr/hsr_slave.c
> > +++ b/net/hsr/hsr_slave.c
> > @@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
> > goto finish_consume;
> > }
> >
> > - /* For HSR, only tagged frames are expected, but for PRP
> > - * there could be non tagged frames as well from Single
> > - * attached nodes (SANs).
> > + /* For HSR, only tagged frames are expected (unless the device offloads
> > + * HSR tag removal), but for PRP there could be non tagged frames as
> > + * well from Single attached nodes (SANs).
> > */
> > protocol = eth_hdr(skb)->h_proto;
> > - if (hsr->proto_ops->invalid_dan_ingress_frame &&
> > +
> > + if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
> > + hsr->proto_ops->invalid_dan_ingress_frame &&
> > hsr->proto_ops->invalid_dan_ingress_frame(protocol))
> > goto finish_pass;
> >
> > @@ -137,6 +139,9 @@ static int hsr_portdev_setup(struct hsr_priv *hsr, struct net_device *dev,
> > master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> > hsr_dev = master->dev;
> >
> > + dev->flags |= IFF_SLAVE;
> > + dev->priv_flags |= IFF_HSR;
> > +
> > res = netdev_upper_dev_link(dev, hsr_dev, extack);
> > if (res)
> > goto fail_upper_dev_link;
> > --
> > 2.11.0
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support
2021-02-01 15:29 ` Vladimir Oltean
@ 2021-02-01 21:25 ` George McCollister
0 siblings, 0 replies; 17+ messages in thread
From: George McCollister @ 2021-02-01 21:25 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 1, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Feb 01, 2021 at 08:05:03AM -0600, George McCollister wrote:
> > Add offloading for HSR/PRP (IEC 62439-3) tag insertion, tag removal
> > forwarding and duplication supported by the xrs7000 series switches.
> >
> > Only HSR v1 and PRP v1 are supported by the xrs7000 series switches (HSR
> > v0 is not).
> >
> > Signed-off-by: George McCollister <george.mccollister@gmail.com>
> > ---
>
> Does this switch discard duplicates or does it not? If it does, what
> algorithm does it use? Does it not need some sort of runtime
> communication with the hsr master, like for the nodes table?
> How many streams can it keep track of? What happens when the ring is
> larger than the switch can keep track of in its internal Link Redundancy
> Entity?
It does discard duplicates.
The datasheet says:
"For HSR frames received from a HSR port, it is first checked if the
source MAC address exists in the MAC address table and if the source
node is located in non-HSR/PRP port. The duplicate detection is then
done by first looking at the stored HSR sequence numbers for the other
HSR redundant port: if one matches with the incoming frame’s HSR Tag’s
sequence number, we have a duplicate. Additionally, it is checked
whether a frame with this same sequence number and source MAC address,
that in from this same port has already been forwarded, in which case
the frame is circulating in the ring/network and has to be deleted. If
the frame is neither duplicate nor circulating, it is forwarded
towards its destination(s)."
The datasheet is publicly available here:
https://www.flexibilis.com/downloads/xrs/SpeedChip_XRS7000_3000_User_Manual.pdf
The IEC 62439-3:2016 spec makes it sound like it's the responsibility
of the network designer to make sure it's not possible :
"The maximum time t skewMax between two copies is a network property,
estimated by the
network designer based on the number of bridges and the traffic for a
particular application, e.g. 12 ms."
I don't see how large the table is in the switch. It shows a per model
"HSR proxy node table size" in the datasheet but I think that is just
the table used for the RedBox use case. It also says "Recommended HSR
network size" is up to 512 hops.
The switch does let you change ProxyNodeTableForgetTime (RedBox use
case only I think) and EntryForgetTime in the ADDRESS_AGING register.
The Linux software HSR implementation currently has all of this sort
of thing hardcoded and doesn't implement EntryForgetTime according to
the spec. In the future I can see adding support for this in software
HSR and then later in hardware as well.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-01 19:43 ` George McCollister
@ 2021-02-02 0:37 ` Vladimir Oltean
2021-02-02 14:49 ` George McCollister
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-02 0:37 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 01, 2021 at 01:43:43PM -0600, George McCollister wrote:
> > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> > > index ab953a1a0d6c..161b8da6a21d 100644
> > > --- a/net/hsr/hsr_device.c
> > > +++ b/net/hsr/hsr_device.c
> > > @@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
> > > * being, for PRP it is a trailer and for HSR it is a
> > > * header
> > > */
> > > - skb = dev_alloc_skb(sizeof(struct hsr_tag) +
> > > - sizeof(struct hsr_sup_tag) +
> > > + skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
> > > sizeof(struct hsr_sup_payload) + hlen + tlen);
> >
> > Question 1: why are you no longer allocating struct hsr_tag (or struct prp_rct,
> > which has the same size)?
>
> Because the tag is no longer being included in the supervisory frame
> here. If I understand correctly hsr_create_tagged_frame and
> prp_create_tagged_frame will create a new skb with HSR_HLEN added
> later.
I'm mostly doing static analysis of the code, which makes everything
more difficult and also my review more inaccurate. I'll try to give your
patches more testing when reviewing further, but I just got stuck into
trying to understand them first.
So your change makes fill_frame_info classify supervision frames as
skb_std instead of skb_hsr or skb_prp. The tag is added only in
hsr_create_tagged_frame right before dispatch to the egress port.
But that means that there are places like for example
hsr_handle_sup_frame which clearly don't like that: it checks whether
there's a tagged skb in either frame->skb_hsr or frame->skb_prp, but not
in frame->skb_std, so it now does basically nothing.
Don't we need hsr_handle_sup_frame?
> > In hsr->proto_ops->fill_frame_info in the call path above, the skb is
> > still put either into frame->skb_hsr or into frame->skb_prp, but not
> > into frame->skb_std, even if it does not contain a struct hsr_tag.
>
> Are you sure? My patch changes hsr_fill_frame_info and
> prp_fill_frame_info not to do that if port->type is HSR_PT_MASTER
> which I'm pretty certain it always is when sending supervisory frames
> like this. If I've overlooked something let me know.
You're right, I had figured it out myself in the comment below where I
called it a kludge.
> >
> > Also, which code exactly will insert the hsr_tag later? I assume
> > hsr_fill_tag via hsr->proto_ops->create_tagged_frame?
>
> Correct.
I think it's too late, see above.
> > > - if (!hsr->prot_version)
> > > - proto = ETH_P_PRP;
> > > - else
> > > - proto = ETH_P_HSR;
> > > -
> > > - skb = hsr_init_skb(master, proto);
> > > + skb = hsr_init_skb(master, ETH_P_PRP);
> >
> > Question 2: why is this correct, setting skb->protocol to ETH_P_PRP
> > (HSR v0) regardless of prot_version? Also, why is the change necessary?
>
> This part is not intuitive and I don't have a copy of the documents
> where v0 was defined. It's unfortunate this code even supports v0
> because AFAIK no one else uses it; but it's in here so we have to keep
> supporting it I guess.
> In v1 the tag has an eth type of 0x892f and the encapsulated
> supervisory frame has a type of 0x88fb. In v0 0x88fb is used for the
> eth type and there is no encapsulation type. So... this is correct
> however I compared supervisory frame generation before and after this
> patch for v0 and I found a problem. My changes make it add 0x88fb
> again later for v0 which it's not supposed to do. I'll have to fix
> that part somehow.
Step 1: Sign up for HSR maintainership, it's currently orphan
Step 2: Delete HSRv0 support
Step 3: See if anyone shouts, probably not
Step 4: Profit.
> >
> > Why is it such a big deal if supervision frames have HSR/PRP tag or not?
>
> Because if the switch does automatic HSR/PRP tag insertion it will end
> up in there twice. You simply can't send anything with an HSR/PRP tag
> if this is offloaded.
When exactly will your hardware push a second HSR tag when the incoming
packet already contains one? Obviously for tagged packets coming from
the ring it should not do that. It must be treating the CPU port special
somehow, but I don't understand how.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-02 0:37 ` Vladimir Oltean
@ 2021-02-02 14:49 ` George McCollister
2021-02-03 21:16 ` Vladimir Oltean
2021-02-06 23:43 ` Vladimir Oltean
0 siblings, 2 replies; 17+ messages in thread
From: George McCollister @ 2021-02-02 14:49 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 1, 2021 at 6:37 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Feb 01, 2021 at 01:43:43PM -0600, George McCollister wrote:
> > > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> > > > index ab953a1a0d6c..161b8da6a21d 100644
> > > > --- a/net/hsr/hsr_device.c
> > > > +++ b/net/hsr/hsr_device.c
> > > > @@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
> > > > * being, for PRP it is a trailer and for HSR it is a
> > > > * header
> > > > */
> > > > - skb = dev_alloc_skb(sizeof(struct hsr_tag) +
> > > > - sizeof(struct hsr_sup_tag) +
> > > > + skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
> > > > sizeof(struct hsr_sup_payload) + hlen + tlen);
> > >
> > > Question 1: why are you no longer allocating struct hsr_tag (or struct prp_rct,
> > > which has the same size)?
> >
> > Because the tag is no longer being included in the supervisory frame
> > here. If I understand correctly hsr_create_tagged_frame and
> > prp_create_tagged_frame will create a new skb with HSR_HLEN added
> > later.
>
> I'm mostly doing static analysis of the code, which makes everything
> more difficult and also my review more inaccurate. I'll try to give your
> patches more testing when reviewing further, but I just got stuck into
> trying to understand them first.
I don't blame you at all. I've spent hours (maybe an understatement)
looking at the hsr code. Reviewing this patch without already being
familiar with the code or the standard would be very daunting.
>
> So your change makes fill_frame_info classify supervision frames as
> skb_std instead of skb_hsr or skb_prp. The tag is added only in
> hsr_create_tagged_frame right before dispatch to the egress port.
>
> But that means that there are places like for example
> hsr_handle_sup_frame which clearly don't like that: it checks whether
> there's a tagged skb in either frame->skb_hsr or frame->skb_prp, but not
> in frame->skb_std, so it now does basically nothing.
>
> Don't we need hsr_handle_sup_frame?
This part of the hsr code is very confusing and gave me problems at
first. Everywhere in the hsr_forward_do loop port is the destination
port. When it checks port->type == HSR_PT_MASTER before calling
hsr_handle_sup_frame it is checking for supervisory frames going to
the master port not from it. That is to say hsr_handle_sup_frame is
only called on incoming supervisory frames. This patch only addresses
generation of supervisory frames, that is to say outgoing supervisory
frames.
I may need to address this in the next patch for the case when removal
of the hsr/prp tag is offloaded on incoming frames.
>
> > > In hsr->proto_ops->fill_frame_info in the call path above, the skb is
> > > still put either into frame->skb_hsr or into frame->skb_prp, but not
> > > into frame->skb_std, even if it does not contain a struct hsr_tag.
> >
> > Are you sure? My patch changes hsr_fill_frame_info and
> > prp_fill_frame_info not to do that if port->type is HSR_PT_MASTER
> > which I'm pretty certain it always is when sending supervisory frames
> > like this. If I've overlooked something let me know.
>
> You're right, I had figured it out myself in the comment below where I
> called it a kludge.
>
> > >
> > > Also, which code exactly will insert the hsr_tag later? I assume
> > > hsr_fill_tag via hsr->proto_ops->create_tagged_frame?
> >
> > Correct.
>
> I think it's too late, see above.
I'm not saying it's impossible I missed something but I did test this
code pretty well with HSR v1 with and without offload on a ring with a
Moxa PT-G503. I even inspected everything on the wire to make sure it
was correct. I tested PRP as well though now I can't remember if I
inspected it on the wire so I'll make sure to check it again before
sending the next patch version.
>
> > > > - if (!hsr->prot_version)
> > > > - proto = ETH_P_PRP;
> > > > - else
> > > > - proto = ETH_P_HSR;
> > > > -
> > > > - skb = hsr_init_skb(master, proto);
> > > > + skb = hsr_init_skb(master, ETH_P_PRP);
> > >
> > > Question 2: why is this correct, setting skb->protocol to ETH_P_PRP
> > > (HSR v0) regardless of prot_version? Also, why is the change necessary?
> >
> > This part is not intuitive and I don't have a copy of the documents
> > where v0 was defined. It's unfortunate this code even supports v0
> > because AFAIK no one else uses it; but it's in here so we have to keep
> > supporting it I guess.
> > In v1 the tag has an eth type of 0x892f and the encapsulated
> > supervisory frame has a type of 0x88fb. In v0 0x88fb is used for the
> > eth type and there is no encapsulation type. So... this is correct
> > however I compared supervisory frame generation before and after this
> > patch for v0 and I found a problem. My changes make it add 0x88fb
> > again later for v0 which it's not supposed to do. I'll have to fix
> > that part somehow.
>
> Step 1: Sign up for HSR maintainership, it's currently orphan
> Step 2: Delete HSRv0 support
> Step 3: See if anyone shouts, probably not
> Step 4: Profit.
not a bad idea however user space defaults to using v0 when doing:
ip link add name hsr0 type hsr slave1 eth0 slave2 eth1
To use v1 you need to append "version 1".
It seems like it might be a hard sell to break the userspace default
even if no one in their right mind is using it. Still think it's
possible?
>
> > >
> > > Why is it such a big deal if supervision frames have HSR/PRP tag or not?
> >
> > Because if the switch does automatic HSR/PRP tag insertion it will end
> > up in there twice. You simply can't send anything with an HSR/PRP tag
> > if this is offloaded.
>
> When exactly will your hardware push a second HSR tag when the incoming
> packet already contains one? Obviously for tagged packets coming from
> the ring it should not do that. It must be treating the CPU port special
> somehow, but I don't understand how.
From the datasheet I linked before:
"At input the HSR tag is always removed if the port is in HSR mode. At
output a HSR tag is added if the output port is in HSR mode."
I don't see a great description of what happens internally when it's
forwarding from one redundant port to the other when in HSR (not PRP)
but perhaps it strips off the tag information, saves it and reapplies
it as it's going out? The redundant ports are in HSR mode, the CPU
facing port is not. Anyway I can tell you from using it, it does add a
second HSR tag if the CPU port sends a frame with one and the frames
going between the ring redundancy ports are forwarded with their
single tag.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 2/4] net: hsr: add offloading support
2021-02-01 15:23 ` Vladimir Oltean
2021-02-01 20:15 ` George McCollister
@ 2021-02-03 20:43 ` George McCollister
1 sibling, 0 replies; 17+ messages in thread
From: George McCollister @ 2021-02-03 20:43 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Mon, Feb 1, 2021 at 9:23 AM Vladimir Oltean <olteanv@gmail.com> wrote:
[snip]
> > @@ -357,6 +367,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > {
> > struct hsr_port *port;
> > struct sk_buff *skb;
> > + bool sent = false;
> >
> > hsr_for_each_port(frame->port_rcv->hsr, port) {
> > struct hsr_priv *hsr = port->hsr;
> > @@ -372,6 +383,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> > if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
> > continue;
> >
> > + /* If hardware duplicate generation is enabled, only send out
> > + * one port.
> > + */
> > + if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
> > + continue;
> > +
>
> Is this right? It looks to me that you are not bypassing only duplicate
> generation, but also duplicate elimination.
I missed this part in my last reply.
When the duplicate generation is offloaded it simply doesn't attempt
to send the frame out more than one slave device.
It won't skip delivering to the master because NETIF_F_HW_HSR_DUP is
not set on the master device. Again the way this loop works is
confusing (at least to me) because it's handling incoming and outgoing
frames in the same loop.
The duplicate elimination prevents sending a frame with the same
sequence number and source twice to the same device.
It's not bypassing the duplicate elimination. You could certainly have
a switch that sends frames out both redundant ports automatically but
didn't eliminate duplicates being delivered to the CPU facing port.
Let me know if you still think there is a problem and we can discuss it further.
>
> I think this is duplicate elimination:
>
> /* Don't send frame over port where it has been sent before.
> * Also fro SAN, this shouldn't be done.
> */
> if (!frame->is_from_san &&
> hsr_register_frame_out(port, frame->node_src,
> frame->sequence_nr))
> continue;
>
> and this is duplicate generation:
>
> hsr_for_each_port(frame->port_rcv->hsr, port) {
> ...
> skb->dev = port->dev;
> if (port->type == HSR_PT_MASTER)
> hsr_deliver_master(skb, port->dev, frame->node_src);
> else
> hsr_xmit(skb, port, frame);
> }
>
> So if this is the description of NETIF_F_HW_HSR_DUP:
>
> | Duplication involves the switch automatically sending a single frame
> | from the CPU port to both redundant ports. This is required because the
> | inserted HSR/PRP header/trailer must contain the same sequence number
> | on the frames sent out both redundant ports.
>
> then NETIF_F_HW_HSR_DUP is a misnomer. You should think of either
> grouping duplicate elimination and generation together, or rethinking
> the whole features system.
>
> > /* Don't send frame over port where it has been sent before.
> > * Also fro SAN, this shouldn't be done.
> > */
[snip]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-02 14:49 ` George McCollister
@ 2021-02-03 21:16 ` Vladimir Oltean
2021-02-06 23:43 ` Vladimir Oltean
1 sibling, 0 replies; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-03 21:16 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
> > > This part is not intuitive and I don't have a copy of the documents
> > > where v0 was defined. It's unfortunate this code even supports v0
> > > because AFAIK no one else uses it; but it's in here so we have to keep
> > > supporting it I guess.
> > > In v1 the tag has an eth type of 0x892f and the encapsulated
> > > supervisory frame has a type of 0x88fb. In v0 0x88fb is used for the
> > > eth type and there is no encapsulation type. So... this is correct
> > > however I compared supervisory frame generation before and after this
> > > patch for v0 and I found a problem. My changes make it add 0x88fb
> > > again later for v0 which it's not supposed to do. I'll have to fix
> > > that part somehow.
> >
> > Step 1: Sign up for HSR maintainership, it's currently orphan
> > Step 2: Delete HSRv0 support
> > Step 3: See if anyone shouts, probably not
> > Step 4: Profit.
>
> not a bad idea however user space defaults to using v0 when doing:
> ip link add name hsr0 type hsr slave1 eth0 slave2 eth1
>
> To use v1 you need to append "version 1".
>
> It seems like it might be a hard sell to break the userspace default
> even if no one in their right mind is using it. Still think it's
> possible?
While HSRv0 is the default, IFLA_HSR_VERSION won't be put in the netlink
message generated by iproute2 unless you explicitly write "version 0".
So it's not like ip-link will now error out on a default RTM_NEWLINK
message, the kernel will just use a different (and more sane, according
to you) default.
Removing support for a protocol is pretty radical, but I guess if you
can make a convincing argument that nobody depends on it, it may get
accepted.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-02 14:49 ` George McCollister
2021-02-03 21:16 ` Vladimir Oltean
@ 2021-02-06 23:43 ` Vladimir Oltean
2021-02-08 15:26 ` George McCollister
1 sibling, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2021-02-06 23:43 UTC (permalink / raw)
To: George McCollister
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
> > > > Why is it such a big deal if supervision frames have HSR/PRP tag or not?
> > >
> > > Because if the switch does automatic HSR/PRP tag insertion it will end
> > > up in there twice. You simply can't send anything with an HSR/PRP tag
> > > if this is offloaded.
> >
> > When exactly will your hardware push a second HSR tag when the incoming
> > packet already contains one? Obviously for tagged packets coming from
> > the ring it should not do that. It must be treating the CPU port special
> > somehow, but I don't understand how.
>
> From the datasheet I linked before:
> "At input the HSR tag is always removed if the port is in HSR mode. At
> output a HSR tag is added if the output port is in HSR mode."
> I don't see a great description of what happens internally when it's
> forwarding from one redundant port to the other when in HSR (not PRP)
> but perhaps it strips off the tag information, saves it and reapplies
> it as it's going out? The redundant ports are in HSR mode, the CPU
> facing port is not. Anyway I can tell you from using it, it does add a
> second HSR tag if the CPU port sends a frame with one and the frames
> going between the ring redundancy ports are forwarded with their
> single tag.
So if I understand correctly, the CPU port is configured as an interlink
port, which according to the standard can operate in 3 modes:
1) HSR-SAN: the traffic on the interlink is not HSR, not PRP
2) HSR-PRP: the traffic on the interlink is PRP-tagged as “A” or “B”
3) HSR-HSR the traffic on the interlink is HSR-tagged.
What you are saying is equivalent to the CPU port being configured for a
HSR-SAN interlink. If the CPU port was configured as HSR-HSR interlink,
this change would have not been necessary.
However 6.7 Allowed Port Modes of the XRS7000 datasheet you shared says:
| Not every port of XRS is allowed to be configured in every mode, Table
| 25 lists the allowed modes for each port.
That table basically says that while any port can operate as a 'non-HSR,
non-PRP' interlink, only port 3 of the XRS7004 can operate as an HSR
interlink. So it is more practical to you to leave the CPU port as a
normal interlink and leave the switch push the tags.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag
2021-02-06 23:43 ` Vladimir Oltean
@ 2021-02-08 15:26 ` George McCollister
0 siblings, 0 replies; 17+ messages in thread
From: George McCollister @ 2021-02-08 15:26 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Jakub Kicinski, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Jonathan Corbet, netdev
On Sat, Feb 6, 2021 at 5:43 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
> > > > > Why is it such a big deal if supervision frames have HSR/PRP tag or not?
> > > >
> > > > Because if the switch does automatic HSR/PRP tag insertion it will end
> > > > up in there twice. You simply can't send anything with an HSR/PRP tag
> > > > if this is offloaded.
> > >
> > > When exactly will your hardware push a second HSR tag when the incoming
> > > packet already contains one? Obviously for tagged packets coming from
> > > the ring it should not do that. It must be treating the CPU port special
> > > somehow, but I don't understand how.
> >
> > From the datasheet I linked before:
> > "At input the HSR tag is always removed if the port is in HSR mode. At
> > output a HSR tag is added if the output port is in HSR mode."
> > I don't see a great description of what happens internally when it's
> > forwarding from one redundant port to the other when in HSR (not PRP)
> > but perhaps it strips off the tag information, saves it and reapplies
> > it as it's going out? The redundant ports are in HSR mode, the CPU
> > facing port is not. Anyway I can tell you from using it, it does add a
> > second HSR tag if the CPU port sends a frame with one and the frames
> > going between the ring redundancy ports are forwarded with their
> > single tag.
>
> So if I understand correctly, the CPU port is configured as an interlink
> port, which according to the standard can operate in 3 modes:
> 1) HSR-SAN: the traffic on the interlink is not HSR, not PRP
> 2) HSR-PRP: the traffic on the interlink is PRP-tagged as “A” or “B”
> 3) HSR-HSR the traffic on the interlink is HSR-tagged.
>
> What you are saying is equivalent to the CPU port being configured for a
> HSR-SAN interlink. If the CPU port was configured as HSR-HSR interlink,
> this change would have not been necessary.
>
> However 6.7 Allowed Port Modes of the XRS7000 datasheet you shared says:
>
> | Not every port of XRS is allowed to be configured in every mode, Table
> | 25 lists the allowed modes for each port.
>
> That table basically says that while any port can operate as a 'non-HSR,
> non-PRP' interlink, only port 3 of the XRS7004 can operate as an HSR
> interlink. So it is more practical to you to leave the CPU port as a
> normal interlink and leave the switch push the tags.
If one were to set "HSR/PRP enabled" and "Port is HSR/PRP interlink
port" in HSR_CFG on the CPU port it wouldn't be possible (per the
datasheet) to enable the management or PTP timer trailer. Those modes
are intended for interlinking with a second HSR/PRP switch's
interlink. I was trying to keep this commit message somewhat switch
model agnostic and not dive into the details of the XRS7000 series. Do
you think I should describe all of this in detail in this commit
message?
-George
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-02-08 15:30 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01 14:04 [RESEND PATCH net-next 0/4] add HSR offloading support for DSA switches George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag George McCollister
2021-02-01 14:59 ` Vladimir Oltean
2021-02-01 19:43 ` George McCollister
2021-02-02 0:37 ` Vladimir Oltean
2021-02-02 14:49 ` George McCollister
2021-02-03 21:16 ` Vladimir Oltean
2021-02-06 23:43 ` Vladimir Oltean
2021-02-08 15:26 ` George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 2/4] net: hsr: add offloading support George McCollister
2021-02-01 15:23 ` Vladimir Oltean
2021-02-01 20:15 ` George McCollister
2021-02-03 20:43 ` George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 3/4] net: dsa: add support for offloading HSR George McCollister
2021-02-01 14:05 ` [RESEND PATCH net-next 4/4] net: dsa: xrs700x: add HSR offloading support George McCollister
2021-02-01 15:29 ` Vladimir Oltean
2021-02-01 21:25 ` George McCollister
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.