Re: [RESEND PATCH net-next 1/4] net: hsr: generate supervision frame without HSR tag

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

quoted

 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?

quoted

      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.

quoted

      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.

quoted

              /* 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