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 <da...@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
>>
