On Tue, Sep 8, 2020 at 6:39 PM Murali Karicheri <m-kariche...@ti.com> wrote:
>
> 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);
It's a bit hard to see, since this is a draft patch on top of the
existing patch. But I think so.
Only, instead of dst + movelen, you can use src.
> }
>
>
> > 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?
It's good to err on the safe side. That should probably be a separate patch.
