From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elad Nachman Subject: Re: [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames Date: Tue, 8 May 2018 10:11:19 +0300 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org, eladv6@gmail.com To: Toshiaki Makita Return-path: Received: from mail-wr0-f193.google.com ([209.85.128.193]:34424 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754269AbeEHHLX (ORCPT ); Tue, 8 May 2018 03:11:23 -0400 Received: by mail-wr0-f193.google.com with SMTP id p18-v6so31146120wrm.1 for ; Tue, 08 May 2018 00:11:22 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> >> --- >> 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. >