From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 14:06:27
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(-)
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 14:06:27
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
@@ -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.
@@ -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.*/
@@ -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);
@@ -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);returnskb_clone(frame->skb_hsr,GFP_ATOMIC);+}elseif(port->dev->features&NETIF_F_HW_HSR_TAG_INS){+returnskb_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));}+boolhsr_drop_frame(structhsr_frame_info*frame,structhsr_port*port)+{+if(port->dev->features&NETIF_F_HW_HSR_FWD)+returnprp_drop_frame(frame,port);++returnfalse;+}+/* Forward the frame through all devices except:*-Backthroughthereceivingdevice*-Ifit'saHSRframe:throughadevicewhereithaspassedbefore
@@ -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+*oneport.+*/+if((port->dev->features&NETIF_F_HW_HSR_DUP)&&sent)+continue;+/* Don't send frame over port where it has been sent before.*AlsofroSAN,thisshouldn'tbedone.*/
@@ -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)*Table8.
@@ -171,13 +172,6 @@ struct hsr_port {enumhsr_port_typetype;};-/* used by driver internally to differentiate various protocols */-enumhsr_version{-HSR_V0=0,-HSR_V1,-PRP_V1,-};-structhsr_frame_info;structhsr_node;
@@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)gotofinish_consume;}-/* For HSR, only tagged frames are expected, but for PRP-*therecouldbenontaggedframesaswellfromSingle-*attachednodes(SANs).+/* For HSR, only tagged frames are expected (unless the device offloads+*HSRtagremoval),butforPRPtherecouldbenontaggedframesas+*wellfromSingleattachednodes(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))gotofinish_pass;
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 14:06:38
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(+)
@@ -94,6 +96,13 @@ struct dsa_switchdev_event_work {u16vid;};+/* DSA_NOTIFIER_HSR_* */+structdsa_notifier_hsr_info{+structnet_device*hsr;+intsw_index;+intport;+};+structdsa_slave_priv{/* Copy of CPU port xmit for faster access in slave transmit hot path */structsk_buff*(*xmit)(structsk_buff*skb,
@@ -174,6 +183,8 @@ int dsa_port_vlan_del(struct dsa_port *dp,conststructswitchdev_obj_port_vlan*vlan);intdsa_port_link_register_of(structdsa_port*dp);voiddsa_port_link_unregister_of(structdsa_port*dp);+intdsa_port_hsr_join(structdsa_port*dp,structnet_device*hsr);+voiddsa_port_hsr_leave(structdsa_port*dp,structnet_device*hsr);externconststructphylink_mac_opsdsa_port_phylink_mac_ops;staticinlinebooldsa_port_offloads_netdev(structdsa_port*dp,
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 14:06:45
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(-)
@@ -496,6 +498,108 @@ static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,xrs700x_bridge_common(ds,port,bridge,false);}+staticintxrs700x_hsr_join(structdsa_switch*ds,intport,+structnet_device*hsr)+{+unsignedintval=XRS_HSR_CFG_HSR_PRP;+structdsa_port*partner=NULL,*dp;+structxrs700x*priv=ds->priv;+structnet_device*slave;+enumhsr_versionver;+intret;++ret=hsr_get_version(hsr,&ver);+if(ret)+returnret;++if(ver==HSR_V1)+val|=XRS_HSR_CFG_HSR;+elseif(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+*redundantportshavesignedup.+*/+if(!partner)+return0;++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+*enableforwarding.+*/+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;++return0;+}++staticvoidxrs700x_hsr_leave(structdsa_switch*ds,intport,+structnet_device*hsr)+{+structdsa_port*partner=NULL,*dp;+structxrs700x*priv=ds->priv;+structnet_device*slave;+unsignedintval;++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);+}+staticconststructdsa_switch_opsxrs700x_ops={.get_tag_protocol=xrs700x_get_tag_protocol,.setup=xrs700x_setup,
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-01 15:02:34
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
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?
@@ -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?
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?
quoted hunk
/* HSR tagged frame :- Data or Supervision */
frame->skb_std = NULL;
frame->skb_prp = NULL;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-01 15:24:44
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
quoted hunk
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
@@ -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?
@@ -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.*/
@@ -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);
@@ -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);returnskb_clone(frame->skb_hsr,GFP_ATOMIC);+}elseif(port->dev->features&NETIF_F_HW_HSR_TAG_INS){+returnskb_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));}+boolhsr_drop_frame(structhsr_frame_info*frame,structhsr_port*port)+{+if(port->dev->features&NETIF_F_HW_HSR_FWD)+returnprp_drop_frame(frame,port);++returnfalse;+}+/* Forward the frame through all devices except:*-Backthroughthereceivingdevice*-Ifit'saHSRframe:throughadevicewhereithaspassedbefore
@@ -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+*oneport.+*/+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.
quoted hunk
/* Don't send frame over port where it has been sent before.
* Also fro SAN, this shouldn't be done.
*/
@@ -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)*Table8.
@@ -171,13 +172,6 @@ struct hsr_port {enumhsr_port_typetype;};-/* used by driver internally to differentiate various protocols */-enumhsr_version{-HSR_V0=0,-HSR_V1,-PRP_V1,-};-structhsr_frame_info;structhsr_node;
@@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)gotofinish_consume;}-/* For HSR, only tagged frames are expected, but for PRP-*therecouldbenontaggedframesaswellfromSingle-*attachednodes(SANs).+/* For HSR, only tagged frames are expected (unless the device offloads+*HSRtagremoval),butforPRPtherecouldbenontaggedframesas+*wellfromSingleattachednodes(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))gotofinish_pass;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-01 15:30:44
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?
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 19:45:48
On Mon, Feb 1, 2021 at 8:59 AM Vladimir Oltean [off-list ref] wrote:
On Mon, Feb 01, 2021 at 08:05:00AM -0600, George McCollister wrote:
quoted
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
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?
@@ -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.
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.
quoted
/* HSR tagged frame :- Data or Supervision */
frame->skb_std = NULL;
frame->skb_prp = NULL;
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 20:16:45
On Mon, Feb 1, 2021 at 9:23 AM Vladimir Oltean [off-list ref] wrote:
On Mon, Feb 01, 2021 at 08:05:01AM -0600, George McCollister wrote:
quoted
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
quoted
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
quoted
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
@@ -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.
quoted
+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?
@@ -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.*/
@@ -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);
@@ -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);returnskb_clone(frame->skb_hsr,GFP_ATOMIC);+}elseif(port->dev->features&NETIF_F_HW_HSR_TAG_INS){+returnskb_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));}+boolhsr_drop_frame(structhsr_frame_info*frame,structhsr_port*port)+{+if(port->dev->features&NETIF_F_HW_HSR_FWD)+returnprp_drop_frame(frame,port);++returnfalse;+}+/* Forward the frame through all devices except:*-Backthroughthereceivingdevice*-Ifit'saHSRframe:throughadevicewhereithaspassedbefore
@@ -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+*oneport.+*/+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.
quoted
/* Don't send frame over port where it has been sent before.
* Also fro SAN, this shouldn't be done.
*/
@@ -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)*Table8.
@@ -171,13 +172,6 @@ struct hsr_port {enumhsr_port_typetype;};-/* used by driver internally to differentiate various protocols */-enumhsr_version{-HSR_V0=0,-HSR_V1,-PRP_V1,-};-structhsr_frame_info;structhsr_node;
@@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)gotofinish_consume;}-/* For HSR, only tagged frames are expected, but for PRP-*therecouldbenontaggedframesaswellfromSingle-*attachednodes(SANs).+/* For HSR, only tagged frames are expected (unless the device offloads+*HSRtagremoval),butforPRPtherecouldbenontaggedframesas+*wellfromSingleattachednodes(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))gotofinish_pass;
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-01 21:26:17
On Mon, Feb 1, 2021 at 9:29 AM Vladimir Oltean [off-list ref] wrote:
On Mon, Feb 01, 2021 at 08:05:03AM -0600, George McCollister wrote:
quoted
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.
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?
quoted
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.
quoted
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.
quoted
quoted
- 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.
quoted
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.
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.
quoted
quoted
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.
quoted
quoted
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.
quoted
quoted
quoted
- 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?
quoted
quoted
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.
@@ -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.
quoted
/* Don't send frame over port where it has been sent before.
* Also fro SAN, this shouldn't be done.
*/
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-03 21:17:35
On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
quoted
quoted
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.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-06 23:47:23
On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
quoted
quoted
quoted
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.
From: George McCollister <george.mccollister@gmail.com> Date: 2021-02-08 15:30:53
On Sat, Feb 6, 2021 at 5:43 PM Vladimir Oltean [off-list ref] wrote:
On Tue, Feb 02, 2021 at 08:49:25AM -0600, George McCollister wrote:
quoted
quoted
quoted
quoted
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