Hi Willem,

Thanks for the response!
On 9/4/20 11:45 AM, Willem de Bruijn wrote:
On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-kariche...@ti.com> wrote:

This patch add support for creating vlan interfaces
over hsr/prp interface.

Signed-off-by: Murali Karicheri <m-kariche...@ti.com>
---
  net/hsr/hsr_device.c  |  4 ----
  net/hsr/hsr_forward.c | 16 +++++++++++++---
  2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index ab953a1a0d6c..e1951579a3ad 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)

         /* Prevent recursive tx locking */
         dev->features |= NETIF_F_LLTX;
-       /* VLAN on top of HSR needs testing and probably some work on
-        * hsr_header_create() etc.
-        */
-       dev->features |= NETIF_F_VLAN_CHALLENGED;
         /* Not sure about this. Taken from bridge code. netdev_features.h says
          * it means "Does not change network namespaces".
          */
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index cadfccd7876e..de21df30b0d9 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -208,6 +208,7 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
                                     struct hsr_port *port, u8 proto_version)
  {
         struct hsr_ethhdr *hsr_ethhdr;
+       unsigned char *pc;
         int lsdu_size;

         /* pad to minimum packet size which is 60 + 6 (HSR tag) */
@@ -218,7 +219,18 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
         if (frame->is_vlan)
                 lsdu_size -= 4;

-       hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
+       pc = skb_mac_header(skb);
+       if (frame->is_vlan)
+               /* This 4-byte shift (size of a vlan tag) does not
+                * mean that the ethhdr starts there. But rather it
+                * provides the proper environment for accessing
+                * the fields, such as hsr_tag etc., just like
+                * when the vlan tag is not there. This is because
+                * the hsr tag is after the vlan tag.
+                */
+               hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
+       else
+               hsr_ethhdr = (struct hsr_ethhdr *)pc;

Instead, I would pass the header from the caller, which knows the
offset because it moves the previous headers to make space.

So if I understood you correctly a diff for the above would like this
where pass dst + movelen as  struct hsr_ethhdr *hsr_ethhdr
pointer to hsr_fill_tag(), right?

a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index de21df30b0d9..4d9192c8bcf8 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -204,11 +204,10 @@ static void hsr_set_path_id(struct hsr_ethhdr *hsr_ethhdr,
 }

 static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
+                                   struct hsr_ethhdr *hsr_ethhdr,
                                    struct hsr_frame_info *frame,
struct hsr_port *port, u8 proto_version)
 {
-       struct hsr_ethhdr *hsr_ethhdr;
-       unsigned char *pc;
        int lsdu_size;

        /* pad to minimum packet size which is 60 + 6 (HSR tag) */
@@ -219,19 +218,6 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
        if (frame->is_vlan)
                lsdu_size -= 4;

-       pc = skb_mac_header(skb);
-       if (frame->is_vlan)
-               /* This 4-byte shift (size of a vlan tag) does not
-                * mean that the ethhdr starts there. But rather it
-                * provides the proper environment for accessing
-                * the fields, such as hsr_tag etc., just like
-                * when the vlan tag is not there. This is because
-                * the hsr tag is after the vlan tag.
-                */
-               hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
-       else
-               hsr_ethhdr = (struct hsr_ethhdr *)pc;
-
        hsr_set_path_id(hsr_ethhdr, port);
        set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
        hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
@@ -280,10 +266,12 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
        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
+       /* dst point to the start of hsr tag. So pass it to fill the
+        * hsr info. Also 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);
+       return hsr_fill_tag(skb, (struct hsr_ethhdr *)(dst + movelen),
+                           frame, port, port->hsr->prot_version);
 }


Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
which means code should parse the headers instead of hardcoding
VLAN_HLEN.


iec-62439-3 standard only talks about VLAN (TPID 0x8100), not about
QinQ. So what I could do is to check and bail out if 802.1ad frame is
received at the interface from upper layer. Something like below and
frame will get dropped.

@@ -519,6 +507,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,

        if (proto == htons(ETH_P_8021Q))
                frame->is_vlan = true;
+       else if (proto == htons(ETH_P_8021AD))
+               return -1; /* Don't support 802.1ad */

        if (frame->is_vlan) {
                vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;

What do you think?

Thanks.
         hsr_set_path_id(hsr_ethhdr, port);
         set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
@@ -511,8 +523,6 @@ static int fill_frame_info(struct hsr_frame_info *frame,
         if (frame->is_vlan) {
                 vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
                 proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
-               /* FIXME: */
-               netdev_warn_once(skb->dev, "VLAN not yet supported");
         }

         frame->is_from_san = false;
--
2.17.1


--
Murali Karicheri
Texas Instruments

Reply via email to