* [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames @ 2018-05-08 6:01 Elad Nachman 2018-05-08 6:43 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-08 6:01 UTC (permalink / raw) To: davem, netdev; +Cc: eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..ced2d34 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == NETIF_F_HW_VLAN_CTAG_RX && !__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames 2018-05-08 6:01 [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames Elad Nachman @ 2018-05-08 6:43 ` Toshiaki Makita 2018-05-08 7:11 ` Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-05-08 6:43 UTC (permalink / raw) To: Elad Nachman; +Cc: davem, netdev On 2018/05/08 15:01, Elad Nachman wrote: > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, > and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() > instead of using the hard-coded value of ETH_P_8021Q. > > Signed-off-by: Elad Nachman <eladn@gilat.com> > > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index b65e2d1..ced2d34 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) > > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > u16 vlanid; > + __be16 vlan_proto; > > if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > NETIF_F_HW_VLAN_CTAG_RX && > !__vlan_get_tag(skb, &vlanid)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); This is what devices with NETIF_F_HW_VLAN_STAG_RX are supposed to do, not NETIF_F_HW_VLAN_CTAG_RX. By the way this looks like doing the same thing as skb_vlan_untag in __netif_receive_skb_core, so seems unnecessary to add HW_VLAN_STAG_RX. Alternatively you can check if vlan_proto is 8021Q here. -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames 2018-05-08 6:43 ` Toshiaki Makita @ 2018-05-08 7:11 ` Elad Nachman 2018-05-08 7:34 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-08 7:11 UTC (permalink / raw) To: Toshiaki Makita; +Cc: davem, netdev, eladv6 Currently running: ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100 On eth0=stmmac succeeds, but the end result is that the vlan device gets proto 802.1q instead of proto 802.1ad and drops the received packet. Without the patch packets gets dropped for a seemingly "correct" 802.1ad ip link configuration. If NETIF_F_HW_VLAN_STAG_RX is a requirement for the driver for supporting 802.1ad protocols then the Linux kernel should return error when user-space requests to create a vlan device with proto 802.1ad for physical devices which lacks NETIF_F_HW_VLAN_STAG_RX, which is not currently the case. skb_vlan_untag() does nothing if __vlan_hwaccel_put_tag() was already called before (in the driver). The only possible alternative is to completely remove stmmac_rx_vlan() from the stmmac code and let skb_vlan_untag() handles things in a generic way. On 08/05/18 09:43, Toshiaki Makita wrote: > On 2018/05/08 15:01, Elad Nachman wrote: >> stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). >> >> The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, >> and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . >> >> This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol >> (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). >> >> This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() >> instead of using the hard-coded value of ETH_P_8021Q. >> >> Signed-off-by: Elad Nachman <eladn@gilat.com> >> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index b65e2d1..ced2d34 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) >> >> static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) >> { >> - struct ethhdr *ehdr; >> + struct vlan_ethhdr *veth; >> u16 vlanid; >> + __be16 vlan_proto; >> >> if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == >> NETIF_F_HW_VLAN_CTAG_RX && >> !__vlan_get_tag(skb, &vlanid)) { >> /* pop the vlan tag */ >> - ehdr = (struct ethhdr *)skb->data; >> - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); >> + veth = (struct vlan_ethhdr *)skb->data; >> + vlan_proto = veth->h_vlan_proto; >> + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); >> skb_pull(skb, VLAN_HLEN); >> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); >> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > > This is what devices with NETIF_F_HW_VLAN_STAG_RX are supposed to do, > not NETIF_F_HW_VLAN_CTAG_RX. > > By the way this looks like doing the same thing as skb_vlan_untag in > __netif_receive_skb_core, so seems unnecessary to add HW_VLAN_STAG_RX. > Alternatively you can check if vlan_proto is 8021Q here. > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames 2018-05-08 7:11 ` Elad Nachman @ 2018-05-08 7:34 ` Toshiaki Makita 2018-05-11 7:31 ` [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q " Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-05-08 7:34 UTC (permalink / raw) To: Elad Nachman; +Cc: davem, netdev On 2018/05/08 16:11, Elad Nachman wrote: > Currently running: > ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100 > > On eth0=stmmac succeeds, but the end result is that the vlan device gets proto 802.1q instead of proto 802.1ad and drops the received packet. Without the patch packets gets dropped for a seemingly "correct" 802.1ad ip link configuration. > > If NETIF_F_HW_VLAN_STAG_RX is a requirement for the driver for supporting 802.1ad protocols then the Linux kernel should return error when user-space requests to create a vlan device with proto 802.1ad for physical devices which lacks NETIF_F_HW_VLAN_STAG_RX, which is not currently the case. No. You can create 802.1ad devices without HW_VLAN_STAG_RX, but you should not strip 802.1ad tag in driver without HW_VLAN_STAG_RX. __netif_receive_skb_core should handle them if the device does not have HW_VLAN_STAG_RX. > skb_vlan_untag() does nothing if __vlan_hwaccel_put_tag() was already called before (in the driver). The only possible alternative is to completely remove stmmac_rx_vlan() from the stmmac code and let skb_vlan_untag() handles things in a generic way. You cannot remove an already added feature in the driver. Alternatively you can skip stripping vlan if vlan_proto is not 8021Q. Something like this. if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == NETIF_F_HW_VLAN_CTAG_RX && !__vlan_get_tag(skb, &vlanid)) { veth = (struct vlan_ethhdr *)skb->data; vlan_proto = veth->h_vlan_proto; if (vlan_proto == htons(ETH_P_8021Q)) { /* pop the vlan tag */ memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); } } } -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-08 7:34 ` Toshiaki Makita @ 2018-05-11 7:31 ` Elad Nachman 2018-05-17 16:43 ` David Miller 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-11 7:31 UTC (permalink / raw) To: Toshiaki Makita, davem, netdev; +Cc: eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() without checking the actual VLAN tag. This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix only strips the VLAN tag for 802.1Q tagged protocols. 802.1AD frames will be handled later by skb_vlan_untag() . Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..b230ab5 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,17 +3293,21 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == NETIF_F_HW_VLAN_CTAG_RX && !__vlan_get_tag(skb, &vlanid)) { - /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); - skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + if (likely(vlan_proto == htons(ETH_P_8021Q))) { + /* pop the vlan tag */ + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); + skb_pull(skb, VLAN_HLEN); + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + } } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-11 7:31 ` [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q " Elad Nachman @ 2018-05-17 16:43 ` David Miller 2018-05-21 15:48 ` David Miller 0 siblings, 1 reply; 27+ messages in thread From: David Miller @ 2018-05-17 16:43 UTC (permalink / raw) To: eladv6; +Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue From: Elad Nachman <eladv6@gmail.com> Date: Fri, 11 May 2018 10:31:40 +0300 > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan > before calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with > 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding > the parameter on call to __vlan_hwaccel_put_tag() without checking > the actual VLAN tag. > > This causes packets not to be passed to the VLAN slave if it was > created with 802.1AD protocol (ip link add link eth0 eth0.100 type > vlan proto 802.1ad id 100). > > This fix only strips the VLAN tag for 802.1Q tagged > protocols. 802.1AD frames will be handled later by skb_vlan_untag(). > > Signed-off-by: Elad Nachman <eladn@gilat.com> Giuseppe and Alexandre, please review this patch. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-17 16:43 ` David Miller @ 2018-05-21 15:48 ` David Miller 2018-05-21 16:42 ` Florian Fainelli 0 siblings, 1 reply; 27+ messages in thread From: David Miller @ 2018-05-21 15:48 UTC (permalink / raw) To: eladv6; +Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue From: David Miller <davem@davemloft.net> Date: Thu, 17 May 2018 12:43:56 -0400 (EDT) > Giuseppe and Alexandre, please review this patch. If nobody thinks this patch is important enough to actually review, I'm tossing it. Sorry. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-21 15:48 ` David Miller @ 2018-05-21 16:42 ` Florian Fainelli 2018-05-22 8:56 ` Jose Abreu 0 siblings, 1 reply; 27+ messages in thread From: Florian Fainelli @ 2018-05-21 16:42 UTC (permalink / raw) To: David Miller, eladv6, Jose.Abreu Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue On 05/21/2018 08:48 AM, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Thu, 17 May 2018 12:43:56 -0400 (EDT) > >> Giuseppe and Alexandre, please review this patch. > > If nobody thinks this patch is important enough to actually > review, I'm tossing it. > > Sorry. > How about looping in Jose? -- Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-21 16:42 ` Florian Fainelli @ 2018-05-22 8:56 ` Jose Abreu 2018-05-23 15:00 ` Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Jose Abreu @ 2018-05-22 8:56 UTC (permalink / raw) To: Florian Fainelli, David Miller, eladv6, Jose.Abreu Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue On 21-05-2018 17:42, Florian Fainelli wrote: > On 05/21/2018 08:48 AM, David Miller wrote: >> From: David Miller <davem@davemloft.net> >> Date: Thu, 17 May 2018 12:43:56 -0400 (EDT) >> >>> Giuseppe and Alexandre, please review this patch. >> If nobody thinks this patch is important enough to actually >> review, I'm tossing it. >> >> Sorry. >> > How about looping in Jose? Thanks for the cc Florian! Elad, Looking at this patch I have a couple of questions and suggestions: I see that most drivers use this pattern so, are they all broken? or is this a special case? You can also get the inner/outer vlan tag by reading desc0 of receive descriptor. Which can make this completely agnostic of VLAN tag. Thanks and Best Regards, Jose Miguel Abreu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-22 8:56 ` Jose Abreu @ 2018-05-23 15:00 ` Elad Nachman 2018-05-24 12:56 ` Jose Abreu 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-23 15:00 UTC (permalink / raw) To: Jose Abreu, Florian Fainelli, David Miller Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue Jose, I am not sure which drivers you have checked. I guess most non-networking embedded drivers never use 802.1AD so they stay broken unknowingly. Specifically, I have tested Intel e1000e based card which works correctly versus stmmac which works incorrectly. If you check netdev.c in e1000e then the vlan strip is conditional upon checking of 802.1Q bit in the descriptor, which does not happen in stmmac_main.c - the vlan stripping happens based on any tag header check. Once I applied my patch things started working - did not have to patch e1000e. The problem is that ip link allows to create 802.1ad devices which are not 0x8100 tagged but stmmac and probably other drivers handles the non 0x8100 tag incorrectly and the vlan slave discards the frame. Regarding the getting the tag from the descriptor - this is of course a possibility. My original patch handled stripping of all tags but then I was told here that I cannot strip 802.1ad tags without the NETIF_F_HW_VLAN_STAG_RX feature, which is probably correct regardless of the source of the vlan tag - skb or descriptor. I can also add the NETIF_F_HW_VLAN_STAG_RX feature plus the following original v1 patch: --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 2018-04-11 17:04:00.586057300 +0300 +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 2018-04-11 17:05:33.601992400 +0300 @@ -3293,17 +3293,19 @@ dma_map_err: static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == NETIF_F_HW_VLAN_CTAG_RX && !__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } There are three reasons why I prefer using this approach rather than the descriptor approach: A. It is inline with the original driver code. B. Using descriptor vlan will require to completely rewrite stmmac_rx_vlan and will result in at least 2-3 times line counts in the patch. C. I have no idea if first silicon implementations of DWMAC IP had some kind of HW bug (for example AXI clock glitch) which caused sampling of the VLAN tag in the descriptors to be unstable, and since I have no access to such hardware for regression I risk breaking stmmac for such old SOCs in case they decide to jump kernel versions to the latest. Thanks, Elad. On 22/05/18 11:56, Jose Abreu wrote: > On 21-05-2018 17:42, Florian Fainelli wrote: >> On 05/21/2018 08:48 AM, David Miller wrote: >>> From: David Miller <davem@davemloft.net> >>> Date: Thu, 17 May 2018 12:43:56 -0400 (EDT) >>> >>>> Giuseppe and Alexandre, please review this patch. >>> If nobody thinks this patch is important enough to actually >>> review, I'm tossing it. >>> >>> Sorry. >>> >> How about looping in Jose? > > Thanks for the cc Florian! > > Elad, > > Looking at this patch I have a couple of questions and suggestions: > > I see that most drivers use this pattern so, are they all broken? > or is this a special case? > > You can also get the inner/outer vlan tag by reading desc0 of > receive descriptor. Which can make this completely agnostic of > VLAN tag. > > Thanks and Best Regards, > Jose Miguel Abreu > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames 2018-05-23 15:00 ` Elad Nachman @ 2018-05-24 12:56 ` Jose Abreu 2018-05-24 16:56 ` [PATCH v3 net] stmmac: Added support for 802.1ad S-TAG stripping Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Jose Abreu @ 2018-05-24 12:56 UTC (permalink / raw) To: Elad Nachman, Jose Abreu, Florian Fainelli, David Miller Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue On 23-05-2018 16:00, Elad Nachman wrote: > Jose, > > I am not sure which drivers you have checked. I guess most non-networking embedded drivers never use 802.1AD > so they stay broken unknowingly. > Specifically, I have tested Intel e1000e based card which works correctly versus stmmac which works incorrectly. > > If you check netdev.c in e1000e then the vlan strip is conditional upon checking of 802.1Q bit in the descriptor, > which does not happen in stmmac_main.c - the vlan stripping happens based on any tag header check. > Once I applied my patch things started working - did not have to patch e1000e. The problem is that ip link allows to create 802.1ad devices which are not 0x8100 tagged but stmmac and probably other drivers handles the non 0x8100 tag incorrectly and the vlan slave discards the frame. > > Regarding the getting the tag from the descriptor - this is of course a possibility. My original patch handled stripping of all tags but then I was told here that I cannot strip 802.1ad tags without the NETIF_F_HW_VLAN_STAG_RX feature, which is probably correct regardless of the source of the vlan tag - skb or descriptor. > > I can also add the NETIF_F_HW_VLAN_STAG_RX feature plus the following original v1 patch: > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 2018-04-11 17:04:00.586057300 +0300 > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 2018-04-11 17:05:33.601992400 +0300 > @@ -3293,17 +3293,19 @@ dma_map_err: > > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > u16 vlanid; > + __be16 vlan_proto; > > if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > NETIF_F_HW_VLAN_CTAG_RX && > !__vlan_get_tag(skb, &vlanid)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > } > } > > There are three reasons why I prefer using this approach rather than the descriptor approach: > > A. It is inline with the original driver code. > B. Using descriptor vlan will require to completely rewrite stmmac_rx_vlan and will result in at least 2-3 times line counts in the patch. > C. I have no idea if first silicon implementations of DWMAC IP had some kind of HW bug (for example AXI clock glitch) which caused sampling of the VLAN tag in the descriptors to be unstable, and since I have no access to such hardware for regression I risk breaking stmmac for such old SOCs in case they decide to jump kernel versions to the latest. Your approach seems okay by me. Please submit a patch with this to netdev for proper review. Thanks and Best Regards, Jose Miguel Abreu > > Thanks, > > Elad. > > On 22/05/18 11:56, Jose Abreu wrote: >> On 21-05-2018 17:42, Florian Fainelli wrote: >>> On 05/21/2018 08:48 AM, David Miller wrote: >>>> From: David Miller <davem@davemloft.net> >>>> Date: Thu, 17 May 2018 12:43:56 -0400 (EDT) >>>> >>>>> Giuseppe and Alexandre, please review this patch. >>>> If nobody thinks this patch is important enough to actually >>>> review, I'm tossing it. >>>> >>>> Sorry. >>>> >>> How about looping in Jose? >> Thanks for the cc Florian! >> >> Elad, >> >> Looking at this patch I have a couple of questions and suggestions: >> >> I see that most drivers use this pattern so, are they all broken? >> or is this a special case? >> >> You can also get the inner/outer vlan tag by reading desc0 of >> receive descriptor. Which can make this completely agnostic of >> VLAN tag. >> >> Thanks and Best Regards, >> Jose Miguel Abreu >> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 net] stmmac: Added support for 802.1ad S-TAG stripping 2018-05-24 12:56 ` Jose Abreu @ 2018-05-24 16:56 ` Elad Nachman 2018-05-25 0:34 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-24 16:56 UTC (permalink / raw) To: Jose Abreu, Florian Fainelli, David Miller Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue, eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. NETIF_F_HW_VLAN_STAG_RX was added to the net device features to reflect this new support. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..2d2f37f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == - NETIF_F_HW_VLAN_CTAG_RX && + if ((dev->features & (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX)) == + (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX) && !__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } @@ -4344,7 +4346,7 @@ int stmmac_dvr_probe(struct device *device, ndev->watchdog_timeo = msecs_to_jiffies(watchdog); #ifdef STMMAC_VLAN_TAG_USED /* Both mac100 and gmac support receive VLAN tag detection */ - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; + ndev->features |= (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX); #endif priv->msg_enable = netif_msg_init(debug, default_msg_level); -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 net] stmmac: Added support for 802.1ad S-TAG stripping 2018-05-24 16:56 ` [PATCH v3 net] stmmac: Added support for 802.1ad S-TAG stripping Elad Nachman @ 2018-05-25 0:34 ` Toshiaki Makita 2018-05-26 19:24 ` [PATCH v4 net] stmmac: 802.1ad tag stripping support fix Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-05-25 0:34 UTC (permalink / raw) To: Elad Nachman, Jose Abreu, Florian Fainelli, David Miller Cc: netdev, peppe.cavallaro, alexandre.torgue On 2018/05/25 1:56, Elad Nachman wrote: > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, > and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() > instead of using the hard-coded value of ETH_P_8021Q. > NETIF_F_HW_VLAN_STAG_RX was added to the net device features to reflect this new support. > > Signed-off-by: Elad Nachman <eladn@gilat.com> > > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index b65e2d1..2d2f37f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) > > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > u16 vlanid; > + __be16 vlan_proto; > > - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > - NETIF_F_HW_VLAN_CTAG_RX && > + if ((dev->features & (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX)) == > + (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX) && This is basically not a correct condition since you cannot strip CTAG if HW_VLAN_STAG_RX is disabled even when HW_VLAN_CTAG_RX is enabled. The correct behavior is stripping CTAG when CTAG_RX is enabled and stripping STAG when STAG_RX is enabled, so this code cannot be protocol-agnostic. I suggested handling only CTAG in this driver because I thought adding STAG support will make this unnecessarily complicated. But I now actually noticed that this driver seems not able to toggle CTAG_RX nor STAG_RX because hw_features does not include them. So this code should work even if the condition is wrong, but in the first place why we need to check if dev->features includes CTAG_RX here... it's always included. It seems removing this check will be sufficient. > !__vlan_get_tag(skb, &vlanid)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > } > } > > @@ -4344,7 +4346,7 @@ int stmmac_dvr_probe(struct device *device, > ndev->watchdog_timeo = msecs_to_jiffies(watchdog); > #ifdef STMMAC_VLAN_TAG_USED > /* Both mac100 and gmac support receive VLAN tag detection */ > - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; > + ndev->features |= (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX); > #endif > priv->msg_enable = netif_msg_init(debug, default_msg_level); > > -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 net] stmmac: 802.1ad tag stripping support fix 2018-05-25 0:34 ` Toshiaki Makita @ 2018-05-26 19:24 ` Elad Nachman 2018-05-28 0:44 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-26 19:24 UTC (permalink / raw) To: Toshiaki Makita, Jose Abreu, Florian Fainelli, David Miller Cc: netdev, peppe.cavallaro, alexandre.torgue, eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. NETIF_F_HW_VLAN_CTAG_RX check was removed to be in line with the driver actual abilities. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..284e6a7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,17 +3293,17 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == - NETIF_F_HW_VLAN_CTAG_RX && - !__vlan_get_tag(skb, &vlanid)) { + if (!__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4 net] stmmac: 802.1ad tag stripping support fix 2018-05-26 19:24 ` [PATCH v4 net] stmmac: 802.1ad tag stripping support fix Elad Nachman @ 2018-05-28 0:44 ` Toshiaki Makita 2018-05-30 5:48 ` [PATCH v5 net] stmmac: 802.1ad tag stripping fix Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-05-28 0:44 UTC (permalink / raw) To: Elad Nachman, David Miller Cc: Jose Abreu, Florian Fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/05/27 4:24, Elad Nachman wrote: > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, > and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() > instead of using the hard-coded value of ETH_P_8021Q. > NETIF_F_HW_VLAN_CTAG_RX check was removed to be in line with the driver actual abilities. > > Signed-off-by: Elad Nachman <eladn@gilat.com> > I might have not been clear enough but you still need this hunk with this change. > - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; > + ndev->features |= (NETIF_F_HW_VLAN_CTAG_RX|NETIF_F_HW_VLAN_STAG_RX); Also I guess spaces are preferred around '|'. Would you check it by checkpatch.pl? -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-05-28 0:44 ` Toshiaki Makita @ 2018-05-30 5:48 ` Elad Nachman 2018-05-30 6:08 ` Toshiaki Makita 2018-06-03 14:33 ` David Miller 0 siblings, 2 replies; 27+ messages in thread From: Elad Nachman @ 2018-05-30 5:48 UTC (permalink / raw) To: Toshiaki Makita, David Miller Cc: Jose Abreu, Florian Fainelli, netdev, peppe.cavallaro, alexandre.torgue, eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. NETIF_F_HW_VLAN_CTAG_RX check was removed and NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver actual abilities. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..f680bcf 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,17 +3293,17 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; u16 vlanid; + __be16 vlan_proto; - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == - NETIF_F_HW_VLAN_CTAG_RX && - !__vlan_get_tag(skb, &vlanid)) { + if (!__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } @@ -4344,7 +4344,7 @@ int stmmac_dvr_probe(struct device *device, ndev->watchdog_timeo = msecs_to_jiffies(watchdog); #ifdef STMMAC_VLAN_TAG_USED /* Both mac100 and gmac support receive VLAN tag detection */ - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; #endif priv->msg_enable = netif_msg_init(debug, default_msg_level); -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-05-30 5:48 ` [PATCH v5 net] stmmac: 802.1ad tag stripping fix Elad Nachman @ 2018-05-30 6:08 ` Toshiaki Makita 2018-05-30 6:16 ` Elad Nachman 2018-06-03 14:33 ` David Miller 1 sibling, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-05-30 6:08 UTC (permalink / raw) To: Elad Nachman Cc: David Miller, Jose Abreu, Florian Fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/05/30 14:48, Elad Nachman wrote: > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, > and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() > instead of using the hard-coded value of ETH_P_8021Q. > NETIF_F_HW_VLAN_CTAG_RX check was removed and NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver actual abilities. > > Signed-off-by: Elad Nachman <eladn@gilat.com> > > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index b65e2d1..f680bcf 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3293,17 +3293,17 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) > > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > u16 vlanid; > + __be16 vlan_proto; > > - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > - NETIF_F_HW_VLAN_CTAG_RX && > - !__vlan_get_tag(skb, &vlanid)) { > + if (!__vlan_get_tag(skb, &vlanid)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > } Should this function contents be surrounded by #ifdef STMMAC_VLAN_TAG_USED, since the features is enabled only when it is defined? Otherwise looks good to me from the perspective of vlan features. > } > > @@ -4344,7 +4344,7 @@ int stmmac_dvr_probe(struct device *device, > ndev->watchdog_timeo = msecs_to_jiffies(watchdog); > #ifdef STMMAC_VLAN_TAG_USED > /* Both mac100 and gmac support receive VLAN tag detection */ > - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; > + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; > #endif > priv->msg_enable = netif_msg_init(debug, default_msg_level); > > -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-05-30 6:08 ` Toshiaki Makita @ 2018-05-30 6:16 ` Elad Nachman 2018-05-30 6:26 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-05-30 6:16 UTC (permalink / raw) To: Toshiaki Makita Cc: David Miller, Jose Abreu, Florian Fainelli, netdev, peppe.cavallaro, alexandre.torgue, eladv6 Interesting question. That's the way the driver was originally written and I tried to minimize the changes in the patch. Anyway, common.h (included by stmmac_main.c) contains the following: #if IS_ENABLED(CONFIG_VLAN_8021Q) #define STMMAC_VLAN_TAG_USED #include <linux/if_vlan.h> #endif So the define in question kicks in only once you enable 802.1Q support in the kernel .config . Thanks, Elad. On 30/05/18 09:08, Toshiaki Makita wrote: > On 2018/05/30 14:48, Elad Nachman wrote: >> stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). >> >> The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, >> and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . >> >> This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol >> (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). >> >> This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() >> instead of using the hard-coded value of ETH_P_8021Q. >> NETIF_F_HW_VLAN_CTAG_RX check was removed and NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver actual abilities. >> >> Signed-off-by: Elad Nachman <eladn@gilat.com> >> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index b65e2d1..f680bcf 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -3293,17 +3293,17 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) >> >> static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) >> { >> - struct ethhdr *ehdr; >> + struct vlan_ethhdr *veth; >> u16 vlanid; >> + __be16 vlan_proto; >> >> - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == >> - NETIF_F_HW_VLAN_CTAG_RX && >> - !__vlan_get_tag(skb, &vlanid)) { >> + if (!__vlan_get_tag(skb, &vlanid)) { >> /* pop the vlan tag */ >> - ehdr = (struct ethhdr *)skb->data; >> - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); >> + veth = (struct vlan_ethhdr *)skb->data; >> + vlan_proto = veth->h_vlan_proto; >> + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); >> skb_pull(skb, VLAN_HLEN); >> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); >> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); >> } > > Should this function contents be surrounded by > #ifdef STMMAC_VLAN_TAG_USED, since the features is enabled only when it > is defined? > Otherwise looks good to me from the perspective of vlan features. > >> } >> >> @@ -4344,7 +4344,7 @@ int stmmac_dvr_probe(struct device *device, >> ndev->watchdog_timeo = msecs_to_jiffies(watchdog); >> #ifdef STMMAC_VLAN_TAG_USED >> /* Both mac100 and gmac support receive VLAN tag detection */ >> - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; >> + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; >> #endif >> priv->msg_enable = netif_msg_init(debug, default_msg_level); >> >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-05-30 6:16 ` Elad Nachman @ 2018-05-30 6:26 ` Toshiaki Makita 0 siblings, 0 replies; 27+ messages in thread From: Toshiaki Makita @ 2018-05-30 6:26 UTC (permalink / raw) To: Elad Nachman Cc: David Miller, Jose Abreu, Florian Fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/05/30 15:16, Elad Nachman wrote: > Interesting question. That's the way the driver was originally written and I tried to minimize the changes in the patch. > Anyway, common.h (included by stmmac_main.c) contains the following: > > #if IS_ENABLED(CONFIG_VLAN_8021Q) > #define STMMAC_VLAN_TAG_USED > #include <linux/if_vlan.h> > #endif > > So the define in question kicks in only once you enable 802.1Q support in the kernel .config . So, we end up with stripping vlan even though the device does not have HW_VLAN_CTAG/STAG_RX when CONFIG_VLAN_8021Q is disabled, since the patch anyway removed the original condition... -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-05-30 5:48 ` [PATCH v5 net] stmmac: 802.1ad tag stripping fix Elad Nachman 2018-05-30 6:08 ` Toshiaki Makita @ 2018-06-03 14:33 ` David Miller 2018-06-04 0:49 ` Toshiaki Makita 1 sibling, 1 reply; 27+ messages in thread From: David Miller @ 2018-06-03 14:33 UTC (permalink / raw) To: eladv6 Cc: makita.toshiaki, Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue From: Elad Nachman <eladv6@gmail.com> Date: Wed, 30 May 2018 08:48:25 +0300 > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > u16 vlanid; > + __be16 vlan_proto; Please order local variables from longest to shortest line. > > - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > - NETIF_F_HW_VLAN_CTAG_RX && > - !__vlan_get_tag(skb, &vlanid)) { > + if (!__vlan_get_tag(skb, &vlanid)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > } > } I can't see how it is valid to do an unconditional software VLAN untagging even when VLAN is disabled in the kernel config or the NETIF_F_* feature bits are not set. At a minimum that feature test has to stay there, and when it's clear we let the generic VLAN code untag the packet. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v5 net] stmmac: 802.1ad tag stripping fix 2018-06-03 14:33 ` David Miller @ 2018-06-04 0:49 ` Toshiaki Makita 2018-06-08 9:19 ` [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-06-04 0:49 UTC (permalink / raw) To: David Miller, eladv6 Cc: Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/06/03 23:33, David Miller wrote: > From: Elad Nachman <eladv6@gmail.com> > Date: Wed, 30 May 2018 08:48:25 +0300 > >> static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) >> { >> - struct ethhdr *ehdr; >> + struct vlan_ethhdr *veth; >> u16 vlanid; >> + __be16 vlan_proto; > > Please order local variables from longest to shortest line. > >> >> - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == >> - NETIF_F_HW_VLAN_CTAG_RX && >> - !__vlan_get_tag(skb, &vlanid)) { >> + if (!__vlan_get_tag(skb, &vlanid)) { >> /* pop the vlan tag */ >> - ehdr = (struct ethhdr *)skb->data; >> - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); >> + veth = (struct vlan_ethhdr *)skb->data; >> + vlan_proto = veth->h_vlan_proto; >> + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); >> skb_pull(skb, VLAN_HLEN); >> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); >> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); >> } >> } > > I can't see how it is valid to do an unconditional software VLAN > untagging even when VLAN is disabled in the kernel config or the > NETIF_F_* feature bits are not set. Right. It is not valid. > > At a minimum that feature test has to stay there, and when it's clear > we let the generic VLAN code untag the packet. Since NETIF_F_HW_VLAN_*_RX are not protocol agnostic, we need two kind of similar checking here. veth = (struct vlan_ethhdr *)skb->data; vlan_proto = veth->h_vlan_proto; if ((vlan_proto == htons(ETH_P_8021Q) && dev->features & NETIF_F_HW_VLAN_CTAG_RX) || (vlan_proto == htons(ETH_P_8021AD) && dev->features & NETIF_F_HW_VLAN_STAG_RX) { vlanid = ntohs(veth->h_vlan_TCI); memmove(...); skb_pull(...); __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } An alternative way is not to check vlan_proto or features here but compile this code only when VLAN is enabled in the kernel config. This can be valid only because this driver does not have NETIF_F_HW_VLAN_*_RX in hw_features and they can not be toggled for now. static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { #ifdef STMMAC_VLAN_TAG_USED ... if (!__vlan_get_tag(skb, &vlanid)) { ... __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } #endif } -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected 2018-06-04 0:49 ` Toshiaki Makita @ 2018-06-08 9:19 ` Elad Nachman 2018-06-10 19:29 ` David Miller 0 siblings, 1 reply; 27+ messages in thread From: Elad Nachman @ 2018-06-08 9:19 UTC (permalink / raw) To: Toshiaki Makita, David Miller Cc: Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue, eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. NETIF_F_HW_VLAN_CTAG_RX check was removed and instead the strip action is dependent upon a preprocessor define which is defined when 802.1Q support is selected in the kernel config. NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver actual abilities. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b65e2d1..707917d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3293,18 +3293,20 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; +#ifdef STMMAC_VLAN_TAG_USED + struct vlan_ethhdr *veth; + __be16 vlan_proto; u16 vlanid; - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == - NETIF_F_HW_VLAN_CTAG_RX && - !__vlan_get_tag(skb, &vlanid)) { + if (!__vlan_get_tag(skb, &vlanid)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } +#endif } @@ -4344,7 +4346,7 @@ int stmmac_dvr_probe(struct device *device, ndev->watchdog_timeo = msecs_to_jiffies(watchdog); #ifdef STMMAC_VLAN_TAG_USED /* Both mac100 and gmac support receive VLAN tag detection */ - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; #endif priv->msg_enable = netif_msg_init(debug, default_msg_level); -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected 2018-06-08 9:19 ` [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected Elad Nachman @ 2018-06-10 19:29 ` David Miller 2018-06-11 0:50 ` Toshiaki Makita 0 siblings, 1 reply; 27+ messages in thread From: David Miller @ 2018-06-10 19:29 UTC (permalink / raw) To: eladv6 Cc: makita.toshiaki, Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue From: Elad Nachman <eladv6@gmail.com> Date: Fri, 8 Jun 2018 12:19:29 +0300 > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before > calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with > 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding > the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created > with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into > __vlan_hwaccel_put_tag() instead of using the hard-coded value of > ETH_P_8021Q. > > NETIF_F_HW_VLAN_CTAG_RX check was removed and instead the strip action > is dependent upon a preprocessor define which is defined when 802.1Q > support is selected in the kernel config. > > NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver > actual abilities. > > Signed-off-by: Elad Nachman <eladn@gilat.com> You can't remove the NETIF_F_* checks. If the user doesn't have VLAN offloading enabled, the VLAN tags should be left in the packet as-is. It can't be controlled by a CPP check. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected 2018-06-10 19:29 ` David Miller @ 2018-06-11 0:50 ` Toshiaki Makita 2018-06-15 6:57 ` [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping Elad Nachman 0 siblings, 1 reply; 27+ messages in thread From: Toshiaki Makita @ 2018-06-11 0:50 UTC (permalink / raw) To: David Miller, eladv6 Cc: Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/06/11 4:29, David Miller wrote: > From: Elad Nachman <eladv6@gmail.com> > Date: Fri, 8 Jun 2018 12:19:29 +0300 > >> stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before >> calling napi_gro_receive(). >> >> The function assumes VLAN tagged frames are always tagged with >> 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding >> the parameter on call to __vlan_hwaccel_put_tag() . >> >> This causes packets not to be passed to the VLAN slave if it was created >> with 802.1AD protocol >> (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). >> >> This fix passes the protocol from the VLAN header into >> __vlan_hwaccel_put_tag() instead of using the hard-coded value of >> ETH_P_8021Q. >> >> NETIF_F_HW_VLAN_CTAG_RX check was removed and instead the strip action >> is dependent upon a preprocessor define which is defined when 802.1Q >> support is selected in the kernel config. >> >> NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver >> actual abilities. >> >> Signed-off-by: Elad Nachman <eladn@gilat.com> > > You can't remove the NETIF_F_* checks. > > If the user doesn't have VLAN offloading enabled, the VLAN tags should > be left in the packet as-is. > > It can't be controlled by a CPP check. David, NETIF_F_HW_VALN_*_RX is not user-controllable in this driver because hw_features does not include them. But this can break when those features are added to hw_features so if David does not like this situation I think we need the condition anyway. -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping 2018-06-11 0:50 ` Toshiaki Makita @ 2018-06-15 6:57 ` Elad Nachman 2018-06-15 7:19 ` Toshiaki Makita 2018-06-15 16:06 ` David Miller 0 siblings, 2 replies; 27+ messages in thread From: Elad Nachman @ 2018-06-15 6:57 UTC (permalink / raw) To: Toshiaki Makita, David Miller Cc: Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue, eladv6 stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive(). The function assumes VLAN tagged frames are always tagged with 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() . This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag() instead of using the hard-coded value of ETH_P_8021Q. NETIF_F_HW_VLAN_STAG_RX check was added and the strip action is now dependent on the correct combination of features and the detected vlan tag. NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver actual abilities. Signed-off-by: Elad Nachman <eladn@gilat.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 11fb7c7..c4ffbfb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3182,17 +3182,22 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) { - struct ethhdr *ehdr; + struct vlan_ethhdr *veth; + __be16 vlan_proto; u16 vlanid; - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == - NETIF_F_HW_VLAN_CTAG_RX && - !__vlan_get_tag(skb, &vlanid)) { + veth = (struct vlan_ethhdr *)skb->data; + vlan_proto = veth->h_vlan_proto; + + if ((vlan_proto == htons(ETH_P_8021Q) && + dev->features & NETIF_F_HW_VLAN_CTAG_RX) || + (vlan_proto == htons(ETH_P_8021AD) && + dev->features & NETIF_F_HW_VLAN_STAG_RX)) { /* pop the vlan tag */ - ehdr = (struct ethhdr *)skb->data; - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); + vlanid = ntohs(veth->h_vlan_TCI); + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); skb_pull(skb, VLAN_HLEN); - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); } } @@ -4235,7 +4240,7 @@ int stmmac_dvr_probe(struct device *device, ndev->watchdog_timeo = msecs_to_jiffies(watchdog); #ifdef STMMAC_VLAN_TAG_USED /* Both mac100 and gmac support receive VLAN tag detection */ - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; #endif priv->msg_enable = netif_msg_init(debug, default_msg_level); -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping 2018-06-15 6:57 ` [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping Elad Nachman @ 2018-06-15 7:19 ` Toshiaki Makita 2018-06-15 16:06 ` David Miller 1 sibling, 0 replies; 27+ messages in thread From: Toshiaki Makita @ 2018-06-15 7:19 UTC (permalink / raw) To: Elad Nachman, David Miller Cc: Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue On 2018/06/15 15:57, Elad Nachman wrote: > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before > calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with > 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding > the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created > with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into > __vlan_hwaccel_put_tag() instead of using the hard-coded value of > ETH_P_8021Q. > > NETIF_F_HW_VLAN_STAG_RX check was added and the strip action is now > dependent on the correct combination of features and the detected vlan tag. > > NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver > actual abilities. > > Signed-off-by: Elad Nachman <eladn@gilat.com> Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 11fb7c7..c4ffbfb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3182,17 +3182,22 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) > > static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb) > { > - struct ethhdr *ehdr; > + struct vlan_ethhdr *veth; > + __be16 vlan_proto; > u16 vlanid; > > - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) == > - NETIF_F_HW_VLAN_CTAG_RX && > - !__vlan_get_tag(skb, &vlanid)) { > + veth = (struct vlan_ethhdr *)skb->data; > + vlan_proto = veth->h_vlan_proto; > + > + if ((vlan_proto == htons(ETH_P_8021Q) && > + dev->features & NETIF_F_HW_VLAN_CTAG_RX) || > + (vlan_proto == htons(ETH_P_8021AD) && > + dev->features & NETIF_F_HW_VLAN_STAG_RX)) { > /* pop the vlan tag */ > - ehdr = (struct ethhdr *)skb->data; > - memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2); > + vlanid = ntohs(veth->h_vlan_TCI); > + memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2); > skb_pull(skb, VLAN_HLEN); > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid); > + __vlan_hwaccel_put_tag(skb, vlan_proto, vlanid); > } > } > > @@ -4235,7 +4240,7 @@ int stmmac_dvr_probe(struct device *device, > ndev->watchdog_timeo = msecs_to_jiffies(watchdog); > #ifdef STMMAC_VLAN_TAG_USED > /* Both mac100 and gmac support receive VLAN tag detection */ > - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX; > + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX; > #endif > priv->msg_enable = netif_msg_init(debug, default_msg_level); > > -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping 2018-06-15 6:57 ` [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping Elad Nachman 2018-06-15 7:19 ` Toshiaki Makita @ 2018-06-15 16:06 ` David Miller 1 sibling, 0 replies; 27+ messages in thread From: David Miller @ 2018-06-15 16:06 UTC (permalink / raw) To: eladv6 Cc: makita.toshiaki, Jose.Abreu, f.fainelli, netdev, peppe.cavallaro, alexandre.torgue From: Elad Nachman <eladv6@gmail.com> Date: Fri, 15 Jun 2018 09:57:39 +0300 > stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before > calling napi_gro_receive(). > > The function assumes VLAN tagged frames are always tagged with > 802.1Q protocol, and assigns ETH_P_8021Q to the skb by hard-coding > the parameter on call to __vlan_hwaccel_put_tag() . > > This causes packets not to be passed to the VLAN slave if it was created > with 802.1AD protocol > (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100). > > This fix passes the protocol from the VLAN header into > __vlan_hwaccel_put_tag() instead of using the hard-coded value of > ETH_P_8021Q. > > NETIF_F_HW_VLAN_STAG_RX check was added and the strip action is now > dependent on the correct combination of features and the detected vlan tag. > > NETIF_F_HW_VLAN_STAG_RX feature was added to be in line with the driver > actual abilities. > > Signed-off-by: Elad Nachman <eladn@gilat.com> Applied, thank you. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-06-15 16:06 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-08 6:01 [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames Elad Nachman 2018-05-08 6:43 ` Toshiaki Makita 2018-05-08 7:11 ` Elad Nachman 2018-05-08 7:34 ` Toshiaki Makita 2018-05-11 7:31 ` [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q " Elad Nachman 2018-05-17 16:43 ` David Miller 2018-05-21 15:48 ` David Miller 2018-05-21 16:42 ` Florian Fainelli 2018-05-22 8:56 ` Jose Abreu 2018-05-23 15:00 ` Elad Nachman 2018-05-24 12:56 ` Jose Abreu 2018-05-24 16:56 ` [PATCH v3 net] stmmac: Added support for 802.1ad S-TAG stripping Elad Nachman 2018-05-25 0:34 ` Toshiaki Makita 2018-05-26 19:24 ` [PATCH v4 net] stmmac: 802.1ad tag stripping support fix Elad Nachman 2018-05-28 0:44 ` Toshiaki Makita 2018-05-30 5:48 ` [PATCH v5 net] stmmac: 802.1ad tag stripping fix Elad Nachman 2018-05-30 6:08 ` Toshiaki Makita 2018-05-30 6:16 ` Elad Nachman 2018-05-30 6:26 ` Toshiaki Makita 2018-06-03 14:33 ` David Miller 2018-06-04 0:49 ` Toshiaki Makita 2018-06-08 9:19 ` [PATCH v6 net] stmmac: strip all VLAN tag types when kernel 802.1Q support is selected Elad Nachman 2018-06-10 19:29 ` David Miller 2018-06-11 0:50 ` Toshiaki Makita 2018-06-15 6:57 ` [PATCH v7 net] stmmac: added support for 802.1ad vlan stripping Elad Nachman 2018-06-15 7:19 ` Toshiaki Makita 2018-06-15 16:06 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.