Date:   Mon, 1 Feb 2021 13:43:43 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: 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 <olteanv@...il.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@...il.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
> >

Powered by blists - more mailing lists