Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> This patch add support for creating vlan interfaces
> over hsr/prp interface.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@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.

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

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