| Message ID | 20200901195415.4840-2-m-karicheri2@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support for VLAN interface over HSR/PRP | expand |
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 >
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-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. > 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
On Tue, Sep 8, 2020 at 6:39 PM Murali Karicheri <m-karicheri2@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-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. > > > 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.
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; 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;
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(-) -- 2.17.1