diff mbox

[REVERT] Re: [forcedeth bug] Re: [GIT] Networking

Message ID 20110805101625.GA11502@elte.hu
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Ingo Molnar Aug. 5, 2011, 10:16 a.m. UTC
* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> >  0891b0e08937: forcedeth: fix vlans
> 
> Hm, forcedeth is still giving me trouble even on latest -git that has 
> the above fix included.
> 
> The symptom is a stuck interface, no packets in. There's a frame 
> error RX packet:
> 
>  [root@mercury ~]# ifconfig eth0
>  eth0      Link encap:Ethernet  HWaddr 00:13:D4:DC:41:12  
>            inet addr:10.0.1.13  Bcast:10.0.1.255  Mask:255.255.255.0
>            UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>            RX packets:0 errors:1 dropped:0 overruns:0 frame:1
>            TX packets:531 errors:0 dropped:0 overruns:0 carrier:0
>            collisions:0 txqueuelen:1000 
>            RX bytes:0 (0.0 b)  TX bytes:34112 (33.3 KiB)
>            Interrupt:35 
> 
> Weirdly enough a defconfig x86 bootup works just fine - it's certain 
> .config combinations that trigger the bug. I've attached such a 
> config.
> 
> Note that at least once i've observed a seemingly good kernel going 
> 'bad' after a couple of minutes uptime. I've also observed 
> intermittent behavior - apparent lost packets and a laggy network.
> 
> I have done 3 failed attempts to bisect it any further - i got to the 
> commit that got fixed by:
> 
>   0891b0e08937: forcedeth: fix vlans
> 
> ... but that's something we already knew.
> 
> Let me know if there's any data i can provide to help debug this 
> problem.

I have reverted the two forcedeth commits:

  0891b0e08937: forcedeth: fix vlans
  3326c784c9f4: forcedeth: do vlan cleanup

and also reverted two vlan commits that the pre-cleanup driver 
depended on:

  ffcf9b767293: vlan: kill vlan_gro_frags and vlan_gro_receive
  7890a5b9cbfd: vlan: kill ndo_vlan_rx_register

and this finally gave me a working forcedeth driver. I've attached 
the working revert below.

Thanks,

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller Aug. 5, 2011, 10:19 a.m. UTC | #1
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 5 Aug 2011 12:16:25 +0200

> I have reverted the two forcedeth commits:
> 
>   0891b0e08937: forcedeth: fix vlans
>   3326c784c9f4: forcedeth: do vlan cleanup
> 
> and also reverted two vlan commits that the pre-cleanup driver 
> depended on:
> 
>   ffcf9b767293: vlan: kill vlan_gro_frags and vlan_gro_receive
>   7890a5b9cbfd: vlan: kill ndo_vlan_rx_register
> 
> and this finally gave me a working forcedeth driver. I've attached 
> the working revert below.

Jiri please diagnose this immediately otherwise I will have to apply
Ingo's reverts.

Ingo has been reporting this regression for days and you haven't said
anything.  That's not acceptable.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Aug. 5, 2011, 10:26 a.m. UTC | #2
Fri, Aug 05, 2011 at 12:19:53PM CEST, davem@davemloft.net wrote:
>From: Ingo Molnar <mingo@elte.hu>
>Date: Fri, 5 Aug 2011 12:16:25 +0200
>
>> I have reverted the two forcedeth commits:
>> 
>>   0891b0e08937: forcedeth: fix vlans
>>   3326c784c9f4: forcedeth: do vlan cleanup
>> 
>> and also reverted two vlan commits that the pre-cleanup driver 
>> depended on:
>> 
>>   ffcf9b767293: vlan: kill vlan_gro_frags and vlan_gro_receive
>>   7890a5b9cbfd: vlan: kill ndo_vlan_rx_register
>> 
>> and this finally gave me a working forcedeth driver. I've attached 
>> the working revert below.
>
>Jiri please diagnose this immediately otherwise I will have to apply
>Ingo's reverts.
>
>Ingo has been reporting this regression for days and you haven't said
>anything.  That's not acceptable.

Sorry, but first time I saw this was ~3hours ago. Looking at it
(reserving systems, compiling with ingo's config, etc) since then...

/me cannot be faster.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index e55df30..537b695 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -820,6 +820,9 @@  struct fe_priv {
 	struct nv_skb_map *tx_end_flip;
 	int tx_stop;
 
+	/* vlan fields */
+	struct vlan_group *vlangrp;
+
 	/* msi/msi-x fields */
 	u32 msi_flags;
 	struct msix_entry msi_x_entry[NV_MSI_X_MAX_VECTORS];
@@ -2763,20 +2766,17 @@  static int nv_rx_process_optimized(struct net_device *dev, int limit)
 			skb->protocol = eth_type_trans(skb, dev);
 			prefetch(skb->data);
 
-			vlanflags = le32_to_cpu(np->get_rx.ex->buflow);
-
-			/*
-			 * There's need to check for NETIF_F_HW_VLAN_RX here.
-			 * Even if vlan rx accel is disabled,
-			 * NV_RX3_VLAN_TAG_PRESENT is pseudo randomly set.
-			 */
-			if (dev->features & NETIF_F_HW_VLAN_RX &&
-			    vlanflags & NV_RX3_VLAN_TAG_PRESENT) {
-				u16 vid = vlanflags & NV_RX3_VLAN_TAG_MASK;
-
-				__vlan_hwaccel_put_tag(skb, vid);
+			if (likely(!np->vlangrp)) {
+				napi_gro_receive(&np->napi, skb);
+			} else {
+				vlanflags = le32_to_cpu(np->get_rx.ex->buflow);
+				if (vlanflags & NV_RX3_VLAN_TAG_PRESENT) {
+					vlan_gro_receive(&np->napi, np->vlangrp,
+							 vlanflags & NV_RX3_VLAN_TAG_MASK, skb);
+				} else {
+					napi_gro_receive(&np->napi, skb);
+				}
 			}
-			napi_gro_receive(&np->napi, skb);
 
 			dev->stats.rx_packets++;
 			dev->stats.rx_bytes += len;
@@ -4484,27 +4484,6 @@  static u32 nv_fix_features(struct net_device *dev, u32 features)
 	return features;
 }
 
-static void nv_vlan_mode(struct net_device *dev, u32 features)
-{
-	struct fe_priv *np = get_nvpriv(dev);
-
-	spin_lock_irq(&np->lock);
-
-	if (features & NETIF_F_HW_VLAN_RX)
-		np->txrxctl_bits |= NVREG_TXRXCTL_VLANSTRIP;
-	else
-		np->txrxctl_bits &= ~NVREG_TXRXCTL_VLANSTRIP;
-
-	if (features & NETIF_F_HW_VLAN_TX)
-		np->txrxctl_bits |= NVREG_TXRXCTL_VLANINS;
-	else
-		np->txrxctl_bits &= ~NVREG_TXRXCTL_VLANINS;
-
-	writel(np->txrxctl_bits, get_hwbase(dev) + NvRegTxRxControl);
-
-	spin_unlock_irq(&np->lock);
-}
-
 static int nv_set_features(struct net_device *dev, u32 features)
 {
 	struct fe_priv *np = netdev_priv(dev);
@@ -4525,9 +4504,6 @@  static int nv_set_features(struct net_device *dev, u32 features)
 		spin_unlock_irq(&np->lock);
 	}
 
-	if (changed & (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX))
-		nv_vlan_mode(dev, features);
-
 	return 0;
 }
 
@@ -4903,6 +4879,29 @@  static const struct ethtool_ops ops = {
 	.self_test = nv_self_test,
 };
 
+static void nv_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
+{
+	struct fe_priv *np = get_nvpriv(dev);
+
+	spin_lock_irq(&np->lock);
+
+	/* save vlan group */
+	np->vlangrp = grp;
+
+	if (grp) {
+		/* enable vlan on MAC */
+		np->txrxctl_bits |= NVREG_TXRXCTL_VLANSTRIP | NVREG_TXRXCTL_VLANINS;
+	} else {
+		/* disable vlan on MAC */
+		np->txrxctl_bits &= ~NVREG_TXRXCTL_VLANSTRIP;
+		np->txrxctl_bits &= ~NVREG_TXRXCTL_VLANINS;
+	}
+
+	writel(np->txrxctl_bits, get_hwbase(dev) + NvRegTxRxControl);
+
+	spin_unlock_irq(&np->lock);
+}
+
 /* The mgmt unit and driver use a semaphore to access the phy during init */
 static int nv_mgmt_acquire_sema(struct net_device *dev)
 {
@@ -5209,6 +5208,7 @@  static const struct net_device_ops nv_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address	= nv_set_mac_address,
 	.ndo_set_multicast_list	= nv_set_multicast,
+	.ndo_vlan_rx_register	= nv_vlan_rx_register,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= nv_poll_controller,
 #endif
@@ -5226,6 +5226,7 @@  static const struct net_device_ops nv_netdev_ops_optimized = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address	= nv_set_mac_address,
 	.ndo_set_multicast_list	= nv_set_multicast,
+	.ndo_vlan_rx_register	= nv_vlan_rx_register,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= nv_poll_controller,
 #endif
@@ -5338,16 +5339,15 @@  static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 		np->txrxctl_bits |= NVREG_TXRXCTL_RXCHECK;
 		dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_SG |
 			NETIF_F_TSO | NETIF_F_RXCSUM;
+		dev->features |= dev->hw_features;
 	}
 
 	np->vlanctl_bits = 0;
 	if (id->driver_data & DEV_HAS_VLAN) {
 		np->vlanctl_bits = NVREG_VLANCONTROL_ENABLE;
-		dev->hw_features |= NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX;
+		dev->features |= NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX;
 	}
 
-	dev->features |= dev->hw_features;
-
 	np->pause_flags = NV_PAUSEFRAME_RX_CAPABLE | NV_PAUSEFRAME_RX_REQ | NV_PAUSEFRAME_AUTONEG;
 	if ((id->driver_data & DEV_HAS_PAUSEFRAME_TX_V1) ||
 	    (id->driver_data & DEV_HAS_PAUSEFRAME_TX_V2) ||
@@ -5615,8 +5615,6 @@  static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 		goto out_error;
 	}
 
-	nv_vlan_mode(dev, dev->features);
-
 	netif_carrier_off(dev);
 
 	dev_info(&pci_dev->dev, "ifname %s, PHY OUI 0x%x @ %d, addr %pM\n",
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 44da482..f2a4892 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -108,6 +108,12 @@  extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
 extern bool vlan_do_receive(struct sk_buff **skb);
 extern struct sk_buff *vlan_untag(struct sk_buff *skb);
+extern gro_result_t
+vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp,
+		 unsigned int vlan_tci, struct sk_buff *skb);
+extern gro_result_t
+vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
+	       unsigned int vlan_tci);
 
 #else
 static inline struct net_device *
@@ -139,6 +145,20 @@  static inline struct sk_buff *vlan_untag(struct sk_buff *skb)
 {
 	return skb;
 }
+
+static inline gro_result_t
+vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp,
+		 unsigned int vlan_tci, struct sk_buff *skb)
+{
+	return GRO_DROP;
+}
+
+static inline gro_result_t
+vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
+	       unsigned int vlan_tci)
+{
+	return GRO_DROP;
+}
 #endif
 
 /**
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ddee79b..4537bff 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -768,6 +768,12 @@  struct netdev_tc_txq {
  *	3. Update dev->stats asynchronously and atomically, and define
  *	   neither operation.
  *
+ * void (*ndo_vlan_rx_register)(struct net_device *dev, struct vlan_group *grp);
+ *	If device support VLAN receive acceleration
+ *	(ie. dev->features & NETIF_F_HW_VLAN_RX), then this function is called
+ *	when vlan groups for the device changes.  Note: grp is NULL
+ *	if no vlan's groups are being used.
+ *
  * void (*ndo_vlan_rx_add_vid)(struct net_device *dev, unsigned short vid);
  *	If device support VLAN filtering (dev->features & NETIF_F_HW_VLAN_FILTER)
  *	this function is called when a VLAN id is registered.
@@ -886,6 +892,8 @@  struct net_device_ops {
 						     struct rtnl_link_stats64 *storage);
 	struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
 
+	void			(*ndo_vlan_rx_register)(struct net_device *dev,
+						        struct vlan_group *grp);
 	void			(*ndo_vlan_rx_add_vid)(struct net_device *dev,
 						       unsigned short vid);
 	void			(*ndo_vlan_rx_kill_vid)(struct net_device *dev,
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 8970ba1..d24c464 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -134,6 +134,8 @@  void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 		vlan_gvrp_uninit_applicant(real_dev);
 
 		rcu_assign_pointer(real_dev->vlgrp, NULL);
+		if (ops->ndo_vlan_rx_register)
+			ops->ndo_vlan_rx_register(real_dev, NULL);
 
 		/* Free the group, after all cpu's are done. */
 		call_rcu(&grp->rcu, vlan_rcu_free);
@@ -205,6 +207,8 @@  int register_vlan_dev(struct net_device *dev)
 	grp->nr_vlans++;
 
 	if (ngrp) {
+		if (ops->ndo_vlan_rx_register && (real_dev->features & NETIF_F_HW_VLAN_RX))
+			ops->ndo_vlan_rx_register(real_dev, ngrp);
 		rcu_assign_pointer(real_dev->vlgrp, ngrp);
 	}
 	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 5f27f8e..68b04ea 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -96,6 +96,22 @@  u16 vlan_dev_vlan_id(const struct net_device *dev)
 }
 EXPORT_SYMBOL(vlan_dev_vlan_id);
 
+gro_result_t vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp,
+			      unsigned int vlan_tci, struct sk_buff *skb)
+{
+	__vlan_hwaccel_put_tag(skb, vlan_tci);
+	return napi_gro_receive(napi, skb);
+}
+EXPORT_SYMBOL(vlan_gro_receive);
+
+gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
+			    unsigned int vlan_tci)
+{
+	__vlan_hwaccel_put_tag(napi->skb, vlan_tci);
+	return napi_gro_frags(napi);
+}
+EXPORT_SYMBOL(vlan_gro_frags);
+
 static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
 {
 	if (skb_cow(skb, skb_headroom(skb)) < 0)