| Message ID | 20210201140503.130625-2-george.mccollister@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,net-next,1/4] net: hsr: generate supervision frame without HSR tag | expand |
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 >
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 > >
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.
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.
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.
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.
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
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;
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(-)