From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ivan Vecera Subject: Re: [PATCH net v2] bna: fix vlan tag stripping and implement its toggling Date: Fri, 28 Feb 2014 14:13:01 +0100 Message-ID: <53108B5D.9030807@redhat.com> References: <1393581630-5433-1-git-send-email-ivecera@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, rmody@brocade.com, jiri@resnulli.us, fw@strlen.de To: netdev@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32710 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751838AbaB1NNN (ORCPT ); Fri, 28 Feb 2014 08:13:13 -0500 In-Reply-To: <1393581630-5433-1-git-send-email-ivecera@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On 02/28/2014 11:00 AM, Ivan Vecera wrote: > The recent commit "fe1624c bna: RX Filter Enhancements" disables > VLAN tag stripping if the NIC is in promiscuous mode. This causes > __vlan_hwaccel_put_tag() is called when the stripping is disabled. > Because of this VLAN over bna does not work and causes BUGs in conjunction > with openvswitch like this: > > --- > [18370.665074] kernel BUG at include/linux/skbuff.h:1497! > ... > [18370.876769] Call Trace: > [18370.879455] > [18370.881569] [] internal_dev_recv+0xb6/0x130 [openvswitch] > [18370.889552] [] ovs_vport_send+0x1d/0x80 [openvswitch] > [18370.896922] [] do_output+0x2a/0x50 [openvswitch] > [18370.903814] [] do_execute_actions+0x19d/0xb20 [openvswitch] > [18370.911757] [] ovs_execute_actions+0x2b/0x30 [openvswitch] > [18370.919602] [] ovs_dp_process_received_packet+0x92/0x120 [openvswitch] > [18370.928597] [] ? find_busiest_group+0x103/0x880 > [18370.935396] [] ovs_vport_receive+0x2a/0x30 [openvswitch] > [18370.943053] [] netdev_frame_hook+0xc1/0x120 [openvswitch] > [18370.950802] [] __netif_receive_skb_core+0x262/0x840 > [18370.957982] [] ? flush_ptrace_hw_breakpoint+0x10/0x40 > [18370.965349] [] __netif_receive_skb+0x18/0x60 > [18370.971855] [] netif_receive_skb+0x33/0xb0 > [18370.978172] [] napi_gro_frags+0x116/0x170 > [18370.984407] [] bnad_napi_poll_rx+0x3f4/0x8f0 [bna] > [18370.991488] [] net_rx_action+0x15a/0x250 > ... > --- > > The following patch makes the following changes so the driver: > 1. Implements .ndo_set_features so an user can toggle VLAN tag stripping > on/off > 2. Does not disable VLAN tag stripping in promiscuous mode so the driver > respect user's choice > 3. Calls __vlan_hwaccel_put_tag() only when the stripping is enabled > > v2: > - make bnad_set_features static (thanks Florian Westphal) > - enable/disable VLAN stripping during open WRT hw_features > > Signed-off-by: Ivan Vecera > --- > drivers/net/ethernet/brocade/bna/bnad.c | 40 ++++++++++++++++++++++++--------- > 1 file changed, 30 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/brocade/bna/bnad.c b/drivers/net/ethernet/brocade/bna/bnad.c > index cf64f3d..b0b2fa1 100644 > --- a/drivers/net/ethernet/brocade/bna/bnad.c > +++ b/drivers/net/ethernet/brocade/bna/bnad.c > @@ -707,7 +707,8 @@ bnad_cq_process(struct bnad *bnad, struct bna_ccb *ccb, int budget) > else > skb_checksum_none_assert(skb); > > - if (flags & BNA_CQ_EF_VLAN) > + if ((flags & BNA_CQ_EF_VLAN) && > + (bnad->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)) > __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(cmpl->vlan_tag)); > > if (BNAD_RXBUF_IS_SK_BUFF(unmap_q->type)) > @@ -2094,7 +2095,9 @@ bnad_init_rx_config(struct bnad *bnad, struct bna_rx_config *rx_config) > rx_config->q1_buf_size = BFI_SMALL_RXBUF_SIZE; > } > > - rx_config->vlan_strip_status = BNA_STATUS_T_ENABLED; > + rx_config->vlan_strip_status = > + (bnad->netdev->hw_features & NETIF_F_HW_VLAN_CTAG_RX) ? > + BNA_STATUS_T_ENABLED : BNA_STATUS_T_DISABLED; > } Just another typo should be ->features ^^^^^ and not ->hw_features. -> v3 > > static void > @@ -3245,11 +3248,6 @@ bnad_set_rx_mode(struct net_device *netdev) > BNA_RXMODE_ALLMULTI; > bna_rx_mode_set(bnad->rx_info[0].rx, new_mode, mode_mask, NULL); > > - if (bnad->cfg_flags & BNAD_CF_PROMISC) > - bna_rx_vlan_strip_disable(bnad->rx_info[0].rx); > - else > - bna_rx_vlan_strip_enable(bnad->rx_info[0].rx); > - > spin_unlock_irqrestore(&bnad->bna_lock, flags); > } > > @@ -3374,6 +3372,27 @@ bnad_vlan_rx_kill_vid(struct net_device *netdev, __be16 proto, u16 vid) > return 0; > } > > +static int bnad_set_features(struct net_device *dev, netdev_features_t features) > +{ > + struct bnad *bnad = netdev_priv(dev); > + netdev_features_t changed = features ^ dev->features; > + > + if ((changed & NETIF_F_HW_VLAN_CTAG_RX) && netif_running(dev)) { > + unsigned long flags; > + > + spin_lock_irqsave(&bnad->bna_lock, flags); > + > + if (features & NETIF_F_HW_VLAN_CTAG_RX) > + bna_rx_vlan_strip_enable(bnad->rx_info[0].rx); > + else > + bna_rx_vlan_strip_disable(bnad->rx_info[0].rx); > + > + spin_unlock_irqrestore(&bnad->bna_lock, flags); > + } > + > + return 0; > +} > + > #ifdef CONFIG_NET_POLL_CONTROLLER > static void > bnad_netpoll(struct net_device *netdev) > @@ -3421,6 +3440,7 @@ static const struct net_device_ops bnad_netdev_ops = { > .ndo_change_mtu = bnad_change_mtu, > .ndo_vlan_rx_add_vid = bnad_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = bnad_vlan_rx_kill_vid, > + .ndo_set_features = bnad_set_features, > #ifdef CONFIG_NET_POLL_CONTROLLER > .ndo_poll_controller = bnad_netpoll > #endif > @@ -3433,14 +3453,14 @@ bnad_netdev_init(struct bnad *bnad, bool using_dac) > > netdev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM | > NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > - NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_TX; > + NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_TX | > + NETIF_F_HW_VLAN_CTAG_RX; > > netdev->vlan_features = NETIF_F_SG | NETIF_F_HIGHDMA | > NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > NETIF_F_TSO | NETIF_F_TSO6; > > - netdev->features |= netdev->hw_features | > - NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER; > + netdev->features |= netdev->hw_features | NETIF_F_HW_VLAN_CTAG_FILTER; > > if (using_dac) > netdev->features |= NETIF_F_HIGHDMA; >