[v2,net] stmmac: strip vlan tag on reception only for 8021q tagged frames

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

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.

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.

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?

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

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
>

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